Improve TabHelper to clarify the use of Namespaces for the nav_link

4 files changed, 120 insertions, 42 deletions

diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index bb401b03709..ef4560023af 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -13,7 +13,7 @@ module ApplicationHelper
 
   # Check if a particular controller is the current one
   #
-  # args - One or more controller names to check
+  # args - One or more controller names to check (using path notation when inside namespaces)
   #
   # Examples
   #
@@ -21,6 +21,11 @@ module ApplicationHelper
   #   current_controller?(:tree)           # => true
   #   current_controller?(:commits)        # => false
   #   current_controller?(:commits, :tree) # => true
+  #
+  #   # On Admin::ApplicationController
+  #   current_controller?(:application)         # => true
+  #   current_controller?('admin/application')  # => true
+  #   current_controller?('gitlab/application') # => false
   def current_controller?(*args)
     args.any? do |v|
       v.to_s.downcase == controller.controller_name || v.to_s.downcase == controller.controller_path
diff --git a/app/helpers/tab_helper.rb b/app/helpers/tab_helper.rb
index e310fda51d7..d91f0f78db7 100644
--- a/app/helpers/tab_helper.rb
+++ b/app/helpers/tab_helper.rb
@@ -8,7 +8,7 @@ module TabHelper
   # element is the value passed to the block.
   #
   # options - The options hash used to determine if the element is "active" (default: {})
-  #           :controller   - One or more controller names to check (optional).
+  #           :controller   - One or more controller names to check, use path notation when namespaced (optional).
   #           :action       - One or more action names to check (optional).
   #           :path         - A shorthand path, such as 'dashboard#index', to check (optional).
   #           :html_options - Extra options to be passed to the list element (optional).
@@ -42,6 +42,20 @@ module TabHelper
   #   nav_link(controller: :tree, html_options: {class: 'home'}) { "Hello" }
   #   # => '<li class="home active">Hello</li>'
   #
+  #   # For namespaced controllers like Admin::AppearancesController#show
+  #
+  #   # Controller and namespace matches
+  #   nav_link(controller: 'admin/appearances') { "Hello" }
+  #   # => '<li class="active">Hello</li>'
+  #
+  #   # Controller and namespace matches but action doesn't
+  #   nav_link(controller: 'admin/appearances', action: :edit) { "Hello" }
+  #   # => '<li>Hello</li>'
+  #
+  #   # Shorthand path with namespace
+  #   nav_link(path: 'admin/appearances#show') { "Hello"}
+  #   # => '<li class="active">Hello</li>'
+  #
   # Returns a list item element String
   def nav_link(options = {}, &block)
     klass = active_nav_link?(options) ? 'active' : ''
diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb
index 14297a1a544..1238cfbd1e7 100644
--- a/spec/helpers/application_helper_spec.rb
+++ b/spec/helpers/application_helper_spec.rb
@@ -3,48 +3,66 @@ require 'spec_helper'
 
 describe ApplicationHelper do
   describe 'current_controller?' do
-    it 'returns true when controller matches argument' do
+    before do
       stub_controller_name('foo')
+    end
 
-      expect(helper.current_controller?(:foo)).to eq true
+    it 'returns true when controller matches argument' do
+      expect(helper.current_controller?(:foo)).to be_truthy
     end
 
     it 'returns false when controller does not match argument' do
-      stub_controller_name('foo')
-
-      expect(helper.current_controller?(:bar)).to eq false
+      expect(helper.current_controller?(:bar)).to be_falsey
     end
 
     it 'takes any number of arguments' do
-      stub_controller_name('foo')
+      expect(helper.current_controller?(:baz, :bar)).to be_falsey
+      expect(helper.current_controller?(:baz, :bar, :foo)).to be_truthy
+    end
+
+    context 'when namespaced' do
+      before do
+        stub_controller_path('bar/foo')
+      end
+
+      it 'returns true when controller matches argument' do
+        expect(helper.current_controller?(:foo)).to be_truthy
+      end
 
