Merge branch 'jk-backport-issuables-refactor' into 'master'

Refactor issuables index actions

See merge request gitlab-org/gitlab-ce!15242

app/controllers/concerns/issuable_actions.rb

@@ -9,9 +9,7 @@ module IssuableActions
 
   def show
     respond_to do |format|
-      format.html do
-        render show_view
-      end
+      format.html
       format.json do
         render json: serializer.represent(issuable, serializer: params[:serializer])
       end
@@ -152,10 +150,6 @@ module IssuableActions
     end
   end
 
-  def show_view
-    'show'
-  end
-
   def serializer
     raise NotImplementedError
   end

app/controllers/concerns/issuable_collections.rb

@@ -4,58 +4,44 @@ module IssuableCollections
   include Gitlab::IssuableMetadata
 
   included do
-    helper_method :issues_finder
-    helper_method :merge_requests_finder
+    helper_method :finder
   end
 
   private
 
-  def set_issues_index
-    @collection_type    = "Issue"
-    @issues             = issues_collection
-    @issues             = @issues.page(params[:page])
-    @issuable_meta_data = issuable_meta_data(@issues, @collection_type)
-    @total_pages        = issues_page_count(@issues)
+  def set_issuables_index
+    @issuables          = issuables_collection
+    @issuables          = @issuables.page(params[:page])
+    @issuable_meta_data = issuable_meta_data(@issuables, collection_type)
+    @total_pages        = issuable_page_count
 
-    return if redirect_out_of_range(@issues, @total_pages)
+    return if redirect_out_of_range(@total_pages)
 
     if params[:label_name].present?
-      @labels = LabelsFinder.new(current_user, project_id: @project.id, title: params[:label_name]).execute
+      labels_params = { project_id: @project.id, title: params[:label_name] }
+      @labels = LabelsFinder.new(current_user, labels_params).execute
     end
 
     @users = []
+    if params[:assignee_id].present?
+      assignee = User.find_by_id(params[:assignee_id])
+      @users.push(assignee) if assignee
     end
 
-  def issues_collection
-    issues_finder.execute.preload(:project, :author, :assignees, :labels, :milestone, project: :namespace)
+    if params[:author_id].present?
+      author = User.find_by_id(params[:author_id])
+      @users.push(author) if author
     end
-
-  def merge_requests_collection
-    merge_requests_finder.execute.preload(
-      :source_project,
-      :target_project,
-      :author,
-      :assignee,
-      :labels,
-      :milestone,
-      head_pipeline: :project,
-      target_project: :namespace,
-      merge_request_diff: :merge_request_diff_commits
-    )
-  end
-
-  def issues_finder
-    @issues_finder ||= issuable_finder_for(IssuesFinder)
   end
 
-  def merge_requests_finder
-    @merge_requests_finder ||= issuable_finder_for(MergeRequestsFinder)
+  def issuables_collection
+    finder.execute.preload(preload_for_collection)
   end
 
-  def redirect_out_of_range(relation, total_pages)
+  def redirect_out_of_range(total_pages)
     return false if total_pages.zero?
 
-    out_of_range = relation.current_page > total_pages
+    out_of_range = @issuables.current_page > total_pages
 
     if out_of_range
       redirect_to(url_for(params.merge(page: total_pages, only_path: true)))
@@ -64,12 +50,8 @@ module IssuableCollections
     out_of_range
   end
 
-  def issues_page_count(relation)
-    page_count_for_relation(relation, issues_finder.row_count)
-  end
-
-  def merge_requests_page_count(relation)
-    page_count_for_relation(relation, merge_requests_finder.row_count)
+  def issuable_page_count
+    page_count_for_relation(@issuables, finder.row_count)
   end
 
   def page_count_for_relation(relation, row_count)
@@ -145,4 +127,31 @@ module IssuableCollections
     else value
     end
   end
+
+  def finder
+    return @finder if defined?(@finder)
+
+    @finder = issuable_finder_for(@finder_type)
+  end
+
+  def collection_type
+    @collection_type ||= case finder
+                         when IssuesFinder
+                           'Issue'
+                         when MergeRequestsFinder
+                           'MergeRequest'
+                         end
+  end
+
+  def preload_for_collection
+    @preload_for_collection ||= case collection_type
+                                when 'Issue'
+                                  [:project, :author, :assignees, :labels, :milestone, project: :namespace]
+                                when 'MergeRequest'
+                                  [
+                                    :source_project, :target_project, :author, :assignee, :labels, :milestone,
+                                    head_pipeline: :project, target_project: :namespace, merge_request_diff: :merge_request_diff_commits
+                                  ]
+                                end
+  end
 end

