fix(group): Calculate member count as a whole#2257
Conversation
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
migmartri
left a comment
migmartri
left a comment
There was a problem hiding this comment.
Let's also add a migration to fix existing group
|
|
||
| // Only set the member count and add member if userID is provided | ||
| // Set initial member count to 0, we'll update it after transaction | ||
| builder = builder.SetMemberCount(0) |
There was a problem hiding this comment.
why isn't this a defalt in the DB?
There was a problem hiding this comment.
It's the default, it was a just to make it explicit. I can remove it if it causes confusion
| now := time.Now() | ||
|
|
||
| // Find all group IDs where this user is a member in this organization | ||
| groupMemberships, grpMemErr := tx.GroupMembership.Query().Where( |
There was a problem hiding this comment.
After a user is removed from an organization, we need to update the member count for all groups they were part of. This change handles that by retrieving all associated groups and recalculating their member counts
There was a problem hiding this comment.
but I don't understand who changing from setting -1 to recalculating adds this amount of logic. Or is it that we didn't do it properly before?
There was a problem hiding this comment.
The previous logic was missing a condition where it was decrementing all memberships even if it was marked as deleted. I've reverted the changes and used the old logic to take into account that part.
| now := time.Now() | ||
|
|
||
| // Find all group IDs where this user is a member in this organization | ||
| groupMemberships, grpMemErr := tx.GroupMembership.Query().Where( |
There was a problem hiding this comment.
but I don't understand who changing from setting -1 to recalculating adds this amount of logic. Or is it that we didn't do it properly before?
|
please add a migration to fix existing groups, thanks |
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
| WHERE gm."group_id" = g."id" | ||
| AND gm."deleted_at" IS NULL | ||
| ) | ||
| WHERE 1=1; -- Apply to all groups |
There was a problem hiding this comment.
No, that means count all memberships that are not marked as deleted for the group, aka, active memberships
There was a problem hiding this comment.
but should we update only non-deleted groups?
There was a problem hiding this comment.
let's remove the where please
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
| WHERE gm."group_id" = g."id" | ||
| AND gm."deleted_at" IS NULL | ||
| ) | ||
| WHERE 1=1; -- Apply to all groups |
There was a problem hiding this comment.
but should we update only non-deleted groups?
| group.HasMembersWith(user.ID(userID)), | ||
| group.HasGroupUsersWith(groupmembership.UserID(userID), groupmembership.DeletedAtIsNil()), | ||
| group.HasOrganizationWith(organization.ID(orgID)), | ||
| group.MemberCountGT(0), |
There was a problem hiding this comment.
I think this is wrong, it's not guarantee that the computation is correct does it?
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
2 participants
This patch updates the way group membership counts are maintained. Instead of incrementing or decrementing the counter on each insertion or deletion, the total number of members is now recalculated after every transaction and the count is updated accordingly.