maxmustermann/gitlab
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Merge branch '343173-move-creation-of-external-cross-references-into-background-worker' into 'master'

Browse source 
Move creation of external cross-references into background worker

See merge request gitlab-org/gitlab!72794
This commit is contained in:
Etienne Baqué 2021-11-10 19:15:57 +00:00
parent ba22262435 200f731230
commit 66abfcb84b
8 changed files with 304 additions and 228 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
app/services/system_notes/issuables_service.rb
View file

@ -176,7 +176,13 @@ def cross_reference(mentioner)
body = cross_reference_note_content(gfm_reference)
if noteable.is_a?(ExternalIssue)
noteable.project.external_issue_tracker.create_cross_reference_note(noteable, mentioner, author)
Integrations::CreateExternalCrossReferenceWorker.perform_async(
noteable.project_id,
noteable.id,
mentioner.class.name,
mentioner.id,
author.id
)
else
track_cross_reference_action
create_note(NoteSummary.new(noteable, noteable.project, author, body, action: 'cross_reference'))

9
app/workers/all_queues.yml
View file

@ -2231,6 +2231,15 @@
:weight: 2
:idempotent: true
:tags: []
- :name: integrations_create_external_cross_reference
:worker_name: Integrations::CreateExternalCrossReferenceWorker
:feature_category: :integrations
:has_external_dependencies:
:urgency: :low
:resource_boundary: :unknown
:weight: 1
:idempotent: true
:tags: []
- :name: invalid_gpg_signature_update
:worker_name: InvalidGpgSignatureUpdateWorker
:feature_category: :source_code_management

47
app/workers/integrations/create_external_cross_reference_worker.rb Normal file
View file

@ -0,0 +1,47 @@
# frozen_string_literal: true
module Integrations
class CreateExternalCrossReferenceWorker
include ApplicationWorker
data_consistency :delayed
feature_category :integrations
urgency :low
idempotent!
deduplicate :until_executed, including_scheduled: true
loggable_arguments 2
def perform(project_id, external_issue_id, mentionable_type, mentionable_id, author_id)
project = Project.find_by_id(project_id) || return
author = User.find_by_id(author_id) || return
mentionable = find_mentionable(mentionable_type, mentionable_id, project) || return
external_issue = ExternalIssue.new(external_issue_id, project)
project.external_issue_tracker.create_cross_reference_note(
external_issue,
mentionable,
author
)
end
private
def find_mentionable(mentionable_type, mentionable_id, project)
mentionable_class = mentionable_type.safe_constantize
# Passing an invalid mentionable_class is a developer error, so we don't want to retry the job
# but still track the exception on production, and raise it in development.
unless mentionable_class && mentionable_class < Mentionable
Gitlab::ErrorTracking.track_and_raise_for_dev_exception(ArgumentError.new("Unexpected class '#{mentionable_type}' is not a Mentionable"))
return
end
if mentionable_type == 'Commit'
project.commit(mentionable_id)
else
mentionable_class.find_by_id(mentionable_id)
end
end
end
end

2
config/sidekiq_queues.yml
View file

@ -201,6 +201,8 @@
- 1
- - incident_management_pending_escalations_alert_create
- 1
- - integrations_create_external_cross_reference
- 1
- - invalid_gpg_signature_update
- 2
- - irker

134
spec/models/integrations/jira_spec.rb
View file

