From c2b330dc62c931fc234ca46d0e422fce2f99e560 Mon Sep 17 00:00:00 2001 From: Arturo Herrero Date: Wed, 21 Oct 2020 13:28:45 +0100 Subject: [PATCH 1/3] Fix project callbacks when propagating integrations After introducing the propagation of integrations using batching and queues https://gitlab.com/gitlab-org/gitlab/-/merge_requests/42128, we are not updating the project callbacks: has_external_issue_tracker and has_external_wiki. The reason for that is that the batch was uncached but after the merge request batch is scoped to the query. In this case, the order is important because we do two things: - Create the integration for those projects without integration. - Update has_external_issue_tracker and has_external_wiki in those projects. If we first create the integrations when we then try to update the callbacks, there is nothing to update since the batch scope is empty at this time. We have to update first and then create the integrations. --- app/services/bulk_create_integration_service.rb | 10 ++++------ ...fix-propagate-integrations-for-project-callback.yml | 5 +++++ spec/services/bulk_create_integration_service_spec.rb | 2 +- 3 files changed, 10 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/268133-fix-propagate-integrations-for-project-callback.yml diff --git a/app/services/bulk_create_integration_service.rb b/app/services/bulk_create_integration_service.rb index 23b89b0d8a9db3..b75a6c1b91e9af 100644 --- a/app/services/bulk_create_integration_service.rb +++ b/app/services/bulk_create_integration_service.rb @@ -11,6 +11,8 @@ def execute service_list = ServiceList.new(batch, service_hash, association).to_array Service.transaction do + run_callbacks(batch) if association == 'project' + results = bulk_insert(*service_list) if integration.data_fields_present? @@ -18,8 +20,6 @@ def execute bulk_insert(*data_list) end - - run_callbacks(batch) if association == 'project' end end @@ -33,17 +33,15 @@ def bulk_insert(klass, columns, values_array) klass.insert_all(items_to_insert, returning: [:id]) end - # rubocop: disable CodeReuse/ActiveRecord def run_callbacks(batch) if integration.issue_tracker? - Project.where(id: batch.select(:id)).update_all(has_external_issue_tracker: true) + batch.update_all(has_external_issue_tracker: true) end if integration.type == 'ExternalWikiService' - Project.where(id: batch.select(:id)).update_all(has_external_wiki: true) + batch.update_all(has_external_wiki: true) end end - # rubocop: enable CodeReuse/ActiveRecord def service_hash if integration.template? diff --git a/changelogs/unreleased/268133-fix-propagate-integrations-for-project-callback.yml b/changelogs/unreleased/268133-fix-propagate-integrations-for-project-callback.yml new file mode 100644 index 00000000000000..6af3e240d160de --- /dev/null +++ b/changelogs/unreleased/268133-fix-propagate-integrations-for-project-callback.yml @@ -0,0 +1,5 @@ +--- +title: Fix project callbacks when propagating integrations +merge_request: 45781 +author: +type: fixed diff --git a/spec/services/bulk_create_integration_service_spec.rb b/spec/services/bulk_create_integration_service_spec.rb index 5d896f78b351e2..344b7cf885e302 100644 --- a/spec/services/bulk_create_integration_service_spec.rb +++ b/spec/services/bulk_create_integration_service_spec.rb @@ -72,7 +72,7 @@ context 'with a project association' do let!(:project) { create(:project) } let(:created_integration) { project.jira_service } - let(:batch) { Project.all } + let(:batch) { Project.without_integration(integration) } let(:association) { 'project' } it_behaves_like 'creates integration from batch ids' -- GitLab From e186a47d9f112357866ba8d5774cce5cf58f4937 Mon Sep 17 00:00:00 2001 From: Arturo Herrero Date: Thu, 22 Oct 2020 11:41:30 +0100 Subject: [PATCH 2/3] Update project callbacks if integration is active Originally, we only call here to propagate service templates if they are active. Now, we are propagating instance-level integrations and group-level integrations (active or not). has_external_issue_tracker and has_external_wiki assume that the integration is active. So, we only need to update the project callbacks if the integration is active. --- .../bulk_create_integration_service.rb | 4 +- spec/factories/projects.rb | 4 +- spec/requests/api/projects_spec.rb | 4 +- .../bulk_create_integration_service_spec.rb | 39 +++++++++++++++++-- 4 files changed, 42 insertions(+), 9 deletions(-) diff --git a/app/services/bulk_create_integration_service.rb b/app/services/bulk_create_integration_service.rb index b75a6c1b91e9af..2cec7e719891e4 100644 --- a/app/services/bulk_create_integration_service.rb +++ b/app/services/bulk_create_integration_service.rb @@ -34,11 +34,11 @@ def bulk_insert(klass, columns, values_array) end def run_callbacks(batch) - if integration.issue_tracker? + if integration.issue_tracker? && integration.active? batch.update_all(has_external_issue_tracker: true) end - if integration.type == 'ExternalWikiService' + if integration.type == 'ExternalWikiService' && integration.active? batch.update_all(has_external_wiki: true) end end diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index 87e4a8e355d306..639fff06cecef6 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -10,8 +10,10 @@ factory :project, class: 'Project' do sequence(:name) { |n| "project#{n}" } path { name.downcase.gsub(/\s/, '_') } - # Behaves differently to nil due to cache_has_external_issue_tracker + + # Behaves differently to nil due to cache_has_external_* methods. has_external_issue_tracker { false } + has_external_wiki { false } # Associations namespace diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 2abcb39a1c8cd2..306478774c1a5c 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -890,7 +890,7 @@ expect(response).to have_gitlab_http_status(:created) project.each_pair do |k, v| - next if %i[has_external_issue_tracker issues_enabled merge_requests_enabled wiki_enabled storage_version].include?(k) + next if %i[has_external_issue_tracker has_external_wiki issues_enabled merge_requests_enabled wiki_enabled storage_version].include?(k) expect(json_response[k.to_s]).to eq(v) end @@ -1309,7 +1309,7 @@ expect(response).to have_gitlab_http_status(:created) project.each_pair do |k, v| - next if %i[has_external_issue_tracker path storage_version].include?(k) + next if %i[has_external_issue_tracker has_external_wiki path storage_version].include?(k) expect(json_response[k.to_s]).to eq(v) end diff --git a/spec/services/bulk_create_integration_service_spec.rb b/spec/services/bulk_create_integration_service_spec.rb index 344b7cf885e302..838b199fe89820 100644 --- a/spec/services/bulk_create_integration_service_spec.rb +++ b/spec/services/bulk_create_integration_service_spec.rb @@ -41,7 +41,7 @@ end end - shared_examples 'runs project callbacks' do + shared_examples 'updates project callbacks' do it 'updates projects#has_external_issue_tracker for issue tracker services' do described_class.new(integration, batch, association).execute @@ -53,7 +53,6 @@ ExternalWikiService.create!( instance: true, active: true, - push_events: false, external_wiki_url: 'http://external-wiki-url.com' ) end @@ -66,6 +65,30 @@ end end + shared_examples 'does not update project callbacks' do + it 'does not update projects#has_external_issue_tracker for issue tracker services' do + described_class.new(integration, batch, association).execute + + expect(project.reload.has_external_issue_tracker).to eq(false) + end + + context 'with an inactive external wiki integration' do + let(:integration) do + ExternalWikiService.create!( + instance: true, + active: false, + external_wiki_url: 'http://external-wiki-url.com' + ) + end + + it 'does not update projects#has_external_wiki for external wiki services' do + described_class.new(integration, batch, association).execute + + expect(project.reload.has_external_wiki).to eq(false) + end + end + end + context 'with an instance-level integration' do let(:integration) { instance_integration } @@ -77,7 +100,15 @@ it_behaves_like 'creates integration from batch ids' it_behaves_like 'updates inherit_from_id' - it_behaves_like 'runs project callbacks' + it_behaves_like 'updates project callbacks' + + context 'when integration is not active' do + before do + integration.update!(active: false) + end + + it_behaves_like 'does not update project callbacks' + end end context 'with a group association' do @@ -101,7 +132,7 @@ let(:association) { 'project' } it_behaves_like 'creates integration from batch ids' - it_behaves_like 'runs project callbacks' + it_behaves_like 'updates project callbacks' end end end -- GitLab From 7f9140cfd9ab3f16a195209e11aabffd422582dd Mon Sep 17 00:00:00 2001 From: Arturo Herrero Date: Thu, 22 Oct 2020 11:54:18 +0100 Subject: [PATCH 3/3] Extract external wiki service factory --- spec/factories/services.rb | 7 +++++++ .../bulk_create_integration_service_spec.rb | 16 ++-------------- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/spec/factories/services.rb b/spec/factories/services.rb index 139970808170c2..f4b73b6ed70b64 100644 --- a/spec/factories/services.rb +++ b/spec/factories/services.rb @@ -139,6 +139,13 @@ end end + factory :external_wiki_service do + project + type { ExternalWikiService } + active { true } + external_wiki_url { 'http://external-wiki-url.com' } + end + factory :open_project_service do project active { true } diff --git a/spec/services/bulk_create_integration_service_spec.rb b/spec/services/bulk_create_integration_service_spec.rb index 838b199fe89820..140b7a84d40fe1 100644 --- a/spec/services/bulk_create_integration_service_spec.rb +++ b/spec/services/bulk_create_integration_service_spec.rb @@ -49,13 +49,7 @@ end context 'with an external wiki integration' do - let(:integration) do - ExternalWikiService.create!( - instance: true, - active: true, - external_wiki_url: 'http://external-wiki-url.com' - ) - end + let(:integration) { create(:external_wiki_service, :instance) } it 'updates projects#has_external_wiki for external wiki services' do described_class.new(integration, batch, association).execute @@ -73,13 +67,7 @@ end context 'with an inactive external wiki integration' do - let(:integration) do - ExternalWikiService.create!( - instance: true, - active: false, - external_wiki_url: 'http://external-wiki-url.com' - ) - end + let(:integration) { create(:external_wiki_service, :instance, active: false) } it 'does not update projects#has_external_wiki for external wiki services' do described_class.new(integration, batch, association).execute -- GitLab