Improve the developer experience with the new Ability deprecations

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

@@ -5,13 +5,13 @@
 
 <% content_for :page_actions do %>
   <li>
-    <% if can?(:read, Spree::PromotionCode) %>
+    <% if can?(:show, Spree::PromotionCode) %>
       <%= link_to t('spree.view_promotion_codes_list'), admin_promotion_promotion_codes_path(promotion_id: @promotion.id), class: 'btn btn-primary' %>
 
       <%= link_to t('spree.download_promotion_codes_list'), admin_promotion_promotion_codes_path(promotion_id: @promotion.id, format: :csv), class: 'btn btn-primary' %>
     <% end %>
 
-    <% if can?(:read, Spree::PromotionCodeBatch) %>
+    <% if can?(:show, Spree::PromotionCodeBatch) %>
       <%= link_to plural_resource_name(Spree::PromotionCodeBatch), admin_promotion_promotion_code_batches_path(promotion_id: @promotion.id), class: 'btn btn-primary' %>
     <% end %>
   </li>

backend/app/views/spree/admin/shared/_order_submenu.html.erb

@@ -20,13 +20,13 @@
       <%= link_to plural_resource_name(Spree::Shipment), spree.edit_admin_order_url(@order) %>
     </li>
 
-    <% if can? :read, Spree::Adjustment %>
+    <% if can? :show, Spree::Adjustment %>
       <li class="<%= "active" if current == "Adjustments" %>" data-hook='admin_order_tabs_adjustments'>
         <%= link_to plural_resource_name(Spree::Adjustment), spree.admin_order_adjustments_url(@order) %>
       </li>
     <% end %>
 
-    <% if can?(:read, Spree::Payment) %>
+    <% if can?(:show, Spree::Payment) %>
       <li class="<%= "active" if current == "Payments" %>" data-hook='admin_order_tabs_payments'>
         <%= link_to plural_resource_name(Spree::Payment), spree.admin_order_payments_url(@order) %>
       </li>
@@ -38,15 +38,15 @@
       </li>
     <% end %>
 
-    <% if can? :read, Spree::ReturnAuthorization %>
+    <% if can? :show, Spree::ReturnAuthorization %>
       <% if @order.completed? %>
         <li class="tab <%= "active" if current == "Return Authorizations" %>" data-hook='admin_order_tabs_return_authorizations'>
           <%= link_to t('spree.admin.tab.rma'), spree.admin_order_return_authorizations_url(@order) %>
         </li>
       <% end %>
     <% end %>
 
-    <% if can? :read, Spree::CustomerReturn %>
+    <% if can? :show, Spree::CustomerReturn %>
       <% if @order.completed? %>
         <li class="<%= "active" if current == "Customer Returns" %>" data-hook='admin_order_tabs_customer_returns'>
           <%= link_to plural_resource_name(Spree::CustomerReturn), spree.admin_order_customer_returns_url(@order) %>

backend/app/views/spree/admin/shared/_payments_tabs.html.erb

@@ -1,7 +1,7 @@
 <% content_for :tabs do %>
   <nav>
     <ul class="tabs" data-hook="admin_settings_payments_tabs">
-      <% if can?(:read, Spree::PaymentMethod) %>
+      <% if can?(:show, Spree::PaymentMethod) %>
         <%= settings_tab_item plural_resource_name(Spree::PaymentMethod), spree.admin_payment_methods_path %>
       <% end %>
     </ul>

backend/app/views/spree/admin/shared/_settings_checkout_tabs.html.erb

@@ -1,23 +1,23 @@
 <% content_for :tabs do %>
   <nav>
     <ul class="tabs" data-hook="admin_settings_checkout_tabs">
-      <% if can?(:read, Spree::RefundReason) %>
+      <% if can?(:show, Spree::RefundReason) %>
         <%= settings_tab_item plural_resource_name(Spree::RefundReason), spree.admin_refund_reasons_path %>
       <% end %>
 
