diff --git a/app/services/system_notes/issuables_service.rb b/app/services/system_notes/issuables_service.rb index 62aead352aa8..94629ae7609c 100644 --- a/app/services/system_notes/issuables_service.rb +++ b/app/services/system_notes/issuables_service.rb @@ -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')) diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index ae39bd602337..4fa74195a993 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -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 diff --git a/app/workers/integrations/create_external_cross_reference_worker.rb b/app/workers/integrations/create_external_cross_reference_worker.rb new file mode 100644 index 000000000000..02c1315249e1 --- /dev/null +++ b/app/workers/integrations/create_external_cross_reference_worker.rb @@ -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 diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 00e1fb6b6696..3f0216e78ac1 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -201,6 +201,8 @@ - 1 - - incident_management_pending_escalations_alert_create - 1 +- - integrations_create_external_cross_reference + - 1 - - invalid_gpg_signature_update - 2 - - irker diff --git a/spec/models/integrations/jira_spec.rb b/spec/models/integrations/jira_spec.rb index 7e4dcea279a9..1d81668f97d7 100644 --- a/spec/models/integrations/jira_spec.rb +++ b/spec/models/integrations/jira_spec.rb @@ -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 diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index d8c4ee1e78f9..ce0122ae3014 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -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| diff --git a/spec/services/system_notes/issuables_service_spec.rb b/spec/services/system_notes/issuables_service_spec.rb index 71a28a89cd84..fd481aa6ddb0 100644 --- a/spec/services/system_notes/issuables_service_spec.rb +++ b/spec/services/system_notes/issuables_service_spec.rb @@ -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 diff --git a/spec/workers/integrations/create_external_cross_reference_worker_spec.rb b/spec/workers/integrations/create_external_cross_reference_worker_spec.rb new file mode 100644 index 000000000000..61723f44aa59 --- /dev/null +++ b/spec/workers/integrations/create_external_cross_reference_worker_spec.rb @@ -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