summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/models/merge_request.rb21
-rw-r--r--app/presenters/merge_request_presenter.rb2
-rw-r--r--app/services/merge_requests/post_merge_service.rb2
-rw-r--r--app/services/merge_requests/reopen_service.rb1
-rw-r--r--changelogs/unreleased/issue_44821.yml5
-rw-r--r--lib/api/merge_requests.rb2
-rw-r--r--spec/factories/merge_requests.rb4
-rw-r--r--spec/lib/gitlab/import_export/all_models.yml1
-rw-r--r--spec/models/merge_request_spec.rb72
-rw-r--r--spec/presenters/merge_request_presenter_spec.rb2
-rw-r--r--spec/requests/api/merge_requests_spec.rb1
-rw-r--r--spec/services/issues/reopen_service_spec.rb2
-rw-r--r--spec/services/merge_requests/merge_service_spec.rb1
-rw-r--r--spec/services/merge_requests/post_merge_service_spec.rb2
-rw-r--r--spec/services/merge_requests/reopen_service_spec.rb6
15 files changed, 112 insertions, 12 deletions
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index b4090fd8baf..e390ab8ddf9 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -52,6 +52,8 @@ class MergeRequest < ActiveRecord::Base
class_name: 'MergeRequestsClosingIssues',
dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
+ has_many :cached_closes_issues, through: :merge_requests_closing_issues, source: :issue
+
belongs_to :assignee, class_name: "User"
serialize :merge_params, Hash # rubocop:disable Cop/ActiveRecordSerialize
@@ -755,8 +757,9 @@ 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)
+ def cache_merge_request_closes_issues!(current_user = self.author)
return unless project.issues_enabled?
+ return if closed? || merged?
transaction do
self.merge_requests_closing_issues.delete_all
@@ -769,6 +772,18 @@ class MergeRequest < ActiveRecord::Base
end
end
+ def visible_closing_issues_for(current_user = self.author)
+ strong_memoize(:visible_closing_issues_for) do
+ if self.target_project.has_external_issue_tracker?
+ closes_issues(current_user)
+ else
+ cached_closes_issues.select do |issue|
+ Ability.allowed?(current_user, :read_issue, issue)
+ end
+ end
+ end
+ 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
@@ -788,7 +803,7 @@ class MergeRequest < ActiveRecord::Base
ext = Gitlab::ReferenceExtractor.new(project, current_user)
ext.analyze("#{title}\n#{description}")
- ext.issues - closes_issues(current_user)
+ ext.issues - visible_closing_issues_for(current_user)
end
def target_project_path
@@ -836,7 +851,7 @@ class MergeRequest < ActiveRecord::Base
end
def merge_commit_message(include_description: false)
- closes_issues_references = closes_issues.map do |issue|
+ closes_issues_references = visible_closing_issues_for.map do |issue|
issue.to_reference(target_project)
end
diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb
index ffa238a63d5..8c4eac3c31d 100644
--- a/app/presenters/merge_request_presenter.rb
+++ b/app/presenters/merge_request_presenter.rb
@@ -206,7 +206,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
end
def closing_issues
- @closing_issues ||= closes_issues(current_user)
+ @closing_issues ||= visible_closing_issues_for(current_user)
end
def pipeline
diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb
index 3d2aea4e9b6..f26e3bee06f 100644
--- a/app/services/merge_requests/post_merge_service.rb
+++ b/app/services/merge_requests/post_merge_service.rb
@@ -25,7 +25,7 @@ module MergeRequests
def close_issues(merge_request)
return unless merge_request.target_branch == project.default_branch
- closed_issues = merge_request.closes_issues(current_user)
+ closed_issues = merge_request.visible_closing_issues_for(current_user)
closed_issues.each do |issue|
if can?(current_user, :update_issue, issue)
diff --git a/app/services/merge_requests/reopen_service.rb b/app/services/merge_requests/reopen_service.rb
index f2fc13ad028..f6cbe769ef4 100644
--- a/app/services/merge_requests/reopen_service.rb
+++ b/app/services/merge_requests/reopen_service.rb
@@ -14,6 +14,7 @@ module MergeRequests
merge_request.mark_as_unchecked
invalidate_cache_counts(merge_request, users: merge_request.assignees)
merge_request.update_project_counter_caches
+ merge_request.cache_merge_request_closes_issues!(current_user)
end
merge_request
diff --git a/changelogs/unreleased/issue_44821.yml b/changelogs/unreleased/issue_44821.yml
new file mode 100644
index 00000000000..b1807e069af
--- /dev/null
+++ b/changelogs/unreleased/issue_44821.yml
@@ -0,0 +1,5 @@
+---
+title: Retrieve merge request closing issues from database cache
+merge_request: 20911
+author:
+type: fixed
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index 2621c9f8fc2..abad418771c 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -380,7 +380,7 @@ module API
end
get ':id/merge_requests/:merge_request_iid/closes_issues' do
merge_request = find_merge_request_with_access(params[:merge_request_iid])
- issues = ::Kaminari.paginate_array(merge_request.closes_issues(current_user))
+ issues = ::Kaminari.paginate_array(merge_request.visible_closing_issues_for(current_user))
issues = paginate(issues)
external_issues, internal_issues = issues.partition { |issue| issue.is_a?(ExternalIssue) }
diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb
index f722bb9cb0d..f3bea17385b 100644
--- a/spec/factories/merge_requests.rb
+++ b/spec/factories/merge_requests.rb
@@ -100,6 +100,10 @@ FactoryBot.define do
end
end
+ after(:create) do |merge_request, evaluator|
+ merge_request.cache_merge_request_closes_issues!
+ end
+
factory :merged_merge_request, traits: [:merged]
factory :closed_merge_request, traits: [:closed]
factory :reopened_merge_request, traits: [:opened]
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index db5aab0cd76..375a93b28a6 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -87,6 +87,7 @@ merge_requests:
- merge_request_diff
- events
- merge_requests_closing_issues
+- cached_closes_issues
- metrics
- timelogs
- head_pipeline
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index b0d9d03bf6c..5904f575f74 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -310,6 +310,51 @@ describe MergeRequest do
end
end
+ describe '#visible_closing_issues_for' do
+ let(:guest) { create(:user) }
+ let(:developer) { create(:user) }
+ let(:issue_1) { create(:issue, project: subject.source_project) }
+ let(:issue_2) { create(:issue, project: subject.source_project) }
+ let(:confidential_issue) { create(:issue, :confidential, project: subject.source_project) }
+
+ before do
+ subject.project.add_developer(subject.author)
+ subject.target_branch = subject.project.default_branch
+ commit = double('commit1', safe_message: "Fixes #{issue_1.to_reference} #{issue_2.to_reference} #{confidential_issue.to_reference}")
+ allow(subject).to receive(:commits).and_return([commit])
+ end
+
+ it 'shows only allowed issues to guest' do
+ subject.project.add_guest(guest)
+
+ subject.cache_merge_request_closes_issues!
+
+ expect(subject.visible_closing_issues_for(guest)).to match_array([issue_1, issue_2])
+ end
+
+ it 'shows only allowed issues to developer' do
+ subject.project.add_developer(developer)
+
+ subject.cache_merge_request_closes_issues!
+
+ expect(subject.visible_closing_issues_for(developer)).to match_array([issue_1, confidential_issue, issue_2])
+ end
+
+ context 'when external issue tracker is enabled' do
+ before do
+ subject.project.has_external_issue_tracker = true
+ subject.project.save!
+ end
+
+ it 'calls non #closes_issues to retrieve data' do
+ expect(subject).to receive(:closes_issues)
+ expect(subject).not_to receive(:cached_closes_issues)
+
+ subject.visible_closing_issues_for
+ end
+ end
+ end
+
describe '#cache_merge_request_closes_issues!' do
before do
subject.project.add_developer(subject.author)
@@ -324,6 +369,25 @@ describe MergeRequest do
expect { subject.cache_merge_request_closes_issues!(subject.author) }.to change(subject.merge_requests_closing_issues, :count).by(1)
end
+ it 'does not cache closed issues when merge request is closed' do
+ issue = create :issue, project: subject.project
+ commit = double('commit1', safe_message: "Fixes #{issue.to_reference}")
+
+ allow(subject).to receive(:commits).and_return([commit])
+ allow(subject).to receive(:state).and_return("closed")
+
+ expect { subject.cache_merge_request_closes_issues!(subject.author) }.not_to change(subject.merge_requests_closing_issues, :count)
+ end
+
+ it 'does not cache closed issues when merge request is merged' do
+ issue = create :issue, project: subject.project
+ commit = double('commit1', safe_message: "Fixes #{issue.to_reference}")
+ allow(subject).to receive(:commits).and_return([commit])
+ allow(subject).to receive(:state).and_return("merged")
+
+ expect { subject.cache_merge_request_closes_issues!(subject.author) }.not_to change(subject.merge_requests_closing_issues, :count)
+ end
+
context 'when both internal and external issue trackers are enabled' do
before do
subject.project.has_external_issue_tracker = true
@@ -632,6 +696,7 @@ describe MergeRequest do
allow(subject).to receive(:commits).and_return([commit])
allow(subject.project).to receive(:default_branch)
.and_return(subject.target_branch)
+ subject.cache_merge_request_closes_issues!
expect(subject.issues_mentioned_but_not_closing(subject.author)).to match_array([mentioned_issue])
end
@@ -649,6 +714,8 @@ describe MergeRequest do
end
it 'detects issues mentioned in description but not closed' do
+ subject.cache_merge_request_closes_issues!
+
expect(subject.issues_mentioned_but_not_closing(subject.author).map(&:to_s)).to match_array(['TEST-2'])
end
end
@@ -779,9 +846,8 @@ describe MergeRequest do
subject.project.add_developer(subject.author)
subject.description = "This issue Closes #{issue.to_reference}"
-
- allow(subject.project).to receive(:default_branch)
- .and_return(subject.target_branch)
+ allow(subject.project).to receive(:default_branch).and_return(subject.target_branch)
+ subject.cache_merge_request_closes_issues!
expect(subject.merge_commit_message)
.to match("Closes #{issue.to_reference}")
diff --git a/spec/presenters/merge_request_presenter_spec.rb b/spec/presenters/merge_request_presenter_spec.rb
index 46ba6f442f5..a1b52d8692d 100644
--- a/spec/presenters/merge_request_presenter_spec.rb
+++ b/spec/presenters/merge_request_presenter_spec.rb
@@ -117,9 +117,9 @@ describe MergeRequestPresenter do
before do
project.add_developer(user)
-
allow(resource.project).to receive(:default_branch)
.and_return(resource.target_branch)
+ resource.cache_merge_request_closes_issues!
end
describe '#closing_issues_links' do
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index 1716d182782..4de834bf93a 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -954,6 +954,7 @@ describe API::MergeRequests do
issue = create(:issue, project: project)
mr = merge_request.tap do |mr|
mr.update_attribute(:description, "Closes #{issue.to_reference(mr.project)}")
+ mr.cache_merge_request_closes_issues!
end
get api("/projects/#{project.id}/merge_requests/#{mr.iid}/closes_issues", user)
diff --git a/spec/services/issues/reopen_service_spec.rb b/spec/services/issues/reopen_service_spec.rb
index 3b617d4ca54..2a56075419b 100644
--- a/spec/services/issues/reopen_service_spec.rb
+++ b/spec/services/issues/reopen_service_spec.rb
@@ -20,7 +20,7 @@ describe Issues::ReopenService do
end
end
- context 'when user is authrized to reopen issue' do
+ context 'when user is authorized to reopen issue' do
let(:user) { create(:user) }
before do
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index 9dd235f6660..5d96b5ce27c 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -49,6 +49,7 @@ describe MergeRequests::MergeService do
issue = create :issue, project: project
commit = double('commit', safe_message: "Fixes #{issue.to_reference}")
allow(merge_request).to receive(:commits).and_return([commit])
+ merge_request.cache_merge_request_closes_issues!
service.execute(merge_request)
diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb
index ba2b062875b..5ad6f5528f9 100644
--- a/spec/services/merge_requests/post_merge_service_spec.rb
+++ b/spec/services/merge_requests/post_merge_service_spec.rb
@@ -53,7 +53,7 @@ describe MergeRequests::PostMergeService do
allow(project).to receive(:default_branch).and_return('foo')
issue = create(:issue, project: project)
- allow(merge_request).to receive(:closes_issues).and_return([issue])
+ allow(merge_request).to receive(:visible_closing_issues_for).and_return([issue])
allow_any_instance_of(Issues::CloseService).to receive(:execute).with(issue, commit: merge_request).and_raise
expect { described_class.new(project, user, {}).execute(merge_request) }.to raise_error
diff --git a/spec/services/merge_requests/reopen_service_spec.rb b/spec/services/merge_requests/reopen_service_spec.rb
index e10eaa95da4..21e71509ed6 100644
--- a/spec/services/merge_requests/reopen_service_spec.rb
+++ b/spec/services/merge_requests/reopen_service_spec.rb
@@ -47,6 +47,12 @@ describe MergeRequests::ReopenService do
end
end
+ it 'caches merge request closing issues' do
+ expect(merge_request).to receive(:cache_merge_request_closes_issues!)
+
+ described_class.new(project, user, {}).execute(merge_request)
+ end
+
it 'updates metrics' do
metrics = merge_request.metrics
service = double(MergeRequestMetricsService)
View Lorry Controller Status