Merge pull request #273 from Senjai/more_auth_tweaks

More auth tweaks

backend/app/controllers/spree/admin/stock_transfers_controller.rb

@@ -7,8 +7,7 @@ class StockTransfersController < ResourceController
  { translation_key: :name, attr_name: :name }
  ]
 
- before_filter :load_readable_stock_locations, only: :index
- before_filter :load_transferable_stock_locations, only: :new
+ before_filter :load_transferable_stock_locations, only: [:index, :new]
  before_filter :load_variant_display_attributes, only: [:receive, :edit, :show, :tracking_info]
  before_filter :load_destination_stock_locations, only: :edit
  before_filter :ensure_access_to_stock_location, only: :create
@@ -102,16 +101,8 @@ def authorize_transfer_attributes!
  authorize! :create, duplicate
  end
 
- def accessible_stock_locations
- Spree::StockLocation.accessible_by(current_ability, :index)
- end
-
  def transferable_stock_locations
- accessible_stock_locations.accessible_by(current_ability, :transfer)
- end
-
- def load_readable_stock_locations
- @stock_locations = accessible_stock_locations
+ Spree::StockLocation.accessible_by(current_ability, :transfer)
  end
 
  def load_transferable_stock_locations

backend/app/views/spree/admin/promotions/_actions.html.erb

@@ -4,13 +4,15 @@
  <% options = options_for_select( Rails.application.config.spree.promotions.actions.map(&:name).map {|name| [ Spree.t("promotion_action_types.#{name.demodulize.underscore}.name"), name] } ) %>
  <fieldset>
  <legend align="center"><%= Spree.t(:promotion_actions) %></legend>
- <div class="field">
- <%= label_tag :action_type, Spree.t(:add_action_of_type)%>
- <%= select_tag 'action_type', options, :class => 'select2 fullwidth' %> 
- </div>
- <div class="filter-actions actions">
- <%= button Spree.t(:add), 'plus' %>
- </div>
+ <% if can?(:update, @promotion) %>
+ <div class="field">
+ <%= label_tag :action_type, Spree.t(:add_action_of_type)%>
+ <%= select_tag 'action_type', options, :class => 'select2 fullwidth' %>
+ </div>
+ <div class="filter-actions actions">
+ <%= button Spree.t(:add), 'plus' %>
+ </div>
+ <% end %>
  </fieldset>
  <% end %>
 
@@ -24,9 +26,11 @@
  </div>
  <% end %>
  </div>
- <div class="filter-actions actions promotion-update">
- <%= button Spree.t('actions.update'), 'refresh' %>
- </div>
+ <% if can?(:update, @promotion) %>
+ <div class="filter-actions actions promotion-update">
+ <%= button Spree.t('actions.update'), 'refresh' %>
+ </div>
+ <% end %>
  <% end %>
 
 </fieldset>

backend/app/views/spree/admin/promotions/_promotion_action.html.erb

@@ -1,11 +1,13 @@
 <div class="promotion_action promotion-block <%= promotion_action.type.to_s.demodulize.underscore %>" id="<%= dom_id promotion_action %>">
  <% type_name = promotion_action.class.name.demodulize.underscore %>
  <h6 class="promotion-title"><%= Spree.t("promotion_action_types.#{type_name}.description") %></h6>
- <%= link_to_with_icon 'trash', '', spree.admin_promotion_promotion_action_path(@promotion, promotion_action), :remote => true, :method => :delete, :class => 'delete' %>
- 
+ <% if can?(:destroy, promotion_action) %>
+ <%= link_to_with_icon 'trash', '', spree.admin_promotion_promotion_action_path(@promotion, promotion_action), :remote => true, :method => :delete, :class => 'delete' %>
+ <% end %>
+
  <% param_prefix = "promotion[promotion_actions_attributes][#{promotion_action.id}]" %>
  <%= hidden_field_tag "#{param_prefix}[id]", promotion_action.id %>
- 
+
  <%= render :partial => "spree/shared/error_messages", :locals => { :target => promotion_action } %>
  <%= render :partial => "spree/admin/promotions/actions/#{type_name}",
  :locals => { :promotion_action => promotion_action, :param_prefix => param_prefix } %>

backend/app/views/spree/admin/promotions/_promotion_rule.html.erb

@@ -1,10 +1,12 @@
 <div class="promotion_rule promotion-block alpha omega eight columns" id="<%= dom_id promotion_rule %>">
  <% type_name = promotion_rule.class.name.demodulize.underscore %>
  <h6 class='promotion-title <%= 'no-text' if type_name == 'user_logged_in' || type_name == 'first_order'%>'><%= Spree.t("promotion_rule_types.#{type_name}.description") %></h6>
