Merge pull request #5309 from solidusio/elia/menu-items-cleanup

[Backend] cleanup `MenuItems` generation

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

@@ -42,45 +42,49 @@ def admin_page_title
       # Make an admin tab that covers one or more resources supplied by symbols
       # Option hash may follow. Valid options are
       #   * :label to override link text, otherwise based on the first resource name (translated)
-      #   * :route to override automatically determining the default route
       #   * :match_path as an alternative way to control when the tab is active, /products would match /admin/products, /admin/products/5/variants etc.
       #   * :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)
-        options = { label: args.first.to_s }
+      #   * :selected to explicitly control whether the tab is active
+      def tab(*args, &block)
+        options = args.last.is_a?(Hash) ? args.pop.dup : {}
+        css_classes = Array(options[:css_class])
+
+        if options.key?(:route)
+          Spree::Deprecation.warn "Passing a route to #tab is deprecated. Please pass a url instead."
+          options[:url] ||= spree.send("#{options[:route]}_path")
+        end
 
-        if args.last.is_a?(Hash)
-          options = options.merge(args.pop)
+        if args.any?
+          Spree::Deprecation.warn "Passing resources to #tab is deprecated. Please use the `label:` and `match_path:` options instead."
+          options[:label] ||= args.first
+          options[:url] ||= spree.send("admin_#{args.first}_path")
+          options[:selected] = args.include?(controller.controller_name.to_sym)
         end
-        options[:route] ||= "admin_#{args.first}"
 
-        destination_url = options[:url] || spree.send("#{options[:route]}_path")
+        options[:url] ||= spree.send("admin_#{options[:label]}_path")
         label = t(options[:label], scope: [:spree, :admin, :tab])
 
