From d0db72983e2170a61bcef26c99ceb3df313fd10a Mon Sep 17 00:00:00 2001 From: Dongqing Hu Date: Wed, 18 Jan 2017 17:05:09 +0800 Subject: use the current_user parameter in MergeRequest#issues_mentioned_but_not_closing --- app/models/merge_request.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 10251302db8..881a49d4ac2 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -576,7 +576,7 @@ class MergeRequest < ActiveRecord::Base 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 -- cgit v1.2.1 From da81add825b0b7b4a3b4351f67fbab25a947922c Mon Sep 17 00:00:00 2001 From: Dongqing Hu Date: Thu, 19 Jan 2017 12:18:49 +0800 Subject: should pass in current_user from MergeRequestsHelper --- app/helpers/merge_requests_helper.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index 8c2c4e8833b..01f6ada8d76 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) -- cgit v1.2.1 From 821ab7cf7dc45932167986005013044e346c8823 Mon Sep 17 00:00:00 2001 From: Dongqing Hu Date: Wed, 25 Jan 2017 10:44:05 +0800 Subject: tests for #mr_closes_issues and #mr_issues_mentioned_but_not_closing in MergeRequestsHelper --- ...ser in MergeRequest and MergeRequestsHelper.yml | 4 ++ spec/helpers/merge_requests_helper_spec.rb | 82 ++++++++++++++++++++++ 2 files changed, 86 insertions(+) create mode 100644 changelogs/unreleased/pass in current_user in MergeRequest and MergeRequestsHelper.yml 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 1f221487393..408ee93f7df 100644 --- a/spec/helpers/merge_requests_helper_spec.rb +++ b/spec/helpers/merge_requests_helper_spec.rb @@ -77,4 +77,86 @@ describe MergeRequestsHelper do expect(mr_widget_refresh_url(nil)).to end_with('') 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 -- cgit v1.2.1 From bbbef273f74a59a18cf534e147e79e90888d7656 Mon Sep 17 00:00:00 2001 From: Dongqing Hu Date: Tue, 31 Jan 2017 22:42:54 +0800 Subject: Remove MergeRequest#closes_issue?; Remove the default parameter value for #cache_merge_request_closes_issues! and #issues_mentioned_but_not_closing --- app/models/merge_request.rb | 8 ++------ spec/models/merge_request_spec.rb | 6 +++--- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 881a49d4ac2..9e5e5a3b70a 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -541,7 +541,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 @@ -553,10 +553,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 @@ -570,7 +566,7 @@ 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) diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 861426acbc3..cfb5ff0ff8b 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 -- cgit v1.2.1