-      <% if can?(:read, Spree::ReimbursementType) %>
+      <% if can?(:show, Spree::ReimbursementType) %>
         <%= settings_tab_item plural_resource_name(Spree::ReimbursementType), spree.admin_reimbursement_types_path %>
       <% end %>
 
-      <% if can?(:read, Spree::ReturnReason) %>
+      <% if can?(:show, Spree::ReturnReason) %>
         <%= settings_tab_item plural_resource_name(Spree::ReturnReason), spree.admin_return_reasons_path %>
       <% end %>
 
-      <% if can?(:read, Spree::AdjustmentReason) %>
+      <% if can?(:show, Spree::AdjustmentReason) %>
         <%= settings_tab_item plural_resource_name(Spree::AdjustmentReason), spree.admin_adjustment_reasons_path %>
       <% end %>
 
-      <% if can?(:read, Spree::StoreCreditReason) %>
+      <% if can?(:show, Spree::StoreCreditReason) %>
         <%= settings_tab_item plural_resource_name(Spree::StoreCreditReason), spree.admin_store_credit_reasons_path %>
       <% end %>
     </ul>

backend/app/views/spree/admin/shared/_settings_sub_menu.html.erb

@@ -3,24 +3,24 @@
     <%= tab :stores, label: :stores, url: spree.admin_stores_path %>
   <% end %>
 
-  <% if can?(:read, Spree::PaymentMethod) %>
+  <% if can?(:show, Spree::PaymentMethod) %>
     <%= tab :payments, url: spree.admin_payment_methods_path %>
   <% end %>
 
-  <% if can?(:read, Spree::TaxCategory) || can?(:read, Spree::TaxRate) %>
+  <% if can?(:show, Spree::TaxCategory) || can?(:show, Spree::TaxRate) %>
     <%= tab :taxes, url: spree.admin_tax_categories_path, match_path: %r(tax_categories|tax_rates) %>
   <% end %>
 
-  <% if can?(:read, Spree::RefundReason) || can?(:read, Spree::ReimbursementType) ||
-    can?(:show, Spree::ReturnReason) || can?(:read, Spree::AdjustmentReason) %>
+  <% if can?(:show, Spree::RefundReason) || can?(:show, Spree::ReimbursementType) ||
+    can?(:show, Spree::ReturnReason) || can?(:show, Spree::AdjustmentReason) %>
     <%= tab :checkout, url: spree.admin_refund_reasons_path, match_path: %r(refund_reasons|reimbursement_types|return_reasons|adjustment_reasons|store_credit_reasons) %>
   <% end %>
 
-  <% if can?(:read, Spree::ShippingMethod) || can?(:read, Spree::ShippingCategory) || can?(:read, Spree::StockLocation) %>
+  <% if can?(:show, Spree::ShippingMethod) || can?(:show, Spree::ShippingCategory) || can?(:show, Spree::StockLocation) %>
     <%= tab :shipping, url: spree.admin_shipping_methods_path, match_path: %r(shipping_methods|shipping_categories|stock_locations) %>
   <% end %>
 
-  <% if can?(:read, Spree::Zone) %>
+  <% if can?(:show, Spree::Zone) %>
     <%= tab :zones, url: spree.admin_zones_path %>
   <% end %>
 </ul>

backend/app/views/spree/admin/shared/_shipping_tabs.html.erb

@@ -1,16 +1,16 @@
 <% content_for :tabs do %>
   <nav>
     <ul class="tabs" data-hook="admin_settings_shipping_tabs">
-      <% if can?(:read, Spree::ShippingMethod) %>
+      <% if can?(:show, Spree::ShippingMethod) %>
         <%= settings_tab_item plural_resource_name(Spree::ShippingMethod), spree.admin_shipping_methods_path %>
       <% end %>
 
