From cf34ede24b14fe6e1523a4a2d735dfadf5af13d4 Mon Sep 17 00:00:00 2001 From: Sam Word Date: Mon, 29 Jul 2024 15:19:22 -0400 Subject: [PATCH 1/7] Added new worker & service to reassign placeholder users, start worker in service --- .../import/source_users_controller.rb | 4 +- app/models/import/source_user.rb | 2 + .../source_user_placeholder_reference.rb | 52 ++++ .../accept_reassignment_service.rb | 31 +++ ...assign_placeholder_user_records_service.rb | 69 +++++ app/workers/all_queues.yml | 9 + ...eassign_placeholder_user_records_worker.rb | 63 +++++ config/sidekiq_queues.yml | 2 + spec/factories/import_source_users.rb | 5 + .../import/source_users_finder_spec.rb | 5 +- .../source_user_placeholder_reference_spec.rb | 176 ++++++++++++ spec/models/import/source_user_spec.rb | 21 +- .../import/source_users_controller_spec.rb | 6 + .../accept_reassignment_service_spec.rb | 62 +++++ .../resend_notification_service_spec.rb | 2 +- ...n_placeholder_user_records_service_spec.rb | 250 ++++++++++++++++++ spec/workers/every_sidekiq_worker_spec.rb | 1 + ...gn_placeholder_user_records_worker_spec.rb | 83 ++++++ 18 files changed, 836 insertions(+), 7 deletions(-) create mode 100644 app/services/import/source_users/accept_reassignment_service.rb create mode 100644 app/services/users/reassign_placeholder_user_records_service.rb create mode 100644 app/workers/users/reassign_placeholder_user_records_worker.rb create mode 100644 spec/services/import/source_users/accept_reassignment_service_spec.rb create mode 100644 spec/services/users/reassign_placeholder_user_records_service_spec.rb create mode 100644 spec/workers/users/reassign_placeholder_user_records_worker_spec.rb diff --git a/app/controllers/import/source_users_controller.rb b/app/controllers/import/source_users_controller.rb index 9cd4353fb1485e..415fd6f171839c 100644 --- a/app/controllers/import/source_users_controller.rb +++ b/app/controllers/import/source_users_controller.rb @@ -12,9 +12,9 @@ class SourceUsersController < ApplicationController feature_category :importers def accept - if source_user.accept - # TODO: This is where we enqueue the job to assign the contributions. + result = ::Import::SourceUsers::AcceptReassignmentService.new(source_user, current_user: current_user).execute + if result.status == :success flash[:raw] = banner('accept_invite') redirect_to(dashboard_groups_path) else diff --git a/app/models/import/source_user.rb b/app/models/import/source_user.rb index 29141ad4d91ee6..9bab14c2845a31 100644 --- a/app/models/import/source_user.rb +++ b/app/models/import/source_user.rb @@ -2,6 +2,7 @@ module Import class SourceUser < ApplicationRecord + include AfterCommitQueue include Gitlab::SQL::Pattern self.table_name = 'import_source_users' @@ -20,6 +21,7 @@ class SourceUser < ApplicationRecord validates :namespace_id, :import_type, :source_hostname, :source_user_identifier, :status, presence: true validates :placeholder_user_id, presence: true, unless: :completed? + validates :reassign_to_user_id, presence: true, if: :reassignment_in_progress? scope :for_namespace, ->(namespace_id) { where(namespace_id: namespace_id) } scope :by_statuses, ->(statuses) { where(status: statuses) } diff --git a/app/models/import/source_user_placeholder_reference.rb b/app/models/import/source_user_placeholder_reference.rb index 4542e82f114164..9026fa55ce44b7 100644 --- a/app/models/import/source_user_placeholder_reference.rb +++ b/app/models/import/source_user_placeholder_reference.rb @@ -3,6 +3,7 @@ module Import class SourceUserPlaceholderReference < ApplicationRecord include BulkInsertSafe + include EachBatch self.table_name = 'import_source_user_placeholder_references' @@ -18,6 +19,12 @@ class SourceUserPlaceholderReference < ApplicationRecord attribute :composite_key, :ind_jsonb + scope :model_groups_for_source_user, ->(source_user) do + where(source_user: source_user) + .select(:model, :user_reference_column) + .group(:model, :user_reference_column) + end + # If an element is ever added to this array, ensure that `.from_serialized` handles receiving # older versions of the array by filling in missing values with defaults. We'd keep that in place # for at least one release cycle to ensure backward compatibility. @@ -60,6 +67,51 @@ def from_serialized(serialized_reference) new(attributes.merge(created_at: Time.zone.now)) end + + # Model relations are yielded in a block to ensure all relations will be batched, regardless of the model + def model_relations_for_source_user_reference(model:, source_user:, user_reference_column:, batch_size: 500) + # Look up model from alias after https://gitlab.com/gitlab-org/gitlab/-/issues/467522 + model = model.constantize + + where( + source_user: source_user, + model: model.to_s, + user_reference_column: user_reference_column + ).each_batch(of: batch_size) do |placeholder_reference_batch| + primary_key = model.primary_key + model_relation = nil + + # This is the simplest way to check for composite pkey for now. In Rails 7.1, composite primary keys will be + # fully supported: https://guides.rubyonrails.org/7_1_release_notes.html#composite-primary-keys + if primary_key.nil? + # rubocop: disable Database/AvoidUsingPluckWithoutLimit -- plucking on batch, subquery only possible in rails 7.1 + composite_keys = placeholder_reference_batch.pluck(:composite_key) + + model_relation = model.where( + "#{composite_key_columns(composite_keys)} IN #{composite_key_values(composite_keys)}" + ) + # rubocop: enable Database/AvoidUsingPluckWithoutLimit + else + model_relation = model.primary_key_in(placeholder_reference_batch.select(:numeric_key)) + end + + yield([model_relation, placeholder_reference_batch]) + end + end + + def composite_key_columns(composite_keys) + composite_key_columns = composite_keys.first.keys + tuple(composite_key_columns) + end + + def composite_key_values(composite_keys) + keys = composite_keys.map { |composite_key| tuple(composite_key.values) } + tuple(keys) + end + + def tuple(values) + "(#{values.join(', ')})" + end end private diff --git a/app/services/import/source_users/accept_reassignment_service.rb b/app/services/import/source_users/accept_reassignment_service.rb new file mode 100644 index 00000000000000..3d87bd4f6535ba --- /dev/null +++ b/app/services/import/source_users/accept_reassignment_service.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module Import + module SourceUsers + class AcceptReassignmentService < BaseService + def initialize(import_source_user, current_user:) + @import_source_user = import_source_user + @current_user = current_user + end + + def execute + return error_invalid_permissions unless current_user_matches_reassign_to_user + + if import_source_user.accept + Users::ReassignPlaceholderUserRecordsWorker.perform_async(import_source_user.id) + ServiceResponse.success(payload: import_source_user) + else + ServiceResponse.error(payload: import_source_user, message: import_source_user.errors.full_messages) + end + end + + private + + def current_user_matches_reassign_to_user + return false if current_user.nil? + + current_user.id == import_source_user.reassign_to_user_id + end + end + end +end diff --git a/app/services/users/reassign_placeholder_user_records_service.rb b/app/services/users/reassign_placeholder_user_records_service.rb new file mode 100644 index 00000000000000..f4f0ef7da605d4 --- /dev/null +++ b/app/services/users/reassign_placeholder_user_records_service.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +module Users + class ReassignPlaceholderUserRecordsService + NoReassignToUser = Class.new(StandardError) + + attr_accessor :import_source_user + + def initialize(import_source_user) + @import_source_user = import_source_user + end + + def execute + return unless import_source_user.reassignment_in_progress? + + Import::SourceUserPlaceholderReference.model_groups_for_source_user(import_source_user).each do |reference_group| + user_reference_column = reference_group.user_reference_column + + begin + Import::SourceUserPlaceholderReference.model_relations_for_source_user_reference( + model: reference_group.model, + source_user: import_source_user, + user_reference_column: user_reference_column + ) do |model_relation, placeholder_references| + reassign_placeholder_records(model_relation, placeholder_references, user_reference_column) + end + rescue NameError => e + ::Import::Framework::Logger.error( + message: "#{reference_group.model} is not a model, #{user_reference_column} cannot be reassigned.", + error: e.message, + source_user_id: import_source_user&.id + ) + + next + end + end + + import_source_user.complete! + end + + private + + def reassign_placeholder_records(model_relation, placeholder_references, user_reference_column) + ApplicationRecord.transaction do + model_relation.update_all({ user_reference_column => import_source_user.reassign_to_user_id }) + placeholder_references.delete_all + end + rescue ActiveRecord::RecordNotUnique + ApplicationRecord.transaction do + granular_reassign_placeholder_records(model_relation, user_reference_column) + placeholder_references.delete_all + end + end + + def granular_reassign_placeholder_records(model_relation, user_reference_column) + model_relation.each do |record| + record.update!({ user_reference_column => import_source_user.reassign_to_user_id }) + rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid + ::Import::Framework::Logger.warn( + message: "Unable to reassign record, reassigned user not invalid or not unique", + error: record.errors.full_messages, + source_user_id: import_source_user.id + ) + + next + end + end + end +end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 154e5efc7c91aa..d06874356579c9 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -4125,6 +4125,15 @@ :weight: 1 :idempotent: false :tags: [] +- :name: users_reassign_placeholder_user_records + :worker_name: Users::ReassignPlaceholderUserRecordsWorker + :feature_category: :importers + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: users_record_last_activity :worker_name: Users::RecordLastActivityWorker :feature_category: :seat_cost_management diff --git a/app/workers/users/reassign_placeholder_user_records_worker.rb b/app/workers/users/reassign_placeholder_user_records_worker.rb new file mode 100644 index 00000000000000..003ffaa4a86c59 --- /dev/null +++ b/app/workers/users/reassign_placeholder_user_records_worker.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +module Users + class ReassignPlaceholderUserRecordsWorker + include ApplicationWorker + include Gitlab::Utils::StrongMemoize + + idempotent! + data_consistency :sticky + feature_category :importers + deduplicate :until_executed + sidekiq_options retry: 5, dead: false + sidekiq_options max_retries_after_interruption: 20 + + sidekiq_retries_exhausted do |msg, exception| + new.perform_failure(exception, msg['args']) + end + + def perform(import_source_user_id) + @import_source_user = Import::SourceUser.find_by_id(import_source_user_id) + + return unless Feature.enabled?( + :importer_user_mapping, + User.actor_from_id(import_source_user&.reassigned_by_user_id) + ) + + return unless import_source_user_valid? + + Users::ReassignPlaceholderUserRecordsService.new(import_source_user).execute + end + + def perform_failure(exception, import_source_user_id) + @import_source_user = Import::SourceUser.find_by_id(import_source_user_id) + + log_and_fail_reassignment(exception) + end + + private + + attr_reader :import_source_user + + def import_source_user_valid? + return true if import_source_user && import_source_user.reassignment_in_progress? + + ::Import::Framework::Logger.warn( + message: 'Unable to begin reassignment because Import source user has an invlid status or does not exist', + source_user_id: import_source_user&.id + ) + + false + end + + def log_and_fail_reassignment(exception) + ::Import::Framework::Logger.error( + message: 'Failed to reassign placeholder user', + error: exception.message, + source_user_id: import_source_user&.id + ) + + import_source_user.fail_reassignment! + end + end +end diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 158f28de8ab9bb..fd58b4bec84c70 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -815,6 +815,8 @@ - 1 - - upload_checksum - 1 +- - users_reassign_placeholder_user_records + - 1 - - users_record_last_activity - 1 - - users_track_namespace_visits diff --git a/spec/factories/import_source_users.rb b/spec/factories/import_source_users.rb index d6b5d25d73a4b0..9dce9048fd9d4f 100644 --- a/spec/factories/import_source_users.rb +++ b/spec/factories/import_source_users.rb @@ -26,6 +26,11 @@ status { 1 } end + trait :reassignment_in_progress do + with_reassign_to_user + status { 2 } + end + trait :completed do status { 5 } end diff --git a/spec/finders/import/source_users_finder_spec.rb b/spec/finders/import/source_users_finder_spec.rb index ba08c54440d1f7..941b23fdfbd9c1 100644 --- a/spec/finders/import/source_users_finder_spec.rb +++ b/spec/finders/import/source_users_finder_spec.rb @@ -7,7 +7,10 @@ let_it_be(:group) { create(:group) } let_it_be(:source_user_1) { create(:import_source_user, namespace: group, status: 0, source_name: 'b') } let_it_be(:source_user_2) { create(:import_source_user, namespace: group, status: 1, source_name: 'c') } - let_it_be(:source_user_3) { create(:import_source_user, namespace: group, status: 2, source_name: 'a') } + let_it_be(:source_user_3) do + create(:import_source_user, :with_reassign_to_user, namespace: group, status: 2, source_name: 'a') + end + let_it_be(:import_source_users) { [source_user_1, source_user_2, source_user_3] } let(:params) { {} } diff --git a/spec/models/import/source_user_placeholder_reference_spec.rb b/spec/models/import/source_user_placeholder_reference_spec.rb index e2cdaf4a3eede9..ec86fa374e7033 100644 --- a/spec/models/import/source_user_placeholder_reference_spec.rb +++ b/spec/models/import/source_user_placeholder_reference_spec.rb @@ -37,6 +37,60 @@ def validation_errors(...) end end + describe 'scopes' do + describe '.model_groups_for_source_user' do + let_it_be(:source_user_1) { create(:import_source_user) } + let_it_be(:source_user_2) { create(:import_source_user) } + + let_it_be(:issue_references_1) do + create_list( + :import_source_user_placeholder_reference, + 2, + source_user: source_user_1, + model: Issue.to_s, + user_reference_column: 'author_id' + ) + end + + let_it_be(:note_author_id_reference_1) do + create( + :import_source_user_placeholder_reference, + source_user: source_user_1, + model: Note.to_s, + user_reference_column: 'author_id' + ) + end + + let_it_be(:note_updated_by_id_reference_1) do + create( + :import_source_user_placeholder_reference, + source_user: source_user_1, + model: Note.to_s, + user_reference_column: 'updated_by_id' + ) + end + + let_it_be(:merge_request_reference_2) do + create( + :import_source_user_placeholder_reference, + source_user: source_user_2, + model: MergeRequest.to_s, + user_reference_column: 'author_id' + ) + end + + it 'returns groups of models and user reference columns for a source user' do + mapped_reference_groups = described_class + .model_groups_for_source_user(source_user_1) + .map { |reference_group| [reference_group.model, reference_group.user_reference_column] } + + expect(mapped_reference_groups).to match_array( + [%w[Note author_id], %w[Note updated_by_id], %w[Issue author_id]] + ) + end + end + end + it 'is destroyed when source user is destroyed' do reference = create(:import_source_user_placeholder_reference) @@ -184,4 +238,126 @@ def validation_errors(...) end end end + + describe '.model_relations_for_source_user_reference', :aggregate_failures do + let_it_be(:source_user_1) { create(:import_source_user) } + let_it_be(:source_user_2) { create(:import_source_user) } + + # Issue + let_it_be(:issue_author_id_1) { create(:issue, author_id: source_user_1.placeholder_user_id) } + let_it_be(:issue_author_id_2) { create(:issue, author_id: source_user_2.placeholder_user_id) } + let_it_be(:issue_closed_by_id_1) { create(:issue, closed_by_id: source_user_1.placeholder_user_id) } + let_it_be(:issue_author_id_reference_1) do + create( + :import_source_user_placeholder_reference, + source_user: source_user_1, + model: Issue.to_s, + user_reference_column: 'author_id', + numeric_key: issue_author_id_1.id + ) + end + + let_it_be(:issue_author_id_reference_2) do + create( + :import_source_user_placeholder_reference, + source_user: source_user_2, + model: Issue.to_s, + user_reference_column: 'author_id', + numeric_key: issue_author_id_2.id + ) + end + + let_it_be(:issue_closed_by_id_reference_1) do + create( + :import_source_user_placeholder_reference, + source_user: source_user_1, + model: Issue.to_s, + user_reference_column: 'closed_by_id', + numeric_key: issue_closed_by_id_1.id + ) + end + + # IssueAssignee + let_it_be(:issue_assignee_1) do + issue_author_id_1.issue_assignees.create!( + user_id: source_user_1.placeholder_user_id, issue_id: issue_author_id_1.id + ) + end + + let_it_be(:issue_assignee_2) do + issue_author_id_1.issue_assignees.create!( + user_id: source_user_2.placeholder_user_id, issue_id: issue_author_id_1.id + ) + end + + let_it_be(:issue_assignee_reference_1) do + create( + :import_source_user_placeholder_reference, + source_user: source_user_1, + model: IssueAssignee.to_s, + user_reference_column: 'user_id', + numeric_key: nil, + composite_key: { user_id: source_user_1.placeholder_user_id, issue_id: issue_author_id_1.id } + ) + end + + let_it_be(:issue_assignee_reference_2) do + create( + :import_source_user_placeholder_reference, + source_user: source_user_2, + model: IssueAssignee.to_s, + user_reference_column: 'user_id', + numeric_key: nil, + composite_key: { user_id: source_user_2.placeholder_user_id, issue_id: issue_author_id_1.id } + ) + end + + it 'yields numeric pkey model relations and placeholder reference relation' do + expect do |block| + described_class.model_relations_for_source_user_reference( + model: 'Issue', source_user: source_user_1, user_reference_column: 'author_id', &block + ) + end.to yield_with_args( + [ + match_array(issue_author_id_1), + match_array(issue_author_id_reference_1) + ] + ) + end + + it 'yields composite key model relation and placeholder reference relation' do + expect do |block| + described_class.model_relations_for_source_user_reference( + model: 'IssueAssignee', source_user: source_user_1, user_reference_column: 'user_id', &block + ) + end.to yield_with_args( + [ + match_array(have_attributes(user_id: issue_assignee_1.user_id, issue_id: issue_assignee_1.issue_id)), + match_array(issue_assignee_reference_1) + ] + ) + end + + context 'when a placeholder reference does not map to a real model' do + let!(:invalid_model) { 'ThisWillNeverMapToARealModel' } + let!(:user_reference_column) { 'user_id' } + + let!(:invalid_placeholder_reference) do + create( + :import_source_user_placeholder_reference, + source_user: source_user_1, + model: invalid_model, + user_reference_column: user_reference_column + ) + end + + it 'raises an error' do + expect do |block| + described_class.model_relations_for_source_user_reference( + model: invalid_model, source_user: source_user_1, user_reference_column: user_reference_column, &block + ) + end.to raise_error(NameError) + end + end + end end diff --git a/spec/models/import/source_user_spec.rb b/spec/models/import/source_user_spec.rb index fd03b44d78bf6a..fb97ed1ccb325b 100644 --- a/spec/models/import/source_user_spec.rb +++ b/spec/models/import/source_user_spec.rb @@ -19,6 +19,13 @@ it { is_expected.not_to validate_presence_of(:placeholder_user_id) } end + + it 'validates reassign_to_user_id if status is reassignment_in_progress' do + import_source_user = build(:import_source_user, :reassignment_in_progress, reassign_to_user: nil) + + expect(import_source_user).to be_invalid + expect(import_source_user.errors[:reassign_to_user_id]).to eq(["can't be blank"]) + end end describe 'scopes' do @@ -62,9 +69,15 @@ it 'begins in pending state' do expect(described_class.new.pending_reassignment?).to eq(true) end + + it 'does not transition to reassignment_in_progress without a reassign_to_user' do + import_source_user = create(:import_source_user, :awaiting_approval, reassign_to_user: nil) + + expect { import_source_user.accept! }.to raise_error(StateMachines::InvalidTransition) + end end - describe 'after_transition callback' do + describe 'after_transition callbacks' do subject(:source_user) { create(:import_source_user, :awaiting_approval, :with_reassign_to_user) } it 'does not unset reassign_to_user on other transitions' do @@ -163,9 +176,11 @@ describe '.sort_by_attribute' do let_it_be(:namespace) { create(:namespace) } let_it_be(:source_user_1) { create(:import_source_user, namespace: namespace, status: 4, source_name: 'd') } - let_it_be(:source_user_2) { create(:import_source_user, namespace: namespace, status: 2, source_name: 'b') } + let_it_be(:source_user_2) { create(:import_source_user, namespace: namespace, status: 3, source_name: 'c') } let_it_be(:source_user_3) { create(:import_source_user, namespace: namespace, status: 1, source_name: 'a') } - let_it_be(:source_user_4) { create(:import_source_user, namespace: namespace, status: 3, source_name: 'c') } + let_it_be(:source_user_4) do + create(:import_source_user, :with_reassign_to_user, namespace: namespace, status: 2, source_name: 'b') + end let(:sort_by_attribute) { described_class.sort_by_attribute(method).pluck(attribute) } diff --git a/spec/requests/import/source_users_controller_spec.rb b/spec/requests/import/source_users_controller_spec.rb index ad9206563b39f4..98a2edddaab365 100644 --- a/spec/requests/import/source_users_controller_spec.rb +++ b/spec/requests/import/source_users_controller_spec.rb @@ -52,6 +52,12 @@ it { expect { accept_invite }.to change { source_user.reload.reassignment_in_progress? }.from(false).to(true) } + it 'enqueues the job to reassign contributions' do + expect(Users::ReassignPlaceholderUserRecordsWorker).to receive(:perform_async).with(source_user.id) + + accept_invite + end + it 'redirects with a notice when accepted' do accept_invite diff --git a/spec/services/import/source_users/accept_reassignment_service_spec.rb b/spec/services/import/source_users/accept_reassignment_service_spec.rb new file mode 100644 index 00000000000000..20a8f520613817 --- /dev/null +++ b/spec/services/import/source_users/accept_reassignment_service_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Import::SourceUsers::AcceptReassignmentService, feature_category: :importers do + let(:import_source_user) { create(:import_source_user, :awaiting_approval, :with_reassign_to_user) } + let(:current_user) { import_source_user.reassign_to_user } + let(:service) { described_class.new(import_source_user, current_user: current_user) } + + describe '#execute' do + it 'returns success' do + expect(service.execute).to be_success + end + + it 'sets the source user to accepted' do + service.execute + + expect(import_source_user.reload).to be_reassignment_in_progress + end + + it 'enqueues the job to reassign contributions' do + expect(Users::ReassignPlaceholderUserRecordsWorker).to receive(:perform_async).with(import_source_user.id) + + service.execute + end + + shared_examples 'current user does not have permission to accept reassignment' do + it 'returns error no permissions' do + result = service.execute + + expect(result).to be_error + expect(result.message).to eq('You have insufficient permissions to update the import source user') + end + + it 'does not enqueue the job to reassign contributions' do + expect(Users::ReassignPlaceholderUserRecordsWorker).not_to receive(:perform_async) + + service.execute + end + end + + context 'when the current user is not the user to reassign contributions to' do + let(:current_user) { create(:user) } + + it_behaves_like 'current user does not have permission to accept reassignment' + end + + context 'when there is no user to reassign to' do + before do + import_source_user.update!(reassign_to_user: nil) + end + + it_behaves_like 'current user does not have permission to accept reassignment' + + context 'and no current user is provided' do + let(:current_user) { nil } + + it_behaves_like 'current user does not have permission to accept reassignment' + end + end + end +end diff --git a/spec/services/import/source_users/resend_notification_service_spec.rb b/spec/services/import/source_users/resend_notification_service_spec.rb index 0144a1100e0554..6a95e6a1774c1d 100644 --- a/spec/services/import/source_users/resend_notification_service_spec.rb +++ b/spec/services/import/source_users/resend_notification_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Import::SourceUsers::ResendNotificationService, feature_category: :importers do - let_it_be_with_reload(:import_source_user) { create(:import_source_user, :awaiting_approval) } + let_it_be_with_reload(:import_source_user) { create(:import_source_user, :with_reassign_to_user, :awaiting_approval) } let_it_be(:current_user) { import_source_user.namespace.owner } let(:service) { described_class.new(import_source_user, current_user: current_user) } diff --git a/spec/services/users/reassign_placeholder_user_records_service_spec.rb b/spec/services/users/reassign_placeholder_user_records_service_spec.rb new file mode 100644 index 00000000000000..55880e595263fb --- /dev/null +++ b/spec/services/users/reassign_placeholder_user_records_service_spec.rb @@ -0,0 +1,250 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::ReassignPlaceholderUserRecordsService, feature_category: :importers do + let_it_be(:namespace) { create(:namespace) } + let_it_be_with_reload(:source_user) do + create(:import_source_user, + :with_reassign_to_user, + :reassignment_in_progress, + namespace: namespace + ) + end + + let_it_be_with_reload(:other_source_user) do + create(:import_source_user, + :with_reassign_to_user, + :reassignment_in_progress, + namespace: namespace + ) + end + + let_it_be(:placeholder_user_id) { source_user.placeholder_user_id } + let_it_be(:other_placeholder_user_id) { other_source_user.placeholder_user_id } + let_it_be(:real_user_id) { source_user.reassign_to_user_id } + + # MergeRequests + let_it_be_with_reload(:merge_requests) { create_list(:merge_request, 3, author_id: placeholder_user_id) } + + let_it_be_with_reload(:other_merge_request) do + create(:merge_request, author_id: other_placeholder_user_id) + end + + # Approvals + let_it_be_with_reload(:merge_request_approval) do + create(:approval, merge_request: other_merge_request, user_id: placeholder_user_id) + end + + let_it_be_with_reload(:other_merge_request_approval) do + create(:approval, merge_request: merge_requests[0], user_id: other_placeholder_user_id) + end + + # Issues + let_it_be_with_reload(:issue) { create(:issue, author_id: placeholder_user_id, closed_by_id: placeholder_user_id) } + let_it_be_with_reload(:issue_closed) { create(:issue, closed_by_id: placeholder_user_id) } + + # IssueAssignees + let_it_be_with_reload(:issue_assignee) do + issue.issue_assignees.create!(user_id: placeholder_user_id, issue_id: issue.id) + end + + # Notes + let_it_be_with_reload(:authored_note) { create(:note, author_id: placeholder_user_id) } + let_it_be_with_reload(:updated_note) { create(:note, updated_by_id: placeholder_user_id) } + + # GroupMembers + let_it_be_with_reload(:group_member) { create(:group_member, user_id: placeholder_user_id) } + + subject(:service) { described_class.new(source_user) } + + before do + # Create import_source_user_placeholder_reference for memoized records + # MergeRequests + merge_requests.each { |mr| create_placeholder_reference(source_user, mr, user_column: 'author_id') } + create_placeholder_reference(other_source_user, other_merge_request, user_column: 'author_id') + + # Approvals + create_placeholder_reference(source_user, merge_request_approval, user_column: 'user_id') + create_placeholder_reference(other_source_user, other_merge_request_approval, user_column: 'user_id') + + # Issues + create_placeholder_reference(source_user, issue, user_column: 'author_id') + create_placeholder_reference(source_user, issue, user_column: 'closed_by_id') + create_placeholder_reference(source_user, issue_closed, user_column: 'closed_by_id') + + # IssueAssignees + create_placeholder_reference( + source_user, + issue.issue_assignees.find_by(user_id: placeholder_user_id), + user_column: 'user_id', + composite_key: { user_id: placeholder_user_id, issue_id: issue.id } + ) + + # Notes + create_placeholder_reference(source_user, authored_note, user_column: 'author_id') + create_placeholder_reference(source_user, updated_note, user_column: 'updated_by_id') + + # GroupMembers + create_placeholder_reference(source_user, group_member, user_column: 'user_id') + end + + describe '#execute', :aggregate_failures do + shared_examples 'a successful reassignment' do + it 'completes the reassignment' do + service.execute + + expect(source_user.reload).to be_completed + end + + it 'deletes placeholder references for the source user' do + expect { service.execute }.to change { + Import::SourceUserPlaceholderReference.where(source_user: source_user).count + }.to(0) + end + + it 'does not update any records that do not belong to the source user' do + expect { service.execute }.to not_change { other_merge_request.reload.author_id } + .from(other_placeholder_user_id) + .and not_change { other_merge_request_approval.reload.user_id }.from(other_placeholder_user_id) + end + + it 'does not delete any placeholder references that do not belong to the source user' do + expect { service.execute }.to not_change { + Import::SourceUserPlaceholderReference.where(source_user: other_source_user).count + } + end + end + + context 'when a user can be reassigned without error' do + it 'updates actual records from the source user\'s placeholder reference records' do + expect { service.execute }.to change { merge_requests[0].reload.author_id } + .from(placeholder_user_id).to(real_user_id) + .and change { merge_requests[1].reload.author_id }.from(placeholder_user_id).to(real_user_id) + .and change { merge_requests[2].reload.author_id }.from(placeholder_user_id).to(real_user_id) + .and change { merge_request_approval.reload.user_id }.from(placeholder_user_id).to(real_user_id) + .and change { issue.reload.author_id }.from(placeholder_user_id).to(real_user_id) + .and change { issue.reload.closed_by_id }.from(placeholder_user_id).to(real_user_id) + .and change { issue_closed.reload.closed_by_id }.from(placeholder_user_id).to(real_user_id) + .and change { authored_note.reload.author_id }.from(placeholder_user_id).to(real_user_id) + .and change { updated_note.reload.updated_by_id }.from(placeholder_user_id).to(real_user_id) + .and change { group_member.reload.user_id }.from(placeholder_user_id).to(real_user_id) + .and change { IssueAssignee.where({ user_id: real_user_id, issue_id: issue.id }).count }.from(0).to(1) + end + + it_behaves_like 'a successful reassignment' + end + + context 'when the source user is not in reassignment_in_progress status' do + before do + source_user.update!(status: 0) + end + + it 'does not reassign any contributions' do + expect { service.execute }.to not_change { merge_requests[0].reload.author_id }.from(placeholder_user_id) + .and not_change { merge_requests[1].reload.author_id }.from(placeholder_user_id) + .and not_change { merge_requests[2].reload.author_id }.from(placeholder_user_id) + .and not_change { merge_request_approval.reload.user_id }.from(placeholder_user_id) + .and not_change { issue.reload.author_id }.from(placeholder_user_id) + .and not_change { authored_note.reload.author_id }.from(placeholder_user_id) + .and not_change { updated_note.reload.updated_by_id }.from(placeholder_user_id) + .and not_change { group_member.reload.user_id }.from(placeholder_user_id) + .and not_change { IssueAssignee.where({ user_id: real_user_id, issue_id: issue.id }).count }.from(0) + end + + it 'does not complete the source user' do + expect { service.execute }.to not_change { source_user.status } + end + + it 'does not delete and placeholder references' do + expect { service.execute }.to not_change { + Import::SourceUserPlaceholderReference.where(source_user: source_user).count + } + end + end + + context 'when a placeholder reference is for a nonexistant model' do + let_it_be(:invalid_model) { 'ThisWillNeverMapToARealModel' } + let_it_be(:user_reference_column) { 'user_id' } + + let_it_be(:invalid_placeholder_reference) do + create( + :import_source_user_placeholder_reference, + source_user: source_user, + model: invalid_model, + user_reference_column: user_reference_column + ) + end + + it 'logs an error' do + expect(::Import::Framework::Logger).to receive(:error).with( + hash_including( + message: "#{invalid_model} is not a model, #{user_reference_column} cannot be reassigned.", + source_user_id: source_user.id + ) + ) + + service.execute + end + + it 'does not delete the invalid placeholder reference' do + expect { service.execute }.not_to change { invalid_placeholder_reference.reload.present? }.from(true) + end + + it 'completes the reassignment' do + service.execute + + expect(source_user.reload).to be_completed + end + end + + context 'when a record is no longer unique before reassignment' do + let_it_be_with_reload(:duplicate_merge_request_approval) do + create(:approval, merge_request: other_merge_request, user_id: real_user_id) + end + + it 'updates actual records except non-uniqie record', :aggregate_failures do + expect { service.execute }.to change { merge_requests[0].reload.author_id } + .from(placeholder_user_id).to(real_user_id) + .and change { merge_requests[1].reload.author_id }.from(placeholder_user_id).to(real_user_id) + .and change { merge_requests[2].reload.author_id }.from(placeholder_user_id).to(real_user_id) + .and change { issue.reload.author_id }.from(placeholder_user_id).to(real_user_id) + .and change { issue.reload.closed_by_id }.from(placeholder_user_id).to(real_user_id) + .and change { issue_closed.reload.closed_by_id }.from(placeholder_user_id).to(real_user_id) + .and change { authored_note.reload.author_id }.from(placeholder_user_id).to(real_user_id) + .and change { updated_note.reload.updated_by_id }.from(placeholder_user_id).to(real_user_id) + .and change { group_member.reload.user_id }.from(placeholder_user_id).to(real_user_id) + .and change { IssueAssignee.where({ user_id: real_user_id, issue_id: issue.id }).count }.from(0).to(1) + + expect { service.execute }.not_to change { merge_request_approval.reload.user_id }.from(placeholder_user_id) + end + + it 'logs a warning' do + expect(::Import::Framework::Logger).to receive(:warn).with( + hash_including( + message: "Unable to reassign record, reassigned user not invalid or not unique", + source_user_id: source_user.id + ) + ) + + service.execute + end + + it_behaves_like 'a successful reassignment' + end + end + + def create_placeholder_reference(source_user, object, user_column:, composite_key: nil) + numeric_key = object.id if composite_key.nil? + + create( + :import_source_user_placeholder_reference, + source_user: source_user, + namespace: source_user.namespace, + model: object.class.name, + user_reference_column: user_column, + numeric_key: numeric_key, + composite_key: composite_key + ) + end +end diff --git a/spec/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb index 1fcdf1441e1078..9c42a286b30942 100644 --- a/spec/workers/every_sidekiq_worker_spec.rb +++ b/spec/workers/every_sidekiq_worker_spec.rb @@ -465,6 +465,7 @@ 'UpdateMergeRequestsWorker' => 3, 'UpdateProjectStatisticsWorker' => 3, 'UploadChecksumWorker' => 3, + 'Users::ReassignPlaceholderUserRecordsWorker' => 5, 'Vulnerabilities::Statistics::AdjustmentWorker' => 3, 'VulnerabilityExports::ExportDeletionWorker' => 3, 'VulnerabilityExports::ExportWorker' => 3, diff --git a/spec/workers/users/reassign_placeholder_user_records_worker_spec.rb b/spec/workers/users/reassign_placeholder_user_records_worker_spec.rb new file mode 100644 index 00000000000000..c3326509ab01da --- /dev/null +++ b/spec/workers/users/reassign_placeholder_user_records_worker_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::ReassignPlaceholderUserRecordsWorker, feature_category: :importers do + let(:import_source_user) do + create(:import_source_user, :with_reassign_to_user, :reassignment_in_progress) + end + + let(:job_args) { import_source_user.id } + + describe '#perform' do + before do + allow(Users::ReassignPlaceholderUserRecordsService).to receive(:new).and_call_original + end + + it_behaves_like 'an idempotent worker' do + it 'enqueues service to map records to real users' do + expect(Users::ReassignPlaceholderUserRecordsService).to receive(:new).once + + perform_multiple(job_args) + end + + shared_examples 'an invalid source user' do + it 'does not enqueue service to map records to real users' do + expect(Users::ReassignPlaceholderUserRecordsService).not_to receive(:new) + + perform_multiple(job_args) + end + + it 'logs a warning that the reassignment process was not started' do + expect(::Import::Framework::Logger).to receive(:warn).with({ + message: 'Unable to begin reassignment because Import source user has an invlid status or does not exist', + source_user_id: import_source_user&.id + }).twice + + perform_multiple(job_args) + end + end + + context 'when import source user is not reassignment_in_progress status' do + let(:import_source_user) { create(:import_source_user, :awaiting_approval) } + + it_behaves_like 'an invalid source user' + end + + context 'when import source user does not exist' do + let(:import_source_user) { nil } + let(:job_args) { [-1] } + + it_behaves_like 'an invalid source user' + end + + context 'when the feature flag is disabled' do + before do + stub_feature_flags(importer_user_mapping: false) + end + + it 'does not enqueue service to map records to real users' do + expect(Users::ReassignPlaceholderUserRecordsService).not_to receive(:new) + + perform_multiple(job_args) + end + end + end + end + + describe '#sidekiq_retries_exhausted' do + it 'logs the failure and sets the source user status to failed', :aggregate_failures do + exception = StandardError.new('Some error') + + expect(::Import::Framework::Logger).to receive(:error).with({ + message: 'Failed to reassign placeholder user', + error: exception.message, + source_user_id: import_source_user.id + }) + + described_class.sidekiq_retries_exhausted_block.call({ 'args' => job_args }, exception) + + expect(import_source_user.reload).to be_failed + end + end +end -- GitLab From a37eace6f0f63c0dd0bc815c8a6d49f45e0d48a5 Mon Sep 17 00:00:00 2001 From: Sam Word Date: Wed, 31 Jul 2024 17:43:05 -0400 Subject: [PATCH 2/7] Minor spelling and oversight fixes --- app/models/import/source_user.rb | 1 - app/services/users/reassign_placeholder_user_records_service.rb | 2 +- app/workers/users/reassign_placeholder_user_records_worker.rb | 2 +- .../users/reassign_placeholder_user_records_service_spec.rb | 2 +- .../users/reassign_placeholder_user_records_worker_spec.rb | 2 +- 5 files changed, 4 insertions(+), 5 deletions(-) diff --git a/app/models/import/source_user.rb b/app/models/import/source_user.rb index 9bab14c2845a31..93ce06528f8882 100644 --- a/app/models/import/source_user.rb +++ b/app/models/import/source_user.rb @@ -2,7 +2,6 @@ module Import class SourceUser < ApplicationRecord - include AfterCommitQueue include Gitlab::SQL::Pattern self.table_name = 'import_source_users' diff --git a/app/services/users/reassign_placeholder_user_records_service.rb b/app/services/users/reassign_placeholder_user_records_service.rb index f4f0ef7da605d4..19873bb9d23420 100644 --- a/app/services/users/reassign_placeholder_user_records_service.rb +++ b/app/services/users/reassign_placeholder_user_records_service.rb @@ -57,7 +57,7 @@ def granular_reassign_placeholder_records(model_relation, user_reference_column) record.update!({ user_reference_column => import_source_user.reassign_to_user_id }) rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid ::Import::Framework::Logger.warn( - message: "Unable to reassign record, reassigned user not invalid or not unique", + message: "Unable to reassign record, reassigned user is invalid or not unique", error: record.errors.full_messages, source_user_id: import_source_user.id ) diff --git a/app/workers/users/reassign_placeholder_user_records_worker.rb b/app/workers/users/reassign_placeholder_user_records_worker.rb index 003ffaa4a86c59..ee3632683e9984 100644 --- a/app/workers/users/reassign_placeholder_user_records_worker.rb +++ b/app/workers/users/reassign_placeholder_user_records_worker.rb @@ -43,7 +43,7 @@ def import_source_user_valid? return true if import_source_user && import_source_user.reassignment_in_progress? ::Import::Framework::Logger.warn( - message: 'Unable to begin reassignment because Import source user has an invlid status or does not exist', + message: 'Unable to begin reassignment because Import source user has an invalid status or does not exist', source_user_id: import_source_user&.id ) diff --git a/spec/services/users/reassign_placeholder_user_records_service_spec.rb b/spec/services/users/reassign_placeholder_user_records_service_spec.rb index 55880e595263fb..82e68938972403 100644 --- a/spec/services/users/reassign_placeholder_user_records_service_spec.rb +++ b/spec/services/users/reassign_placeholder_user_records_service_spec.rb @@ -222,7 +222,7 @@ it 'logs a warning' do expect(::Import::Framework::Logger).to receive(:warn).with( hash_including( - message: "Unable to reassign record, reassigned user not invalid or not unique", + message: "Unable to reassign record, reassigned user is invalid or not unique", source_user_id: source_user.id ) ) diff --git a/spec/workers/users/reassign_placeholder_user_records_worker_spec.rb b/spec/workers/users/reassign_placeholder_user_records_worker_spec.rb index c3326509ab01da..d5d2dca5c49de9 100644 --- a/spec/workers/users/reassign_placeholder_user_records_worker_spec.rb +++ b/spec/workers/users/reassign_placeholder_user_records_worker_spec.rb @@ -30,7 +30,7 @@ it 'logs a warning that the reassignment process was not started' do expect(::Import::Framework::Logger).to receive(:warn).with({ - message: 'Unable to begin reassignment because Import source user has an invlid status or does not exist', + message: 'Unable to begin reassignment because Import source user has an invalid status or does not exist', source_user_id: import_source_user&.id }).twice -- GitLab From a28eca33583135193e33f57dcebdfd7ebb41b66d Mon Sep 17 00:00:00 2001 From: Sam Word Date: Wed, 31 Jul 2024 17:46:14 -0400 Subject: [PATCH 3/7] Moved worker and service to import namespace --- ...eassign_placeholder_user_records_service.rb | 2 +- .../accept_reassignment_service.rb | 2 +- app/workers/all_queues.yml | 18 +++++++++--------- ...reassign_placeholder_user_records_worker.rb | 4 ++-- config/sidekiq_queues.yml | 4 ++-- .../import/source_users_controller_spec.rb | 2 +- ...gn_placeholder_user_records_service_spec.rb | 2 +- .../accept_reassignment_service_spec.rb | 4 ++-- spec/workers/every_sidekiq_worker_spec.rb | 2 +- ...ign_placeholder_user_records_worker_spec.rb | 10 +++++----- 10 files changed, 25 insertions(+), 25 deletions(-) rename app/services/{users => import}/reassign_placeholder_user_records_service.rb (99%) rename app/workers/{users => import}/reassign_placeholder_user_records_worker.rb (94%) rename spec/services/{users => import}/reassign_placeholder_user_records_service_spec.rb (99%) rename spec/workers/{users => import}/reassign_placeholder_user_records_worker_spec.rb (83%) diff --git a/app/services/users/reassign_placeholder_user_records_service.rb b/app/services/import/reassign_placeholder_user_records_service.rb similarity index 99% rename from app/services/users/reassign_placeholder_user_records_service.rb rename to app/services/import/reassign_placeholder_user_records_service.rb index 19873bb9d23420..690a0c77e53356 100644 --- a/app/services/users/reassign_placeholder_user_records_service.rb +++ b/app/services/import/reassign_placeholder_user_records_service.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -module Users +module Import class ReassignPlaceholderUserRecordsService NoReassignToUser = Class.new(StandardError) diff --git a/app/services/import/source_users/accept_reassignment_service.rb b/app/services/import/source_users/accept_reassignment_service.rb index 3d87bd4f6535ba..f0bbab94c35407 100644 --- a/app/services/import/source_users/accept_reassignment_service.rb +++ b/app/services/import/source_users/accept_reassignment_service.rb @@ -12,7 +12,7 @@ def execute return error_invalid_permissions unless current_user_matches_reassign_to_user if import_source_user.accept - Users::ReassignPlaceholderUserRecordsWorker.perform_async(import_source_user.id) + Import::ReassignPlaceholderUserRecordsWorker.perform_async(import_source_user.id) ServiceResponse.success(payload: import_source_user) else ServiceResponse.error(payload: import_source_user, message: import_source_user.errors.full_messages) diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index d06874356579c9..d56505f76eb64b 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -3261,6 +3261,15 @@ :weight: 1 :idempotent: true :tags: [] +- :name: import_reassign_placeholder_user_records + :worker_name: Import::ReassignPlaceholderUserRecordsWorker + :feature_category: :importers + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: import_refresh_import_jid :worker_name: Gitlab::Import::RefreshImportJidWorker :feature_category: :importers @@ -4125,15 +4134,6 @@ :weight: 1 :idempotent: false :tags: [] -- :name: users_reassign_placeholder_user_records - :worker_name: Users::ReassignPlaceholderUserRecordsWorker - :feature_category: :importers - :has_external_dependencies: false - :urgency: :low - :resource_boundary: :unknown - :weight: 1 - :idempotent: true - :tags: [] - :name: users_record_last_activity :worker_name: Users::RecordLastActivityWorker :feature_category: :seat_cost_management diff --git a/app/workers/users/reassign_placeholder_user_records_worker.rb b/app/workers/import/reassign_placeholder_user_records_worker.rb similarity index 94% rename from app/workers/users/reassign_placeholder_user_records_worker.rb rename to app/workers/import/reassign_placeholder_user_records_worker.rb index ee3632683e9984..d9927bb9bc6c5d 100644 --- a/app/workers/users/reassign_placeholder_user_records_worker.rb +++ b/app/workers/import/reassign_placeholder_user_records_worker.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -module Users +module Import class ReassignPlaceholderUserRecordsWorker include ApplicationWorker include Gitlab::Utils::StrongMemoize @@ -26,7 +26,7 @@ def perform(import_source_user_id) return unless import_source_user_valid? - Users::ReassignPlaceholderUserRecordsService.new(import_source_user).execute + Import::ReassignPlaceholderUserRecordsService.new(import_source_user).execute end def perform_failure(exception, import_source_user_id) diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index fd58b4bec84c70..918294f5469094 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -407,6 +407,8 @@ - 2 - - import_load_placeholder_references - 1 +- - import_reassign_placeholder_user_records + - 1 - - import_refresh_import_jid - 1 - - incident_management @@ -815,8 +817,6 @@ - 1 - - upload_checksum - 1 -- - users_reassign_placeholder_user_records - - 1 - - users_record_last_activity - 1 - - users_track_namespace_visits diff --git a/spec/requests/import/source_users_controller_spec.rb b/spec/requests/import/source_users_controller_spec.rb index 98a2edddaab365..b12a1f8fc29a72 100644 --- a/spec/requests/import/source_users_controller_spec.rb +++ b/spec/requests/import/source_users_controller_spec.rb @@ -53,7 +53,7 @@ it { expect { accept_invite }.to change { source_user.reload.reassignment_in_progress? }.from(false).to(true) } it 'enqueues the job to reassign contributions' do - expect(Users::ReassignPlaceholderUserRecordsWorker).to receive(:perform_async).with(source_user.id) + expect(Import::ReassignPlaceholderUserRecordsWorker).to receive(:perform_async).with(source_user.id) accept_invite end diff --git a/spec/services/users/reassign_placeholder_user_records_service_spec.rb b/spec/services/import/reassign_placeholder_user_records_service_spec.rb similarity index 99% rename from spec/services/users/reassign_placeholder_user_records_service_spec.rb rename to spec/services/import/reassign_placeholder_user_records_service_spec.rb index 82e68938972403..b89fa7e45a7f37 100644 --- a/spec/services/users/reassign_placeholder_user_records_service_spec.rb +++ b/spec/services/import/reassign_placeholder_user_records_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Users::ReassignPlaceholderUserRecordsService, feature_category: :importers do +RSpec.describe Import::ReassignPlaceholderUserRecordsService, feature_category: :importers do let_it_be(:namespace) { create(:namespace) } let_it_be_with_reload(:source_user) do create(:import_source_user, diff --git a/spec/services/import/source_users/accept_reassignment_service_spec.rb b/spec/services/import/source_users/accept_reassignment_service_spec.rb index 20a8f520613817..a125680ffd6da4 100644 --- a/spec/services/import/source_users/accept_reassignment_service_spec.rb +++ b/spec/services/import/source_users/accept_reassignment_service_spec.rb @@ -19,7 +19,7 @@ end it 'enqueues the job to reassign contributions' do - expect(Users::ReassignPlaceholderUserRecordsWorker).to receive(:perform_async).with(import_source_user.id) + expect(Import::ReassignPlaceholderUserRecordsWorker).to receive(:perform_async).with(import_source_user.id) service.execute end @@ -33,7 +33,7 @@ end it 'does not enqueue the job to reassign contributions' do - expect(Users::ReassignPlaceholderUserRecordsWorker).not_to receive(:perform_async) + expect(Import::ReassignPlaceholderUserRecordsWorker).not_to receive(:perform_async) service.execute end diff --git a/spec/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb index 9c42a286b30942..1a8faaa9850828 100644 --- a/spec/workers/every_sidekiq_worker_spec.rb +++ b/spec/workers/every_sidekiq_worker_spec.rb @@ -465,7 +465,7 @@ 'UpdateMergeRequestsWorker' => 3, 'UpdateProjectStatisticsWorker' => 3, 'UploadChecksumWorker' => 3, - 'Users::ReassignPlaceholderUserRecordsWorker' => 5, + 'Import::ReassignPlaceholderUserRecordsWorker' => 5, 'Vulnerabilities::Statistics::AdjustmentWorker' => 3, 'VulnerabilityExports::ExportDeletionWorker' => 3, 'VulnerabilityExports::ExportWorker' => 3, diff --git a/spec/workers/users/reassign_placeholder_user_records_worker_spec.rb b/spec/workers/import/reassign_placeholder_user_records_worker_spec.rb similarity index 83% rename from spec/workers/users/reassign_placeholder_user_records_worker_spec.rb rename to spec/workers/import/reassign_placeholder_user_records_worker_spec.rb index d5d2dca5c49de9..baa99decaf43cd 100644 --- a/spec/workers/users/reassign_placeholder_user_records_worker_spec.rb +++ b/spec/workers/import/reassign_placeholder_user_records_worker_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Users::ReassignPlaceholderUserRecordsWorker, feature_category: :importers do +RSpec.describe Import::ReassignPlaceholderUserRecordsWorker, feature_category: :importers do let(:import_source_user) do create(:import_source_user, :with_reassign_to_user, :reassignment_in_progress) end @@ -11,19 +11,19 @@ describe '#perform' do before do - allow(Users::ReassignPlaceholderUserRecordsService).to receive(:new).and_call_original + allow(Import::ReassignPlaceholderUserRecordsService).to receive(:new).and_call_original end it_behaves_like 'an idempotent worker' do it 'enqueues service to map records to real users' do - expect(Users::ReassignPlaceholderUserRecordsService).to receive(:new).once + expect(Import::ReassignPlaceholderUserRecordsService).to receive(:new).once perform_multiple(job_args) end shared_examples 'an invalid source user' do it 'does not enqueue service to map records to real users' do - expect(Users::ReassignPlaceholderUserRecordsService).not_to receive(:new) + expect(Import::ReassignPlaceholderUserRecordsService).not_to receive(:new) perform_multiple(job_args) end @@ -57,7 +57,7 @@ end it 'does not enqueue service to map records to real users' do - expect(Users::ReassignPlaceholderUserRecordsService).not_to receive(:new) + expect(Import::ReassignPlaceholderUserRecordsService).not_to receive(:new) perform_multiple(job_args) end -- GitLab From ba6f76ad04793f1f5a6158fb6dfade19ba60e0e0 Mon Sep 17 00:00:00 2001 From: Sam Word Date: Thu, 1 Aug 2024 17:12:44 -0400 Subject: [PATCH 4/7] Keep placeholder references if reassignment not possible --- .../source_user_placeholder_reference.rb | 10 ++ ...assign_placeholder_user_records_service.rb | 25 ++-- .../source_user_placeholder_reference_spec.rb | 134 +++++++++++++----- ...n_placeholder_user_records_service_spec.rb | 30 ++-- 4 files changed, 136 insertions(+), 63 deletions(-) diff --git a/app/models/import/source_user_placeholder_reference.rb b/app/models/import/source_user_placeholder_reference.rb index 9026fa55ce44b7..e06dfaa90a4363 100644 --- a/app/models/import/source_user_placeholder_reference.rb +++ b/app/models/import/source_user_placeholder_reference.rb @@ -57,6 +57,12 @@ def to_serialized Gitlab::Json.dump(attributes.slice(*SERIALIZABLE_ATTRIBUTES).to_h.values) end + def model_record + model_class = model.constantize + model_relation = numeric_key? ? model_class.primary_key_in(numeric_key) : model_class.where(composite_key) + model_relation.where({ user_reference_column => source_user.placeholder_user_id }).first + end + class << self def from_serialized(serialized_reference) deserialized = Gitlab::Json.parse(serialized_reference) @@ -95,6 +101,10 @@ def model_relations_for_source_user_reference(model:, source_user:, user_referen model_relation = model.primary_key_in(placeholder_reference_batch.select(:numeric_key)) end + model_relation = model_relation.where({ user_reference_column => source_user.placeholder_user_id }) + + next if model_relation.empty? + yield([model_relation, placeholder_reference_batch]) end end diff --git a/app/services/import/reassign_placeholder_user_records_service.rb b/app/services/import/reassign_placeholder_user_records_service.rb index 690a0c77e53356..27c22146166e06 100644 --- a/app/services/import/reassign_placeholder_user_records_service.rb +++ b/app/services/import/reassign_placeholder_user_records_service.rb @@ -46,24 +46,21 @@ def reassign_placeholder_records(model_relation, placeholder_references, user_re placeholder_references.delete_all end rescue ActiveRecord::RecordNotUnique - ApplicationRecord.transaction do - granular_reassign_placeholder_records(model_relation, user_reference_column) - placeholder_references.delete_all + placeholder_references.each do |placeholder_reference| + reassign_placeholder_record(placeholder_reference, user_reference_column) end end - def granular_reassign_placeholder_records(model_relation, user_reference_column) - model_relation.each do |record| - record.update!({ user_reference_column => import_source_user.reassign_to_user_id }) - rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid - ::Import::Framework::Logger.warn( - message: "Unable to reassign record, reassigned user is invalid or not unique", - error: record.errors.full_messages, - source_user_id: import_source_user.id - ) - - next + def reassign_placeholder_record(placeholder_reference, user_reference_column) + ApplicationRecord.transaction do + placeholder_reference.model_record.update!({ user_reference_column => import_source_user.reassign_to_user_id }) + placeholder_reference.destroy! end + rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid + ::Import::Framework::Logger.warn( + message: "Unable to reassign record, reassigned user is invalid or not unique", + source_user_id: import_source_user.id + ) end end end diff --git a/spec/models/import/source_user_placeholder_reference_spec.rb b/spec/models/import/source_user_placeholder_reference_spec.rb index ec86fa374e7033..f4f000eef479ac 100644 --- a/spec/models/import/source_user_placeholder_reference_spec.rb +++ b/spec/models/import/source_user_placeholder_reference_spec.rb @@ -239,7 +239,7 @@ def validation_errors(...) end end - describe '.model_relations_for_source_user_reference', :aggregate_failures do + describe 'model_record methods' do let_it_be(:source_user_1) { create(:import_source_user) } let_it_be(:source_user_2) { create(:import_source_user) } @@ -312,51 +312,111 @@ def validation_errors(...) ) end - it 'yields numeric pkey model relations and placeholder reference relation' do - expect do |block| - described_class.model_relations_for_source_user_reference( - model: 'Issue', source_user: source_user_1, user_reference_column: 'author_id', &block - ) - end.to yield_with_args( - [ - match_array(issue_author_id_1), - match_array(issue_author_id_reference_1) - ] - ) - end + describe '#model_record' do + it 'returns the numeric pkey model record the placeholder reference refers to' do + expect(issue_author_id_reference_1.model_record).to eq(issue_author_id_1) + end - it 'yields composite key model relation and placeholder reference relation' do - expect do |block| - described_class.model_relations_for_source_user_reference( - model: 'IssueAssignee', source_user: source_user_1, user_reference_column: 'user_id', &block - ) - end.to yield_with_args( - [ - match_array(have_attributes(user_id: issue_assignee_1.user_id, issue_id: issue_assignee_1.issue_id)), - match_array(issue_assignee_reference_1) - ] - ) - end + it 'returns the composite key model record the placeholder reference refers to' do + record = issue_assignee_reference_1.model_record - context 'when a placeholder reference does not map to a real model' do - let!(:invalid_model) { 'ThisWillNeverMapToARealModel' } - let!(:user_reference_column) { 'user_id' } + expect([record.user_id, record.issue_id]).to eq([issue_assignee_1.user_id, issue_assignee_1.issue_id]) + end - let!(:invalid_placeholder_reference) do - create( - :import_source_user_placeholder_reference, - source_user: source_user_1, - model: invalid_model, - user_reference_column: user_reference_column + context 'when the model record no longer belongs the reference\'s placeholder user' do + let!(:another_user) { create(:user) } + + before do + issue_closed_by_id_1.update!(closed_by_id: another_user.id) + end + + it 'does not return the record' do + expect(issue_closed_by_id_reference_1.model_record).to be_nil + end + end + + context 'when the placeholder reference does not map to a real model' do + let!(:invalid_model) { 'ThisWillNeverMapToARealModel' } + let!(:user_reference_column) { 'user_id' } + + let!(:invalid_placeholder_reference) do + create( + :import_source_user_placeholder_reference, + source_user: source_user_1, + model: invalid_model, + user_reference_column: user_reference_column + ) + end + + it 'raises an error' do + expect { invalid_placeholder_reference.model_record }.to raise_error(NameError) + end + end + end + + describe '.model_relations_for_source_user_reference', :aggregate_failures do + it 'yields numeric pkey model relations and placeholder reference relation' do + expect do |block| + described_class.model_relations_for_source_user_reference( + model: 'Issue', source_user: source_user_1, user_reference_column: 'author_id', &block + ) + end.to yield_with_args( + [ + match_array(issue_author_id_1), + match_array(issue_author_id_reference_1) + ] ) end - it 'raises an error' do + it 'yields composite key model relation and placeholder reference relation' do expect do |block| described_class.model_relations_for_source_user_reference( - model: invalid_model, source_user: source_user_1, user_reference_column: user_reference_column, &block + model: 'IssueAssignee', source_user: source_user_1, user_reference_column: 'user_id', &block + ) + end.to yield_with_args( + [ + match_array(have_attributes(user_id: issue_assignee_1.user_id, issue_id: issue_assignee_1.issue_id)), + match_array(issue_assignee_reference_1) + ] + ) + end + + context 'when a placeholder record exists but the record does not belong to a placeholder' do + let!(:another_user) { create(:user) } + + before do + issue_closed_by_id_1.update!(closed_by_id: another_user.id) + end + + it 'does not yield the record' do + expect do |block| + described_class.model_relations_for_source_user_reference( + model: 'Issue', source_user: source_user_1, user_reference_column: 'closed_by_id', &block + ) + end.not_to yield_control + end + end + + context 'when a placeholder reference does not map to a real model' do + let!(:invalid_model) { 'ThisWillNeverMapToARealModel' } + let!(:user_reference_column) { 'user_id' } + + let!(:invalid_placeholder_reference) do + create( + :import_source_user_placeholder_reference, + source_user: source_user_1, + model: invalid_model, + user_reference_column: user_reference_column ) - end.to raise_error(NameError) + end + + it 'raises an error' do + expect do |block| + described_class.model_relations_for_source_user_reference( + model: invalid_model, source_user: source_user_1, user_reference_column: user_reference_column, &block + ) + end.to raise_error(NameError) + end end end end diff --git a/spec/services/import/reassign_placeholder_user_records_service_spec.rb b/spec/services/import/reassign_placeholder_user_records_service_spec.rb index b89fa7e45a7f37..045785ad1c2c3b 100644 --- a/spec/services/import/reassign_placeholder_user_records_service_spec.rb +++ b/spec/services/import/reassign_placeholder_user_records_service_spec.rb @@ -97,12 +97,6 @@ expect(source_user.reload).to be_completed end - it 'deletes placeholder references for the source user' do - expect { service.execute }.to change { - Import::SourceUserPlaceholderReference.where(source_user: source_user).count - }.to(0) - end - it 'does not update any records that do not belong to the source user' do expect { service.execute }.to not_change { other_merge_request.reload.author_id } .from(other_placeholder_user_id) @@ -132,6 +126,12 @@ .and change { IssueAssignee.where({ user_id: real_user_id, issue_id: issue.id }).count }.from(0).to(1) end + it 'deletes reassigned placeholder references for the source user' do + expect { service.execute }.to change { + Import::SourceUserPlaceholderReference.where(source_user: source_user).count + }.to(0) + end + it_behaves_like 'a successful reassignment' end @@ -220,16 +220,22 @@ end it 'logs a warning' do - expect(::Import::Framework::Logger).to receive(:warn).with( - hash_including( - message: "Unable to reassign record, reassigned user is invalid or not unique", - source_user_id: source_user.id - ) - ) + expect(::Import::Framework::Logger).to receive(:warn).with({ + message: "Unable to reassign record, reassigned user is invalid or not unique", + source_user_id: source_user.id + }) service.execute end + it 'does not delete placeholder references for unassigned records' do + service.execute + + expect( + Import::SourceUserPlaceholderReference.where(source_user: source_user).pluck(:numeric_key) + ).to eq([merge_request_approval.id]) + end + it_behaves_like 'a successful reassignment' end end -- GitLab From 16e85b3ecd348ab6834647d6a8069852862ec9d5 Mon Sep 17 00:00:00 2001 From: Sam Word Date: Fri, 2 Aug 2024 15:40:01 -0400 Subject: [PATCH 5/7] Pluck keys from references to avoid CrossSchemaAccessErrors --- .../import/source_user_placeholder_reference.rb | 13 ++++++++----- .../reassign_placeholder_user_records_service.rb | 15 +++++++-------- ...ssign_placeholder_user_records_service_spec.rb | 10 +++++++++- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/app/models/import/source_user_placeholder_reference.rb b/app/models/import/source_user_placeholder_reference.rb index e06dfaa90a4363..ab27dc7ff5e55b 100644 --- a/app/models/import/source_user_placeholder_reference.rb +++ b/app/models/import/source_user_placeholder_reference.rb @@ -25,6 +25,8 @@ class SourceUserPlaceholderReference < ApplicationRecord .group(:model, :user_reference_column) end + MODEL_BATCH_LIMIT = 500 + # If an element is ever added to this array, ensure that `.from_serialized` handles receiving # older versions of the array by filling in missing values with defaults. We'd keep that in place # for at least one release cycle to ensure backward compatibility. @@ -75,7 +77,7 @@ def from_serialized(serialized_reference) end # Model relations are yielded in a block to ensure all relations will be batched, regardless of the model - def model_relations_for_source_user_reference(model:, source_user:, user_reference_column:, batch_size: 500) + def model_relations_for_source_user_reference(model:, source_user:, user_reference_column:) # Look up model from alias after https://gitlab.com/gitlab-org/gitlab/-/issues/467522 model = model.constantize @@ -83,23 +85,24 @@ def model_relations_for_source_user_reference(model:, source_user:, user_referen source_user: source_user, model: model.to_s, user_reference_column: user_reference_column - ).each_batch(of: batch_size) do |placeholder_reference_batch| + ).each_batch(of: MODEL_BATCH_LIMIT) do |placeholder_reference_batch| primary_key = model.primary_key model_relation = nil # This is the simplest way to check for composite pkey for now. In Rails 7.1, composite primary keys will be # fully supported: https://guides.rubyonrails.org/7_1_release_notes.html#composite-primary-keys + # .pluck is used instead of .select to avoid CrossSchemaAccessErrors on CI tables + # rubocop: disable Database/AvoidUsingPluckWithoutLimit -- plucking limited by placeholder batch if primary_key.nil? - # rubocop: disable Database/AvoidUsingPluckWithoutLimit -- plucking on batch, subquery only possible in rails 7.1 composite_keys = placeholder_reference_batch.pluck(:composite_key) model_relation = model.where( "#{composite_key_columns(composite_keys)} IN #{composite_key_values(composite_keys)}" ) - # rubocop: enable Database/AvoidUsingPluckWithoutLimit else - model_relation = model.primary_key_in(placeholder_reference_batch.select(:numeric_key)) + model_relation = model.primary_key_in(placeholder_reference_batch.pluck(:numeric_key)) end + # rubocop: enable Database/AvoidUsingPluckWithoutLimit model_relation = model_relation.where({ user_reference_column => source_user.placeholder_user_id }) diff --git a/app/services/import/reassign_placeholder_user_records_service.rb b/app/services/import/reassign_placeholder_user_records_service.rb index 27c22146166e06..3e34f89a181fcb 100644 --- a/app/services/import/reassign_placeholder_user_records_service.rb +++ b/app/services/import/reassign_placeholder_user_records_service.rb @@ -14,19 +14,20 @@ def execute return unless import_source_user.reassignment_in_progress? Import::SourceUserPlaceholderReference.model_groups_for_source_user(import_source_user).each do |reference_group| + model = reference_group.model user_reference_column = reference_group.user_reference_column begin Import::SourceUserPlaceholderReference.model_relations_for_source_user_reference( - model: reference_group.model, + model: model, source_user: import_source_user, user_reference_column: user_reference_column ) do |model_relation, placeholder_references| - reassign_placeholder_records(model_relation, placeholder_references, user_reference_column) + reassign_placeholder_records_batch(model_relation, placeholder_references, user_reference_column) end rescue NameError => e ::Import::Framework::Logger.error( - message: "#{reference_group.model} is not a model, #{user_reference_column} cannot be reassigned.", + message: "#{model} is not a model, #{user_reference_column} cannot be reassigned.", error: e.message, source_user_id: import_source_user&.id ) @@ -40,7 +41,7 @@ def execute private - def reassign_placeholder_records(model_relation, placeholder_references, user_reference_column) + def reassign_placeholder_records_batch(model_relation, placeholder_references, user_reference_column) ApplicationRecord.transaction do model_relation.update_all({ user_reference_column => import_source_user.reassign_to_user_id }) placeholder_references.delete_all @@ -52,10 +53,8 @@ def reassign_placeholder_records(model_relation, placeholder_references, user_re end def reassign_placeholder_record(placeholder_reference, user_reference_column) - ApplicationRecord.transaction do - placeholder_reference.model_record.update!({ user_reference_column => import_source_user.reassign_to_user_id }) - placeholder_reference.destroy! - end + placeholder_reference.model_record.update!({ user_reference_column => import_source_user.reassign_to_user_id }) + placeholder_reference.destroy! rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid ::Import::Framework::Logger.warn( message: "Unable to reassign record, reassigned user is invalid or not unique", diff --git a/spec/services/import/reassign_placeholder_user_records_service_spec.rb b/spec/services/import/reassign_placeholder_user_records_service_spec.rb index 045785ad1c2c3b..4b953079b21486 100644 --- a/spec/services/import/reassign_placeholder_user_records_service_spec.rb +++ b/spec/services/import/reassign_placeholder_user_records_service_spec.rb @@ -56,6 +56,9 @@ # GroupMembers let_it_be_with_reload(:group_member) { create(:group_member, user_id: placeholder_user_id) } + # Ci::Builds - schema is gitlab_ci + let_it_be_with_reload(:ci_build) { create(:ci_build, user_id: placeholder_user_id) } + subject(:service) { described_class.new(source_user) } before do @@ -87,6 +90,9 @@ # GroupMembers create_placeholder_reference(source_user, group_member, user_column: 'user_id') + + # Ci::Builds + create_placeholder_reference(source_user, ci_build, user_column: 'user_id') end describe '#execute', :aggregate_failures do @@ -229,7 +235,9 @@ end it 'does not delete placeholder references for unassigned records' do - service.execute + expect { service.execute }.to change { + Import::SourceUserPlaceholderReference.where(source_user: source_user).count + }.to(1) expect( Import::SourceUserPlaceholderReference.where(source_user: source_user).pluck(:numeric_key) -- GitLab From 633181ee82b6b78b9d2e999741a800e061fa6fa7 Mon Sep 17 00:00:00 2001 From: Sam Word Date: Mon, 5 Aug 2024 13:23:14 -0400 Subject: [PATCH 6/7] Fix undercoverage: test references are destroyed in granular fallback --- .../reassign_placeholder_user_records_service_spec.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/spec/services/import/reassign_placeholder_user_records_service_spec.rb b/spec/services/import/reassign_placeholder_user_records_service_spec.rb index 4b953079b21486..7e43dc229b7249 100644 --- a/spec/services/import/reassign_placeholder_user_records_service_spec.rb +++ b/spec/services/import/reassign_placeholder_user_records_service_spec.rb @@ -36,6 +36,10 @@ create(:approval, merge_request: other_merge_request, user_id: placeholder_user_id) end + let_it_be_with_reload(:merge_request_approval_2) do + create(:approval, merge_request: merge_requests[0], user_id: placeholder_user_id) + end + let_it_be_with_reload(:other_merge_request_approval) do create(:approval, merge_request: merge_requests[0], user_id: other_placeholder_user_id) end @@ -69,6 +73,7 @@ # Approvals create_placeholder_reference(source_user, merge_request_approval, user_column: 'user_id') + create_placeholder_reference(source_user, merge_request_approval_2, user_column: 'user_id') create_placeholder_reference(other_source_user, other_merge_request_approval, user_column: 'user_id') # Issues @@ -123,6 +128,7 @@ .and change { merge_requests[1].reload.author_id }.from(placeholder_user_id).to(real_user_id) .and change { merge_requests[2].reload.author_id }.from(placeholder_user_id).to(real_user_id) .and change { merge_request_approval.reload.user_id }.from(placeholder_user_id).to(real_user_id) + .and change { merge_request_approval_2.reload.user_id }.from(placeholder_user_id).to(real_user_id) .and change { issue.reload.author_id }.from(placeholder_user_id).to(real_user_id) .and change { issue.reload.closed_by_id }.from(placeholder_user_id).to(real_user_id) .and change { issue_closed.reload.closed_by_id }.from(placeholder_user_id).to(real_user_id) @@ -151,6 +157,7 @@ .and not_change { merge_requests[1].reload.author_id }.from(placeholder_user_id) .and not_change { merge_requests[2].reload.author_id }.from(placeholder_user_id) .and not_change { merge_request_approval.reload.user_id }.from(placeholder_user_id) + .and not_change { merge_request_approval_2.reload.user_id }.from(placeholder_user_id) .and not_change { issue.reload.author_id }.from(placeholder_user_id) .and not_change { authored_note.reload.author_id }.from(placeholder_user_id) .and not_change { updated_note.reload.updated_by_id }.from(placeholder_user_id) @@ -214,6 +221,7 @@ .from(placeholder_user_id).to(real_user_id) .and change { merge_requests[1].reload.author_id }.from(placeholder_user_id).to(real_user_id) .and change { merge_requests[2].reload.author_id }.from(placeholder_user_id).to(real_user_id) + .and change { merge_request_approval_2.reload.user_id }.from(placeholder_user_id).to(real_user_id) .and change { issue.reload.author_id }.from(placeholder_user_id).to(real_user_id) .and change { issue.reload.closed_by_id }.from(placeholder_user_id).to(real_user_id) .and change { issue_closed.reload.closed_by_id }.from(placeholder_user_id).to(real_user_id) -- GitLab From 4f17cd7229aaa308b9e24576f09f80f86d007b86 Mon Sep 17 00:00:00 2001 From: Sam Word Date: Tue, 6 Aug 2024 13:21:49 -0400 Subject: [PATCH 7/7] Added params hash for easier future iterations --- app/workers/import/reassign_placeholder_user_records_worker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/workers/import/reassign_placeholder_user_records_worker.rb b/app/workers/import/reassign_placeholder_user_records_worker.rb index d9927bb9bc6c5d..44a512e5994eb2 100644 --- a/app/workers/import/reassign_placeholder_user_records_worker.rb +++ b/app/workers/import/reassign_placeholder_user_records_worker.rb @@ -16,7 +16,7 @@ class ReassignPlaceholderUserRecordsWorker new.perform_failure(exception, msg['args']) end - def perform(import_source_user_id) + def perform(import_source_user_id, _params = {}) @import_source_user = Import::SourceUser.find_by_id(import_source_user_id) return unless Feature.enabled?( -- GitLab