summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2016-10-11 17:20:38 +0000
committerDouwe Maan <douwe@gitlab.com>2016-10-11 17:20:38 +0000
commitd57d892e3fd4d73a477ba5c1b0049864f7c9e6b2 (patch)
tree40e6f028bd2769b405099c742956ebf4260929e7
parent9db841127bef1509d32ae5f3ff2458923c84a19d (diff)
parent6606642f8f352267d9f645778a789b79d98a6ca8 (diff)
downloadgitlab-ce-d57d892e3fd4d73a477ba5c1b0049864f7c9e6b2.tar.gz
Merge branch 'assign-issues-for-merge-request-18876' into 'master'
Ability to bulk assign issues to author of merge request ## What does this MR do? Provides a link to auto-assign issues to the author of a merge request, when they are mentioned as being closed by the MR. ## Are there points in the code the reviewer needs to double check? ## Why was this MR needed? To help avoid working on a MR without having assigned related issues to self ## What are the relevant issue numbers? Fixes #18876 ## Screenshots (if relevant) ![ScreenShot-P216](/uploads/1af5e71a0a0ff0a60c5d7b54c0e09d9c/ScreenShot-P216.png) ## Tasks - [x] Refactor or move away from using `BulkUpdateService` - [x] ~~Consider alternate link message when only a subset of issues will be assigned~~ - [x] Minimize repeated calls to expensive `closes_issues` method - [x] Move away from using inflector for pluralization and fix flash message - [x] Change auth `before_action` and fallback to error flash message - [x] Shouldn't overwrite current assignee if one exists ## Does this MR meet the acceptance criteria? - [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added - [x] ~~[Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)~~ - [x] ~~API support added~~ - Tests - [x] Added for this feature/bug - [x] All builds are passing - [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides) - [x] Branch has no merge conflicts with `master` (if you do - rebase it please) - [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits) See merge request !5725
-rw-r--r--CHANGELOG1
-rw-r--r--app/controllers/projects/merge_requests_controller.rb23
-rw-r--r--app/helpers/merge_requests_helper.rb13
-rw-r--r--app/services/merge_requests/assign_issues_service.rb35
-rw-r--r--app/views/projects/merge_requests/widget/_open.html.haml1
-rw-r--r--config/routes/project.rb1
-rw-r--r--spec/controllers/projects/merge_requests_controller_spec.rb48
-rw-r--r--spec/features/merge_requests/assign_issues_spec.rb51
-rw-r--r--spec/services/merge_requests/assign_issues_service_spec.rb49
9 files changed, 221 insertions, 1 deletions
diff --git a/CHANGELOG b/CHANGELOG
index ba6c3f7c558..0a81b68018a 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -89,6 +89,7 @@ v 8.13.0 (unreleased)
- Fix broken repository 500 errors in project list
- Fix Pipeline list commit column width should be adjusted
- Close todos when accepting merge requests via the API !6486 (tonygambone)
+ - Ability to batch assign issues relating to a merge request to the author. !5725 (jamedjo)
- Changed Slack service user referencing from full name to username (Sebastian Poxhofer)
- Retouch environments list and deployments list
- Add Container Registry on/off status to Admin Area !6638 (the-undefined)
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index ffd9833e3b1..869d96b86f4 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -10,7 +10,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
before_action :module_enabled
before_action :merge_request, only: [
:edit, :update, :show, :diffs, :commits, :conflicts, :builds, :pipelines, :merge, :merge_check,
- :ci_status, :toggle_subscription, :cancel_merge_when_build_succeeds, :remove_wip, :resolve_conflicts
+ :ci_status, :toggle_subscription, :cancel_merge_when_build_succeeds, :remove_wip, :resolve_conflicts, :assign_related_issues
]
before_action :validates_merge_request, only: [:show, :diffs, :commits, :builds, :pipelines]
before_action :define_show_vars, only: [:show, :diffs, :commits, :conflicts, :builds, :pipelines]
@@ -31,6 +31,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
# Allow modify merge_request
before_action :authorize_update_merge_request!, only: [:close, :edit, :update, :remove_wip, :sort]
+ before_action :authenticate_user!, only: [:assign_related_issues]
+
before_action :authorize_can_resolve_conflicts!, only: [:conflicts, :resolve_conflicts]
def index
@@ -354,6 +356,25 @@ class Projects::MergeRequestsController < Projects::ApplicationController
render layout: false
end
+ def assign_related_issues
+ result = MergeRequests::AssignIssuesService.new(project, current_user, merge_request: @merge_request).execute
+
+ respond_to do |format|
+ format.html do
+ case result[:count]
+ when 0
+ flash[:error] = "Failed to assign you issues related to the merge request"
+ when 1
+ flash[:notice] = "1 issue has been assigned to you"
+ else
+ flash[:notice] = "#{result[:count]} issues have been assigned to you"
+ end
+
+ redirect_to(merge_request_path(@merge_request))
+ end
+ end
+ end
+
def ci_status
pipeline = @merge_request.pipeline
if pipeline
diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
index 8abe7865fed..b0a76765d97 100644
--- a/app/helpers/merge_requests_helper.rb
+++ b/app/helpers/merge_requests_helper.rb
@@ -72,6 +72,19 @@ module MergeRequestsHelper
)
end
+ def mr_assign_issues_link
+ issues = MergeRequests::AssignIssuesService.new(@project,
+ current_user,
+ merge_request: @merge_request,
+ closes_issues: mr_closes_issues
+ ).assignable_issues
+ path = assign_related_issues_namespace_project_merge_request_path(@project.namespace, @project, @merge_request)
+ if issues.present?
+ pluralize_this_issue = issues.count > 1 ? "these issues" : "this issue"
+ link_to "Assign yourself to #{pluralize_this_issue}", path, method: :post
+ end
+ end
+
def source_branch_with_namespace(merge_request)
branch = link_to(merge_request.source_branch, namespace_project_commits_path(merge_request.source_project.namespace, merge_request.source_project, merge_request.source_branch))
diff --git a/app/services/merge_requests/assign_issues_service.rb b/app/services/merge_requests/assign_issues_service.rb
new file mode 100644
index 00000000000..f636e5fec4f
--- /dev/null
+++ b/app/services/merge_requests/assign_issues_service.rb
@@ -0,0 +1,35 @@
+module MergeRequests
+ class AssignIssuesService < BaseService
+ def assignable_issues
+ @assignable_issues ||= begin
+ if current_user == merge_request.author
+ closes_issues.select do |issue|
+ !issue.assignee_id? && can?(current_user, :admin_issue, issue)
+ end
+ else
+ []
+ end
+ end
+ end
+
+ def execute
+ assignable_issues.each do |issue|
+ Issues::UpdateService.new(issue.project, current_user, assignee_id: current_user.id).execute(issue)
+ end
+
+ {
+ count: assignable_issues.count
+ }
+ end
+
+ private
+
+ def merge_request
+ params[:merge_request]
+ end
+
+ def closes_issues
+ @closes_issues ||= params[:closes_issues] || merge_request.closes_issues(current_user)
+ end
+ end
+end
diff --git a/app/views/projects/merge_requests/widget/_open.html.haml b/app/views/projects/merge_requests/widget/_open.html.haml
index 6f5ee5f16c5..842b6df310d 100644
--- a/app/views/projects/merge_requests/widget/_open.html.haml
+++ b/app/views/projects/merge_requests/widget/_open.html.haml
@@ -35,3 +35,4 @@
Accepting this merge request will close #{"issue".pluralize(mr_closes_issues.size)}
= succeed '.' do
!= markdown issues_sentence(mr_closes_issues), pipeline: :gfm, author: @merge_request.author
+ = mr_assign_issues_link
diff --git a/config/routes/project.rb b/config/routes/project.rb
index 4c1da5c4df5..dd0e84b1475 100644
--- a/config/routes/project.rb
+++ b/config/routes/project.rb
@@ -277,6 +277,7 @@ resources :namespaces, path: '/', constraints: { id: /[a-zA-Z.0-9_\-]+/ }, only:
post :remove_wip
get :diff_for_path
post :resolve_conflicts
+ post :assign_related_issues
end
collection do
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 742edd8ba3d..84298f8bef4 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -708,4 +708,52 @@ describe Projects::MergeRequestsController do
end
end
end
+
+ describe 'POST assign_related_issues' do
+ let(:issue1) { create(:issue, project: project) }
+ let(:issue2) { create(:issue, project: project) }
+
+ def post_assign_issues
+ merge_request.update!(description: "Closes #{issue1.to_reference} and #{issue2.to_reference}",
+ author: user,
+ source_branch: 'feature',
+ target_branch: 'master')
+
+ post :assign_related_issues,
+ namespace_id: project.namespace.to_param,
+ project_id: project.to_param,
+ id: merge_request.iid
+ end
+
+ it 'shows a flash message on success' do
+ post_assign_issues
+
+ expect(flash[:notice]).to eq '2 issues have been assigned to you'
+ end
+
+ it 'correctly pluralizes flash message on success' do
+ issue2.update!(assignee: user)
+
+ post_assign_issues
+
+ expect(flash[:notice]).to eq '1 issue has been assigned to you'
+ end
+
+ it 'calls MergeRequests::AssignIssuesService' do
+ expect(MergeRequests::AssignIssuesService).to receive(:new).
+ with(project, user, merge_request: merge_request).
+ and_return(double(execute: { count: 1 }))
+
+ post_assign_issues
+ end
+
+ it 'is skipped when not signed in' do
+ project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+ sign_out(:user)
+
+ expect(MergeRequests::AssignIssuesService).not_to receive(:new)
+
+ post_assign_issues
+ end
+ end
end
diff --git a/spec/features/merge_requests/assign_issues_spec.rb b/spec/features/merge_requests/assign_issues_spec.rb
new file mode 100644
index 00000000000..43cc6f2a2a7
--- /dev/null
+++ b/spec/features/merge_requests/assign_issues_spec.rb
@@ -0,0 +1,51 @@
+require 'rails_helper'
+
+feature 'Merge request issue assignment', js: true, feature: true do
+ let(:user) { create(:user) }
+ let(:project) { create(:project, :public) }
+ let(:issue1) { create(:issue, project: project) }
+ let(:issue2) { create(:issue, project: project) }
+ let(:merge_request) { create(:merge_request, :simple, source_project: project, author: user, description: "fixes #{issue1.to_reference} and #{issue2.to_reference}") }
+ let(:service) { MergeRequests::AssignIssuesService.new(merge_request, user, user, project) }
+
+ before do
+ project.team << [user, :developer]
+ end
+
+ def visit_merge_request(current_user = nil)
+ login_as(current_user || user)
+ visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+ end
+
+ context 'logged in as author' do
+ scenario 'updates related issues' do
+ visit_merge_request
+ click_link "Assign yourself to these issues"
+
+ expect(page).to have_content "2 issues have been assigned to you"
+ end
+
+ it 'returns user to the merge request' do
+ visit_merge_request
+ click_link "Assign yourself to these issues"
+
+ expect(page).to have_content merge_request.description
+ end
+
+ it "doesn't display if related issues are already assigned" do
+ [issue1, issue2].each { |issue| issue.update!(assignee: user) }
+
+ visit_merge_request
+
+ expect(page).not_to have_content "Assign yourself"
+ end
+ end
+
+ context 'not MR author' do
+ it "doesn't not show assignment link" do
+ visit_merge_request(create(:user))
+
+ expect(page).not_to have_content "Assign yourself"
+ end
+ end
+end
diff --git a/spec/services/merge_requests/assign_issues_service_spec.rb b/spec/services/merge_requests/assign_issues_service_spec.rb
new file mode 100644
index 00000000000..7aeb95a15ea
--- /dev/null
+++ b/spec/services/merge_requests/assign_issues_service_spec.rb
@@ -0,0 +1,49 @@
+require 'spec_helper'
+
+describe MergeRequests::AssignIssuesService, services: true do
+ let(:user) { create(:user) }
+ let(:project) { create(:project, :public) }
+ let(:issue) { create(:issue, project: project) }
+ let(:merge_request) { create(:merge_request, :simple, source_project: project, author: user, description: "fixes #{issue.to_reference}") }
+ let(:service) { described_class.new(project, user, merge_request: merge_request) }
+
+ before do
+ project.team << [user, :developer]
+ end
+
+ it 'finds unassigned issues fixed in merge request' do
+ expect(service.assignable_issues.map(&:id)).to include(issue.id)
+ end
+
+ it 'ignores issues already assigned to any user' do
+ issue.update!(assignee: create(:user))
+
+ expect(service.assignable_issues).to be_empty
+ end
+
+ it 'ignores issues the user cannot update assignee on' do
+ project.team.truncate
+
+ expect(service.assignable_issues).to be_empty
+ end
+
+ it 'ignores all issues unless current_user is merge_request.author' do
+ merge_request.update!(author: create(:user))
+
+ expect(service.assignable_issues).to be_empty
+ end
+
+ it 'accepts precomputed data for closes_issues' do
+ issue2 = create(:issue, project: project)
+ service2 = described_class.new(project,
+ user,
+ merge_request: merge_request,
+ closes_issues: [issue, issue2])
+
+ expect(service2.assignable_issues.count).to eq 2
+ end
+
+ it 'assigns these to the merge request owner' do
+ expect { service.execute }.to change { issue.reload.assignee }.to(user)
+ end
+end