app/controllers/concerns/issues_action.rb

@@ -3,14 +3,14 @@ module IssuesAction
   include IssuableCollections
 
   def issues
-    @label = issues_finder.labels.first
+    @finder_type = IssuesFinder
+    @label = finder.labels.first
 
-    @issues = issues_collection
+    @issues = issuables_collection
               .non_archived
               .page(params[:page])
 
-    @collection_type    = "Issue"
-    @issuable_meta_data = issuable_meta_data(@issues, @collection_type)
+    @issuable_meta_data = issuable_meta_data(@issues, collection_type)
 
     respond_to do |format|
       format.html

app/controllers/concerns/merge_requests_action.rb

@@ -3,13 +3,12 @@ module MergeRequestsAction
   include IssuableCollections
 
   def merge_requests
-    @label = merge_requests_finder.labels.first
+    @finder_type = MergeRequestsFinder
+    @label = finder.labels.first
 
-    @merge_requests = merge_requests_collection
-                      .page(params[:page])
+    @merge_requests = issuables_collection.page(params[:page])
 
-    @collection_type    = "MergeRequest"
-    @issuable_meta_data = issuable_meta_data(@merge_requests, @collection_type)
+    @issuable_meta_data = issuable_meta_data(@merge_requests, collection_type)
   end
 
   private

app/controllers/projects/issues_controller.rb

@@ -10,7 +10,7 @@ class Projects::IssuesController < Projects::ApplicationController
 
   before_action :check_issues_available!
   before_action :issue, except: [:index, :new, :create, :bulk_update]
-  before_action :set_issues_index, only: [:index]
+  before_action :set_issuables_index, only: [:index]
 
   # Allow write(create) issue
   before_action :authorize_create_issue!, only: [:new, :create]
@@ -24,15 +24,7 @@ class Projects::IssuesController < Projects::ApplicationController
   respond_to :html
 
   def index
-    if params[:assignee_id].present?
-      assignee = User.find_by_id(params[:assignee_id])
-      @users.push(assignee) if assignee
-    end
-
-    if params[:author_id].present?
-      author = User.find_by_id(params[:author_id])
-      @users.push(author) if author
-    end
+    @issues = @issuables
 
     respond_to do |format|
       format.html
@@ -252,4 +244,9 @@ class Projects::IssuesController < Projects::ApplicationController
     update_params = issue_params.merge(spammable_params)
     Issues::UpdateService.new(project, current_user, update_params)
   end
+
+  def set_issuables_index
+    @finder_type = IssuesFinder
+    super
+  end
 end

app/controllers/projects/merge_requests_controller.rb

@@ -10,33 +10,12 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
 
   before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort]
 
+  before_action :set_issuables_index, only: [:index]
+
   before_action :authenticate_user!, only: [:assign_related_issues]
 
   def index
-    @collection_type    = "MergeRequest"
-    @merge_requests     = merge_requests_collection
-    @merge_requests     = @merge_requests.page(params[:page])
-    @merge_requests     = @merge_requests.preload(merge_request_diff: :merge_request)
-    @issuable_meta_data = issuable_meta_data(@merge_requests, @collection_type)
-    @total_pages        = merge_requests_page_count(@merge_requests)
-
-    return if redirect_out_of_range(@merge_requests, @total_pages)
-
-    if params[:label_name].present?
-      labels_params = { project_id: @project.id, title: params[:label_name] }
-      @labels = LabelsFinder.new(current_user, labels_params).execute
-    end
-
-    @users = []
-    if params[:assignee_id].present?
-      assignee = User.find_by_id(params[:assignee_id])
-      @users.push(assignee) if assignee
-    end
-
-    if params[:author_id].present?
-      author = User.find_by_id(params[:author_id])
-      @users.push(author) if author
-    end
+    @merge_requests = @issuables
 
     respond_to do |format|
       format.html
@@ -338,4 +317,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     @target_project = @merge_request.target_project
     @target_branches = @merge_request.target_project.repository.branch_names
   end
+
+  def set_issuables_index
+    @finder_type = MergeRequestsFinder
+    super
+  end
 end

app/controllers/projects_controller.rb

@@ -275,7 +275,8 @@ class ProjectsController < Projects::ApplicationController
         @project_wiki = @project.wiki
         @wiki_home = @project_wiki.find_page('home', params[:version_id])
       elsif @project.feature_available?(:issues, current_user)
