Only accept open issues and merge requests

10 files changed, 62 insertions, 19 deletions

diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index eaf14009242..cc8321d97ad 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -116,7 +116,7 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def closed_by_merge_requests
-    @closed_by_mr = @issue.closed_by_merge_requests(current_user)
+    @closed_by_merge_requests ||= @issue.closed_by_merge_requests(current_user)
   end
 
   protected
diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb
index 1adbf3a79b1..fda18e7b316 100644
--- a/app/helpers/issues_helper.rb
+++ b/app/helpers/issues_helper.rb
@@ -84,7 +84,7 @@ module IssuesHelper
   end
 
   def merge_requests_sentence(merge_requests)
-    merge_requests.map(&:to_reference).to_sentence
+    merge_requests.map(&:to_reference).to_sentence(last_word_connector: ', or ')
   end
 
   # Required for Gitlab::Markdown::IssueReferenceFilter
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 0e8bcc1a4ec..efa6a269992 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -86,6 +86,10 @@ module Issuable
     assignee_id_changed?
   end
 
+  def open?
+    opened? || reopened?
+  end
+
   #
   # Votes
   #
diff --git a/app/models/issue.rb b/app/models/issue.rb
index c24a329847c..72183108033 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -98,11 +98,11 @@ class Issue < ActiveRecord::Base
 
   # From all notes on this issue, we'll select the system notes about linked
   # merge requests. Of those, the MRs closing `self` are returned.
-  def closed_by_merge_requests(current_user)
+  def closed_by_merge_requests(current_user = nil)
+    return [] unless open?
+
     notes.system.flat_map do |note|
-      ext = Gitlab::ReferenceExtractor.new(self.project, current_user)
-      ext.analyze(note.note)
-      ext.merge_requests
-    end.uniq.select { |mr| mr.closes_issue?(self) }
+      note.all_references(current_user).merge_requests
+    end.uniq.select { |mr| mr.open? && mr.closes_issue?(self) }
   end
 end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 3ae74ceac68..0042b95c4f1 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -222,10 +222,6 @@ class MergeRequest < ActiveRecord::Base
     self.target_project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::CLOSED).last
   end
 
-  def open?
-    opened? || reopened?
-  end
-
   def work_in_progress?
     !!(title =~ /\A\[?WIP\]?:? /i)
   end
diff --git a/app/views/projects/issues/_closed_by_box.html.haml b/app/views/projects/issues/_closed_by_box.html.haml
index fe886b6d7d7..aef352029d0 100644
--- a/app/views/projects/issues/_closed_by_box.html.haml
+++ b/app/views/projects/issues/_closed_by_box.html.haml
@@ -1,6 +1,3 @@
-.issue-closed-by-widget 
-  %i.fa.fa-check
-  - if @closed_by_mr.count == 1
-    This issue will be closed when #{gfm(@closed_by_mr.first.to_reference)} is accepted.
-  -else 
-    This issue will be closed when any of merge requests #{gfm(merge_requests_sentence(@closed_by_mr.sort))} is accepted.
+.issue-closed-by-widget
+  = icon('check')
+  This issue will be closed automatically when merge request #{gfm(merge_requests_sentence(@closed_by_merge_requests.sort))} is accepted.
diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml
index 309f276882d..f01bf2505da 100644
--- a/app/views/projects/issues/show.html.haml
+++ b/app/views/projects/issues/show.html.haml
@@ -46,7 +46,7 @@
                 = markdown(@issue.description)
             %textarea.hidden.js-task-list-field
               = @issue.description
-  - if @closed_by_mr.present?
+  - if @closed_by_merge_requests.present?
     = render 'projects/issues/closed_by_box'
   .issue-discussion
     = render 'projects/issues/discussion'
diff --git a/spec/helpers/issues_helper_spec.rb b/spec/helpers/issues_helper_spec.rb
index c08ddb4cae1..78a6b631eb2 100644
--- a/spec/helpers/issues_helper_spec.rb
+++ b/spec/helpers/issues_helper_spec.rb
@@ -117,4 +117,14 @@ describe IssuesHelper do
     end
   end
 
+  describe "#merge_requests_sentence" do
+    subject { merge_requests_sentence(merge_requests)}
+    let(:merge_requests) do
+      [ build(:merge_request, iid: 1), build(:merge_request, iid: 2),
+        build(:merge_request, iid: 3)]
+    end
+
+    it { is_expected.to eq("!1, !2, or !3") }
+  end
+
 end
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 8f706f8934b..0f13c4410cd 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -68,7 +68,6 @@ describe Issue, "Issuable" do
     end
   end
 
-
   describe "#to_hook_data" do
     let(:hook_data) { issue.to_hook_data(user) }
 
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index 623332cd2f9..c9aa1b063c6 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -68,6 +68,43 @@ describe Issue do
     end
   end
 
+  describe '#closed_by_merge_requests' do
+    let(:project) { create(:project) }
+    let(:issue)   { create(:issue, project: project, state: "opened")}
+    let(:closed_issue) { build(:issue, project: project, state: "closed")}
+
+    let(:mr) do
+      opts = {
+        title: 'Awesome merge_request',
+        description: "Fixes #{issue.to_reference}",
+        source_branch: 'feature',
+        target_branch: 'master'
+      }
+      MergeRequests::CreateService.new(project, project.owner, opts).execute
+    end
+
+    let(:closed_mr) do
+      opts = {
+        title: 'Awesome merge_request 2',
+        description: "Fixes #{issue.to_reference}",
+        source_branch: 'feature',
+        target_branch: 'master',
+        state: 'closed'
+      }
+      MergeRequests::CreateService.new(project, project.owner, opts).execute
+    end
+
+    it 'returns the merge request to close this issue' do
+      allow(mr).to receive(:closes_issue?).with(issue).and_return(true)
+
+      expect(issue.closed_by_merge_requests).to eq([mr])
+    end
+
+    it "returns an empty array when the current issue is closed already" do
+      expect(closed_issue.closed_by_merge_requests).to eq([])
+    end
+  end
+
   it_behaves_like 'an editable mentionable' do
     subject { create(:issue) }