Listing the pending promotion users for the Admin to Approve in Admin Area
The Backend changes which will integrate with the new UI tab and provide the list of data that would be used to display the pending promotion users.
Open Items
FE / BE Alignment: determine what the Backend should return as response, which will then be consumed by the frontend to display the "Pending Promotion" tab. Something similar to this MR which discusses the Backend/Frontend handshake to display the "Pending Promotion" on the Group and Project Members page.
Re-usability: determine if changes made as part of the Project/Group member listing (structure of backend response and such) can be reused here
Original Issue Contents
List Pending Promotion requests.
Identify how data will be shown on FE
Make BE changes to return data for the same
0 of 2 checklist items completed
· Edited by
Alex Martin
Suraj Tripathichanged title from BE: Add controller data to list pending promotion members (later also groups) to BE: Add data to list pending promotion members on Admin Page
changed title from BE: Add controller data to list pending promotion members (later also groups) to BE: Add data to list pending promotion members on Admin Page
I drafted an MR for /admin Users Promotions Requests tab. Members tab for Groups is still a WIP. Here's a draft of how it could look and the data format:
The ... would have Approve and Reject items: not perfect UI, but its similar to other tabs. Surely the UI would undergo confirmation with Alex and Tim, this is just a first taste for the draft on the contract and initial feedback.
payload draft
NOTE: this is not fixed, nesting can be different, but generally we would need these fields:
{id:42,created_at:"2023-12-06T11:27:35.702Z",user:{id:2,username:"test_4",name:"test_4 test",email:"test_4@example.com",avatar_url:"https://www.gravatar.com/avatar/6a0a00eee383df7ba29d6a9e89916ae9?s=80\u0026d=identicon",badges:[{"text":"Promotion request","variant":"info"}],// Q: do we need badges at all on this tab?profile_url:'/',},requested_by:{id:2,username:"test_4",name:"test_4 test",email:"test_4@example.com",profile_url:'/',},role:{title:'Developer'},context:{url:'http://localhost:3000/gitlab-org',title:'GitLab Org',},}
Though there's more to it, as we'll need to integrate it into the /users page. In the MR I played around with an approach for that, but it's not as straightforward as I'd like. Both users and members pages are narrowly focused on those models, and external entities don't fit perfectly. Would very much appreciate your feedback on the data structure and the draft code: !142423 (diffs)
@alex_martin , hi! Above I shared a screenshot for how the member promotion management could look like on a SM admin area. This is a draft UI, please share your thoughts on it overall. The ... hides approve/reject menu items, which is not ideal, but similar to other tabs. I might improve that and better align field looks with group/pending promotion in actual implementation. We have a Feature Flag behind all this feature, so it won't go to customers just yet. So at the moment we just need your general feedback and approvement.
(I'll update the FE counterpart issue once we get to an agreement)
@kpalchyk similarly appreciative comments that I have for you over here. Thanks for the proposal and putting your UX hat on!
Side note: I just tried to start up my SM instance but getting some Ruby errors , so asking some of these questions without being able to look at the admin area right now, so apologies.
TL;DR -- it looks good, I have some minor notes that are not major. I do wonder why there is not as much consistency between the admin tab and the group members area.
Is the "context" a field used on other tabs? I personally find "Context" as a weird field for saying "this is the group/project the person is being added to" but if it aligns to other tabs, we can keep it.
Any reason why this tab would be called "Pending Promotion" and this one would be "Promotions"? Would it make sense to align the two?
Can "Role" instead be "Requested Role" as you have over here?
Out of curiosity, why do users here show up with the blue "Promotion request" label, but they don't over here.
@alex_martin I am sorry to have no update here. This issue is indeed a Backend issue, let me try to explain the current state of this, along with the entire state of the project, while I answer the questions.
I think this should just have the self-managed label, do you agree?
Yes
Is this a FE or BE issue? Or both? Related MR is associated with Kos but the issue is titled BE:?
This is a BE issue, the related MR is a dummy Frontend MR which Kos has created explaining what the Frontend Admin page would/could look like, assuming dummy return values. The corresponding Frontend issue for this is here. There has been no in depth Backend Frontend discussion to really have a handshake on what Backend should return as response, which will then be consumed by the frontend to display the "Pending Promotion" tab. Something similar to this MR which discusses the Backend/Frontend handshake to display the "Pending Promotion" on the Group and Project Members page.
To confirm, this issue is pretty similar to this one #433173 (closed), right? But the issue we're on right now is the admin page level view, right?
Yes. There are 2 type of issues for the Admin specific views,
Listing the pending promotion users for the Admin to Approve (not to be confused with Listing the users on the Project/Group members page)
The Backend changes which will integrate with the new UI tab and provide the list of data that would be used to display the pending promotion users. These changes will be handled as part of this issue. We hope that the changes made as part of the Project/Group member listing (structure of backend response and such) would be reused here, but that needs to be evaluated.
The Frontend changes will create the frontend and integrate it with the backend changes which return the pending promotion users. The structure of the response has to be agreed with Frontend, who needs to ensure the response is consistent with the pages current status.
Approval flow: When Admin Approves the pending request
Backend changes: This will be the change that actually "Change/Reject the Users Role", when the Admin presses Approve/Reject.
Frontend changes: I think there might be a Frontend change required to actually plug the Approve/Reject Button click to the endpoint which will make the change. There is no issue for this yet, I think.
Regarding the self-managed vs All SaaS labels, I think the current labels on the issues are not 100% representative of the exact focus of the change. From my POV, all the issues right now were created with a focus on self-managed, since the scope for just self-managed is/was too large. The issues having labels All SaaS are mostly because the labels were copied from the parent Epics.
Having said that, it doesn't mean all the work done here is useless for All SaaS, I think most(all) of the basic structure for the Project/Group Members page would be reused. But I haven't done a 100% analysis of the exact delta between SaaS and SelfManaged changes yet.
Frontend changes: I think there might be a Frontend change required to actually plug the Approve/Reject Button click to the endpoint which will make the change. There is no issue for this yet, I think.
@suraj_tripathy : The issues having labels All SaaS are mostly because the labels were copied from the parent Epics.
Let's just aim to ensure the labels are accurate. If applicable for both, let's keep both on.
@suraj_tripathy : I think most(all) of the basic structure for the Project/Group Members page would be reused. But I haven't done a 100% analysis of the exact delta between SaaS and SelfManaged changes yet.
@suraj_tripathy I updated the body of this issue to drive clarity based on your comments. Can you please review for accuracy and make any edits needed? Thanks!
@suraj_tripathy : There has been no in depth Backend Frontend discussion to really have a handshake on what Backend should return as response, which will then be consumed by the frontend to display the "Pending Promotion" tab. Something similar to this MR which discusses the Backend/Frontend handshake to display the "Pending Promotion" on the Group and Project Members page.
is there anything @csouthard or I can help with here? Anything blocking us from having this conversation?
These particular fields were used on the FE when we based the data on members themselves, and reused the presentation layer of Members. If the custom components on the FE pass the review, we won't need them. I think we can drop them for now.
Also, iirc, pagination, filtering and sorting also might be needed here. But we can get back to that at a later integration stage.
Thank you @kpalchyk I have removed is_direct_member and type from the entity and have added pagination json to it, so I think we are almost there.. Regarding filtering, I think that we can figure out during integration.
A quick headsup: I've been working on the integration today. I might need to have usernames for users and the helper object with paths (probably for user page, and approve/reject actions).
Here's the current data structure in the test integration MR:
!142423 (diffs)
I've still not figured out pagination or search things. Will continue on this issue tomorrow.
@suraj_tripathy, I think we can be more flexible within the admin app, and instead of data injection, we can do an API integration for the BE-FE. The FE here can really be totally custom. As long as the data structure is more-or-less aligned with the above, we can do injection, REST, or GraphQL. If we base it on the API, we can better prepare for the 4️⃣ [Iteration 4 - All SaaS API] Add Controls f... (&13331) . Afaiu, pagination is currently weird for the data injection (not to mention the sorting and filtering). So we can just head-start with the API. I understand that you can have drafts on this already, so I'm ok with either way. WDYT?
We can have a sync on this for a quicker discussion.
@suraj_tripathy , just a quick-sync clarification: do we plan an API or html-injection here? Afair, the FE is pretty flexible here. The header part of the tab (the bookmarky thingy) is rendered on the BE, and only needs an on/off logic and a total count. While the body of the tab is rendered on the FE, and we can use good-old APIs there.
I'll be working on the FE counterpart this week, so we can coordinate closely.
The header part of the tab (the bookmarky thingy) is rendered on the BE, and only needs an on/off logic and a total count.
Do you mean the code that is added here? Or an EE equivalent of this?
Not sure I understand the on/off logic and a total count part Would you need total count separately from the API response? Can you share where would that be required?
@suraj_tripathy, currently all the tabs have a counter badge ("total count", like Users (42), the one you linked above), and our might have a on/off detection if the tab can be viewed (i.e. feature is on/off).
That's was my thinking anyway, but then this MR landed Enable the new navigation for Admin area > Users (!150288 - merged) . I just noticed that while exploring the code and why my PoC integration MR got conflicts. It changed a lot in how the admin/users page is displayed, which I totally missed (melting_brain_emoji).
regular
filtered
Exploring now how the UI would be displayed, will ping you and Alex on the details.
After that MR landed, our current approach with the custom tab was invalidated. So here's what we can do with the new UI:
1. try to integrate with the current UI on the FE side, and render a custom table under the search pane when [State][=][Pending promotion] is selected in the search pane
pros: it would let us proceed with the BE as-is (GraphQL API) and look similar-ish to seeing pending invitations atm
cons: it's a bit tricky code to implement and support
note: I'll probably try a PoC for this, should be similar-ish to the current approach we did for custom tab, but the search integration might be quite harder.
2. separate out the promotions (and later invitations?) to a new top-level tab (or a new sidebar menu item?)
pros: we'll continue using GraphQL API and the code would be easier to maintain
neutral: promotions (and invitations?) will be on a different tab from Users (is it a cons or a pro?)
cons: we'll probably need to move invitations approvals
note: I tried a PoC with a tab below:
PoC
3. try to integrate with the current UI on the BE side, adding a custom table fields on the FE
pros: the code will be aligned with other "user states"
cons: the code would use a pattern we're trying to distance from (BE-injected data)
cons: we'll have to delay GraphQL API implementation and Suraj will hate me
I lean towards option 1 or 2. Sharing them to hear your thoughts and concerns (including the UIX).
I'm sharing option 3 only for fair transparency, we tried to avoid that approach on group/project promotion requests (although failed for historical reasons, we still want to rewrite it).
P.S: FYI, this is the original issue for that change: Add new search/filter categories for Admin->Users (#238183 - closed) . I have my concerns regarding the new UI, but that's aside. Overall with that MR landing, the SM users UI will now differ from the SaaS users UI. This is not so a code but rather a UIX concern. And I don't know if similar changes were/are planned for the SaaS users page.
@kpalchyk I personally liked the second approach, as it is cleaner. I did have a light concern about the placing of "Seat" related details on the "User" page though, maybe we can add this under the Subscription section? Just a thought.
@kpalchyk thanks for discovering this, providing the video, and writing up the options really clearly.
Recommendation: I would recommend option 1 as it aligns to how the UI has evolved per this issue #238183 (closed) as opposed to leveraging prior tab formatting in conjunction with the new filtered approach.
What we're trying to do: for self-managed, we are trying to add the pending promotion list of users (guest users who could get a higher role & make them billable that the admin needs to approve) to the admin user list.
Problem we're running into: However, running into a question of if we adopt the new filtered approach (screenshot here ; described as option 1 here) or some other option in this list.
Recommendation: I would recommend option 1 as it aligns to how the UI has evolved per this issue #238183 (closed) as opposed to leveraging prior tab formatting in conjunction with the new filtered approach.
Thanks for the ping @alex_martin I'm really surprised by the decision to remove all of the tabs in #238183 (closed) (I was not aware of this). I definitely agree with moving majority of tabs to filters but Pending approvals seems like a top level action item that needs more prominence.
It was the only place where an Owner could see the number of pending approvals
Switching to that tab changed the table layout to promote Approve/Reject as primary and secondary CTA's.
Now those actions are hidden behind a dot menu which does not feel as efficient. (especially because this page doe not have bulk edit functionality)
The above example is with User Caps turned on so I could test the new filtering functionality. It now takes quite a few steps to not only filter pending users but to action on them, I have to click into the dot menu.
Lastly, if I were to close the user caps banner:
How would I know there were any pending users?
Do we send reminder emails?
Does the banner re-appear after set amount of time?
TL;DR I agree with Option 1 as an MVC, but I really think we should consider bringing back tabs for Pending approvals and Guest promotions...
given that you think we should have a tab for pending approvals, should we instead go with option 2?
IMHO Option 2 would be ideal. Option 1 was me thinking more in line with an MVC (because the new user list page is already in production) and not to be too much of a blocker . But maybe in the long run going with option 1 adds more work anyway since the recommendation would be to expose pending tabs.
cc @esybrant for awareness and if you have any opinions.
Context
What we're trying to do: for self-managed, we are trying to add the pending promotion list of users (guest users who could get a higher role & make them billable that the admin needs to approve) to the admin user list so an Admin can approve.
Problem we're running into: However, the user list has a new filtered approach (screenshot here ; described as option 1 here) that removes all tabs apart from Cohort.
My proposal: We should consider bringing back tabs for Pending approvals and include a Guest promotions pending tab. Because:
It was the only place where an Owner could see the number of pending approvals
Switching to that tab changed the table layout to promote Approve/Reject as primary and secondary CTA's.
I think, going with option 2 would be the easiest way forward. And I like it most. Though it would require us moving the pending invitations later on.
I agree with @timnoah that the UIX for the pending invitations could be better. I think the issue that removed tabs suggested that the invitations tab remains, but it wasn't implemented (will ask if it was going to be). Also, I think fields Projects, Groups, and Last activity are irrelevant there. And yes it'd be better to have immediately available action buttons. And maybe some details on where to and by whom the user was invited would be handy. I also wonder if we'll want to join those two tabs (promotions and invitations) at some point.
I agree with @suraj_tripathy that users and promotions are not exactly the same — that's why we faced issues from the get go on integrating them into a single app (at least on the FE). I think this is where our categories leak: promotions and invitations both about Users and Seats. Having them side-to-side on tabs as in screenshots Tim shared above, imo, is a good enough separation. But we can experiment moving them further as Suraj proposes. Or have them mentioned on Seats page, linking to these tabs on Users page. Open for discussion here.
@alex_martin, I'll proceed with option 2 then, as shown in a PoC screenshot below, adding a top-level tab with a counter (Pending guest promotions (3)) on Users page. We won't have the search and sort bar in the first iteration, I think. We can add them separately. Open for a discussion and for pivoting the approach.
I also wonder if we'll want to join those two tabs (promotions and invitations) at some point.
Big thanks @kpalchyk I totally agree with your comment above! We'll need some time to figure out if combing pending invites and guest promotions is feasible.
I just have one comment on the awesome POC you've been working on:
Not wanting to add too much scope to this work but is it possible to include Projects and Groups to this screen? I think its worthwhile data to include when making the promotion decision. The other two would be Requested role and Last activity. Current role seems redundant since we know they are always moving from a Guest role.
supportive of the option 2 decision and that we won't have the search and sort bar in the first iteration
two questions regarding the naming of the tab Pending guest promotions:
Am I wrong in that other non-billable to billable promotions would show up here, too? Not just guest? For example, would minimal access folks ever show up here?
Relevant as a potential counter to Tim's point of: Current role seems redundant since we know they are always moving from a Guest role.
Did we change our decision over here to call this tab Role Promotions ?
Not wanting to add too much scope to this work but is it possible to include Projects and Groups to this screen? I think its worthwhile data to include when making the promotion decision. The other two would be Requested role and Last activity.
Supportive of these additions if not too much additional work. If it is lots of heavy lifting, let's include in a future iteration?
Not wanting to add too much scope to this work but is it possible to include Projects and Groups to this screen? I think its worthwhile data to include when making the promotion decision
@timnoah I think the problem with this would be that there could be list of Projects/Group for each user. A promotion request is per User per Source (either group or project), for the sake of Admin approval we are aggregating those requests per user (I have explained this in a bit more detail here). Tldr being a single user can be promoted on multiple projects or groups so we could have multiple records per user. So I think RequestedRole would correspond to the Max Requested Role for the aggregation of requests for the user.
@timnoah: Not wanting to add too much scope to this work but is it possible to include Projects and Groups to this screen? I think its worthwhile data to include when making the promotion decision. The other two would be Requested role and Last activity. Current role seems redundant since we know they are always moving from a Guest role.
Tim, are you thinking about number of projects and groups that the user is in at a non-billable role? Or showing the number of groups/projects where a role elevation was asked for? I think, as a fallback, the user's admin page has a view for user's groups and projects:
admin/user/groups and projects
For context: As Suraj points out — we're showing here an aggregated list of requests, aggregated by the user. Under the hood there might be 1 or 10 promotion requests per each user, in projects or in groups. I think those could be important details for making the decision. I was thinking that maybe we could later display details of those requests as a > collapse/expand details.
P.S: @suraj_tripathy, if we are to proceed on this path: group count and last activity are already on User GraphQL resolver. And we might need to add the project count too. That should be easy, right?
@alex_martin: Am I wrong in that other non-billable to billable promotions would show up here, too? Not just guest? For example, would minimal access folks ever show up here?
I think they might be of minimal access role and just be non-billable, i.e. currently that is lower or equal to Guest (and custom roles with limited access?). cc @suraj_tripathy, @timnoah
@alex_martin: Did we change our decision over here to call this tab Role Promotions ?
Ah, I'm just lost in the threads :D Happy to use Role Promotions, thanks for pointing that out!
Tim, are you thinking about number of projects and groups that the user is in at a non-billable role? Or showing the number of groups/projects where a role elevation was asked for?
I should have been clearer here I was thinking about number of projects and groups that the user is in at a non-billable role. It gives a sense of how active they are and might help the Owner make a more informed decision. The more I think about the more I agree this should be included as an expand/collapse functionality to give Owners more context.
I think they might be of minimal access role and just be non-billable, i.e. currently that is lower or equal to Guest (and custom roles with limited access?)
I think those could be important details for making the decision. I was thinking that maybe we could later display details of those requests as a > collapse/expand details.
As mentioned above agreed @kpalchyk we can hopefully iterate towards something like this.
Would we say moving forward for the first iteration we are aligned on the screen below?
context switching is catching up with me -- apologies for again overlooking the limitations that come with aggregating users! Will try to be better!
thanks for everyone's engagement!
@timnoah really appreciate you leaning in here! What you have here LGTM. Only thing (which is not major):
Do we want requested role to instead be requested maximum role per @suraj_tripathy's note"So I think RequestedRole would correspond to the Max Requested Role for the aggregation of requests for the user." Also aligns to Fiona's thinking here.
I think RequestedRole would correspond to the Max Requested Role for the aggregation of requests for the user.
@alex_martin , I did think about that a bit, but didn't find a good name for it. "Max Requested Role" is technically correct, but sounds a bit confusing. Are they asking for a role "up to that level", and either would suit? Also, I think, Fiona mentioned that we're trying to use "maximum" instead of just "max", so "Maximum Requested Role". Maybe "Highest role requested"? Idk, I'm not 100% sold on either.
Though it might be that we're adding things out of generosity, while they are not drawing the full picture. Imagine you see there that Alice is requested a developer role. Would that be enough for a decision to be made? I feel like a company decision-making chain would require who, where, by whom, and to what role are promoted. To see the department, requestor, and the project of the company. Listing users with Max Requested Role feels like we're only half-way there.
I'm thinking of something along the lines of a list of users with details in a sublist, though I'm not sure about the performance of this query:
└──────────────────────────────────────────┘┌──────────────────────────────────────────┐│ ┌─┐ ││ │@│ Alice [reject] [approve] ││ └─┘ │├──────────────────────────────────────────┤│ Developer role at group B by Bob │├──────────────────────────────────────────┤│ Reporter role at group C by Carol │└──────────────────────────────────────────┘┌──────────────────────────────────────────┐
I'm okay proceeding with current UIX, but maybe that's not the final vision for the page?
@timnoah, pardon me for overstepping onto the UIX territory
Side note: and I think those details would be handy in the API too, btw.
Agree that we can proceed with what Tim has and then iterate on the drill down later?
Oh, sure! Those were just thoughts that maybe we want to schedule some follow-ups for later. I think what Tim suggests is a great v1! (intentionally no pings, as we all seem to be in an agreement)
I really think we should consider bringing back tabs for Pending approvals and Guest promotions...
@timnoah I just wanted to follow-up on whether or not the count should be displayed in a tab. I still feel pretty strongly that it shouldn't and Pajamas reinforces that opinion.
When not to use
When presenting a filtered view of content, consider using the filter component instead.
Now I do think it is important to surface these data points. Just not as a badge counter that is filtering a data set of users. I shared some alternatives here.
I am going to assign a weight of 3 for this issue. I think we would reuse most of the Entity and might add some more fields to the Admin Entity for display purpose. It might be more than 3, in which case I will bump up the weight.
Started backend work on this issue. I was reading about weather we should go with RestAPI or GraphQL, and looks like we have a documented preference towards GraphQL over RestAPI.
I will spend some more time to understand what would be ideal here. Initially I was leaning towards RestAPI because we do want to open up APIs in the future for the client and I thought those would be REST API, but now I am divided. Maybe we can open up GraphQL APIs to the clients and they dont have to be REST atall?
I think there will be some Backend effort to display the header part of the tab, besides the API work as described by Kos here. I will spend some time figuring that out tomorrow.
Had some initial discussions with Kos regarding integration with frontend and created a MVP MR to allow admin to query Users pending approval for license seat.
The MR is just working for now, but more structuring needs to happen to make it review ready
Need to move the query into the finder
Need to figure out pagination for the structure, currently have used offset_pagination, not sure if that is the prefered one
Also need to see if any query optimization (indexing is required)
I will break the changes in two MR, one MR updating the finder to allow the new query, and another to add the grapql endpoint.
For now, we need to finalize if the current response in the above MR makes sense for frontend, I have started the thread here with @kpalchyk
MR that adds the Graphql changes for the listing is ready and in initial backend review
There are minor file structure changes that I might have to do due to recent bounded_context related changes. More context here
While working on the above changes I found there were some minor feature check related changes that I can DRY, created a minor MR that utilizes thePromotionManagementUtil for feature check
I have used updated the example in the MR to show the pagination as well. Please let me know if you have any questions/suggestions related to that as well
I think there are four main options here: inject that number sync, have a loading indicator that we'll update async, have only a presence indicator (idk, like a blue •), or have nothing. I lean towards rendering it sync if it's not too expensive. If it is expensive — we can ask Tim and Alex for inputs.
If we render it sync — we can do it through internal functions, ignoring the API.
Regarding pagination:
Thanks for the update! I did use it in the integration MR, I think it's a standard keyset pagination, right? (which, by the way, is not supposed to provide the total count?)
@kpalchyk I have made some changes and have added the count query param to get the total count for the list. I have also made some naming changes based on your initial suggestion.
The query now looks like this, with some sample pagination and total count.
Hey, @suraj_tripathy Thanks for the update! The structure and the query params LGTM!
Regarding count:
I was thinking of getting the count from a ruby controller, during the page load. To display it in the tab header like Tim suggested in #433175 (comment 1898326059) . I think injecting it during server render is the best option UIX-wise, but I'm not sure about the performance (probably that's always a tradeoff).
I started an MR to add the page with the tab: Add page stub for admin promotion management (!152775 - merged)(but got a bit stuck on specs Also, I think I'm doing similar helper extraction as you did with ::MemberManagement::PromotionManagementUtils, will roll that back) . So, I was thinking to use private/direct API from within the controller. But I don't know BE well enough. Happy to use GQL from the controller if you think that'd be best. Would love to hear your thoughts.
I think injecting it during server render is the best option UIX-wise, but I'm not sure about the performance (probably that's always a tradeoff).
@kpalchyk Not sure I fully understand as I am noob at frontend, but why cant GQL be used to get data + count together from the frontend? Is there some concern in hitting a gql endpoint from frontend on page-load/as part of page-load?
Do we hit the gql endpoint from frontend only when user switches to that tab? If yes, maybe we can seperate the call hit the endpoint twice, one to get just the count, that we get on/during page-load, and another to get data, that happens when the user switches the tab.
I personally feel adding data through backend controller isn't ideal, as it does slows the response and is also redundant with GQL already allowing us to get this data
Also, I think I'm doing similar helper extraction as you did with ::MemberManagement::PromotionManagementUtils, will roll that back)
I am sorry about this , I think I should have communicated this more transparently. I think I did not anticipate you picking up backend related changes, and this Util was added as part of the CustomRole changes that I worked on last milestone.
Not sure I fully understand as I am noob at frontend
Don't say that, you have a great grasp of the architecture! And I'm a bigger noob at BE
but why cant GQL be used to get data + count together from the frontend? Is there some concern in hitting a gql endpoint from frontend on page-load/as part of page-load?
It can be done, definitely. As I shared above, there are four options: inject that number sync (server-side), have a loading indicator that we'll update to a number async (client-side), have only a presence indicator (idk, like a blue • either side), or have nothing (doesn't-matter-side :D).
The downside of the client-side requests are:
it's async — meaning that the tabs and (possibly their content would be rendered before the tab header. This tab counter serves as an attention attractor, so it's better to have that counter present asap (or nothing if no requests are pending). Showing a loading indicator there to replace it with a number or an empty space would drive additional attention to it.
the code would be a bit loosely coupled — those tabs are currently rendered on the server, and are rendered on two other pages apart of the Promotion Requests. So the JS would be injected on all three of them. This is not only doable, but I think we have examples of that, but I'd prefer to render that number together with the tab title, if possible (due to the performance concern).
The upside of the client-side vs server-side requests is that the client won't have to wait for that count to be fetched to see, say, "Users" tab.
So I lean towards the server-side rendering, unless we see a performance hit. I think it's a UIX vs Performance thing again. In such situations I tend to sacrifice some performance first (depends on the actual performance hit, surely). I guess this is philosophical. Would be happy to hear your thoughts
.
As to having the count in the GQL: now I thought that maybe our approve / reject mutations would require that number to be returned by the server, to update the counter real time. That's an initial thought, I'm not 100% certain about this yet. Also, we still can use a GQL endpoint from the BE controller, afaiu. And if we once migrate the whole admin page to FE — we could use it.
Anyways, this shouldn't block the MR now.
.
Sidenote: I guess a raw table rows count is faster than the aggregated by user one. So either way we display that number, server or client side, if the performance of the aggregated query is low — we could use non-aggregated number there, in theory. Will need a confirmation with Alex and Tim on that though if we proceed on this thought. I think the main idea there is to indicate that something is present. We can use the alpha marker on this field if you have doubts too.
.
I am sorry about this
Oh no! You made my life easier, I will use your helper and copy-paste the specs!
I think I did not anticipate you picking up backend related changes
I think pages themself are on the edge between BE and FE. I picked it to make progress on the UI. I'll update the MR with the new helper and will ask for your review