-        css_classes = []
+        options[: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?(options[:url])
+          end
+
+        css_classes << 'selected' if options[:selected]
 
         if options[:icon]
-          link = link_to_with_icon(options[:icon], label, destination_url)
+          link = link_to_with_icon(options[:icon], label, options[:url])
           css_classes << 'tab-with-icon'
         else
-          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
-        css_classes << 'selected' if selected
-
-        if options[:css_class]
-          css_classes << options[:css_class]
+          link = link_to(label, options[:url])
         end
-        content_tag('li', link + (yield if block_given?), class: css_classes.join(' ') )
+        block_content = capture(&block) if block_given?
+        content_tag('li', link + block_content.to_s, class: css_classes.join(' ') )
       end
 
       def link_to_clone(resource, options = {})

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

@@ -1,17 +1,19 @@
+<% Spree::Deprecation.warn "Using the #{@virtual_path.inspect} partial is deprecated, please use MenuItem#children instead." %>
+
 <ul class="admin-subnav" data-hook="admin_product_sub_tabs">
   <% if can? :admin, Spree::Product %>
-    <%= tab :products, match_path: '/products' %>
+    <%= tab label: :products, match_path: '/products' %>
   <% end %>
   <% if can? :admin, Spree::OptionType %>
-    <%= tab :option_types, match_path: '/option_types' %>
+    <%= tab label: :option_types, match_path: '/option_types' %>
   <% end %>
   <% if can? :admin, Spree::Property %>
-    <%= tab :properties %>
+    <%= tab label: :properties %>
   <% end %>
   <% if can? :admin, Spree::Taxonomy %>
-    <%= tab :taxonomies %>
+    <%= tab label: :taxonomies %>
   <% end %>
   <% if can? :admin, Spree::Taxon %>
-    <%= tab :taxons, label: :display_order, match_path: '/taxons' %>
+    <%= tab url: spree.admin_taxons_path, label: :display_order, match_path: '/taxons' %>
   <% end %>
 </ul>

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

@@ -1,8 +1,10 @@
+<% Spree::Deprecation.warn "Using the #{@virtual_path.inspect} partial is deprecated, please use MenuItem#children instead." %>
+
 <ul class="admin-subnav" data-hook="admin_promotion_sub_tabs">
   <% if can? :admin, Spree::Promotion %>
-    <%= tab :promotions %>
+    <%= tab label: :promotions %>
   <% end %>
   <% if can? :admin, Spree::PromotionCategory %>
-    <%= tab :promotion_categories %>
+    <%= tab label: :promotion_categories %>
   <% end %>
 </ul>

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

@@ -1,26 +1,28 @@
+<% Spree::Deprecation.warn "Using the #{@virtual_path.inspect} partial is deprecated, please use MenuItem#children instead." %>
+
 <ul class="admin-subnav" data-hook="admin_settings_sub_tabs">
   <% if can?(:admin, Spree::Store) %>
-    <%= tab :stores, label: :stores, url: spree.admin_stores_path %>
+    <%= tab label: :stores, url: spree.admin_stores_path %>
   <% end %>
 
   <% if can?(:admin, Spree::PaymentMethod) %>
-    <%= tab :payments, url: spree.admin_payment_methods_path %>
+    <%= tab label: :payments, url: spree.admin_payment_methods_path %>
   <% end %>
 
   <% if can?(:admin, Spree::TaxCategory) || can?(:admin, Spree::TaxRate) %>
-    <%= tab :taxes, url: spree.admin_tax_categories_path, match_path: %r(tax_categories|tax_rates) %>
+    <%= tab label: :taxes, url: spree.admin_tax_categories_path, match_path: %r(tax_categories|tax_rates) %>
   <% end %>
 
   <% if can?(:admin, Spree::RefundReason) || can?(:admin, 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) %>
+    <%= tab label: :checkout, url: spree.admin_refund_reasons_path, match_path: %r(refund_reasons|reimbursement_types|return_reasons|adjustment_reasons|store_credit_reasons) %>
   <% end %>
 
   <% if can?(:admin, Spree::ShippingMethod) || can?(:admin, Spree::ShippingCategory) || can?(:admin, Spree::StockLocation) %>
-    <%= tab :shipping, url: spree.admin_shipping_methods_path, match_path: %r(shipping_methods|shipping_categories|stock_locations) %>
+    <%= tab label: :shipping, url: spree.admin_shipping_methods_path, match_path: %r(shipping_methods|shipping_categories|stock_locations) %>
   <% end %>
 
   <% if can?(:admin, Spree::Zone) %>
-    <%= tab :zones, url: spree.admin_zones_path %>
+    <%= tab label: :zones, url: spree.admin_zones_path %>
   <% end %>
 </ul>

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

@@ -1,15 +1,27 @@
-<% Spree::Backend::Config.menu_items.sort_by { |item| item.position || Float::INFINITY }.each do |menu_item| %>
-  <% if instance_exec(&menu_item.condition) %>
+<% Spree::Backend::Config.menu_items.each do |menu_item| %>
+  <% if menu_item.render_in?(self) %>
     <%=
       tab(
-        *menu_item.sections,
         icon: menu_item.icon,
         label: menu_item.label,
-        url: menu_item.url.is_a?(Symbol) ? spree.public_send(menu_item.url) : menu_item.url,
-        match_path: menu_item.match_path,
+        url: menu_item.url,
+        selected: menu_item.match_path?(request) || menu_item.children.any? { _1.match_path?(request) },
       ) do
     %>
-      <%- render partial: menu_item.partial if menu_item.partial %>
+      <% if menu_item.render_partial? %>
+        <%- render partial: menu_item.partial %>
+      <% elsif menu_item.children.present? %>
+        <ul class="admin-subnav" data-hook="<%= menu_item.data_hook %>">
+          <%- menu_item.children.each do |child| %>
+            <%= tab(
+              icon: child.icon,
+              label: child.label,
+              url: child.url,
+              selected: child.match_path?(request),
+            ) if child.render_in?(self) %>
+          <% end %>
+        </ul>
+      <% end %>
     <%- end %>
   <% end %>
 <% end %>