-      <% if can?(:read, Spree::ShippingCategory) %>
+      <% if can?(:show, Spree::ShippingCategory) %>
         <%= settings_tab_item plural_resource_name(Spree::ShippingCategory), spree.admin_shipping_categories_path %>
 
       <% end %>
 
-      <% if can?(:read, Spree::StockLocation) %>
+      <% if can?(:show, Spree::StockLocation) %>
         <%= settings_tab_item plural_resource_name(Spree::StockLocation), spree.admin_stock_locations_path %>
       <% end %>
     </ul>

backend/app/views/spree/admin/shared/_taxes_tabs.html.erb

@@ -1,11 +1,11 @@
 <% content_for :tabs do %>
   <nav>
     <ul class="tabs" data-hook="admin_settings_taxes_tabs">
-      <% if can? :read, Spree::TaxCategory %>
+      <% if can? :show, Spree::TaxCategory %>
         <%= settings_tab_item plural_resource_name(Spree::TaxCategory), spree.admin_tax_categories_path %>
       <% end %>
 
-      <% if can? :read, Spree::TaxRate %>
+      <% if can? :show, Spree::TaxRate %>
         <%= settings_tab_item plural_resource_name(Spree::TaxRate), spree.admin_tax_rates_path %>
       <% end %>
     </ul>

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

@@ -8,7 +8,7 @@
 
 <% content_for :page_actions do %>
   <ul class="actions inline-menu">
-    <% if can?(:read, Spree::StockMovement) %>
+    <% if can?(:show, Spree::StockMovement) %>
       <li>
         <%= link_to plural_resource_name(Spree::StockMovement), admin_stock_location_stock_movements_path(@stock_location.id), class: 'btn btn-primary'  %>
       </li>

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

@@ -52,7 +52,7 @@
             </span>
           </td>
           <td>
-            <% if can?(:read, Spree::StockMovement) %>
+            <% if can?(:show, Spree::StockMovement) %>
               <%= link_to plural_resource_name(Spree::StockMovement), admin_stock_location_stock_movements_path(stock_location.id) %>
             <% else %>
               Stock Movements

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

@@ -1,6 +1,6 @@
 <% admin_breadcrumb(t('spree.settings')) %>
 <% admin_breadcrumb(t('spree.admin.tab.shipping')) %>
-<% if can?(:read, Spree::StockLocation) %>
+<% if can?(:show, Spree::StockLocation) %>
   <% admin_breadcrumb(link_to plural_resource_name(Spree::StockLocation), spree.admin_stock_locations_path) %>
 <% else %>
   <% admin_breadcrumb(plural_resource_name(Spree::StockLocation)) %>

backend/app/views/spree/admin/users/_form.html.erb

@@ -11,7 +11,7 @@
       <%= error_message_on :user, :email %>
     <% end %>
 
-    <% if can? :read, Spree::Role %>
+    <% if can? :show, Spree::Role %>
       <div data-hook="admin_user_form_roles" class="field">
         <%= label_tag nil, plural_resource_name(Spree::Role) %>
         <ul>
@@ -35,7 +35,7 @@
       </div>
     <% end %>
 
-    <% if Spree::Config[:can_restrict_stock_management] && can?(:read, Spree::StockLocation) %>
+    <% if Spree::Config[:can_restrict_stock_management] && can?(:show, Spree::StockLocation) %>
       <div data-hook="admin_user_form_stock_locations" class="field">
         <%= label_tag nil, plural_resource_name(Spree::StockLocation) %>
         <ul>

backend/app/views/spree/admin/users/_tabs.html.erb

@@ -19,7 +19,7 @@
           <%= link_to t("spree.admin.user.items"), spree.items_admin_user_path(@user) %>
         </li>
       <% end %>
-      <% if can?(:read, @user.store_credits.new) %>
+      <% if can?(:show, @user.store_credits.new) %>
         <li<%== ' class="active"' if current == :store_credits %>>
           <%= link_to t("spree.admin.user.store_credit"), spree.admin_user_store_credits_path(@user) %>
         </li>

