Return "assign to closed issues link" on MR serializer

7 files changed, 74 insertions, 97 deletions

diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
index cd5345a609e..036f7d93c0e 100644
--- a/app/helpers/merge_requests_helper.rb
+++ b/app/helpers/merge_requests_helper.rb
@@ -47,34 +47,6 @@ module MergeRequestsHelper
     end
   end
 
-  # TODO: Remove when moving mr_change_branches_path data to MR serializer
-  def issues_sentence(issues)
-    # Sorting based on the `#123` or `group/project#123` reference will sort
-    # local issues first.
-    issues.map do |issue|
-      issue.to_reference(@project)
-    end.sort.to_sentence
-  end
-
-  # TODO: Remove when mr_assign_issues_link data goes to MR serializer
-  def mr_closes_issues
-    @mr_closes_issues ||= @merge_request.closes_issues(current_user)
-  end
-
-  # TODO: Move to MR serializer
-  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 mr_change_branches_path(merge_request)
     new_namespace_project_merge_request_path(
       @project.namespace, @project,
diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb
index cd43d8b3317..55fb7cb8720 100644
--- a/app/presenters/merge_request_presenter.rb
+++ b/app/presenters/merge_request_presenter.rb
@@ -112,8 +112,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
   end
 
   def closing_issues_links
-    closes_issues = closes_issues(current_user)
-    markdown issues_sentence(project, closes_issues), pipeline: :gfm, author: author, project: project
+    markdown issues_sentence(project, closing_issues), pipeline: :gfm, author: author, project: project
   end
 
   def mentioned_issues_links
@@ -121,6 +120,19 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
     markdown issues_sentence(project, mentioned_issues), pipeline: :gfm, author: author, project: project
   end
 
+  def assign_to_closing_issues_link
+    issues = MergeRequests::AssignIssuesService.new(project,
+                                                    current_user,
+                                                    merge_request: merge_request,
+                                                    closes_issues: closing_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 can_revert_on_current_merge_request?
     user_can_collaborate_with_project? && can_be_reverted?(current_user)
   end
@@ -131,6 +143,10 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
 
   private
 
+  def closing_issues
+    @closing_issues ||= closes_issues(current_user)
+  end
+
   def pipeline
     @pipeline ||= head_pipeline
   end
diff --git a/app/serializers/merge_request_entity.rb b/app/serializers/merge_request_entity.rb
index c995b32f134..7203a4e8608 100644
--- a/app/serializers/merge_request_entity.rb
+++ b/app/serializers/merge_request_entity.rb
@@ -56,6 +56,10 @@ class MergeRequestEntity < IssuableEntity
   end
 
   expose :issues_links do
+    expose :assign_to_closing do |merge_request|
+      presenter(merge_request).assign_to_closing_issues_link
+    end
+
     expose :closing do |merge_request|
       presenter(merge_request).closing_issues_links
     end
diff --git a/spec/fixtures/api/schemas/entities/merge_request.json b/spec/fixtures/api/schemas/entities/merge_request.json
index 9ba3746fe42..98b8639b767 100644
--- a/spec/fixtures/api/schemas/entities/merge_request.json
+++ b/spec/fixtures/api/schemas/entities/merge_request.json
@@ -53,10 +53,11 @@
     "ci_status": { "type": ["string", "null"] },
     "issues_links": {
       "type": "object",
-      "required": ["closing", "mentioned_but_not_closing"],
+      "required": ["closing", "mentioned_but_not_closing", "assign_to_closing"],
       "properties" : {
         "closing": { "type": "string" },
-        "mentioned_but_not_closing": { "type": "string" }
+        "mentioned_but_not_closing": { "type": "string" },
+        "assing_to_closing": { "type": "string" }
       },
       "additionalProperties": false
     },
diff --git a/spec/helpers/merge_requests_helper_spec.rb b/spec/helpers/merge_requests_helper_spec.rb
index 5ae9ac0955d..f2c9d927388 100644
--- a/spec/helpers/merge_requests_helper_spec.rb
+++ b/spec/helpers/merge_requests_helper_spec.rb
@@ -21,28 +21,6 @@ describe MergeRequestsHelper do
     end
   end
 
-  describe '#issues_sentence' do
-    subject { issues_sentence(issues) }
-    let(:issues) do
-      [build(:issue, iid: 1), build(:issue, iid: 2), build(:issue, iid: 3)]
-    end
-
-    it { is_expected.to eq('#1, #2, and #3') }
-
-    context 'for JIRA issues' do
-      let(:project) { create(:empty_project) }
-      let(:issues) do
-        [
-          ExternalIssue.new('JIRA-123', project),
-          ExternalIssue.new('JIRA-456', project),
-          ExternalIssue.new('FOOBAR-7890', project)
-        ]
-      end
-
-      it { is_expected.to eq('FOOBAR-7890, JIRA-123, and JIRA-456') }
-    end
-  end
-
   describe '#format_mr_branch_names' do
     describe 'within the same project' do
       let(:merge_request) { create(:merge_request) }
@@ -62,46 +40,4 @@ describe MergeRequestsHelper do
       it { is_expected.to eq([source_title, target_title]) }
     end
   end
-
-  describe '#mr_closes_issues' do
-    let(:user_1) { create(:user) }
-    let(:user_2) { create(:user) }
-
-    let(:project_1) { create(:project, :private, creator: user_1, namespace: user_1.namespace) }
-    let(:project_2) { create(:project, :private, creator: user_2, namespace: user_2.namespace) }
-
-    let(:issue_1) { create(:issue, project: project_1) }
-    let(:issue_2) { create(:issue, project: project_2) }
-
-    let(:merge_request) { create(:merge_request, source_project: project_1, target_project: project_1,) }
-
-    let(:merge_request) do
-      create(:merge_request,
-             source_project: project_1, target_project: project_1,
-             description: "Fixes #{issue_1.to_reference} Fixes #{issue_2.to_reference(project_1)}")
-    end
-
-    before do
-      project_1.team << [user_2, :developer]
-      project_2.team << [user_2, :developer]
-      allow(merge_request.project).to receive(:default_branch).and_return(merge_request.target_branch)
-      @merge_request = merge_request
-    end
-
-    context 'user without access to another private project' do
-      let(:current_user) { user_1 }
-
-      it 'cannot see that project\'s issue that will be closed on acceptance' do
-        expect(mr_closes_issues).to contain_exactly(issue_1)
-      end
-    end
-
-    context 'user with access to another private project' do
-      let(:current_user) { user_2 }
-
-      it 'can see that project\'s issue that will be closed on acceptance' do
-        expect(mr_closes_issues).to contain_exactly(issue_1, issue_2)
-      end
-    end
-  end
 end
diff --git a/spec/presenters/merge_request_presenter_spec.rb b/spec/presenters/merge_request_presenter_spec.rb
index e61af6b0a5b..bb5bc4dfb4b 100644
--- a/spec/presenters/merge_request_presenter_spec.rb
+++ b/spec/presenters/merge_request_presenter_spec.rb
@@ -148,6 +148,53 @@ describe MergeRequestPresenter do
         is_expected.not_to match("#{project.full_path}/issues/#{issue_a.iid}")
       end
     end
+
+    describe '#assign_to_closing_issues_link' do
+      subject do
+        described_class.new(resource, current_user: user)
+          .assign_to_closing_issues_link
+      end
+
+      before do
+        assign_issues_service = double(MergeRequests::AssignIssuesService, assignable_issues: assignable_issues)
+        allow(MergeRequests::AssignIssuesService).to receive(:new)
+          .and_return(assign_issues_service)
+      end
+
+      context 'single closing issue' do
+        let(:issue) { create(:issue) }
+        let(:assignable_issues) { [issue] }
+
+        it 'returns correct link with correct text' do
+          is_expected
+            .to match("#{project.full_path}/merge_requests/#{resource.iid}/assign_related_issues")
+
+          is_expected
+            .to match("Assign yourself to this issue")
+        end
+      end
+
+      context 'multiple closing issues' do
+        let(:issues) { create_list(:issue, 2) }
+        let(:assignable_issues) { issues }
+
+        it 'returns correct link with correct text' do
+          is_expected
+            .to match("#{project.full_path}/merge_requests/#{resource.iid}/assign_related_issues")
+
+          is_expected
+            .to match("Assign yourself to these issues")
+        end
+      end
+
+      context 'no closing issue' do
+        let(:assignable_issues) { [] }
+
+        it 'returns correct link with correct text' do
+          is_expected.to be_nil
+        end
+      end
+    end
   end
 
   describe '#cancel_merge_when_pipeline_succeeds_path' do
diff --git a/spec/serializers/merge_request_entity_spec.rb b/spec/serializers/merge_request_entity_spec.rb
index 190e1520609..039ec5d29da 100644
--- a/spec/serializers/merge_request_entity_spec.rb
+++ b/spec/serializers/merge_request_entity_spec.rb
@@ -36,7 +36,8 @@ describe MergeRequestEntity do
   it 'includes issues_links' do
     issues_links = subject[:issues_links]
 
-    expect(issues_links).to include(:closing, :mentioned_but_not_closing)
+    expect(issues_links).to include(:closing, :mentioned_but_not_closing,
+                                    :assign_to_closing)
   end
 
   it 'has important MergeRequest attributes' do