- <%= link_to_with_icon 'trash', '', spree.admin_promotion_promotion_rule_path(@promotion, promotion_rule), :remote => true, :method => :delete, :class => 'delete' %>
- 
+ <% if can?(:destroy, promotion_rule) %>
+ <%= link_to_with_icon 'trash', '', spree.admin_promotion_promotion_rule_path(@promotion, promotion_rule), :remote => true, :method => :delete, :class => 'delete' %>
+ <% end %>
+
  <% param_prefix = "promotion[promotion_rules_attributes][#{promotion_rule.id}]" %>
- <%= hidden_field_tag "#{param_prefix}[id]", promotion_rule.id %> 
+ <%= hidden_field_tag "#{param_prefix}[id]", promotion_rule.id %>
  <%= render :partial => "spree/shared/error_messages", :locals => { :target => promotion_rule } %>
  <%= render :partial => "spree/admin/promotions/rules/#{type_name}", :locals => { :promotion_rule => promotion_rule, :param_prefix => param_prefix } %>
 </div>

backend/app/views/spree/admin/promotions/_rules.html.erb

@@ -4,27 +4,29 @@
  <fieldset>
  <legend align="center"><%= Spree.t(:rules) %></legend>
 
- <div class="field">
- <%= label_tag :promotion_rule_type, Spree.t(:add_rule_of_type) %> 
- <%= select_tag('promotion_rule[type]', options_for_promotion_rule_types(@promotion), :class => 'select2 fullwidth') %>
- </div>
- <div class="filter-actions actions">
- <%= button Spree.t(:add), 'plus' %>
- </div>
+ <% if can?(:update, @promotion) %>
+ <div class="field">
+ <%= label_tag :promotion_rule_type, Spree.t(:add_rule_of_type) %>
+ <%= select_tag('promotion_rule[type]', options_for_promotion_rule_types(@promotion), :class => 'select2 fullwidth') %>
+ </div>
+ <div class="filter-actions actions">
+ <%= button Spree.t(:add), 'plus' %>
+ </div>
+ <% end %>
  </fieldset>
  <% end %>
 
- 
+
  <%= form_for @promotion, :url => object_url, :method => :put do |f| %>
  <fieldset class="no-border-top">
  <div id="promotion-pilicy-select" class="align-center row">
  <% Spree::Promotion::MATCH_POLICIES.each do |policy| %>
  <div class="alpha four columns">
- <label><%= f.radio_button :match_policy, policy %> <%= Spree.t "promotion_form.match_policies.#{policy}" %></label> 
+ <label><%= f.radio_button :match_policy, policy %> <%= Spree.t "promotion_form.match_policies.#{policy}" %></label>
  </div>
  <% end %>
  </div>
- 
+
  <div id="rules" class="filter_list row">
  <% if @promotion.rules.any? %>
  <%= render :partial => 'promotion_rule', :collection => @promotion.rules, :locals => {} %>
@@ -35,11 +37,13 @@
  <% end %>
  </div>
 
- <div class="promotion-update filter-actions actions">
- <%= button Spree.t('actions.update'), 'refresh' %>
- </div>
+ <% if can?(:update, @promotion) %>
+ <div class="promotion-update filter-actions actions">
+ <%= button Spree.t('actions.update'), 'refresh' %>
+ </div>
+ <% end %>
  </fieldset>
  <% end %>
- 
+
 
 </fieldset>

backend/app/views/spree/admin/promotions/edit.html.erb

@@ -7,14 +7,18 @@
 <% content_for :page_actions do %>
  <li>
  <%= button_link_to Spree.t(:back_to_promotions_list), admin_promotions_path, :icon => 'arrow-left' %>
- <%= button_link_to Spree.t(:download_promotion_code_list), admin_promotion_promotion_codes_path(promotion_id: @promotion.id, format: :csv), :icon => 'arrow-down' %>
+ <% if can?(:display, Spree::PromotionCode) %>
+ <%= button_link_to Spree.t(:download_promotion_code_list), admin_promotion_promotion_codes_path(promotion_id: @promotion.id, format: :csv), :icon => 'arrow-down' %>
+ <% end %>
  </li>
 <% end %>
 
 <%= form_for @promotion, :url => object_url, :method => :put do |f| %>
  <fieldset class="no-border-top">
  <%= render :partial => 'form', :locals => { :f => f } %>
- <%= render :partial => 'spree/admin/shared/edit_resource_links' %>
+ <% if can?(:update, @promotion) %>
+ <%= render :partial => 'spree/admin/shared/edit_resource_links' %>
+ <% end %>
  </fieldset>
 <% end %>
 

backend/app/views/spree/admin/promotions/index.html.erb

@@ -93,7 +93,7 @@
  <td class="align-center"><%= Spree.t(:current_promotion_usage, :count => promotion.usage_count) %></td>
  <td class="align-center"><%= promotion.expires_at.to_date.to_s(:short_date) if promotion.expires_at %></td>
  <td class="actions">
