Merge branch '326304-fj-refactor-tab-helper' into 'master'

Increase functionality of nav_link helper method

See merge request gitlab-org/gitlab!57812

app/helpers/tab_helper.rb

@@ -65,6 +65,13 @@ module TabHelper
   #   # When `TreeController#index` is requested
   #   # => '<li>Hello</li>'
   #
+  #   # Paths, controller and actions can be used at the same time
+  #   nav_link(path: 'tree#show', controller: 'admin/appearances') { "Hello" }
+  #
+  #   nav_link(path: 'foo#bar', controller: 'tree') { "Hello" }
+  #   nav_link(path: 'foo#bar', controller: 'tree', action: 'show') { "Hello" }
+  #   nav_link(path: 'foo#bar', action: 'show') { "Hello" }
+  #
   # Returns a list item element String
   def nav_link(options = {}, &block)
     klass = active_nav_link?(options) ? 'active' : ''
@@ -85,34 +92,12 @@ module TabHelper
   def active_nav_link?(options)
     return false if options[:unless]&.call
 
-    if path = options.delete(:path)
-      unless path.respond_to?(:each)
-        path = [path]
-      end
-
-      path.any? do |single_path|
-        current_path?(single_path)
-      end
-    elsif page = options.delete(:page)
-      unless page.respond_to?(:each)
-        page = [page]
-      end
-
-      page.any? do |single_page|
-        current_page?(single_page)
-      end
-    else
-      c = options.delete(:controller)
-      a = options.delete(:action)
+    controller = options.delete(:controller)
+    action = options.delete(:action)
 
-      if c && a
-        # When given both options, make sure BOTH are true
-        current_controller?(*c) && current_action?(*a)
-      else
-        # Otherwise check EITHER option
-        current_controller?(*c) || current_action?(*a)
-      end
-    end
+    route_matches_paths?(options.delete(:path)) ||
+      route_matches_pages?(options.delete(:page)) ||
+      route_matches_controllers_and_or_actions?(controller, action)
   end
 
   def current_path?(path)
@@ -127,4 +112,26 @@ module TabHelper
       'active'
     end
   end
+
+  private
+
+  def route_matches_paths?(paths)
+    Array(paths).compact.any? do |single_path|
+      current_path?(single_path)
+    end
+  end
+
+  def route_matches_pages?(pages)
+    Array(pages).compact.any? do |single_page|
+      current_page?(single_page)
+    end
+  end
+
+  def route_matches_controllers_and_or_actions?(controller, action)
+    if controller && action
+      current_controller?(*controller) && current_action?(*action)
+    else
+      current_controller?(*controller) || current_action?(*action)
+    end
+  end
 end

spec/helpers/tab_helper_spec.rb

@@ -6,81 +6,94 @@ RSpec.describe TabHelper do
   include ApplicationHelper
 
   describe 'nav_link' do
+    using RSpec::Parameterized::TableSyntax
+
     before do
       allow(controller).to receive(:controller_name).and_return('foo')
       allow(self).to receive(:action_name).and_return('foo')
     end
 
     context 'with the content of the li' do
-      it "captures block output" do
+      it 'captures block output' do
         expect(nav_link { "Testing Blocks" }).to match(/Testing Blocks/)
       end
     end
 
-    context 'with controller param' do
-      it "performs checks on the current controller" do
-        expect(nav_link(controller: :foo)).to match(/<li class="active">/)
-        expect(nav_link(controller: :bar)).not_to match(/active/)
-        expect(nav_link(controller: [:foo, :bar])).to match(/active/)
+    it 'passes extra html options to the list element' do
+      expect(nav_link(action: :foo, html_options: { class: 'home' })).to match(/<li class="home active">/)
+      expect(nav_link(html_options: { class: 'active' })).to match(/<li class="active">/)
     end
 
