Merge branch 'fix/13356-issuable-index-of-total-in-sidebar' into 'master'

Fix the "x of y" displayed at the top of Issuables' sidebar

1. We now display the index of the current issuable among all its project's
issuables, of the same type and with the same state.
1. Also, refactored a bit the Issuable helpers into a new `IssuablesHelper`
module.
1. Added acceptance specs for the sidebar counter.

Note: I didn't add a CHANGELOG item since it's a bug fix for an unreleased version.

Fixes #13356.

See merge request !2818

8 files changed, 84 insertions, 95 deletions

diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index 02357e2f23e..ecefa9b006d 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -280,76 +280,6 @@ module ApplicationHelper
     end
   end
 
-  def issuable_link_next(project,issuable)
-    if project.nil?
-      nil
-    elsif current_controller?(:issues)
-      namespace_project_issue_path(project.namespace, project, next_issuable_for(project, issuable.id).try(:iid))
-    elsif current_controller?(:merge_requests)
-      namespace_project_merge_request_path(project.namespace, project, next_issuable_for(project, issuable.id).try(:iid))
-    end
-  end
-
-  def issuable_link_prev(project,issuable)
-    if project.nil?
-      nil
-    elsif current_controller?(:issues)
-      namespace_project_issue_path(project.namespace, project, prev_issuable_for(project, issuable.id).try(:iid))
-    elsif current_controller?(:merge_requests)
-      namespace_project_merge_request_path(project.namespace, project, prev_issuable_for(project, issuable.id).try(:iid))
-    end
-  end
-
-  def issuable_count(entity, project)
-    if project.nil?
-      0
-    elsif current_controller?(:issues)
-      project.issues.send(entity).count
-    elsif current_controller?(:merge_requests)
-      project.merge_requests.send(entity).count
-    end
-  end
-
-  def next_issuable_for(project, id)
-    if project.nil?
-      nil
-    elsif current_controller?(:issues)
-      project.issues.where("id > ?", id).last
-    elsif current_controller?(:merge_requests)
-      project.merge_requests.where("id > ?", id).last
-    end
-  end
-
-  def has_next_issuable?(project, id)
-    if project.nil?
-      nil
-    elsif current_controller?(:issues)
-      project.issues.where("id > ?", id).last
-    elsif current_controller?(:merge_requests)
-      project.merge_requests.where("id > ?", id).last
-    end
-  end
-
-  def prev_issuable_for(project, id)
-    if project.nil?
-      nil
-    elsif current_controller?(:issues)
-      project.issues.where("id < ?", id).first
-    elsif current_controller?(:merge_requests)
-      project.merge_requests.where("id < ?", id).first
-    end
-  end
-
-  def has_prev_issuable?(project, id)
-    if project.nil?
-      nil
-    elsif current_controller?(:issues)
-      project.issues.where("id < ?", id).first
-    elsif current_controller?(:merge_requests)
-      project.merge_requests.where("id < ?", id).first
-    end
-  end
-
   def state_filters_text_for(entity, project)
     titles = {
       opened: "Open"
diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb
new file mode 100644
index 00000000000..91a3aa371ef
--- /dev/null
+++ b/app/helpers/issuables_helper.rb
@@ -0,0 +1,37 @@
+module IssuablesHelper
+
+  def sidebar_gutter_toggle_icon
+    sidebar_gutter_collapsed? ? icon('angle-double-left') : icon('angle-double-right')
+  end
+
+  def sidebar_gutter_collapsed_class
+    "right-sidebar-#{sidebar_gutter_collapsed? ? 'collapsed' : 'expanded'}"
+  end
+
+  def issuables_count(issuable)
+    base_issuable_scope(issuable).maximum(:iid)
+  end
+
+  def next_issuable_for(issuable)
+    base_issuable_scope(issuable).where('iid > ?', issuable.iid).last
+  end
+
+  def prev_issuable_for(issuable)
+    base_issuable_scope(issuable).where('iid < ?', issuable.iid).first
+  end
+
+  private
+
+  def sidebar_gutter_collapsed?
+    cookies[:collapsed_gutter] == 'true'
+  end
+
+  def base_issuable_scope(issuable)
+    issuable.project.send(issuable.class.table_name).send(issuable_state_scope(issuable))
+  end
+
+  def issuable_state_scope(issuable)
+    issuable.open? ? :opened : :closed
+  end
+
+end
diff --git a/app/helpers/nav_helper.rb b/app/helpers/nav_helper.rb
index 75f2ed5e054..29cb753e62c 100644
--- a/app/helpers/nav_helper.rb
+++ b/app/helpers/nav_helper.rb
@@ -3,18 +3,6 @@ module NavHelper
     cookies[:collapsed_nav] == 'true'
   end
 
-  def sidebar_gutter_collapsed_class
-    if cookies[:collapsed_gutter] == 'true'
-      "right-sidebar-collapsed"
-    else
-      "right-sidebar-expanded"
-    end
-  end
-
-  def sidebar_gutter_collapsed?
-    cookies[:collapsed_gutter] == 'true'
-  end
-
   def nav_sidebar_class
     if nav_menu_collapsed?
       "sidebar-collapsed"
@@ -32,9 +20,9 @@ module NavHelper
   end
 
   def page_gutter_class
-    if current_path?('merge_requests#show') || 
-      current_path?('merge_requests#diffs') || 
-      current_path?('merge_requests#commits') || 
+    if current_path?('merge_requests#show') ||
+      current_path?('merge_requests#diffs') ||
+      current_path?('merge_requests#commits') ||
       current_path?('issues#show')
       if cookies[:collapsed_gutter] == 'true'
         "page-gutter right-sidebar-collapsed"
diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml
index ae96a45453f..e1aecdd9436 100644
--- a/app/views/shared/issuable/_sidebar.html.haml
+++ b/app/views/shared/issuable/_sidebar.html.haml
@@ -4,21 +4,18 @@
       %span.issuable-count.pull-left
         = issuable.iid
         of
-        = issuable_count(:all, @project)
+        = issuables_count(issuable)
       %span.pull-right
         %a.gutter-toggle{href: '#'}
-          - if sidebar_gutter_collapsed?
-            = icon('angle-double-left')
-          - else
-            = icon('angle-double-right')
+          = sidebar_gutter_toggle_icon
       .issuable-nav.pull-right.btn-group{role: 'group', "aria-label" => '...'}
-        - if has_prev_issuable?(@project, issuable.id)
-          = link_to 'Prev', issuable_link_prev(@project, issuable), class: 'btn btn-default prev-btn'
+        - if prev_issuable = prev_issuable_for(issuable)
+          = link_to 'Prev', [@project.namespace.becomes(Namespace), @project, prev_issuable], class: 'btn btn-default prev-btn'
         - else
           %a.btn.btn-default.disabled{href: '#'}
             Prev
-        - if has_next_issuable?(@project, issuable.id)
-          = link_to 'Next', issuable_link_next(@project, issuable), class: 'btn btn-default next-btn'
+        - if next_issuable = next_issuable_for(issuable)
+          = link_to 'Next', [@project.namespace.becomes(Namespace), @project, next_issuable], class: 'btn btn-default next-btn'
         - else
           %a.btn.btn-default.disabled{href: '#'}
             Next
diff --git a/features/project/issues/issues.feature b/features/project/issues/issues.feature
index 0b3d03aa2a5..ca2399d85a9 100644
--- a/features/project/issues/issues.feature
+++ b/features/project/issues/issues.feature
@@ -25,9 +25,16 @@ Feature: Project Issues
   Scenario: I visit issue page
     Given I click link "Release 0.4"
     Then I should see issue "Release 0.4"
+    And I should see "1 of 2" in the sidebar
+
+  Scenario: I navigate between issues
+    Given I click link "Release 0.4"
+    Then I click link "Next" in the sidebar
+    Then I should see issue "Tweet control"
+    And I should see "2 of 2" in the sidebar
 
   @javascript
-  Scenario: I visit issue page
+  Scenario: I filter by author
     Given I add a user to project "Shop"
     And I click "author" dropdown
     Then I see current user as the first user
diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature
index ca1ee6b3c2b..5995e787961 100644
--- a/features/project/merge_requests.feature
+++ b/features/project/merge_requests.feature
@@ -39,6 +39,7 @@ Feature: Project Merge Requests
   Scenario: I visit merge request page
     Given I click link "Bug NS-04"
     Then I should see merge request "Bug NS-04"
+    And I should see "1 of 1" in the sidebar
 
   Scenario: I close merge request page
     Given I click link "Bug NS-04"
diff --git a/features/steps/project/issues/issues.rb b/features/steps/project/issues/issues.rb
index d556b73f9fd..09a89e99831 100644
--- a/features/steps/project/issues/issues.rb
+++ b/features/steps/project/issues/issues.rb
@@ -54,6 +54,10 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps
     expect(page).to have_content "Release 0.4"
   end
 
+  step 'I should see issue "Tweet control"' do
+    expect(page).to have_content "Tweet control"
+  end
+
   step 'I click link "New Issue"' do
     click_link "New Issue"
   end
@@ -301,4 +305,5 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps
   def filter_issue(text)
     fill_in 'issue_search', with: text
   end
+
 end
diff --git a/features/steps/shared/issuable.rb b/features/steps/shared/issuable.rb
index 25c2b476f43..2117feaedb8 100644
--- a/features/steps/shared/issuable.rb
+++ b/features/steps/shared/issuable.rb
@@ -119,6 +119,24 @@ module SharedIssuable
     end
   end
 
+  step 'I should see "1 of 1" in the sidebar' do
+    expect_sidebar_content('1 of 1')
+  end
+
+  step 'I should see "1 of 2" in the sidebar' do
+    expect_sidebar_content('1 of 2')
+  end
+
+  step 'I should see "2 of 2" in the sidebar' do
+    expect_sidebar_content('2 of 2')
+  end
+
+  step 'I click link "Next" in the sidebar' do
+    page.within '.issuable-sidebar' do
+      click_link 'Next'
+    end
+  end
+
   def create_issuable_for_project(project_name:, title:, type: :issue)
     project = Project.find_by(name: project_name)
 
@@ -159,4 +177,10 @@ module SharedIssuable
     expect(page).to have_content("mentioned in #{issuable.class.to_s.titleize.downcase} #{issuable.to_reference(project)}")
   end
 
+  def expect_sidebar_content(content)
+    page.within '.issuable-sidebar' do
+      expect(page).to have_content content
+    end
+  end
+
 end