Follow up to #429341 (closed). In addition to the filter, I'd also like to see us improve the Source column in the table to account for the Inherited shared case by indicating that the source includes both an originating group and that is has been inherited.
Current state
Proposal
Simplify the display to show the "most relevant" part of the membership path. If it is direct, just show direct. If it is inherited or shared we just show inherited or shared. If it is both we would show just the last step.
Add text indicating the type of source as follows:
Direct member
Member of a group/project that inherits from a group higher up the tree
Member of a group/project that is from a group that is shared with this group/project
@lohrc Taking a look at this, I think we should try to simplify rather than add by only showing the "most relevant" part of the membership path. For shared and inherited, they would need to go to those groups to do any management so I think it makes sense to just keep it clean and understandable.
By "most relevant" I mean the last place the membership is coming from. If it is direct, just show direct. If it is inherited or shared we just show inherited or shared. If it is both we would show just the last step.
So that would look like this:
Direct member
Member of a group/project that inherits from a group higher up the tree
Member of a group/project that is from a group that is shared with this group/project
@lohrc I think #431066 (comment 1722009457) is more an MVC approach, but I think we could also go a bit further and add in a popover to give further details.
I think 3 is the maximum that is possible based on our sharing rules. So the possible scenarios would be
Shared and Inherited
Shared only
Inherited only
Inherited and then shared (only possible on projects)
For long group names, we may need to truncate. I could also see this as a modal if we feel like this is too much for a popover.
But before doing that wanted to gauge the interest in pursuing this and check on the feasibility of it.
cc @ameliabauerly wanted to get your general impression, but also ask your opinion on:
Is this asking too much of a popover
I stole the dots from activity on issues, I think they add something, but do you think they are too much?
I am slightly worried about discoverability, although we have tooltips indicating inherited today.
I just noticed this issue is technically blocked by #429341 (closed). I don't think this needs to be blocked by that issue. Going to mark it as related instead.
@manojmj I am working on refining this and wondering if you know how we can determine if a member is from a shared group? Similar to how we set is_direct_member in app/serializers/member_entity.rb#L29. I wasn't seeing any method in app/models/member.rb to determine this but I may be missing it.
@peterhegman I read through the issue and I understand that we want to distinguish between direct, inherited AND shared?
It would be a bit tricky to directly assert inherited vs shared in the context of the group we are currently looking at without firing too many queries.
I am thinking:
(!is_direct_member == true) says that the member is either inherited from ancestors or shared.
and then
options[:source].ancestor_ids.include?(member.source.id) == true says that the member is inherited from some group up the ancestor chain.
Members that are not part of both the above conditions can only mean that they are from a shared group.
Would this work? This is the easiest solution I could figure out off the top of my head without firing too many queries. WDYT?
@manojmj played around with that but the problem I am running into is if options[:source] is a Project then the ancestor_ids method is not available. Any suggestions?
@peterhegman unfortunately no direct way around it, we could try
# is member inherited?ifoptions[:source].is_a?(Project)parent_group=options[:source].groupreturnfalseunlessparent_groupparent_group.self_and_ancestor_ids.include?(member.source.id)elseoptions[:source].ancestor_ids.include?(member.source.id)end
Hmm that still doesn't appear to be working for shared members. I am getting false for a project member that is shared from a group.
I'll weight this for frontend as a 1 and then maybe we can send it through backend refinement for a full refinement since it is a little more complicated than expected. @arturoherrero I have weighted the frontend but the backend will be a bit more complicated than expected. Can we line this up for backend refinement?
@peterhegman do you have a branch where you tried it out? It should ideally work
The code in #431066 (comment 1858115025) only checks if a member is inherited or not, so your complete check for a shared member would be something like:
returntrueif!direct_member&&!inherited_member# any member that is not direct or inherited can only be a shared member.
not sure if you already tried in this manner or not Happy to pair on this sometime if you'd prefer that
@manojmj ahh nice, yeah I think that works. I opened !150545 (merged) as a rough draft/starting point for this MR and it appears to be working though needs more testing to be 100% sure.
Let's weight this issue a 3 since group sharing always takes a little extra effort and frontend and backend can collaborate on it in one issue.
@peterhegman awesome, changes in !150545 (merged) looks good to me, we can always improve when we are actually attempting it. It's a good start and 3 sounds fair