feat(projects): When adding members check all memberships#2368
Closed
javirln wants to merge 1 commit intochainloop-dev:mainfrom
Closed
feat(projects): When adding members check all memberships#2368javirln wants to merge 1 commit intochainloop-dev:mainfrom
javirln wants to merge 1 commit intochainloop-dev:mainfrom
Conversation
migmartri
reviewed
Aug 19, 2025
migmartri
reviewed
Aug 19, 2025
javirln
force-pushed
the
feat/consider-all-memberships-for-lookup
branch
from
e50d80d to
99e2ab7
Compare
migmartri
reviewed
Aug 20, 2025
javirln
force-pushed
the
feat/consider-all-memberships-for-lookup
branch
from
99e2ab7 to
2954e25
Compare
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
javirln
force-pushed
the
feat/consider-all-memberships-for-lookup
branch
from
2954e25 to
4be0473
Compare
Member
Author
|
Updated the PR, I've simplified things by just creating a method that checks for the existence of all kind of memberships, direct or inherited. The old code remains untouched. |
jiparis
reviewed
Aug 20, 2025
| existingMembership, err := uc.projectsRepository.FindProjectMembershipByProjectAndID(ctx, orgID, projectID, userUUID, authz.MembershipTypeUser) | ||
| if err != nil && !IsNotFound(err) { | ||
| // Check if the user is already a member of the project (including inherited memberships) | ||
| membershipExists, err := uc.projectsRepository.ExistsProjectMembershipEffective(ctx, orgID, projectID, userUUID, authz.MembershipTypeUser) |
Member
There was a problem hiding this comment.
Do we want this? This will prevent setting a custom role at the project level (inherited as viewer, local as Admin). This was one of our main use cases if I'm not wrong.
Member
Author
There was a problem hiding this comment.
If we take into account the role when checking if the membership exists, we could have both, inherited as viewer and local as admin, that's an easy change.
Member
Author
|
We are going to reevaluate the behavior because we have some doubts on how it should work. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request refactors how project membership checks are performed in the codebase, introducing a clearer distinction between direct and inherited (effective) memberships. It replaces the previous single membership query method with two new methods, ensuring that membership checks are accurate depending on the context—whether only direct membership or all effective memberships (including inherited) should be considered.
Previously, the behavior only checked direct memberships (parentID == nil) when adding a new member to a project. With inheritance now in place, we must verify whether the membership being created already exists through inheritance from another resource.
This inherited membership check applies only when adding new members. For all other operations, only direct memberships are considered, since inherited ones are treated as managed.