@ -857,10 +857,14 @@ def close_issue
let_it_be(:user) { build_stubbed(:user) }
let(:jira_issue) { ExternalIssue.new('JIRA-123', project) }
let(:success_message) { 'SUCCESS: Successfully posted to http://jira.example.com.' }
let(:favicon_path) { "http://localhost/assets/#{find_asset('favicon.png').digest_path}" }
subject { jira_integration.create_cross_reference_note(jira_issue, resource, user) }
shared_examples 'creates a comment on Jira' do
shared_examples 'handles cross-references' do
let(:resource_name) { jira_integration.send(:noteable_name, resource) }
let(:resource_url) { jira_integration.send(:build_entity_url, resource_name, resource.to_param) }
let(:issue_url) { "#{url}/rest/api/2/issue/JIRA-123" }
let(:comment_url) { "#{issue_url}/comment" }
let(:remote_link_url) { "#{issue_url}/remotelink" }
@ -872,12 +876,65 @@ def close_issue
stub_request(:post, remote_link_url).with(basic_auth: [username, password])
end
it 'creates a comment on Jira' do
subject
context 'when enabled' do
before do
allow(jira_integration).to receive(:can_cross_reference?) { true }
end
expect(WebMock).to have_requested(:post, comment_url).with(
body: /mentioned this issue.*on branch \[master/
).once
it 'creates a comment and remote link' do
expect(subject).to eq(success_message)
expect(WebMock).to have_requested(:post, comment_url).with(body: comment_body).once
expect(WebMock).to have_requested(:post, remote_link_url).with(
body: hash_including(
GlobalID: 'GitLab',
relationship: 'mentioned on',
object: {
url: resource_url,
title: "#{resource.model_name.human} - #{resource.title}",
icon: { title: 'GitLab', url16x16: favicon_path },
status: { resolved: false }
}
)
).once
end
context 'when comment already exists' do
before do
allow(jira_integration).to receive(:comment_exists?) { true }
end
it 'does not create a comment or remote link' do
expect(subject).to be_nil
expect(WebMock).not_to have_requested(:post, comment_url)
expect(WebMock).not_to have_requested(:post, remote_link_url)
end
end
context 'when remote link already exists' do
let(:link) { double(object: { 'url' => resource_url }) }
before do
allow(jira_integration).to receive(:find_remote_link).and_return(link)
end
it 'updates the remote link but does not create a comment' do
expect(link).to receive(:save!)
expect(subject).to eq(success_message)
expect(WebMock).not_to have_requested(:post, comment_url)
end
end
end
context 'when disabled' do
before do
allow(jira_integration).to receive(:can_cross_reference?) { false }
end
it 'does not create a comment or remote link' do
expect(subject).to eq("Events for #{resource_name.pluralize.humanize(capitalize: false)} are disabled.")
expect(WebMock).not_to have_requested(:post, comment_url)
expect(WebMock).not_to have_requested(:post, remote_link_url)
end
end
context 'with jira_use_first_ref_by_oid feature flag disabled' do
@ -885,12 +942,10 @@ def close_issue
stub_feature_flags(jira_use_first_ref_by_oid: false)
end
it 'creates a comment on Jira' do
subject
expect(WebMock).to have_requested(:post, comment_url).with(
body: /mentioned this issue.*on branch \[master/
).once
it 'creates a comment and remote link on Jira' do
expect(subject).to eq(success_message)
expect(WebMock).to have_requested(:post, comment_url).with(body: comment_body).once
expect(WebMock).to have_requested(:post, remote_link_url).once
end
end
@ -903,39 +958,38 @@ def close_issue
end
end
context 'when resource is a commit' do
let(:resource) { project.commit('master') }
context 'when disabled' do
before do
allow_next_instance_of(described_class) do |instance|
allow(instance).to receive(:commit_events) { false }
end
end
it { is_expected.to eq('Events for commits are disabled.') }
end
context 'when enabled' do
it_behaves_like 'creates a comment on Jira'
context 'for commits' do
it_behaves_like 'handles cross-references' do
let(:resource) { project.commit('master') }
let(:comment_body) { /mentioned this issue in \[a commit\|.* on branch \[master\|/ }
end
end
context 'when resource is a merge request' do
let(:resource) { build_stubbed(:merge_request, source_project: project) }
context 'when disabled' do
before do
allow_next_instance_of(described_class) do |instance|
allow(instance).to receive(:merge_requests_events) { false }
end
end
it { is_expected.to eq('Events for merge requests are disabled.') }
context 'for issues' do
it_behaves_like 'handles cross-references' do
let(:resource) { build_stubbed(:issue, project: project) }
let(:comment_body) { /mentioned this issue in \[a issue\|/ }
end
end
context 'when enabled' do
it_behaves_like 'creates a comment on Jira'
context 'for merge requests' do
it_behaves_like 'handles cross-references' do
let(:resource) { build_stubbed(:merge_request, source_project: project) }
let(:comment_body) { /mentioned this issue in \[a merge request\|.* on branch \[master\|/ }
end
end
context 'for notes' do
it_behaves_like 'handles cross-references' do
let(:resource) { build_stubbed(:note, project: project) }
let(:comment_body) { /mentioned this issue in \[a note\|/ }
end
end
context 'for snippets' do
it_behaves_like 'handles cross-references' do
let(:resource) { build_stubbed(:snippet, project: project) }
let(:comment_body) { /mentioned this issue in \[a snippet\|/ }
end
end
end

187
spec/services/system_note_service_spec.rb
View file

@ -348,193 +348,6 @@
end
end
describe 'Jira integration' do
include JiraServiceHelper
let(:project) { create(:jira_project, :repository) }
let(:author) { create(:user) }
let(:issue) { create(:issue, project: project) }
let(:merge_request) { create(:merge_request, :simple, target_project: project, source_project: project) }
let(:jira_issue) { ExternalIssue.new("JIRA-1", project)}
let(:jira_tracker) { project.jira_integration }
let(:commit) { project.commit }
let(:comment_url) { jira_api_comment_url(jira_issue.id) }
let(:success_message) { "SUCCESS: Successfully posted to http://jira.example.net." }
before do
stub_jira_integration_test
stub_jira_urls(jira_issue.id)
jira_integration_settings
end
def cross_reference(type, link_exists = false)
noteable = type == 'commit' ? commit : merge_request
links = []
if link_exists
url = if type == 'commit'
"#{Settings.gitlab.base_url}/#{project.namespace.path}/#{project.path}/-/commit/#{commit.id}"
else
"#{Settings.gitlab.base_url}/#{project.namespace.path}/#{project.path}/-/merge_requests/#{merge_request.iid}"
end
link = double(object: { 'url' => url })
links << link
expect(link).to receive(:save!)
end
allow(JIRA::Resource::Remotelink).to receive(:all).and_return(links)
described_class.cross_reference(jira_issue, noteable, author)
end
noteable_types = %w(merge_requests commit)
noteable_types.each do |type|
context "when noteable is a #{type}" do
it "blocks cross reference when #{type.underscore}_events is false" do
jira_tracker.update!("#{type}_events" => false)
expect(cross_reference(type)).to eq(s_('JiraService|Events for %{noteable_model_name} are disabled.') % { noteable_model_name: type.pluralize.humanize.downcase })
end
it "creates cross reference when #{type.underscore}_events is true" do
jira_tracker.update!("#{type}_events" => true)
expect(cross_reference(type)).to eq(success_message)
end
end
context 'when a new cross reference is created' do
it 'creates a new comment and remote link' do
cross_reference(type)
expect(WebMock).to have_requested(:post, jira_api_comment_url(jira_issue))
expect(WebMock).to have_requested(:post, jira_api_remote_link_url(jira_issue))
end
end
context 'when a link exists' do
it 'updates a link but does not create a new comment' do
expect(WebMock).not_to have_requested(:post, jira_api_comment_url(jira_issue))
cross_reference(type, true)
end
end
end
describe "new reference" do
let(:favicon_path) { "http://localhost/assets/#{find_asset('favicon.png').digest_path}" }
before do
allow(JIRA::Resource::Remotelink).to receive(:all).and_return([])
end
context 'for commits' do
it "creates comment" do
result = described_class.cross_reference(jira_issue, commit, author)
expect(result).to eq(success_message)
end
it "creates remote link" do
described_class.cross_reference(jira_issue, commit, author)
expect(WebMock).to have_requested(:post, jira_api_remote_link_url(jira_issue)).with(
body: hash_including(
GlobalID: "GitLab",
relationship: 'mentioned on',
object: {
url: project_commit_url(project, commit),
title: "Commit - #{commit.title}",
icon: { title: "GitLab", url16x16: favicon_path },
status: { resolved: false }
}
)
).once
end
end
context 'for issues' do
let(:issue) { create(:issue, project: project) }
it "creates comment" do
result = described_class.cross_reference(jira_issue, issue, author)
expect(result).to eq(success_message)
end
it "creates remote link" do
described_class.cross_reference(jira_issue, issue, author)
expect(WebMock).to have_requested(:post, jira_api_remote_link_url(jira_issue)).with(
body: hash_including(
GlobalID: "GitLab",
relationship: 'mentioned on',
object: {
url: project_issue_url(project, issue),
title: "Issue - #{issue.title}",
icon: { title: "GitLab", url16x16: favicon_path },
status: { resolved: false }
}
)
).once
end
end
context 'for snippets' do
let(:snippet) { create(:snippet, project: project) }
it "creates comment" do
result = described_class.cross_reference(jira_issue, snippet, author)
expect(result).to eq(success_message)
end
it "creates remote link" do
described_class.cross_reference(jira_issue, snippet, author)
expect(WebMock).to have_requested(:post, jira_api_remote_link_url(jira_issue)).with(
body: hash_including(
GlobalID: "GitLab",
relationship: 'mentioned on',
object: {
url: project_snippet_url(project, snippet),
title: "Snippet - #{snippet.title}",
icon: { title: "GitLab", url16x16: favicon_path },
status: { resolved: false }
}
)
).once
end
end
end
describe "existing reference" do
before do
allow(JIRA::Resource::Remotelink).to receive(:all).and_return([])
message = double('message')
allow(message).to receive(:include?) { true }
allow_next_instance_of(JIRA::Resource::Issue) do |instance|
allow(instance).to receive(:comments).and_return([OpenStruct.new(body: message)])
end
end
it "does not return success message" do
result = described_class.cross_reference(jira_issue, commit, author)
expect(result).not_to eq(success_message)
end
it 'does not try to create comment and remote link' do
subject
expect(WebMock).not_to have_requested(:post, jira_api_comment_url(jira_issue))
expect(WebMock).not_to have_requested(:post, jira_api_remote_link_url(jira_issue))
end
end
end
describe '.change_time_estimate' do
it 'calls TimeTrackingService' do
expect_next_instance_of(::SystemNotes::TimeTrackingService) do |service|

17
spec/services/system_notes/issuables_service_spec.rb
View file

@ -347,6 +347,23 @@ def build_note(old_reviewers, new_reviewers)
end
end
end
context 'with external issue' do
let(:noteable) { ExternalIssue.new('JIRA-123', project) }
let(:mentioner) { project.commit }
it 'queues a background worker' do
expect(Integrations::CreateExternalCrossReferenceWorker).to receive(:perform_async).with(
project.id,
'JIRA-123',
'Commit',
mentioner.id,
author.id
)
subject
end
end
end
end

128
spec/workers/integrations/create_external_cross_reference_worker_spec.rb Normal file
View file

@ -0,0 +1,128 @@
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Integrations::CreateExternalCrossReferenceWorker do
include AfterNextHelpers
using RSpec::Parameterized::TableSyntax
let_it_be(:project) { create(:jira_project, :repository) }
let_it_be(:author) { create(:user) }
let_it_be(:commit) { project.commit }
let_it_be(:issue) { create(:issue, project: project) }
let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
let_it_be(:note) { create(:note, project: project) }
let_it_be(:snippet) { create(:project_snippet, project: project) }
let(:project_id) { project.id }
let(:external_issue_id) { 'JIRA-123' }
let(:mentionable_type) { 'Issue' }
let(:mentionable_id) { issue.id }
let(:author_id) { author.id }
let(:job_args) { [project_id, external_issue_id, mentionable_type, mentionable_id, author_id] }
def perform
described_class.new.perform(*job_args)
end
before do
allow(Project).to receive(:find_by_id).and_return(project)
end
it_behaves_like 'an idempotent worker' do
before do
allow(project.external_issue_tracker).to receive(:create_cross_reference_note)
end
it 'can run multiple times with the same arguments' do
subject
expect(project.external_issue_tracker).to have_received(:create_cross_reference_note)
.exactly(worker_exec_times).times
end
end
it 'has the `until_executed` deduplicate strategy' do
expect(described_class.get_deduplicate_strategy).to eq(:until_executed)
expect(described_class.get_deduplication_options).to include({ including_scheduled: true })
end
# These are the only models where we currently support cross-references,
# although this should be expanded to all `Mentionable` models.
# See https://gitlab.com/gitlab-org/gitlab/-/issues/343975
where(:mentionable_type, :mentionable_id) do
'Commit' | lazy { commit.id }
'Issue' | lazy { issue.id }
'MergeRequest' | lazy { merge_request.id }
'Note' | lazy { note.id }
'Snippet' | lazy { snippet.id }
end
with_them do
it 'creates a cross reference' do
expect(project.external_issue_tracker).to receive(:create_cross_reference_note).with(
be_a(ExternalIssue).and(have_attributes(id: external_issue_id, project: project)),
be_a(mentionable_type.constantize).and(have_attributes(id: mentionable_id)),
be_a(User).and(have_attributes(id: author_id))
)
perform
end
end
describe 'error handling' do
shared_examples 'does not create a cross reference' do
it 'does not create a cross reference' do
expect(project).not_to receive(:external_issue_tracker) if project
perform
end
end
context 'project_id does not exist' do
let(:project_id) { non_existing_record_id }
let(:project) { nil }
it_behaves_like 'does not create a cross reference'
end
context 'author_id does not exist' do
let(:author_id) { non_existing_record_id }
it_behaves_like 'does not create a cross reference'
end
context 'mentionable_id does not exist' do
let(:mentionable_id) { non_existing_record_id }
it_behaves_like 'does not create a cross reference'
end
context 'mentionable_type is not a Mentionable' do
let(:mentionable_type) { 'User' }
before do
expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).with(kind_of(ArgumentError))
end
it_behaves_like 'does not create a cross reference'
end
context 'mentionable_type is not a defined constant' do
let(:mentionable_type) { 'FooBar' }
before do
expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).with(kind_of(ArgumentError))
end
it_behaves_like 'does not create a cross reference'
end
context 'mentionable is a Commit and mentionable_id does not exist' do
let(:mentionable_type) { 'Commit' }
let(:mentionable_id) { non_existing_record_id }
it_behaves_like 'does not create a cross reference'
end
end
end