From bfc0b435f1f5fa9110c956feb260079d75dbe05e Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Fri, 19 Jan 2024 15:13:20 +1300 Subject: [PATCH 1/2] Refactor Integration propagation services As preparation for https://gitlab.com/gitlab-org/gitlab/-/issues/391526 this change refactors the service classes that propagate integrations. This change: - Namespaces services into Integrations::Propagation:: - Removes IntegrationList and DataList models https://gitlab.com/gitlab-org/gitlab/-/issues/391526 --- .rubocop_todo/gitlab/namespaced_class.yml | 3 - .rubocop_todo/rspec/feature_category.yml | 1 - .rubocop_todo/rspec/named_subject.yml | 1 - .../style/inline_disable_annotation.yml | 2 +- .../settings/integrations_controller.rb | 2 +- app/models/data_list.rb | 31 ---------- app/models/integrations/integration_list.rb | 29 ---------- .../bulk_create_integration_service.rb | 35 ------------ .../bulk_update_integration_service.rb | 43 -------------- .../integrations/bulk_operation_hashes.rb | 31 ---------- .../propagation/bulk_operation_hashes.rb | 33 +++++++++++ .../propagation/bulk_create_service.rb | 56 +++++++++++++++++++ .../propagation/bulk_update_service.rb | 47 ++++++++++++++++ .../propagate_integration_group_worker.rb | 2 +- ...e_integration_inherit_descendant_worker.rb | 2 +- .../propagate_integration_inherit_worker.rb | 2 +- .../propagate_integration_project_worker.rb | 2 +- doc/integration/jira/troubleshooting.md | 10 +++- spec/models/data_list_spec.rb | 31 ---------- .../integrations/integration_list_spec.rb | 22 -------- .../propagation/bulk_create_service_spec.rb} | 14 +++-- .../propagation/bulk_update_service_spec.rb} | 6 +- spec/support/rspec_order_todo.yml | 1 - ...propagate_integration_group_worker_spec.rb | 10 ++-- ...egration_inherit_descendant_worker_spec.rb | 6 +- ...opagate_integration_inherit_worker_spec.rb | 6 +- ...opagate_integration_project_worker_spec.rb | 10 ++-- 27 files changed, 178 insertions(+), 260 deletions(-) delete mode 100644 app/models/data_list.rb delete mode 100644 app/models/integrations/integration_list.rb delete mode 100644 app/services/bulk_create_integration_service.rb delete mode 100644 app/services/bulk_update_integration_service.rb delete mode 100644 app/services/concerns/integrations/bulk_operation_hashes.rb create mode 100644 app/services/concerns/integrations/propagation/bulk_operation_hashes.rb create mode 100644 app/services/integrations/propagation/bulk_create_service.rb create mode 100644 app/services/integrations/propagation/bulk_update_service.rb delete mode 100644 spec/models/data_list_spec.rb delete mode 100644 spec/models/integrations/integration_list_spec.rb rename spec/services/{bulk_create_integration_service_spec.rb => integrations/propagation/bulk_create_service_spec.rb} (88%) rename spec/services/{bulk_update_integration_service_spec.rb => integrations/propagation/bulk_update_service_spec.rb} (95%) diff --git a/.rubocop_todo/gitlab/namespaced_class.yml b/.rubocop_todo/gitlab/namespaced_class.yml index 4d3c8eb4a90a4e..7feee77f9ad9b2 100644 --- a/.rubocop_todo/gitlab/namespaced_class.yml +++ b/.rubocop_todo/gitlab/namespaced_class.yml @@ -135,7 +135,6 @@ Gitlab/NamespacedClass: - 'app/models/container_repository.rb' - 'app/models/context_commits_diff.rb' - 'app/models/custom_emoji.rb' - - 'app/models/data_list.rb' - 'app/models/deploy_key.rb' - 'app/models/deploy_keys_project.rb' - 'app/models/deploy_token.rb' @@ -625,9 +624,7 @@ Gitlab/NamespacedClass: - 'app/services/base_project_service.rb' - 'app/services/base_renderer.rb' - 'app/services/base_service.rb' - - 'app/services/bulk_create_integration_service.rb' - 'app/services/bulk_push_event_payload_service.rb' - - 'app/services/bulk_update_integration_service.rb' - 'app/services/cohorts_service.rb' - 'app/services/compare_service.rb' - 'app/services/event_create_service.rb' diff --git a/.rubocop_todo/rspec/feature_category.yml b/.rubocop_todo/rspec/feature_category.yml index 2b2054aed4cd32..7af4781c2a3919 100644 --- a/.rubocop_todo/rspec/feature_category.yml +++ b/.rubocop_todo/rspec/feature_category.yml @@ -4431,7 +4431,6 @@ RSpec/FeatureCategory: - 'spec/models/customer_relations/issue_contact_spec.rb' - 'spec/models/customer_relations/organization_spec.rb' - 'spec/models/cycle_analytics/project_level_stage_adapter_spec.rb' - - 'spec/models/data_list_spec.rb' - 'spec/models/dependency_proxy/blob_spec.rb' - 'spec/models/dependency_proxy/group_setting_spec.rb' - 'spec/models/dependency_proxy/image_ttl_group_policy_spec.rb' diff --git a/.rubocop_todo/rspec/named_subject.yml b/.rubocop_todo/rspec/named_subject.yml index 2e0e343d868d9f..baa9faa200ea3c 100644 --- a/.rubocop_todo/rspec/named_subject.yml +++ b/.rubocop_todo/rspec/named_subject.yml @@ -2730,7 +2730,6 @@ RSpec/NamedSubject: - 'spec/models/integrations/external_wiki_spec.rb' - 'spec/models/integrations/gitlab_slack_application_spec.rb' - 'spec/models/integrations/google_play_spec.rb' - - 'spec/models/integrations/integration_list_spec.rb' - 'spec/models/integrations/irker_spec.rb' - 'spec/models/integrations/jenkins_spec.rb' - 'spec/models/integrations/jira_spec.rb' diff --git a/.rubocop_todo/style/inline_disable_annotation.yml b/.rubocop_todo/style/inline_disable_annotation.yml index f6d56b477b73d6..f0ee70830426b7 100644 --- a/.rubocop_todo/style/inline_disable_annotation.yml +++ b/.rubocop_todo/style/inline_disable_annotation.yml @@ -588,7 +588,6 @@ Style/InlineDisableAnnotation: - 'app/services/bulk_imports/relation_export_service.rb' - 'app/services/bulk_imports/tree_export_service.rb' - 'app/services/bulk_imports/uploads_export_service.rb' - - 'app/services/bulk_update_integration_service.rb' - 'app/services/chat_names/find_user_service.rb' - 'app/services/ci/abort_pipelines_service.rb' - 'app/services/ci/archive_trace_service.rb' @@ -643,6 +642,7 @@ Style/InlineDisableAnnotation: - 'app/services/groups/destroy_service.rb' - 'app/services/groups/import_export/import_service.rb' - 'app/services/groups/transfer_service.rb' + - 'app/services/integrations/propagation/bulk_update_service.rb' - 'app/services/integrations/slack_options/label_search_handler.rb' - 'app/services/integrations/slack_options/user_search_handler.rb' - 'app/services/issuable_base_service.rb' diff --git a/app/controllers/projects/settings/integrations_controller.rb b/app/controllers/projects/settings/integrations_controller.rb index a0e72fb1687401..c3f4ca100ee3e9 100644 --- a/app/controllers/projects/settings/integrations_controller.rb +++ b/app/controllers/projects/settings/integrations_controller.rb @@ -35,7 +35,7 @@ def update integration.inherit_from_id = default_integration.id if saved = integration.save(context: :manual_change) - BulkUpdateIntegrationService.new(default_integration, [integration]).execute + ::Integrations::Propagation::BulkUpdateService.new(default_integration, [integration]).execute end else attributes[:inherit_from_id] = nil diff --git a/app/models/data_list.rb b/app/models/data_list.rb deleted file mode 100644 index e99364b2709266..00000000000000 --- a/app/models/data_list.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -class DataList - def initialize(batch, data_fields_hash, data_fields_klass) - @batch = batch - @data_fields_hash = data_fields_hash - @data_fields_klass = data_fields_klass - end - - def to_array - [data_fields_klass, columns, values] - end - - private - - attr_reader :batch, :data_fields_hash, :data_fields_klass - - def columns - data_fields_hash.keys << data_fields_foreign_key - end - - def data_fields_foreign_key - data_fields_klass.reflections['integration'].foreign_key - end - - def values - batch.map do |record| - data_fields_hash.values << record['id'] - end - end -end diff --git a/app/models/integrations/integration_list.rb b/app/models/integrations/integration_list.rb deleted file mode 100644 index ab03e5c0e0a687..00000000000000 --- a/app/models/integrations/integration_list.rb +++ /dev/null @@ -1,29 +0,0 @@ -# frozen_string_literal: true - -module Integrations - class IntegrationList - def initialize(batch, integration_hash, association) - @batch = batch - @integration_hash = integration_hash - @association = association - end - - def to_array - [Integration, columns, values] - end - - private - - attr_reader :batch, :integration_hash, :association - - def columns - integration_hash.keys << "#{association}_id" - end - - def values - batch.select(:id).map do |record| - integration_hash.values << record.id - end - end - end -end diff --git a/app/services/bulk_create_integration_service.rb b/app/services/bulk_create_integration_service.rb deleted file mode 100644 index 70c77444f13c23..00000000000000 --- a/app/services/bulk_create_integration_service.rb +++ /dev/null @@ -1,35 +0,0 @@ -# frozen_string_literal: true - -class BulkCreateIntegrationService - include Integrations::BulkOperationHashes - - def initialize(integration, batch, association) - @integration = integration - @batch = batch - @association = association - end - - def execute - integration_list = Integrations::IntegrationList.new(batch, integration_hash(:create), association).to_array - - Integration.transaction do - results = bulk_insert(*integration_list) - - if integration.data_fields_present? - data_list = DataList.new(results, data_fields_hash(:create), integration.data_fields.class).to_array - - bulk_insert(*data_list) - end - end - end - - private - - attr_reader :integration, :batch, :association - - def bulk_insert(klass, columns, values_array) - items_to_insert = values_array.map { |array| Hash[columns.zip(array)] } - - klass.insert_all(items_to_insert, returning: [:id]) - end -end diff --git a/app/services/bulk_update_integration_service.rb b/app/services/bulk_update_integration_service.rb deleted file mode 100644 index 57ceec57962119..00000000000000 --- a/app/services/bulk_update_integration_service.rb +++ /dev/null @@ -1,43 +0,0 @@ -# frozen_string_literal: true - -class BulkUpdateIntegrationService - include Integrations::BulkOperationHashes - - def initialize(integration, batch) - @integration = integration - @batch = batch - end - - # rubocop: disable CodeReuse/ActiveRecord - def execute - Integration.transaction do - Integration.where(id: batch_ids).update_all(integration_hash(:update)) - - if integration.data_fields_present? - integration.data_fields.class.where(data_fields_foreign_key => batch_ids) - .update_all( - data_fields_hash(:update) - ) - end - end - end - # rubocop: enable CodeReuse/ActiveRecord - - private - - attr_reader :integration, :batch - - # service_id or integration_id - def data_fields_foreign_key - integration.data_fields.class.reflections['integration'].foreign_key - end - - def batch_ids - @batch_ids ||= - if batch.is_a?(ActiveRecord::Relation) - batch.select(:id) - else - batch.map(&:id) - end - end -end diff --git a/app/services/concerns/integrations/bulk_operation_hashes.rb b/app/services/concerns/integrations/bulk_operation_hashes.rb deleted file mode 100644 index 3f13c764ebe780..00000000000000 --- a/app/services/concerns/integrations/bulk_operation_hashes.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -# Returns hashes of attributes suitable for passing to `.insert_all` or `update_all` -module Integrations - module BulkOperationHashes - private - - def integration_hash(operation) - integration - .to_database_hash - .merge('inherit_from_id' => integration.inherit_from_id || integration.id) - .merge(update_timestamps(operation)) - end - - def data_fields_hash(operation) - integration - .data_fields - .to_database_hash - .merge(update_timestamps(operation)) - end - - def update_timestamps(operation) - time_now = Time.current - - { - 'created_at' => (time_now if operation == :create), - 'updated_at' => time_now - }.compact - end - end -end diff --git a/app/services/concerns/integrations/propagation/bulk_operation_hashes.rb b/app/services/concerns/integrations/propagation/bulk_operation_hashes.rb new file mode 100644 index 00000000000000..5ec0b25e9aa0fd --- /dev/null +++ b/app/services/concerns/integrations/propagation/bulk_operation_hashes.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +# Returns hashes of attributes suitable for passing to `.insert_all` or `update_all` +module Integrations + module Propagation + module BulkOperationHashes + private + + def integration_hash(operation) + integration + .to_database_hash + .merge('inherit_from_id' => integration.inherit_from_id || integration.id) + .merge(update_timestamps(operation)) + end + + def data_fields_hash(operation) + integration + .data_fields + .to_database_hash + .merge(update_timestamps(operation)) + end + + def update_timestamps(operation) + time_now = Time.current + + { + 'created_at' => (time_now if operation == :create), + 'updated_at' => time_now + }.compact + end + end + end +end diff --git a/app/services/integrations/propagation/bulk_create_service.rb b/app/services/integrations/propagation/bulk_create_service.rb new file mode 100644 index 00000000000000..bec6e88b30808e --- /dev/null +++ b/app/services/integrations/propagation/bulk_create_service.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module Integrations + module Propagation + class BulkCreateService + include BulkOperationHashes + + def initialize(integration, batch, association) + @integration = integration + @batch = batch + @association = association + end + + def execute + Integration.transaction do + inserted_ids = bulk_insert_integrations + + bulk_insert_data_fields(inserted_ids) if integration.data_fields_present? + end + end + + private + + attr_reader :integration, :batch, :association + + def bulk_insert_new(model, items_to_insert) + model.insert_all( + items_to_insert, + returning: [:id] + ).pluck('id') # rubocop:disable CodeReuse/ActiveRecord, Database/AvoidUsingPluckWithoutLimit -- Enumerable#pluck + end + + def bulk_insert_integrations + attributes = integration_hash(:create) + + items_to_insert = batch.select(:id).map do |record| + attributes.merge("#{association}_id" => record.id) + end + + bulk_insert_new(Integration, items_to_insert) + end + + def bulk_insert_data_fields(integration_ids) + model = integration.data_fields.class + integration_fk_name = model.reflections['integration'].foreign_key + attributes = data_fields_hash(:create) + + items_to_insert = integration_ids.map do |id| + attributes.merge(integration_fk_name => id) + end + + bulk_insert_new(model, items_to_insert) + end + end + end +end diff --git a/app/services/integrations/propagation/bulk_update_service.rb b/app/services/integrations/propagation/bulk_update_service.rb new file mode 100644 index 00000000000000..e4a4c27c4ebe8d --- /dev/null +++ b/app/services/integrations/propagation/bulk_update_service.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +module Integrations + module Propagation + class BulkUpdateService + include BulkOperationHashes + + def initialize(integration, batch) + @integration = integration + @batch = batch + end + + # rubocop: disable CodeReuse/ActiveRecord + def execute + Integration.transaction do + Integration.where(id: batch_ids).update_all(integration_hash(:update)) + + if integration.data_fields_present? + integration.data_fields.class.where(data_fields_foreign_key => batch_ids) + .update_all( + data_fields_hash(:update) + ) + end + end + end + # rubocop: enable CodeReuse/ActiveRecord + + private + + attr_reader :integration, :batch + + # service_id or integration_id + def data_fields_foreign_key + integration.data_fields.class.reflections['integration'].foreign_key + end + + def batch_ids + @batch_ids ||= + if batch.is_a?(ActiveRecord::Relation) + batch.select(:id) + else + batch.map(&:id) + end + end + end + end +end diff --git a/app/workers/propagate_integration_group_worker.rb b/app/workers/propagate_integration_group_worker.rb index ed08e90f38d92f..c76fa881924eef 100644 --- a/app/workers/propagate_integration_group_worker.rb +++ b/app/workers/propagate_integration_group_worker.rb @@ -23,7 +23,7 @@ def perform(integration_id, min_id, max_id) return if batch.empty? - BulkCreateIntegrationService.new(integration, batch, 'group').execute + Integrations::Propagation::BulkCreateService.new(integration, batch, 'group').execute end # rubocop: enable CodeReuse/ActiveRecord end diff --git a/app/workers/propagate_integration_inherit_descendant_worker.rb b/app/workers/propagate_integration_inherit_descendant_worker.rb index 8b3ecc1f05713e..2d5cf676a81259 100644 --- a/app/workers/propagate_integration_inherit_descendant_worker.rb +++ b/app/workers/propagate_integration_inherit_descendant_worker.rb @@ -17,7 +17,7 @@ def perform(integration_id, min_id, max_id) batch = Integration.inherited_descendants_from_self_or_ancestors_from(integration).where(id: min_id..max_id) - BulkUpdateIntegrationService.new(integration, batch).execute + Integrations::Propagation::BulkUpdateService.new(integration, batch).execute end # rubocop: enable CodeReuse/ActiveRecord end diff --git a/app/workers/propagate_integration_inherit_worker.rb b/app/workers/propagate_integration_inherit_worker.rb index f0a53f8cb076c6..b9f41d41413359 100644 --- a/app/workers/propagate_integration_inherit_worker.rb +++ b/app/workers/propagate_integration_inherit_worker.rb @@ -17,7 +17,7 @@ def perform(integration_id, min_id, max_id) batch = Integration.where(id: min_id..max_id).by_type(integration.type).inherit_from_id(integration.id) - BulkUpdateIntegrationService.new(integration, batch).execute + Integrations::Propagation::BulkUpdateService.new(integration, batch).execute end # rubocop: enable CodeReuse/ActiveRecord end diff --git a/app/workers/propagate_integration_project_worker.rb b/app/workers/propagate_integration_project_worker.rb index bc55b7ff504e91..035bc112e1a760 100644 --- a/app/workers/propagate_integration_project_worker.rb +++ b/app/workers/propagate_integration_project_worker.rb @@ -20,7 +20,7 @@ def perform(integration_id, min_id, max_id) return if batch.empty? - BulkCreateIntegrationService.new(integration, batch, 'project').execute + Integrations::Propagation::BulkCreateService.new(integration, batch, 'project').execute end # rubocop: enable CodeReuse/ActiveRecord end diff --git a/doc/integration/jira/troubleshooting.md b/doc/integration/jira/troubleshooting.md index a9f190068b49a5..45ede3ae44c59b 100644 --- a/doc/integration/jira/troubleshooting.md +++ b/doc/integration/jira/troubleshooting.md @@ -137,7 +137,10 @@ To change all Jira projects to use instance-level integration settings: integration.inherit_from_id = default_integration.id if integration.save(context: :manual_change) - BulkUpdateIntegrationService.new(default_integration, [integration]).execute + # In GitLab 16.9 and later: + Integrations::Propagation::BulkUpdateService.new(default_integration, [integration]).execute + # In GitLab 16.8 and earlier, instead do: + # BulkUpdateIntegrationService.new(default_integration, [integration]).execute end end ``` @@ -171,7 +174,10 @@ To change all Jira projects in a group (and its subgroups) to use group-level in integration.inherit_from_id = default_integration.id if integration.save(context: :manual_change) - BulkUpdateIntegrationService.new(default_integration, [integration]).execute + # In GitLab 16.9 and later: + Integrations::Propagation::BulkUpdateService.new(default_integration, [integration]).execute + # In GitLab 16.8 and earlier, instead do: + # BulkUpdateIntegrationService.new(default_integration, [integration]).execute end end diff --git a/spec/models/data_list_spec.rb b/spec/models/data_list_spec.rb deleted file mode 100644 index 6e01f4786ba21d..00000000000000 --- a/spec/models/data_list_spec.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe DataList do - describe '#to_array' do - let(:jira_integration) { create(:jira_integration) } - let(:zentao_integration) { create(:zentao_integration) } - let(:cases) do - [ - [jira_integration, 'Integrations::JiraTrackerData', 'integration_id'], - [zentao_integration, 'Integrations::ZentaoTrackerData', 'integration_id'] - ] - end - - def data_list(integration) - DataList.new([integration], integration.to_database_hash, integration.data_fields.class).to_array - end - - it 'returns current data' do - cases.each do |integration, data_fields_class_name, foreign_key| - data_fields_klass, columns, values_items = data_list(integration) - - expect(data_fields_klass.to_s).to eq data_fields_class_name - expect(columns.last).to eq foreign_key - values = values_items.first - expect(values.last).to eq integration.id - end - end - end -end diff --git a/spec/models/integrations/integration_list_spec.rb b/spec/models/integrations/integration_list_spec.rb deleted file mode 100644 index 4bb7b100bc0eb7..00000000000000 --- a/spec/models/integrations/integration_list_spec.rb +++ /dev/null @@ -1,22 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Integrations::IntegrationList, feature_category: :integrations do - let_it_be(:projects) { create_pair(:project, :small_repo) } - let(:batch) { Project.where(id: projects.pluck(:id)) } - let(:integration_hash) { { 'active' => 'true', 'category' => 'common' } } - let(:association) { 'project' } - - subject { described_class.new(batch, integration_hash, association) } - - describe '#to_array' do - it 'returns array of Integration, columns, and values' do - expect(subject.to_array).to match_array([ - Integration, - %w[active category project_id], - contain_exactly(['true', 'common', projects.first.id], ['true', 'common', projects.second.id]) - ]) - end - end -end diff --git a/spec/services/bulk_create_integration_service_spec.rb b/spec/services/integrations/propagation/bulk_create_service_spec.rb similarity index 88% rename from spec/services/bulk_create_integration_service_spec.rb rename to spec/services/integrations/propagation/bulk_create_service_spec.rb index 57bdfdbd4cb30d..8888e06ee651eb 100644 --- a/spec/services/bulk_create_integration_service_spec.rb +++ b/spec/services/integrations/propagation/bulk_create_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe BulkCreateIntegrationService, feature_category: :integrations do +RSpec.describe Integrations::Propagation::BulkCreateService, feature_category: :integrations do include JiraIntegrationHelpers before_all do @@ -32,7 +32,7 @@ def attributes(record) expect(attributes(created_integration)).to eq attributes(integration) end - context 'integration with data fields' do + context 'when integration has data fields' do let(:excluded_attributes) { %w[id service_id integration_id created_at updated_at] } it 'updates the data fields from inherited integrations' do @@ -68,7 +68,7 @@ def attributes(record) end end - context 'passing an instance-level integration' do + context 'with an instance-level integration' do let(:integration) { instance_integration } let(:inherit_from_id) { integration.id } @@ -91,14 +91,14 @@ def attributes(record) end end - context 'passing a group integration' do + context 'with a group-level integration' do let_it_be(:group) { create(:group) } context 'with a project association' do let!(:project) { create(:project, group: group) } let(:integration) { create(:jira_integration, :group, group: group) } let(:created_integration) { project.jira_integration } - let(:batch) { Project.where(id: Project.minimum(:id)..Project.maximum(:id)).without_integration(integration).in_namespace(integration.group.self_and_descendants) } + let(:batch) { Project.without_integration(integration).in_namespace(integration.group.self_and_descendants) } let(:association) { 'project' } let(:inherit_from_id) { integration.id } @@ -123,7 +123,9 @@ def attributes(record) it_behaves_like 'creates integration successfully' context 'with different foreign key of data_fields' do - let(:integration) { create(:zentao_integration, :group, group: group, inherit_from_id: instance_integration.id) } + let(:integration) do + create(:zentao_integration, :group, group: group, inherit_from_id: instance_integration.id) + end it_behaves_like 'creates integration successfully' end diff --git a/spec/services/bulk_update_integration_service_spec.rb b/spec/services/integrations/propagation/bulk_update_service_spec.rb similarity index 95% rename from spec/services/bulk_update_integration_service_spec.rb rename to spec/services/integrations/propagation/bulk_update_service_spec.rb index 9095fa9a0fab06..837ddbb64563b0 100644 --- a/spec/services/bulk_update_integration_service_spec.rb +++ b/spec/services/integrations/propagation/bulk_update_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe BulkUpdateIntegrationService, feature_category: :integrations do +RSpec.describe Integrations::Propagation::BulkUpdateService, feature_category: :integrations do include JiraIntegrationHelpers before_all do @@ -17,7 +17,9 @@ end let(:batch) do - Integration.inherited_descendants_from_self_or_ancestors_from(subgroup_integration).where(id: group_integration.id..integration.id) + Integration + .inherited_descendants_from_self_or_ancestors_from(subgroup_integration) + .where(id: group_integration.id..integration.id) end let_it_be(:group) { create(:group) } diff --git a/spec/support/rspec_order_todo.yml b/spec/support/rspec_order_todo.yml index d4f7cbd8e0c467..3a515218b0d6ae 100644 --- a/spec/support/rspec_order_todo.yml +++ b/spec/support/rspec_order_todo.yml @@ -7189,7 +7189,6 @@ - './spec/models/customer_relations/issue_contact_spec.rb' - './spec/models/customer_relations/organization_spec.rb' - './spec/models/cycle_analytics/project_level_stage_adapter_spec.rb' -- './spec/models/data_list_spec.rb' - './spec/models/dependency_proxy/blob_spec.rb' - './spec/models/dependency_proxy/group_setting_spec.rb' - './spec/models/dependency_proxy/image_ttl_group_policy_spec.rb' diff --git a/spec/workers/propagate_integration_group_worker_spec.rb b/spec/workers/propagate_integration_group_worker_spec.rb index 0d797d22137697..a477f368674855 100644 --- a/spec/workers/propagate_integration_group_worker_spec.rb +++ b/spec/workers/propagate_integration_group_worker_spec.rb @@ -13,8 +13,8 @@ let(:job_args) { [integration.id, group.id, subgroup2.id] } it_behaves_like 'an idempotent worker' do - it 'calls to BulkCreateIntegrationService' do - expect(BulkCreateIntegrationService).to receive(:new) + it 'calls to Integrations::Propagation::BulkCreateService' do + expect(Integrations::Propagation::BulkCreateService).to receive(:new) .with(integration, match_array([group, another_group, subgroup1, subgroup2]), 'group').twice .and_return(double(execute: nil)) @@ -24,8 +24,8 @@ context 'with a group integration' do let_it_be(:integration) { create(:redmine_integration, :group, group: group) } - it 'calls to BulkCreateIntegrationService' do - expect(BulkCreateIntegrationService).to receive(:new) + it 'calls to Integrations::Propagation::BulkCreateService' do + expect(Integrations::Propagation::BulkCreateService).to receive(:new) .with(integration, match_array([subgroup1, subgroup2]), 'group').twice .and_return(double(execute: nil)) @@ -36,7 +36,7 @@ context 'with an invalid integration id' do it 'returns without failure' do - expect(BulkCreateIntegrationService).not_to receive(:new) + expect(Integrations::Propagation::BulkCreateService).not_to receive(:new) subject.perform(0, 1, 100) end diff --git a/spec/workers/propagate_integration_inherit_descendant_worker_spec.rb b/spec/workers/propagate_integration_inherit_descendant_worker_spec.rb index d69dd45a2094bf..1b6ffe77875c94 100644 --- a/spec/workers/propagate_integration_inherit_descendant_worker_spec.rb +++ b/spec/workers/propagate_integration_inherit_descendant_worker_spec.rb @@ -11,8 +11,8 @@ it_behaves_like 'an idempotent worker' do let(:job_args) { [group_integration.id, subgroup_integration.id, subgroup_integration.id] } - it 'calls to BulkUpdateIntegrationService' do - expect(BulkUpdateIntegrationService).to receive(:new) + it 'calls to Integrations::Propagation::BulkUpdateService' do + expect(Integrations::Propagation::BulkUpdateService).to receive(:new) .with(group_integration, match_array(subgroup_integration)).twice .and_return(double(execute: nil)) @@ -22,7 +22,7 @@ context 'with an invalid integration id' do it 'returns without failure' do - expect(BulkUpdateIntegrationService).not_to receive(:new) + expect(Integrations::Propagation::BulkUpdateService).not_to receive(:new) subject.perform(0, subgroup_integration.id, subgroup_integration.id) end diff --git a/spec/workers/propagate_integration_inherit_worker_spec.rb b/spec/workers/propagate_integration_inherit_worker_spec.rb index f5535696fd1724..5d2c03196c3a44 100644 --- a/spec/workers/propagate_integration_inherit_worker_spec.rb +++ b/spec/workers/propagate_integration_inherit_worker_spec.rb @@ -12,8 +12,8 @@ it_behaves_like 'an idempotent worker' do let(:job_args) { [integration.id, integration1.id, integration3.id] } - it 'calls to BulkUpdateIntegrationService' do - expect(BulkUpdateIntegrationService).to receive(:new) + it 'calls to Integrations::Propagation::BulkUpdateService' do + expect(Integrations::Propagation::BulkUpdateService).to receive(:new) .with(integration, match_array(integration1)).twice .and_return(double(execute: nil)) @@ -23,7 +23,7 @@ context 'with an invalid integration id' do it 'returns without failure' do - expect(BulkUpdateIntegrationService).not_to receive(:new) + expect(Integrations::Propagation::BulkUpdateService).not_to receive(:new) subject.perform(0, integration1.id, integration3.id) end diff --git a/spec/workers/propagate_integration_project_worker_spec.rb b/spec/workers/propagate_integration_project_worker_spec.rb index c7adf1b826fc93..68d8b29a9736f8 100644 --- a/spec/workers/propagate_integration_project_worker_spec.rb +++ b/spec/workers/propagate_integration_project_worker_spec.rb @@ -13,8 +13,8 @@ let(:job_args) { [integration.id, project1.id, project3.id] } it_behaves_like 'an idempotent worker' do - it 'calls to BulkCreateIntegrationService' do - expect(BulkCreateIntegrationService).to receive(:new) + it 'calls to Integrations::Propagation::BulkCreateService' do + expect(Integrations::Propagation::BulkCreateService).to receive(:new) .with(integration, match_array([project1, project2, project3]), 'project').twice .and_return(double(execute: nil)) @@ -24,8 +24,8 @@ context 'with a group integration' do let_it_be(:integration) { create(:redmine_integration, :group, group: group) } - it 'calls to BulkCreateIntegrationService' do - expect(BulkCreateIntegrationService).to receive(:new) + it 'calls to Integrations::Propagation::BulkCreateService' do + expect(Integrations::Propagation::BulkCreateService).to receive(:new) .with(integration, match_array([project2, project3]), 'project').twice .and_return(double(execute: nil)) @@ -36,7 +36,7 @@ context 'with an invalid integration id' do it 'returns without failure' do - expect(BulkCreateIntegrationService).not_to receive(:new) + expect(Integrations::Propagation::BulkCreateService).not_to receive(:new) subject.perform(0, 1, 100) end -- GitLab From abda5fea7bbb8a71e965525c3c5b36c0f9e153c0 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Thu, 1 Feb 2024 17:21:22 +1300 Subject: [PATCH 2/2] Add reviewer feedback --- .../projects/settings/integrations_controller.rb | 8 ++++---- .../integrations/propagation/bulk_create_service.rb | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/controllers/projects/settings/integrations_controller.rb b/app/controllers/projects/settings/integrations_controller.rb index c3f4ca100ee3e9..ad8ddf182b7138 100644 --- a/app/controllers/projects/settings/integrations_controller.rb +++ b/app/controllers/projects/settings/integrations_controller.rb @@ -34,18 +34,18 @@ def update if use_inherited_settings?(attributes) integration.inherit_from_id = default_integration.id - if saved = integration.save(context: :manual_change) + if updated = integration.save(context: :manual_change) ::Integrations::Propagation::BulkUpdateService.new(default_integration, [integration]).execute end else attributes[:inherit_from_id] = nil integration.attributes = attributes - saved = integration.save(context: :manual_change) + updated = integration.save(context: :manual_change) end respond_to do |format| format.html do - if saved + if updated redirect_to redirect_path, notice: success_message else render 'edit' @@ -53,7 +53,7 @@ def update end format.json do - status = saved ? :ok : :unprocessable_entity + status = updated ? :ok : :unprocessable_entity render json: serialize_as_json, status: status end diff --git a/app/services/integrations/propagation/bulk_create_service.rb b/app/services/integrations/propagation/bulk_create_service.rb index bec6e88b30808e..766516a1ff3f38 100644 --- a/app/services/integrations/propagation/bulk_create_service.rb +++ b/app/services/integrations/propagation/bulk_create_service.rb @@ -27,7 +27,7 @@ def bulk_insert_new(model, items_to_insert) model.insert_all( items_to_insert, returning: [:id] - ).pluck('id') # rubocop:disable CodeReuse/ActiveRecord, Database/AvoidUsingPluckWithoutLimit -- Enumerable#pluck + ).rows.flatten end def bulk_insert_integrations -- GitLab