diff --git a/app/models/user.rb b/app/models/user.rb index 89b7fced7a42dc4512a5012b9e6168a608157140..2354c5e1ac6772cf2a9b46849659f209afb5f4e9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -197,7 +197,11 @@ def update_tracked_fields!(request) has_many :namespace_deletion_schedules, class_name: '::Namespaces::DeletionSchedule', inverse_of: :deleting_user # Groups - has_many :group_members, -> { where(requested_at: nil).where("access_level >= ?", Gitlab::Access::GUEST) }, class_name: 'GroupMember' + has_many :group_members, + -> { where(requested_at: nil).where("access_level >= ?", Gitlab::Access::GUEST) }, + class_name: 'GroupMember', + dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :groups, through: :group_members has_many :groups_with_active_memberships, -> { where(members: { state: ::Member::STATE_ACTIVE }) }, through: :group_members, source: :group has_many :owned_groups, -> { where(members: { access_level: Gitlab::Access::OWNER }) }, through: :group_members, source: :group @@ -221,7 +225,7 @@ def update_tracked_fields!(request) # Projects has_many :groups_projects, through: :groups, source: :projects has_many :personal_projects, through: :namespace, source: :projects - has_many :project_members, -> { where(requested_at: nil) } + has_many :project_members, -> { where(requested_at: nil) }, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :projects, through: :project_members has_many :created_projects, foreign_key: :creator_id, class_name: 'Project', dependent: :nullify # rubocop:disable Cop/ActiveRecordDependent has_many :created_namespace_details, foreign_key: :creator_id, class_name: 'Namespace::Detail' diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 3838df2d7fef1e52aa5f57f7d4f914e8e445abb8..e8d0cbe97562e9f621cc89b18983f18985576bcf 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -232,8 +232,8 @@ it { is_expected.to have_many(:members) } it { is_expected.to have_many(:member_namespaces) } it { is_expected.to have_many(:namespace_deletion_schedules).class_name('::Namespaces::DeletionSchedule').inverse_of(:deleting_user) } - it { is_expected.to have_many(:project_members) } - it { is_expected.to have_many(:group_members) } + it { is_expected.to have_many(:project_members).dependent(:destroy) } + it { is_expected.to have_many(:group_members).dependent(:destroy) } it { is_expected.to have_many(:groups) } it { is_expected.to have_many(:keys).dependent(:destroy) } it { is_expected.to have_many(:expired_today_and_unnotified_keys) } @@ -358,6 +358,53 @@ it { expect(user.color_mode_id).to eq(Gitlab::ColorModes::APPLICATION_DEFAULT) } end + describe 'associations with dependent: :destroy' do + let(:user) { create(:user) } + let!(:project) { create(:project) } + let!(:project_member) { create(:project_member, user: user, project: project) } + let!(:group) { create(:group) } + let!(:group_member) { create(:group_member, user: user, group: group) } + + it 'destroys project_members when user is destroyed directly' do + expect { user.destroy! }.to change { ProjectMember.count }.by(-1) + expect(ProjectMember.exists?(project_member.id)).to be_falsey + end + + it 'destroys group_members when user is destroyed directly' do + expect { user.destroy! }.to change { GroupMember.count }.by(-1) + expect(GroupMember.exists?(group_member.id)).to be_falsey + end + + it 'does not trigger callbacks on associated records when user is destroyed' do + expect_any_instance_of(ProjectMember).to receive(:run_callbacks).with(:find).once + expect_any_instance_of(ProjectMember).to receive(:run_callbacks).with(:initialize).once + expect_any_instance_of(ProjectMember).to receive(:run_callbacks).with(:destroy).once + + expect_any_instance_of(GroupMember).to receive(:run_callbacks).with(:find).once + expect_any_instance_of(GroupMember).to receive(:run_callbacks).with(:initialize).once + expect_any_instance_of(GroupMember).to receive(:run_callbacks).with(:destroy).once + + user.destroy! + end + + context 'with transaction rollback scenarios' do + it 'ensures atomicity by rolling back member deletions if user deletion fails' do + # Setup to make user deletion fail after members are processed + allow(user).to receive(:destroy).and_raise("Simulated error") + + # Wrap in a transaction as would happen in real code + expect do + ApplicationRecord.transaction do + user.destroy! + end + end.to raise_error("Simulated error") + + # Verify project_member still exists (transaction rolled back) + expect(ProjectMember.exists?(project_member.id)).to be_truthy + end + end + end + describe '#user_detail' do let_it_be_with_refind(:user) { create(:user) }