Skip to content

Conversation

javirln
Copy link
Member

@javirln javirln commented Jul 10, 2025

This patch updates the membership repository’s delete logic to also clean up related resources linked to the user being removed. Additionally, it ensures the user is removed from all groups they belong to.

@migmartri
Copy link
Member

Just from top of my mind, would it be easier and more localized to extend

// Delete deletes a membership by ID.
func (r *MembershipRepo) Delete(ctx context.Context, id uuid.UUID) error {
	return r.data.DB.Membership.DeleteOneID(id).Exec(ctx)
}

and in there make sure we delete not only that membership but all the related memberships + the group memberships? all of that from there?

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
@javirln javirln self-assigned this Jul 11, 2025
@javirln javirln requested review from jiparis and migmartri and removed request for jiparis July 11, 2025 07:04
@javirln
Copy link
Member Author

javirln commented Jul 11, 2025

Just from top of my mind, would it be easier and more localized to extend

// Delete deletes a membership by ID.
func (r *MembershipRepo) Delete(ctx context.Context, id uuid.UUID) error {
	return r.data.DB.Membership.DeleteOneID(id).Exec(ctx)
}

and in there make sure we delete not only that membership but all the related memberships + the group memberships? all of that from there?

Yes, that's why I marked as WIP and draft because I was trying to check where else we were cleaning up resources. It is not all centralized in a single place.

@javirln javirln changed the title WIP fix(membership): Removing a user from the org should cleanup resources fix(membership): Removing a user from the org should cleanup resources Jul 11, 2025
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
@javirln javirln marked this pull request as ready for review July 11, 2025 07:06
javirln added 3 commits July 11, 2025 09:12
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
now := time.Now()

// Soft delete all group memberships for this user in this organization
if _, err := tx.GroupMembership.Update().Where(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to do anything about invitations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I don't think so because you can only remove members that are already part of the organization meaning the invitation would be redeemed no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

including the group ones?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are part of the same table just with an additional context but are managed in the same way

Copy link
Member

@migmartri migmartri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@javirln javirln merged commit bd9a441 into chainloop-dev:main Jul 11, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants