Deprecate MenuItem#sections in the backend navigation

Also moves `#icon` from a sequential argument to a keyword argument.

The list of sections was only used to match the current path, because
top-level menu items should be active whenever any of the children
items are active.

backend/app/helpers/spree/admin/navigation_helper.rb

@@ -47,17 +47,21 @@ def admin_page_title
       #   * :match_path can also be a callable that takes a request and determines whether the menu item is selected for the request.
       def tab(*args, &block)
         block_content = capture(&block) if block_given?
-        options = { label: args.first.to_s }
+        options = args.last.is_a?(Hash) ? args.pop : {}
+        css_classes = []
 
-        if args.last.is_a?(Hash)
-          options = options.merge(args.pop)
+        if args.any?
+          Spree::Deprecation.warn "Passing resources to #{self.class.name} is deprecated. Please use the `label:` and `match_path:` options instead."
+          options[:label] ||= args.first
+          options[:route] ||= "admin_#{args.first}"
+          selected = args.include?(controller.controller_name.to_sym)
         end
-        options[:route] ||= "admin_#{args.first}"
+
+        options[:route] ||= "admin_#{options[:label]}"
 
         destination_url = options[:url] || spree.send("#{options[:route]}_path")
         label = t(options[:label], scope: [:spree, :admin, :tab])
 
-        css_classes = []
 
         if options[:icon]
           link = link_to_with_icon(options[:icon], label, destination_url)
@@ -66,16 +70,17 @@ def tab(*args, &block)
           link = link_to(label, destination_url)
         end
 
-        selected = if options[:match_path].is_a? Regexp
-          request.fullpath =~ options[:match_path]
-        elsif options[:match_path].respond_to?(:call)
-          options[:match_path].call(request)
-        elsif options[:match_path]
-          request.fullpath.starts_with?("#{spree.admin_path}#{options[:match_path]}")
-        else
-          request.fullpath.starts_with?(destination_url) ||
-            args.include?(controller.controller_name.to_sym)
-        end
+        selected ||=
+          if options[:match_path].is_a? Regexp
+            request.fullpath =~ options[:match_path]
+          elsif options[:match_path].respond_to?(:call)
+            options[:match_path].call(request)
+          elsif options[:match_path]
+            request.fullpath.starts_with?("#{spree.admin_path}#{options[:match_path]}")
+          else
+            request.fullpath.starts_with?(destination_url)
+          end
+
         css_classes << 'selected' if selected
 
         if options[:css_class]

backend/lib/spree/backend_configuration.rb

@@ -53,9 +53,12 @@ def theme_path(user_theme = nil)
     #
     # Spree::Backend::Config.configure do |config|
     #   config.menu_items << config.class::MenuItem.new(
-    #     [:section],
-    #     'icon-name',
-    #     url: 'https://solidus.io/'
+    #     label: :my_reports,
+    #     icon: 'file-text-o', # see https://fontawesome.com/v4/icons/
+    #     url: :my_admin_reports_path,
+    #     condition: -> { can?(:admin, MyReports) },
+    #     partial: 'spree/admin/shared/my_reports_sub_menu',
+    #     match_path: '/reports',
     #   )
     # end
     #
