diff --git a/app/controllers/import/source_users_controller.rb b/app/controllers/import/source_users_controller.rb index 9cd4353fb1485e76ae484ecca58276f9d5a6ce76..415fd6f171839cd491ec5afa2bf242b9746d6f37 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 29141ad4d91ee6a1f16997837ddf7362850bc143..93ce06528f8882b75940fc82a52745b1bb97c64c 100644 --- a/app/models/import/source_user.rb +++ b/app/models/import/source_user.rb @@ -20,6 +20,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 4542e82f114164450328fbf23b2ad9f45c7c8cda..ab27dc7ff5e55ba4e0afaad648c7e9fd437b38e8 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,14 @@ 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 + + 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. @@ -50,6 +59,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) @@ -60,6 +75,56 @@ 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:) + # 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: 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? + composite_keys = placeholder_reference_batch.pluck(:composite_key) + + model_relation = model.where( + "#{composite_key_columns(composite_keys)} IN #{composite_key_values(composite_keys)}" + ) + else + 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 }) + + next if model_relation.empty? + + 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/reassign_placeholder_user_records_service.rb b/app/services/import/reassign_placeholder_user_records_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..3e34f89a181fcb04a7eb252c0ebb79bf6ac7c506 --- /dev/null +++ b/app/services/import/reassign_placeholder_user_records_service.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +module Import + 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| + model = reference_group.model + user_reference_column = reference_group.user_reference_column + + begin + Import::SourceUserPlaceholderReference.model_relations_for_source_user_reference( + model: model, + source_user: import_source_user, + user_reference_column: user_reference_column + ) do |model_relation, placeholder_references| + reassign_placeholder_records_batch(model_relation, placeholder_references, user_reference_column) + end + rescue NameError => e + ::Import::Framework::Logger.error( + message: "#{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_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 + end + rescue ActiveRecord::RecordNotUnique + placeholder_references.each do |placeholder_reference| + reassign_placeholder_record(placeholder_reference, user_reference_column) + end + end + + def reassign_placeholder_record(placeholder_reference, user_reference_column) + 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", + source_user_id: import_source_user.id + ) + end + end +end 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 0000000000000000000000000000000000000000..f0bbab94c35407fa1918c6ab9b2c0365170ecdee --- /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 + 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) + 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/workers/all_queues.yml b/app/workers/all_queues.yml index 154e5efc7c91aae7ba0b569867082e0f7376c2cb..d56505f76eb64bb817b6b86339110c838a434ee2 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 diff --git a/app/workers/import/reassign_placeholder_user_records_worker.rb b/app/workers/import/reassign_placeholder_user_records_worker.rb new file mode 100644 index 0000000000000000000000000000000000000000..44a512e5994eb2751384ac284cc44a95532cdd1d --- /dev/null +++ b/app/workers/import/reassign_placeholder_user_records_worker.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +module Import + 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, _params = {}) + @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? + + Import::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 invalid 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 158f28de8ab9bb4fa1e86623fd38f6c6fa62eb00..918294f54690945b7d53a50497982a4dcfd3a036 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 diff --git a/spec/factories/import_source_users.rb b/spec/factories/import_source_users.rb index d6b5d25d73a4b06c321990ba3549fcc0dec63d35..9dce9048fd9d4f556fb337f74082c82f566997bb 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 ba08c54440d1f749db1c7bab45f372e4fb1e24c3..941b23fdfbd9c1e750663cac9f2d2281d62f8189 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 e2cdaf4a3eede95b533977223ab7330dddbab60f..f4f000eef479acdb26fe42b55f44548da6254689 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,186 @@ def validation_errors(...) end end end + + 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) } + + # 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 + + 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 'returns the composite key model record the placeholder reference refers to' do + record = issue_assignee_reference_1.model_record + + expect([record.user_id, record.issue_id]).to eq([issue_assignee_1.user_id, issue_assignee_1.issue_id]) + end + + 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 '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 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 + + 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 end diff --git a/spec/models/import/source_user_spec.rb b/spec/models/import/source_user_spec.rb index fd03b44d78bf6ab1d6dc458bd257ca08ce97877e..fb97ed1ccb325b97dd9d2d2bbf887f59b8d0c703 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 ad9206563b39f4d7b5d5fb16b56d208fdc29af63..b12a1f8fc29a7221b77a80d75ebbc4c81dcf9e3c 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(Import::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/reassign_placeholder_user_records_service_spec.rb b/spec/services/import/reassign_placeholder_user_records_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..7e43dc229b7249586ad7133345b70ba30549a220 --- /dev/null +++ b/spec/services/import/reassign_placeholder_user_records_service_spec.rb @@ -0,0 +1,272 @@ +# frozen_string_literal: true + +require 'spec_helper' + +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, + :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(: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 + + # 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) } + + # 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 + # 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(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 + 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') + + # Ci::Builds + create_placeholder_reference(source_user, ci_build, 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 '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 { 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) + .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 '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 + + 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 { 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) + .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 { 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) + .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({ + 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 + 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) + ).to eq([merge_request_approval.id]) + 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/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 0000000000000000000000000000000000000000..a125680ffd6da41ed3820d7ce322b1bed9c75765 --- /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(Import::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(Import::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 0144a1100e05544f2ba1c23629b5bb158a2723a3..6a95e6a1774c1d288c879bdf4c035098f961aca5 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/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb index 1fcdf1441e1078621dc7579178043c4b94a84391..1a8faaa98508282740372ff0a89bab78ee2da0bc 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, + 'Import::ReassignPlaceholderUserRecordsWorker' => 5, 'Vulnerabilities::Statistics::AdjustmentWorker' => 3, 'VulnerabilityExports::ExportDeletionWorker' => 3, 'VulnerabilityExports::ExportWorker' => 3, diff --git a/spec/workers/import/reassign_placeholder_user_records_worker_spec.rb b/spec/workers/import/reassign_placeholder_user_records_worker_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..baa99decaf43cd283dde2c0f86d5a53f3501b9ca --- /dev/null +++ b/spec/workers/import/reassign_placeholder_user_records_worker_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'spec_helper' + +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 + + let(:job_args) { import_source_user.id } + + describe '#perform' do + before do + 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(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(Import::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 invalid 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(Import::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