- <% if can?(:update, promotion) %>
+ <% if can?(:edit, promotion) %>
  <%= link_to_edit promotion, :no_text => true %>
  <% end %>
  <% if can?(:destroy, promotion) %>

core/app/models/spree/permission_sets/promotion_display.rb

@@ -2,10 +2,11 @@ module Spree
  module PermissionSets
  class PromotionDisplay < PermissionSets::Base
  def activate!
- can [:display, :admin], Spree::Promotion
+ can [:display, :admin, :edit], Spree::Promotion
  can [:display, :admin], Spree::PromotionRule
  can [:display, :admin], Spree::PromotionAction
  can [:display, :admin], Spree::PromotionCategory
+ can [:display, :admin], Spree::PromotionCode
  end
  end
  end

core/app/models/spree/permission_sets/restricted_transfer_management.rb

@@ -7,20 +7,36 @@ module PermissionSets
  #
  # Users can be associated with stock locations via the admin user interface.
  #
+ # The logic here is unfortunately rather complex and boils down to:
+ # - A user has read only access to all stock locations (including inactive ones)
+ # - A user can see all stock transfers for their associated stock locations regardless of the
+ # fact that they may not be associated with both the destination and the source, as long as
+ # they are associated with at least one of the two.
+ # - A user can manage stock transfers only if they are associated with both the destination and the source,
+ # or if the user is associated with the source, and the transfer has not yet been assigned a destination.
+ #
  # @see Spree::PermissionSets::Base
  class RestrictedTransferManagement < PermissionSets::Base
  def activate!
  can [:display, :admin], Spree::StockItem
- can [:display, :admin], Spree::StockTransfer
+ # We need display here, as by default users cannot see inactive stock locations.
+ can :display, Spree::StockLocation
 
  if user.stock_locations.any?
- # We need display here, as by default users cannot see inactive stock locations.
- can [:display, :transfer], Spree::StockLocation, id: location_ids
+ can [:admin, :create], Spree::StockTransfer
+ can :display, Spree::StockTransfer, source_location_id: location_ids
+ can :display, Spree::StockTransfer, destination_location_id: location_ids
+ can :manage, Spree::StockTransfer,
+ source_location_id: location_ids,
+ destination_location_id: destination_location_ids
+
+ can :transfer, Spree::StockLocation, id: location_ids
+
  can :update, Spree::StockItem, stock_location_id: location_ids
- can :manage, Spree::StockTransfer, source_location_id: location_ids, destination_location_id: location_ids
+
  can :manage, Spree::TransferItem, stock_transfer: {
  source_location_id: location_ids,
- destination_location_id: location_ids
+ destination_location_id: destination_location_ids
  }
  end
  end
@@ -29,7 +45,11 @@ def activate!
 
  def location_ids
  # either source_location_id or destination_location_id can be nil.
- @ids ||= user.stock_locations.pluck(:id) + [nil]
+ @ids ||= user.stock_locations.pluck(:id)
+ end
+
+ def destination_location_ids
+ location_ids + [nil]
  end
  end
  end

core/spec/models/spree/permission_sets/promotion_display_spec.rb

@@ -14,21 +14,27 @@
  it { is_expected.to be_able_to(:display, Spree::PromotionRule) }
  it { is_expected.to be_able_to(:display, Spree::PromotionAction) }
  it { is_expected.to be_able_to(:display, Spree::PromotionCategory) }
+ it { is_expected.to be_able_to(:display, Spree::PromotionCode) }
  it { is_expected.to be_able_to(:admin, Spree::Promotion) }
  it { is_expected.to be_able_to(:admin, Spree::PromotionRule) }
  it { is_expected.to be_able_to(:admin, Spree::PromotionAction) }
  it { is_expected.to be_able_to(:admin, Spree::PromotionCategory) }
+ it { is_expected.to be_able_to(:admin, Spree::PromotionCode) }
+ it { is_expected.to be_able_to(:edit, Spree::Promotion) }
  end
 
  context "when not activated" do
  it { is_expected.not_to be_able_to(:display, Spree::Promotion) }
  it { is_expected.not_to be_able_to(:display, Spree::PromotionRule) }
  it { is_expected.not_to be_able_to(:display, Spree::PromotionAction) }
  it { is_expected.not_to be_able_to(:display, Spree::PromotionCategory) }
+ it { is_expected.not_to be_able_to(:display, Spree::PromotionCode) }
  it { is_expected.not_to be_able_to(:admin, Spree::Promotion) }
  it { is_expected.not_to be_able_to(:admin, Spree::PromotionRule) }
  it { is_expected.not_to be_able_to(:admin, Spree::PromotionAction) }
  it { is_expected.not_to be_able_to(:admin, Spree::PromotionCategory) }
