Merge branch 'restore-issues_mentioned_but_not_closing' into 'master'

pass in current_user in MergeRequest and MergeRequestsHelper

See merge request !8624

5 files changed, 94 insertions, 12 deletions

diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
index 91b24b8bc29..b5f8c23a667 100644
--- a/app/helpers/merge_requests_helper.rb
+++ b/app/helpers/merge_requests_helper.rb
@@ -64,11 +64,11 @@ module MergeRequestsHelper
   end
 
   def mr_closes_issues
-    @mr_closes_issues ||= @merge_request.closes_issues
+    @mr_closes_issues ||= @merge_request.closes_issues(current_user)
   end
 
   def mr_issues_mentioned_but_not_closing
-    @mr_issues_mentioned_but_not_closing ||= @merge_request.issues_mentioned_but_not_closing
+    @mr_issues_mentioned_but_not_closing ||= @merge_request.issues_mentioned_but_not_closing(current_user)
   end
 
   def mr_change_branches_path(merge_request)
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 43085f69105..c0d4dd0197f 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -546,7 +546,7 @@ class MergeRequest < ActiveRecord::Base
   # Calculating this information for a number of merge requests requires
   # running `ReferenceExtractor` on each of them separately.
   # This optimization does not apply to issues from external sources.
-  def cache_merge_request_closes_issues!(current_user = self.author)
+  def cache_merge_request_closes_issues!(current_user)
     return if project.has_external_issue_tracker?
 
     transaction do
@@ -558,10 +558,6 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
-  def closes_issue?(issue)
-    closes_issues.include?(issue)
-  end
-
   # Return the set of issues that will be closed if this merge request is accepted.
   def closes_issues(current_user = self.author)
     if target_branch == project.default_branch
@@ -575,13 +571,13 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
-  def issues_mentioned_but_not_closing(current_user = self.author)
+  def issues_mentioned_but_not_closing(current_user)
     return [] unless target_branch == project.default_branch
 
     ext = Gitlab::ReferenceExtractor.new(project, current_user)
     ext.analyze(description)
 
-    ext.issues - closes_issues
+    ext.issues - closes_issues(current_user)
   end
 
   def target_project_path
diff --git a/changelogs/unreleased/pass in current_user in MergeRequest and MergeRequestsHelper.yml b/changelogs/unreleased/pass in current_user in MergeRequest and MergeRequestsHelper.yml
new file mode 100644
index 00000000000..0751047c3c0
--- /dev/null
+++ b/changelogs/unreleased/pass in current_user in MergeRequest and MergeRequestsHelper.yml
@@ -0,0 +1,4 @@
+---
+title: pass in current_user in MergeRequest and MergeRequestsHelper
+merge_request: 8624
+author: Dongqing Hu
diff --git a/spec/helpers/merge_requests_helper_spec.rb b/spec/helpers/merge_requests_helper_spec.rb
index 8d268dbcf36..25f23826648 100644
--- a/spec/helpers/merge_requests_helper_spec.rb
+++ b/spec/helpers/merge_requests_helper_spec.rb
@@ -79,4 +79,86 @@ describe MergeRequestsHelper do
       expect(mr_widget_refresh_url(nil)).to eq('')
     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
+
+  describe '#mr_issues_mentioned_but_not_closing' 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) do
+      create(:merge_request,
+             source_project: project_1, target_project: project_1,
+             description: "#{issue_1.to_reference} #{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_issues_mentioned_but_not_closing).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_issues_mentioned_but_not_closing).to contain_exactly(issue_1, issue_2)
+      end
+    end
+  end
 end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index e1e99300489..a01741a9971 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -97,7 +97,7 @@ describe MergeRequest, models: true do
       commit = double('commit1', safe_message: "Fixes #{issue.to_reference}")
       allow(subject).to receive(:commits).and_return([commit])
 
-      expect { subject.cache_merge_request_closes_issues! }.to change(subject.merge_requests_closing_issues, :count).by(1)
+      expect { subject.cache_merge_request_closes_issues!(subject.author) }.to change(subject.merge_requests_closing_issues, :count).by(1)
     end
 
     it 'does not cache issues from external trackers' do
@@ -106,7 +106,7 @@ describe MergeRequest, models: true do
       commit = double('commit1', safe_message: "Fixes #{issue.to_reference}")
       allow(subject).to receive(:commits).and_return([commit])
 
-      expect { subject.cache_merge_request_closes_issues! }.not_to change(subject.merge_requests_closing_issues, :count)
+      expect { subject.cache_merge_request_closes_issues!(subject.author) }.not_to change(subject.merge_requests_closing_issues, :count)
     end
   end
 
@@ -300,7 +300,7 @@ describe MergeRequest, models: true do
       allow(subject.project).to receive(:default_branch).
         and_return(subject.target_branch)
 
-      expect(subject.issues_mentioned_but_not_closing).to match_array([mentioned_issue])
+      expect(subject.issues_mentioned_but_not_closing(subject.author)).to match_array([mentioned_issue])
     end
   end