-      expect(helper.current_controller?(:baz, :bar)).to eq false
-      expect(helper.current_controller?(:baz, :bar, :foo)).to eq true
+      it 'returns true when controller and namespace matches argument in path notation' do
+        expect(helper.current_controller?('bar/foo')).to be_truthy
+      end
+
+      it 'returns false when namespace doesnt match' do
+        expect(helper.current_controller?('foo/foo')).to be_falsey
+      end
     end
 
     def stub_controller_name(value)
       allow(helper.controller).to receive(:controller_name).and_return(value)
     end
+
+    def stub_controller_path(value)
+      allow(helper.controller).to receive(:controller_path).and_return(value)
+    end
   end
 
   describe 'current_action?' do
-    it 'returns true when action matches' do
+    before do
       stub_action_name('foo')
+    end
 
-      expect(helper.current_action?(:foo)).to eq true
+    it 'returns true when action matches' do
+      expect(helper.current_action?(:foo)).to be_truthy
     end
 
     it 'returns false when action does not match' do
-      stub_action_name('foo')
-
-      expect(helper.current_action?(:bar)).to eq false
+      expect(helper.current_action?(:bar)).to be_falsey
     end
 
     it 'takes any number of arguments' do
-      stub_action_name('foo')
-
-      expect(helper.current_action?(:baz, :bar)).to eq false
-      expect(helper.current_action?(:baz, :bar, :foo)).to eq true
+      expect(helper.current_action?(:baz, :bar)).to be_falsey
+      expect(helper.current_action?(:baz, :bar, :foo)).to be_truthy
     end
 
     def stub_action_name(value)
@@ -100,8 +118,7 @@ describe ApplicationHelper do
     end
 
     it 'accepts a custom html_class' do
-      expect(element(html_class: 'custom_class').attr('class'))
-        .to eq 'js-timeago custom_class'
+      expect(element(html_class: 'custom_class').attr('class')).to eq 'js-timeago custom_class'
     end
 
     it 'accepts a custom tooltip placement' do
@@ -114,6 +131,7 @@ describe ApplicationHelper do
 
     it 'add class for the short format' do
       timeago_element = element(short_format: 'short')
+
       expect(timeago_element.attr('class')).to eq 'js-short-timeago'
       expect(timeago_element.next_element).to eq nil
     end
@@ -128,11 +146,9 @@ describe ApplicationHelper do
     context 'when alternate support url is specified' do
       let(:alternate_url) { 'http://company.example.com/getting-help' }
 
-      before do
+      it 'returns the alternate support url' do
         stub_application_setting(help_page_support_url: alternate_url)
-      end
 
-      it 'returns the alternate support url' do
         expect(helper.support_url).to eq(alternate_url)
       end
     end
@@ -155,9 +171,12 @@ describe ApplicationHelper do
   describe '#autocomplete_data_sources' do
     let(:project) { create(:project) }
     let(:noteable_type) { Issue }
+
     it 'returns paths for autocomplete_sources_controller' do
       sources = helper.autocomplete_data_sources(project, noteable_type)
+
       expect(sources.keys).to match_array([:members, :issues, :mergeRequests, :labels, :milestones, :commands])
+
       sources.keys.each do |key|
         expect(sources[key]).not_to be_nil
       end
diff --git a/spec/helpers/tab_helper_spec.rb b/spec/helpers/tab_helper_spec.rb
index b473c0a7416..9abf63d4bd4 100644
--- a/spec/helpers/tab_helper_spec.rb
+++ b/spec/helpers/tab_helper_spec.rb
@@ -9,31 +9,71 @@ describe TabHelper do
       allow(self).to receive(:action_name).and_return('foo')
     end
 
-    it "captures block output" do
-      expect(nav_link { "Testing Blocks" }).to match(/Testing Blocks/)
+    context 'with the content of the li' do
+      it "captures block output" do
+        expect(nav_link { "Testing Blocks" }).to match(/Testing Blocks/)
+      end
     end
 
-    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/)
-    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/)
+      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
+      end
+
+      context 'with namespace in path notation' do
+        before do
+          allow(controller).to receive(:controller_path).and_return('bar/foo')
+        end
 
-    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/)
+        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
+        end
+      end
     end
 
-    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/)
+    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
     end
 
-    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/)
+    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/)
+      end
+
+      context 'with namespace' do
+        before do
+          allow(controller).to receive(:controller_path).and_return('bar/foo')
+        end
+
+        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/)
+        end
+      end
     end
 
     it "passes extra html options to the list element" do