-        @issues = issues_collection.page(params[:page])
+        @finder_type = IssuesFinder
+        @issues = issuables_collection.page(params[:page])
         @collection_type = 'Issue'
         @issuable_meta_data = issuable_meta_data(@issues, @collection_type)
       end

app/helpers/issuables_helper.rb

@@ -249,8 +249,6 @@ module IssuablesHelper
   end
 
   def issuables_count_for_state(issuable_type, state)
-    finder = public_send("#{issuable_type}_finder") # rubocop:disable GitlabSecurity/PublicSend
-
     Gitlab::IssuablesCountForState.new(finder)[state]
   end
 

app/models/concerns/issuable.rb

@@ -17,6 +17,8 @@ module Issuable
   include Importable
   include Editable
   include AfterCommitQueue
+  include Sortable
+  include CreatedAtFilterable
 
   # This object is used to gather issuable meta data for displaying
   # upvotes, downvotes, notes and closing merge requests count for issues and merge requests

app/models/issue.rb

@@ -5,11 +5,9 @@ class Issue < ActiveRecord::Base
   include Issuable
   include Noteable
   include Referable
-  include Sortable
   include Spammable
   include FasterCacheKeys
   include RelativePositioning
-  include CreatedAtFilterable
   include TimeTrackable
 
   DueDateStruct = Struct.new(:title, :name).freeze

app/models/merge_request.rb

@@ -3,9 +3,7 @@ class MergeRequest < ActiveRecord::Base
   include Issuable
   include Noteable
   include Referable
-  include Sortable
   include IgnorableColumn
-  include CreatedAtFilterable
   include TimeTrackable
 
   ignore_column :locked_at,

app/views/projects/issues/_nav_btns.html.haml

@@ -3,8 +3,8 @@
 - if @can_bulk_update
   = button_tag "Edit issues", class: "btn btn-default append-right-10 js-bulk-update-toggle"
 = link_to "New issue", new_project_issue_path(@project,
-                                              issue: { assignee_id: issues_finder.assignee.try(:id),
-                                                       milestone_id: issues_finder.milestones.first.try(:id) }),
+                                              issue: { assignee_id: finder.assignee.try(:id),
+                                                       milestone_id: finder.milestones.first.try(:id) }),
                                               class: "btn btn-new",
                                               title: "New issue",
                                               id: "new_issue_link"

app/views/shared/issuable/_filter.html.haml

-- finder = controller.controller_name == 'issues' ? issues_finder : merge_requests_finder
 - boards_page = controller.controller_name == 'boards'
 
 .issues-filters

spec/controllers/concerns/issuable_collections_spec.rb

@@ -17,60 +17,6 @@ describe IssuableCollections do
     controller
   end
 
-  describe '#redirect_out_of_range' do
-    before do
-      allow(controller).to receive(:url_for)
-    end
-
-    it 'returns true and redirects if the offset is out of range' do
-      relation = double(:relation, current_page: 10)
-
-      expect(controller).to receive(:redirect_to)
-      expect(controller.send(:redirect_out_of_range, relation, 2)).to eq(true)
-    end
-
-    it 'returns false if the offset is not out of range' do
-      relation = double(:relation, current_page: 1)
-
-      expect(controller).not_to receive(:redirect_to)
-      expect(controller.send(:redirect_out_of_range, relation, 2)).to eq(false)
-    end
-  end
-
-  describe '#issues_page_count' do
-    it 'returns the number of issue pages' do
-      project = create(:project, :public)
-
-      create(:issue, project: project)
-
-      finder = IssuesFinder.new(user)
-      issues = finder.execute
-
-      allow(controller).to receive(:issues_finder)
-        .and_return(finder)
-
-      expect(controller.send(:issues_page_count, issues)).to eq(1)
-    end
-  end
-
-  describe '#merge_requests_page_count' do
-    it 'returns the number of merge request pages' do
-      project = create(:project, :public)
-
-      create(:merge_request, source_project: project, target_project: project)
-
-      finder = MergeRequestsFinder.new(user)
-      merge_requests = finder.execute
-
-      allow(controller).to receive(:merge_requests_finder)
-        .and_return(finder)
-
-      pages = controller.send(:merge_requests_page_count, merge_requests)
-
-      expect(pages).to eq(1)
-    end
-  end
-
   describe '#page_count_for_relation' do
     it 'returns the number of pages' do
       relation = double(:relation, limit_value: 20)