summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJarka Kadlecova <jarka@gitlab.com>2017-11-07 14:34:12 +0100
committerJarka Kadlecova <jarka@gitlab.com>2017-11-07 14:34:12 +0100
commitad6e650262c1c152fe5e7d7a09607286b8f9f750 (patch)
tree192ea48a0061418c6802629231dfbbc02098af52
parentdc1e6b436268c00bd1fdf3d15597a4656e029b95 (diff)
downloadgitlab-ce-ad6e650262c1c152fe5e7d7a09607286b8f9f750.tar.gz
Refactor issuables index actions
-rw-r--r--app/controllers/concerns/issuable_actions.rb8
-rw-r--r--app/controllers/concerns/issuable_collections.rb91
-rw-r--r--app/controllers/concerns/issues_action.rb8
-rw-r--r--app/controllers/concerns/merge_requests_action.rb9
-rw-r--r--app/controllers/projects/issues_controller.rb17
-rw-r--r--app/controllers/projects/merge_requests_controller.rb32
-rw-r--r--app/controllers/projects_controller.rb3
-rw-r--r--app/helpers/issuables_helper.rb2
-rw-r--r--app/models/concerns/issuable.rb2
-rw-r--r--app/models/issue.rb2
-rw-r--r--app/models/merge_request.rb2
-rw-r--r--app/views/projects/issues/_nav_btns.html.haml4
-rw-r--r--app/views/shared/issuable/_filter.html.haml1
-rw-r--r--spec/controllers/concerns/issuable_collections_spec.rb54
14 files changed, 80 insertions, 155 deletions
diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb
index 9c222549cdc..072dffaff7a 100644
--- a/app/controllers/concerns/issuable_actions.rb
+++ b/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
diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb
index 3181f517087..2b011bc87b0 100644
--- a/app/controllers/concerns/issuable_collections.rb
+++ b/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 = []
- end
-
- def issues_collection
- issues_finder.execute.preload(:project, :author, :assignees, :labels, :milestone, project: :namespace)
- 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
+ if params[:assignee_id].present?
+ assignee = User.find_by_id(params[:assignee_id])
+ @users.push(assignee) if assignee
+ end
- def issues_finder
- @issues_finder ||= issuable_finder_for(IssuesFinder)
+ if params[:author_id].present?
+ author = User.find_by_id(params[:author_id])
+ @users.push(author) if author
+ end
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
diff --git a/app/controllers/concerns/issues_action.rb b/app/controllers/concerns/issues_action.rb
index 404559c8707..ad594903331 100644
--- a/app/controllers/concerns/issues_action.rb
+++ b/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
diff --git a/app/controllers/concerns/merge_requests_action.rb b/app/controllers/concerns/merge_requests_action.rb
index d3c8e4888bc..8b569a01afd 100644
--- a/app/controllers/concerns/merge_requests_action.rb
+++ b/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
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index d4e763aa5b8..dbc9106ba6d 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/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
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index c86acae8fe4..402420b851e 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/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
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 1688121e27e..2a473ec0cec 100644
--- a/app/controllers/projects_controller.rb
+++ b/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
diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb
index 85407e38532..a9840d19178 100644
--- a/app/helpers/issuables_helper.rb
+++ b/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
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index a928b9d6367..c008fb91a16 100644
--- a/app/models/concerns/issuable.rb
+++ b/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
diff --git a/app/models/issue.rb b/app/models/issue.rb
index fc590f9257e..3b3c7fb7f8b 100644
--- a/app/models/issue.rb
+++ b/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
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index f80601f3484..82d0ae90d77 100644
--- a/app/models/merge_request.rb
+++ b/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,
diff --git a/app/views/projects/issues/_nav_btns.html.haml b/app/views/projects/issues/_nav_btns.html.haml
index 13809da6523..0d39edb7bfd 100644
--- a/app/views/projects/issues/_nav_btns.html.haml
+++ b/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"
diff --git a/app/views/shared/issuable/_filter.html.haml b/app/views/shared/issuable/_filter.html.haml
index d3f0aa2d339..8442d7ff4a2 100644
--- a/app/views/shared/issuable/_filter.html.haml
+++ b/app/views/shared/issuable/_filter.html.haml
@@ -1,4 +1,3 @@
-- finder = controller.controller_name == 'issues' ? issues_finder : merge_requests_finder
- boards_page = controller.controller_name == 'boards'
.issues-filters
diff --git a/spec/controllers/concerns/issuable_collections_spec.rb b/spec/controllers/concerns/issuable_collections_spec.rb
index c9687af4dd2..cd3bf785d34 100644
--- a/spec/controllers/concerns/issuable_collections_spec.rb
+++ b/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)