+ it { is_expected.not_to be_able_to(:admin, Spree::PromotionCode) }
+ it { is_expected.not_to be_able_to(:edit, Spree::Promotion) }
  end
 end
 

core/spec/models/spree/permission_sets/restricted_transfer_management_spec.rb

@@ -49,21 +49,27 @@
  end
 
  it { is_expected.to be_able_to(:display, Spree::StockItem) }
- it { is_expected.to be_able_to(:display, Spree::StockTransfer) }
  it { is_expected.to be_able_to(:admin, Spree::StockItem) }
- it { is_expected.to be_able_to(:admin, Spree::StockTransfer) }
+
+ it { is_expected.to be_able_to(:display, source_location) }
+ it { is_expected.to be_able_to(:display, destination_location) }
 
  context "when the user is associated with one of the locations" do
  let(:stock_locations) {[source_location]}
 
+ it { is_expected.to be_able_to(:display, Spree::StockTransfer) }
+ it { is_expected.to be_able_to(:admin, Spree::StockTransfer) }
+ it { is_expected.to be_able_to(:create, Spree::StockTransfer) }
+
  it { is_expected.to be_able_to(:update, source_stock_item) }
  it { is_expected.not_to be_able_to(:update, destination_stock_item) }
 
  it { is_expected.to be_able_to(:transfer, source_location) }
  it { is_expected.not_to be_able_to(:transfer, destination_location) }
 
- it { is_expected.to be_able_to(:display, source_location) }
- it { is_expected.not_to be_able_to(:display, destination_location) }
+ it { is_expected.to be_able_to(:display, transfer_with_source) }
+ it { is_expected.to be_able_to(:display, transfer_with_source_and_destination) }
+ it { is_expected.not_to be_able_to(:display, transfer_with_destination) }
 
  it { is_expected.to be_able_to(:manage, transfer_with_source) }
  it { is_expected.not_to be_able_to(:manage, transfer_with_destination) }
@@ -78,14 +84,19 @@
  context "when the user is associated with both locations" do
  let(:stock_locations) {[source_location, destination_location]}
 
+ it { is_expected.to be_able_to(:display, Spree::StockTransfer) }
+ it { is_expected.to be_able_to(:admin, Spree::StockTransfer) }
+ it { is_expected.to be_able_to(:create, Spree::StockTransfer) }
+
  it { is_expected.to be_able_to(:update, source_stock_item) }
  it { is_expected.to be_able_to(:update, destination_stock_item) }
 
  it { is_expected.to be_able_to(:transfer, source_location) }
  it { is_expected.to be_able_to(:transfer, destination_location) }
 
- it { is_expected.to be_able_to(:display, source_location) }
- it { is_expected.to be_able_to(:display, destination_location) }
+ it { is_expected.to be_able_to(:display, transfer_with_source) }
+ it { is_expected.to be_able_to(:display, transfer_with_source_and_destination) }
+ it { is_expected.to be_able_to(:display, transfer_with_destination) }
 
  it { is_expected.to be_able_to(:manage, transfer_with_source) }
  it { is_expected.to be_able_to(:manage, transfer_with_destination) }
@@ -99,6 +110,10 @@
  context "when the user is associated with neither location" do
  let(:stock_locations) {[]}
 
+ it { is_expected.not_to be_able_to(:display, Spree::StockTransfer) }
+ it { is_expected.not_to be_able_to(:admin, Spree::StockTransfer) }
+ it { is_expected.not_to be_able_to(:create, Spree::StockTransfer) }
+
  it { is_expected.not_to be_able_to(:update, source_stock_item) }
  it { is_expected.not_to be_able_to(:update, destination_stock_item) }
 
@@ -119,18 +134,17 @@
  let(:user) { create :user }
 
  it { is_expected.not_to be_able_to(:display, Spree::StockTransfer) }
- it { is_expected.not_to be_able_to(:admin, Spree::StockItem) }
  it { is_expected.not_to be_able_to(:admin, Spree::StockTransfer) }
+ it { is_expected.not_to be_able_to(:create, Spree::StockTransfer) }
 
  it { is_expected.not_to be_able_to(:update, source_stock_item) }
  it { is_expected.not_to be_able_to(:update, destination_stock_item) }
 
  it { is_expected.not_to be_able_to(:transfer, source_location) }
  it { is_expected.not_to be_able_to(:transfer, destination_location) }
 
- it { is_expected.not_to be_able_to(:display, source_location) }
- it { is_expected.not_to be_able_to(:display, destination_location) }
-
+ it { is_expected.to_not be_able_to(:display, source_location) }
+ it { is_expected.to_not be_able_to(:display, destination_location) }
 
  it { is_expected.not_to be_able_to(:manage, transfer_with_source) }
  it { is_expected.not_to be_able_to(:manage, transfer_with_destination) }