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

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)

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

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))
 

app/services/merge_requests/assign_issues_service.rb

+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

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

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

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

spec/features/merge_requests/assign_issues_spec.rb

+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

spec/services/merge_requests/assign_issues_service_spec.rb

+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