-      context 'with action param' do
-        it "performs checks on both controller and action when both are present" do
-          expect(nav_link(controller: :bar, action: :foo)).not_to match(/active/)
-          expect(nav_link(controller: :foo, action: :bar)).not_to match(/active/)
-          expect(nav_link(controller: :foo, action: :foo)).to match(/active/)
-        end
+    where(:controller_param, :action_param, :path_param, :active) do
+      nil          | nil          | nil                    | false
+      :foo         | nil          | nil                    | true
+      :bar         | nil          | nil                    | false
+      :bar         | :foo         | nil                    | false
+      :foo         | :bar         | nil                    | false
+      :foo         | :foo         | nil                    | true
+      :bar         | nil          | 'foo#foo'              | true
+      :bar         | nil          | ['foo#foo', 'bar#bar'] | true
+      :bar         | :bar         | ['foo#foo', 'bar#bar'] | true
+      :foo         | nil          | 'bar#foo'              | true
+      :bar         | nil          | 'bar#foo'              | false
+      :foo         | [:foo, :bar] | 'bar#foo'              | true
+      :bar         | :bar         | 'foo#foo'              | true
+      :foo         | :foo         | 'bar#foo'              | true
+      :bar         | :foo         | 'bar#foo'              | false
+      :foo         | :bar         | 'bar#foo'              | false
+      [:foo, :bar] | nil          | nil                    | true
+      [:foo, :bar] | nil          | 'bar#foo'              | true
+      [:foo, :bar] | :foo         | 'bar#foo'              | true
+      nil          | :foo         | nil                    | true
+      nil          | :bar         | nil                    | false
+      nil          | nil          | 'foo#bar'              | false
+      nil          | nil          | 'foo#foo'              | true
+      nil          | :bar         | ['foo#foo', 'bar#bar'] | true
+      nil          | :bar         | 'foo#foo'              | true
+      nil          | :foo         | 'bar#foo'              | true
+      nil          | [:foo, :bar] | nil                    | true
+      nil          | [:foo, :bar] | 'bar#foo'              | true
+      nil          | :bar         | 'bar#foo'              | false
     end
 
-      context 'with namespace in path notation' do
-        before do
-          allow(controller).to receive(:controller_path).and_return('bar/foo')
-        end
+    with_them do
+      specify do
+        result = nav_link(controller: controller_param, action: action_param, path: path_param)
 
-        it 'performs checks on both controller and namespace' do
-          expect(nav_link(controller: 'foo/foo')).not_to match(/active/)
-          expect(nav_link(controller: 'bar/foo')).to match(/active/)
-        end
-
-        context 'with action param' do
-          it "performs checks on both namespace, controller and action when they are all present" do
-            expect(nav_link(controller: 'foo/foo', action: :foo)).not_to match(/active/)
-            expect(nav_link(controller: 'bar/foo', action: :bar)).not_to match(/active/)
-            expect(nav_link(controller: 'bar/foo', action: :foo)).to match(/active/)
-          end
+        if active
+          expect(result).to match(/active/)
+        else
+          expect(result).not_to match(/active/)
         end
       end
     end
 
-    context 'with action param' do
-      it "performs checks on the current action" do
-        expect(nav_link(action: :foo)).to match(/<li class="active">/)
-        expect(nav_link(action: :bar)).not_to match(/active/)
-        expect(nav_link(action: [:foo, :bar])).to match(/active/)
-      end
+    context 'with namespace in path notation' do
+      before do
+        allow(controller).to receive(:controller_path).and_return('bar/foo')
       end
 
-    context 'with path param' do
-      it "accepts a path shorthand" do
-        expect(nav_link(path: 'foo#bar')).not_to match(/active/)
-        expect(nav_link(path: 'foo#foo')).to match(/active/)
+      where(:controller_param, :action_param, :path_param, :active) do
+        'foo/foo' | nil  | nil           | false
+        'bar/foo' | nil  | nil           | true
+        'foo/foo' | :foo | nil           | false
+        'bar/foo' | :bar | nil           | false
+        'bar/foo' | :foo | nil           | true
+        nil       | nil  | 'foo/foo#foo' | false
+        nil       | nil  | 'bar/foo#foo' | true
       end
 
-      context 'with namespace' do
-        before do
-          allow(controller).to receive(:controller_path).and_return('bar/foo')
-        end
+      with_them do
+        specify do
+          result = nav_link(controller: controller_param, action: action_param, path: path_param)
 
-        it 'accepts a path shorthand with namespace' do
-          expect(nav_link(path: 'bar/foo#foo')).to match(/active/)
-          expect(nav_link(path: 'foo/foo#foo')).not_to match(/active/)
+          if active
+            expect(result).to match(/active/)
+          else
+            expect(result).not_to match(/active/)
           end
         end
       end
-
-    it "passes extra html options to the list element" do
-      expect(nav_link(action: :foo, html_options: { class: 'home' })).to match(/<li class="home active">/)
-      expect(nav_link(html_options: { class: 'active' })).to match(/<li class="active">/)
     end
   end
 end