Suraj Tripathichanged title from BE: Add new data for pending promotions to the group members controller to BE: List Pending Promotion Members for Group/Project Member listings
changed title from BE: Add new data for pending promotions to the group members controller to BE: List Pending Promotion Members for Group/Project Member listings
I wanted your help in understanding what format and how would FE require the data in?
To give more context, I am taking about data that would be shown on the Group Members page, related to Pending Promotion Requests. Do you have anything in mind? What information would you prefer.
reviewed_at: nil,created_at: Mon, 15 Jan 2024 20:05:34.846241000 UTC +00:00,updated_at: Mon, 15 Jan 2024 20:06:47.998726000 UTC +00:00,member_id: 96,member_namespace_id: 94, # project namespace or group idrequested_by_id: 69,reviewed_by_id: 69,new_access_level: 10,old_access_level: 50,status: "pending"
Questions
These are the field we have in the table. Do you have a tentative UX in your head?
I would also like to have this discussion for BE: Add data to list pending promotion members ... (#433175 - closed), because effectively the same data needs to be shown just with different scope. For SM admin it would mean for the entire table with state: pending? and on the group level it would mean requests scoped for the group/project
reviewed_at: nil,created_at: Mon, 15 Jan 2024 20:05:34.846241000 UTC +00:00,updated_at: Mon, 15 Jan 2024 20:06:47.998726000 UTC +00:00,member_id: 96,member_namespace_id: 94, # project namespace or group idrequested_by_id: 69,reviewed_by_id: 69,new_access_level: 10,old_access_level: 50,status: "pending"
Sounds about right. But we might need to expand on some the fields there. E.g. user name, user page links, access titles, etc. But those are nuances, as what's crucial is how the data would be fetched on the page. The code in the members apps is narrow-focused, and I don't have a good architectural approach yet.
While the way Admin/Users page works is trickier. Here's how the users data is injected on that page: https://gitlab.com/gitlab-org/gitlab/-/blob/b6cadabf8867ec6715919402d5eec847a9bfff2f/app/controllers/admin/users_controller.rb#L21 . Even the routing within the page (e.g. /admin/users?filter=blocked_pending_approval) is aimed particularly at user management, not membership management (shall we do ?filter=blocked_pending_promotion and have different data injection there? or shall it mount a different FE app to fetch that data differently? how tabs would look meanwhile). Injecting data on admin/users might be trickier.
Creating a separate API as we discussed earlier would be hard to integrate with from the FE POV currently. The code there is really narrow-focused on member data management, injected as I shown above. We can do that, but it would be harder and that code would differ from everything else in the app. I'll probably need you help to playground-test different approaches. I'll reach out to you with more details tomorrow, so we could sync-discuss this on Fri maybe. I know I should've presented interfaces earlier, so sorry for the delay.
For SM admin it would mean for the entire table with state: pending? and on the group level it would mean requests scoped for the group/project
Yeah. For SM — we'll be showing that only to instance admins, while in groups/projects — we'll be showing that to both instance admins and group/project owners (to acknowledge that their request was accepted). I think group/project page won't need approval controls in this iteration.
Creating a separate API as we discussed earlier would be hard to integrate with from the FE POV currently. The code there is really narrow-focused on member data management, injected as I shown above. We can do that, but it would be harder and that code would differ from everything else in the app.
I think the question we need to answer here is, would that be more scalable? I mean, if adding data in the existing integration is easy, sure that can be done. But would it be better to keep this seperate, from a future FE management/scale pov? (kinda like we are thinking about fixing the flow for UsageQuota page?)
I'll probably need you help to playground-test different approaches.
Sure, do let me know what exactly is required, and Ill be happy to provide whatever is possible. I would prefer having what is required of me before any 1:1 meeting, so that I can come prepared and our 1:1 time is more efficiently spent in finalizing a solution.
I think the question we need to answer here is, would that be more scalable? I mean, if adding data in the existing integration is easy, sure that can be done. But would it be better to keep this seperate, from a future FE management/scale pov? (kinda like we are thinking about fixing the flow for UsageQuota page?)
Totally agree with you here. And if it was our application, I'd probably invest in long-term migration. But as it is now — I'm not yet sure how to approach it best.
Sure, do let me know what exactly is required, and Ill be happy to provide whatever is possible.
Thanks! I'm afraid I'm still working on the PoC. I'll ping you once something is ready to discuss it on a 1-1. Again, sorry for the delays
Side question: do you think we need a FF for this? It feels that the code might go through some iterations before being prod-ready. Did you consider one in scope of #433166 (closed)?
Side question: do you think we need a FF for this? It feels that the code might go through some iterations before being prod-ready.
I did add an FF initially, but for BE pov, since we are adding an ApplicationSetting with default false, that works as a guard against unwanted exposure
If you want me to create a FF for Frontend pov, I will create an MR for the same as part of that Epic
Hey, @suraj_tripathy Members tab for Groups is still a WIP (tricky one) — will share something asap, but I drafted an MR for Admin area tab. Shared details in #433175 (comment 1730550132).
Hey, Suraj Sorry for the delay, finally having an update here. Here's a proposed UI that I drafted (based on data interface that I'll share below)
@alex_martin and @suraj_tripathy, I tried to match the UI with how other tabs look and the fields we need to display here. Please, share your thoughts of what should be added/removed/adjusted. Sidenote: later we'll adapt this table to serve for SaaS approvals too.
(I'll update the FE counterpart issue once we get to an agreement)
@suraj_tripathy, here's the MR with particular line of interest for the data interface: !143585 (diffs, comment 1754505194) . Let's discuss sync or async on whether that structure makes sense and how to integrate it. The linked FE MR has a long way ahead, as it would need EE/CE separation, specs, and polishing.
@kpalchyk thank you for this UX draft appreciate your seamless ability to put on your UX hat
Overall it looks good and my only feedback is Technical Writing related, so tagging @fneill for visibility + input.
Promotions Tab Naming: I am late to giving this feedback but realizing it now that that I see this mock-up. I wonder if "Promotions" is the correct term. It is reminding me of my e-mail where essentially marketing spam goes and I don't want folks to be confused. I want them to know that these are role-based promotions, not promotions like marketing spam. Maybe this isn't a real concern?
Max role: should we call this "Current max role".
Non-technical writing q: would it make sense to include a source field to show which project/group the user is being requested as non-guest on?
Edit: additional non-technical writing q: to clarify, this view is for SM or for SaaS or both? I think I am confused by "Sidenote: later we'll adapt this table to serve for SaaS approvals too" but the labels are on this issue are ~SaaS and self-managed
I was thinking about what other terms could be suitable. I think Promotions describes the outcome well - the role is changing in an upward direction to give access to more project actions. To make it more clear, perhaps Role promotions would be more descriptive? The only other alternative I can think of is perhaps "upgrade" or "role upgrade". But as "upgrade" is already used for upgrading subscriptions and version upgrades, it might already be a loaded term there.
Max role: should we call this "Current max role".
Does 'max role' mean the maximum role the user can be promoted to, or is it just their current role? If the latter, I would suggest Current role to keep things simple
Alex: Edit: additional non-technical writing q: to clarify, this view is for SM or for SaaS or both? I think I am confused by "Sidenote: later we'll adapt this table to serve for SaaS approvals too" but the labels are on this issue are ~SaaS and self-managed
Will answer this one first: this is SM, I think I copy-pasted SaaS accidentally. The draft is for SM, but we'll adapt/extend this same view for SaaS.
Alex: Promotions Tab Naming
Fiona: Role promotions
Works for me! Other tabs used single-word names, I think that diverted me to just "Promotions". Those are actually "Pending role promotion requests". So it's quite tricky to shorten.
Alex: Max role: should we call this "Current max role".
Fiona: I would suggest Current role to keep things simple
I agree that this should be read as "Current role". The field you see is actually a read-only copy of an already existing field:
I'm not sure why that is called "Max role". My initial guess was that it was supposed to reflect maximum role within a SaaS namespace, but I'm not sure. I'll dig into this (probably with Suraj's help).
Alex: Non-technical writing q: would it make sense to include a source field to show which project/group the user is being requested as non-guest on?
My assumption was that on SM we display only current group/project membership So this tab is present on particular group/project where the promotion happened. While on SM there would be aggregation on the root namespace level, and there will add that field. @suraj_tripathy , WDYT of this?
Does 'max role' mean the maximum role the user can be promoted to, or is it just their current role? If the latter, I would suggest Current role to keep things simple
@fneill is means the max of their current role. For example, they could be a maintainer in one group and a developer in another. Their max role would then be maintainer. I am OK with Current Role but Current Max Role is technically more accurate but perhaps that level of accuracy is not helpful and just confusing. CC @kpalchyk
Responding to Kos' notes:
Will answer this one first: this is SM, I think I copy-pasted SaaS accidentally. The draft is for SM, but we'll adapt/extend this same view for SaaS.
My assumption was that on SM we display only current group/project membership So this tab is present on particular group/project where the promotion happened. While on SM there would be aggregation on the root namespace level, and there will add that field.
@alex_martin, I did a typo, but in different place
My assumption was that on SM we display only current group/project membership So this tab is present on particular group/project where the promotion happened. While on SM SaaS there would be aggregation on the root namespace level, and there will add that field.
Please, double-check my logic (well, I should've populated issue requirements a while ago).
On SM: for group/members or project/members — we display that group or project direct promotions requests, to inform group or project owners that there is a promotion pending. It's not for management, it's primarily to inform them that something is happening with their request. Approval or rejection of those requests, aggregated across all namespaces, happen in the admin area. There the "source" of the promotion makes sense ("this promotion happened in this group/project + link").
On SaaS: in subgroups/projects — we display only direct promotion requests. Approval or rejection happens on root namespace / members — there we display the requests aggregated through the namespace. So since on SaaS the requests are aggregated — we'll need the "Source" column.
On SM: for group/members or project/members — we display that group or project direct promotions requests, to inform group or project owners that there is a promotion pending. It's not for management, it's primarily to inform them that something is happening with their request. Approval or rejection of those requests, aggregated across all namespaces, happen in the admin area. There the "source" of the promotion makes sense
Yes, this is the "FYI" listing of members queued for Promotion/Role Change.
I think we can say that the "source" column would only be required where Approval happens. In case of SM that happens in the Admin Area, for SaaS that happens on the RootNamespace
@alex_martin@suraj_tripathy thanks for the details about the 'current maximum' of current, I learnt something new today!
I agree, we should aim to keep it as technically accurate. I have a small request to use the full term "Current maximum role" in the UI label, since we try to avoid abbreviations where possible
I had a few things to address in the MR before moving it for review, for the last two days I have been working towards that.
Initially the Entityinitialization while returning the pending_promotion_members was failing because the presenter was not getting used. I understood the issue today and fixed it. I had created the Presenter class in EE, which was not getting used for presenting. I create an empty presenter in CE and extend it in EE and that worked. I think that could be because the model is in CE, so its trying to locate a presenter in CE. I dont think having an empty presenter for the model in CE is an issue though. Now the entire flow seems to be working fine.
Added spec for the presenter and controllers
Things to pick tomorrow:
Adding spec for the project and group helper classes
Moving the MemberApproval scope to a Finder:
Right now I have directly used a scope to get the pending promotions, I think I should move it to a Finder. I was on the fence about using a finder for just one scope, but I think there needs to be a feature guard on this query. Adding the Finder is a bit more work, but is the step in the right direction.
Added spec for Entity and Serializer and updated the spec for controller to handle feature disabled scenario
Regarding moving the MemberApproval scope to a Finder
I have decided to FeatureFlag the scope for this MR, and move it to a Finder in a follow up MR, since this MR is already pretty big(23 file changes).
Updated structure of the response based on recent suggestions from Kos
Finally, moved this MR for review, starting with a review from Kos. Since this is a fairly large MR, it has touched frontend, backend and database changes, and would require 6 approvals .. Hoping to have a smooth review experience
Initial MR is still in backend review. A discussion is on-going to see if its worth discarding the current effort and maybe directly do the API pivot.
Applied review suggestions
Created a new MR to add Finder and refactor the Controller logic to use the finder instead of directly using the scope. This MR is blocked on the initial change though.
Some follow ups during the initial review which I might have to pick up
Move MemberApprovals entirely to EE
Add Index on member_namespace_id and status since that is used in the scope for query
I think the initial discussion will take another day or two. I dont see this getting merged in the next few days
Initial MR is has moved to backend maintainer review
Follow up MR to add Finder and refactor the Controller logic to use the finder instead of directly using the scope is ready but blocked on the above MR
Created another follow up MR that "Adds Index on member_namespace_id and status since that is used in the scope for query", it is in initial backend and database review
Follow up MR to add Finder and refactor the Controller logic to use the finder instead of directly using the scope is in backend maintainer (which should be done today) and initial database review. Hoping to get this merged in the next two days (since TAT for db reviews is 2 days)
Follow up MR that "Adds Index on member_namespace_id and status since that is used in the scope for query", has database maintainer review pending, and hopefully should be merged in a day
Another break out of the primary MR. This adds a new index which is used in the Finder above.
1
I think the weight of this issue should atleast be 5. I think additional work has gone into coming to a working structure with conversations with frontend (involving MRs which were drafted for validation) which have not been accounted(or maybe are part of the initial 3), but lets agree on 5.