core/app/models/spree/ability.rb

@@ -15,7 +15,7 @@ class Ability
     attr_reader :user
 
     CUSTOM_ALIASES_MAP = {
-      destroy: :delete,
+      delete: :destroy,
       display: :read,
       new_action: :create,
       read: :show
@@ -53,20 +53,23 @@ def model_adapter(model_class, action)
     def normalize_action(action)
       return action unless Spree::Config.use_custom_cancancan_actions
 
+      normalized_action = CUSTOM_ALIASES_MAP.fetch(action, action)
+
       if action == :read
-        Spree::Deprecation.warn <<~WARN
+        Spree::Deprecation.warn <<~WARN, caller(3)
           The behavior of CanCanCan `:read` action alias will be changing in Solidus 3.0.
-          The current alias is: `:show, :to => :read`
-          The new alias will be compliant with CanCanCan default: `index, :show, :to => :read`
+          The current alias is: `:show, :to => :read`,
+          the new alias will be compliant with CanCanCan's default: `index, :show, :to => :read`
         WARN
-      elsif action.in? CUSTOM_ALIASES_MAP.keys
-        Spree::Deprecation.warn <<~WARN
-          Calling CanCanCan alias action #{action} is deprecated.
-          In Solidus 3.0 non-standard CanCanCan action aliases will be replaced with default ones
+      elsif CUSTOM_ALIASES_MAP.key? action
+        Spree::Deprecation.warn <<~WARN, caller(3)
+          Calling CanCanCan alias action #{action.inspect} is deprecated.
+          In Solidus 3.0 non-standard CanCanCan action aliases will be replaced with default ones,
+          please replace with #{normalized_action.inspect}.
         WARN
       end
 
-      CUSTOM_ALIASES_MAP.fetch(action, action)
+      normalized_action
     end
 
     # Before, this was the only way to extend this ability. Permission sets have been added since.

core/spec/models/spree/ability_spec.rb

@@ -290,42 +290,42 @@ def initialize(_user)
       ability.can?(:read, Object.new)
 
       expect(Spree::Deprecation).to have_received(:warn)
-        .with(/`:read` action alias will be changing/)
+        .with(a_string_matching(/`:read` action alias will be changing/), any_args)
     end
 
     it 'raises deprecation on display' do
       ability.can?(:display, Object.new)
 
       expect(Spree::Deprecation).to have_received(:warn)
-        .with(/alias action display is deprecated/)
+        .with(a_string_matching(/alias action :display is deprecated/), any_args)
     end
 
     it 'raises deprecation on destroy' do
-      ability.can?(:destroy, Object.new)
+      ability.can?(:delete, Object.new)
 
       expect(Spree::Deprecation).to have_received(:warn)
-        .with(/alias action destroy is deprecated/)
+        .with(a_string_matching(/alias action :delete is deprecated/), any_args)
     end
 
     it 'raises deprecation on :new_action' do
       ability.can?(:new_action, Object.new)
 
       expect(Spree::Deprecation).to have_received(:warn)
-        .with(/alias action new_action is deprecated/)
+        .with(a_string_matching(/alias action :new_action is deprecated/), any_args)
     end
 
     it 'raises deprecation on display' do
       ability.can?(:display, Object.new)
 
       expect(Spree::Deprecation).to have_received(:warn)
-        .with(/alias action display is deprecated/)
+        .with(a_string_matching(/alias action :display is deprecated/), any_args)
     end
 
     it 'raises deprecation when called on models by accessible_by' do
       Spree::Order.accessible_by(ability, :display)
 
       expect(Spree::Deprecation).to have_received(:warn)
-        .with(/alias action display is deprecated/)
+        .with(a_string_matching(/alias action :display is deprecated/), any_args)
     end
   end
 end