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

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"

app/helpers/issuables_helper.rb

+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

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"

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

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

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"

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

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