@@ -75,45 +78,49 @@ def theme_path(user_theme = nil)
     def menu_items
       @menu_items ||= [
         MenuItem.new(
-          ORDER_TABS,
-          'shopping-cart',
+          label: :orders,
+          icon: 'shopping-cart',
           condition: -> { can?(:admin, Spree::Order) },
+          match_path: %r{/(#{ORDER_TABS.join('|')})},
           position: 0
         ),
         MenuItem.new(
-          PRODUCT_TABS,
-          'th-large',
+          label: :products,
+          icon: 'th-large',
           condition: -> { can?(:admin, Spree::Product) },
+          match_path: %r{/(#{PRODUCT_TABS.join('|')})},
           partial: 'spree/admin/shared/product_sub_menu',
           position: 1
         ),
         MenuItem.new(
-          PROMOTION_TABS,
-          'bullhorn',
+          label: :promotions,
+          icon: 'bullhorn',
+          match_path: %r{/(#{PROMOTION_TABS.join('|')})},
           partial: 'spree/admin/shared/promotion_sub_menu',
           condition: -> { can?(:admin, Spree::Promotion) },
           url: :admin_promotions_path,
           position: 2
         ),
         MenuItem.new(
-          STOCK_TABS,
-          'cubes',
-          condition: -> { can?(:admin, Spree::StockItem) },
           label: :stock,
+          icon: 'cubes',
+          match_path: %r{/(#{STOCK_TABS.join('|')})},
+          condition: -> { can?(:admin, Spree::StockItem) },
           url: :admin_stock_items_path,
-          match_path: '/stock_items',
           position: 3
         ),
         MenuItem.new(
-          USER_TABS,
-          'user',
+          label: :users,
+          icon: 'user',
+          match_path: %r{/(#{USER_TABS.join('|')})},
           condition: -> { Spree.user_class && can?(:admin, Spree.user_class) },
           url: :admin_users_path,
           position: 4
         ),
         MenuItem.new(
-          CONFIGURATION_TABS,
-          'wrench',
+          label: :settings,
+          icon: 'wrench',
+          match_path: %r{/(#{CONFIGURATION_TABS.join('|')})},
           condition: -> {
             can?(:admin, Spree::Store) ||
             can?(:admin, Spree::AdjustmentReason) ||
@@ -128,7 +135,6 @@ def menu_items
             can?(:admin, Spree::ReturnReason) ||
             can?(:admin, Spree::Zone)
           },
-          label: :settings,
           partial: 'spree/admin/shared/settings_sub_menu',
           url: :admin_stores_path,
           position: 5

backend/lib/spree/backend_configuration/menu_item.rb

@@ -4,12 +4,13 @@ module Spree
   class BackendConfiguration < Preferences::Configuration
     # An item which should be drawn in the admin menu
     class MenuItem
-      attr_reader :icon, :label, :partial, :condition, :sections, :match_path
+      attr_reader :icon, :label, :partial, :condition, :match_path
+
+      attr_reader :sections
+      deprecate sections: :label, deprecator: Spree::Deprecation
 
       attr_accessor :position
 
-      # @param sections [Array<Symbol>] The sections which are contained within
-      #   this admin menu section.
       # @param icon [String] The icon to draw for this menu item
       # @param condition [Proc] A proc which returns true if this menu item
       #   should be drawn. If nil, it will be replaced with a proc which always
@@ -25,20 +26,28 @@ class MenuItem
       #   you can pass a String, Regexp or callable to identify this menu item. The callable
       #   accepts a request object and returns a Boolean value.
       def initialize(
-        sections,
-        icon,
+        *args,
+        icon: nil,
         condition: nil,
         label: nil,
         partial: nil,
         url: nil,
         position: nil,
         match_path: nil
       )
+        if args.length == 2
+          sections, icon = args
+          label ||= sections.first.to_s
+          Spree::Deprecation.warn "Passing sections to #{self.class.name} is deprecated. Please pass a label instead."
+          Spree::Deprecation.warn "Passing icon to #{self.class.name} is deprecated. Please use the keyword argument instead."
+        elsif args.any?
+          raise ArgumentError, "wrong number of arguments (given #{args.length}, expected 0..2)"
+        end
 
         @condition = condition || -> { true }
         @sections = sections
         @icon = icon
-        @label = label || sections.first
+        @label = label
         @partial = partial
         @url = url
         @position = position

backend/spec/helpers/admin/navigation_helper_spec.rb

@@ -8,26 +8,33 @@
   end
 
   describe "#tab" do
-    context "creating an admin tab" do
+    context "deprecated usage", :silence_deprecations do
+      it "should capitalize the first letter of each word in the tab's label (deprecated)", :silence_deprecations do
+        subject = helper.tab(:orders)
+        expect(subject).to include("Orders")
+      end
+
+      it "should be selected if the controller matches" do
+        allow(controller).to receive(:controller_name).and_return("orders")
+        expect(helper.tab(:orders)).to include('class="selected"')
+      end
+
+      it "should not be selected if the controller does not match" do
+        allow(controller).to receive(:controller_name).and_return("bonobos")
+        expect(helper.tab(:orders)).not_to include('class="selected"')
+      end
+    end
+
+    context "creating an admin tab", :focus do
       it "should capitalize the first letter of each word in the tab's label" do
-        admin_tab = helper.tab(:orders)
+        admin_tab = helper.tab(label: :orders)
         expect(admin_tab).to include("Orders")
       end
     end
 
     describe "selection" do
       context "when match_path option is not supplied" do
-        subject(:tab) { helper.tab(:orders) }
-
-        it "should be selected if the controller matches" do
-          allow(controller).to receive(:controller_name).and_return("orders")
-          expect(subject).to include('class="selected"')
-        end
-
-        it "should not be selected if the controller does not match" do
-          allow(controller).to receive(:controller_name).and_return("bonobos")
-          expect(subject).not_to include('class="selected"')
-        end
+        subject(:tab) { helper.tab(label: :orders) }
 
         it "should be selected if the current path" do
           allow(helper).to receive(:request).and_return(double(ActionDispatch::Request, fullpath: "/admin/orders"))
@@ -47,30 +54,30 @@
 
         it "should be selected if the fullpath matches" do
           allow(controller).to receive(:controller_name).and_return("bonobos")
-          tab = helper.tab(:orders, match_path: '/orders')
+          tab = helper.tab(label: :orders, match_path: '/orders')
           expect(tab).to include('class="selected"')
         end
 
         it "should be selected if the fullpath matches a regular expression" do
           allow(controller).to receive(:controller_name).and_return("bonobos")
-          tab = helper.tab(:orders, match_path: /orders$|orders\//)
+          tab = helper.tab(label: :orders, match_path: /orders$|orders\//)
           expect(tab).to include('class="selected"')
         end
 
         it "should not be selected if the fullpath does not match" do
           allow(controller).to receive(:controller_name).and_return("bonobos")
-          tab = helper.tab(:orders, match_path: '/shady')
+          tab = helper.tab(label: :orders, match_path: '/shady')
           expect(tab).not_to include('class="selected"')
         end
 
         it "should not be selected if the fullpath does not match a regular expression" do
           allow(controller).to receive(:controller_name).and_return("bonobos")
-          tab = helper.tab(:orders, match_path: /shady$|shady\//)
+          tab = helper.tab(label: :orders, match_path: /shady$|shady\//)
           expect(tab).not_to include('class="selected"')
         end
 
         context "when the match_path is a callable" do
-          subject { helper.tab(:orders, match_path: match_path) }
+          subject { helper.tab(label: :orders, match_path: match_path) }
 
           context "when the callable returns false" do
             let(:match_path) { ->(_request) { false } }
@@ -88,7 +95,7 @@
     end
 
     it "should accept a block of content to append" do
-      admin_tab = helper.tab(:orders){ 'foo' }
+      admin_tab = helper.tab(label: :orders){ 'foo' }
       expect(admin_tab).to end_with("foo</li>")
     end
   end

backend/spec/spec_helper.rb

@@ -45,6 +45,7 @@
 require 'spree/testing_support/translations'
 require 'spree/testing_support/job_helpers'
 require 'spree/testing_support/blacklist_urls'
+require 'spree/testing_support/silence_deprecations'
 
 require 'capybara-screenshot/rspec'
 Capybara.save_path = ENV['CIRCLE_ARTIFACTS'] if ENV['CIRCLE_ARTIFACTS']