-
Notifications
You must be signed in to change notification settings - Fork 867
Update Fundamental routing & data architecture for users page #13515
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Fundamental routing & data architecture for users page #13515
Conversation
Build Artifacts
|
|
Thanks @LianaHarris360, this will be very useful update in support of upcoming work. I haven't reviewed all code, just noticed your Slack message and took a very quick high-level peek. So if I missed something below, let's chat. Two important things I've noticed that previously worked and now seem to be lost:
These two are related, and making sure we preserve this should fix issues with page reload and navigating paginated data. In terms of logic flow, following needs to be preserved, no matter of Vuex removal: Click sort button => update URL => fetch data based on detecting URL change Why fetch data based on detecting URL change? URL, in a way, is the source of truth place that keeps sort state. Imagine you reload the page with sorted data. If implemented correctly, data will stay sorted. This is pretty much the same mechanism we use for pagination. |
|
@MisRob Ok, I understand. I did change that up in this PR, I did not realize that the all of the users were meant to be sorted at once. Thanks for pointing it out, I will re-implement the backend sorting and make sure that the page URL is updated with the sorting parameters. |
AlexVelezLl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @LianaHarris360! This is looking good!
Just one main concern: How are we planning to reuse these same side panels in other pages? For example we have common actions among different user pages that have common side panels, for example, the FilterUsersSidePanel is also present in the "users trash" page and in the "new users" page. Will we need to duplicate these subroutes in those pages too? And we will also need to add different PageNames, since they will be children of other different pages.
| }); | ||
| if (shouldResolve()) { | ||
| facilityUsers.value = resp.results.map(_userState); | ||
| if (sorted && ordering.value && order.value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users should be sorted in the backend using the ordering query param, mainly because this fetch is paginated, and if we do this in the frontend, we will be sorting just the returned page, but we can't guarantee that the first page shows the first (criteria-sorted) users among all pages since we are not applying the sorting in the query to the db before the pagination happens.
I suspect this was to enforce the case-insensitiveness in the sorting, right? That bug arises because of this sorting in the consolidate method
kolibri/kolibri/core/auth/api.py
Line 508 in bf0f02b
| output = sorted(output, key=lambda x: x[ordering_param], reverse=reverse) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out, I am re-implementing the sorting so that it is done on the backend and I agree, the case-insensitive sorting should be done in this api file instead.
kolibri/plugins/facility/assets/src/composables/useUserManagement.js
Outdated
Show resolved
Hide resolved
kolibri/plugins/facility/assets/src/composables/useUserManagement.js
Outdated
Show resolved
Hide resolved
kolibri/plugins/facility/assets/src/composables/useUserManagement.js
Outdated
Show resolved
Hide resolved
kolibri/plugins/facility/assets/src/composables/useUserManagement.js
Outdated
Show resolved
Hide resolved
kolibri/plugins/facility/assets/src/composables/__mocks__/useUserManagement.js
Outdated
Show resolved
Hide resolved
kolibri/plugins/facility/assets/src/composables/useUserManagement.js
Outdated
Show resolved
Hide resolved
| components: { | ||
| SidePanelModal, | ||
| }, | ||
| mixins: [commonCoreStrings], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I have been lately trying to move away from the strings mixins and using the translations object in the setup method instead, Im not sure if is a path we want as org, so will rather ask @marcellamaki or @nucleogenesis if that's true. (This is also just nitpick).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll double check with them on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's super crucial - but an alternative could be to do like we do in syncTaskUtils (again not critical or anything).
This does get me wondering about... if we could just be doing const { classesLabel$ } = commonCoreStrings but I'd have to play with it to try later on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1000 Defenitely not crucial at all! 😅
This does get me wondering about... if we could just be doing const { classesLabel$ } = commonCoreStrings but I'd have to play with it to try later on
No, we cant, as commonCoreStrings is a mixin. The way to go would be to import the coreStrings object itself
Lines 36 to 43 in bf0f02b
| import { coreStrings } from 'kolibri/uiText/commonCoreStrings'; | |
| import { enhancedQuizManagementStrings } from 'kolibri-common/strings/enhancedQuizManagementStrings'; | |
| import { PageNames } from '../../../constants'; | |
| export default { | |
| name: 'QuizResourceSelectionHeader', | |
| setup() { | |
| const { searchLabel$, settingsLabel$ } = coreStrings; |
|
@AlexVelezLl We used child routes for side panels in UserPage.vue because it needs to keep track of five, we decided managing them with child routes was the best solution. In contrast, the trash page needs only one side panel, so FilterUsersSidePanel can be added as sub-component. I believe the new users page will also require four or five side panels so using child routes is likely the approach we will want to take there too. |
|
Alright, got it, thanks @LianaHarris360! |
0fb314d to
e1dbda1
Compare
| </PaginatedListContainerWithBackend> | ||
|
|
||
| <!-- For sidepanels --> | ||
| <router-view :selectedUsers="selectedUsers" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also want to pass the list of classes here (although it's not used by all of them).
kolibri/core/auth/api.py
Outdated
|
|
||
| if ordering_param == "username": | ||
| for item in output: | ||
| item["username"] = item["username"].lower() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A downside of doing this this way is that we are overriding the username and full names and they all now appear lowercased.
| Beofre (develop) | After |
|---|---|
![]() |
![]() |
An alternative could be to modify the lambda function the sorted method receives:
output = sorted(output, key=lambda x: x[ordering_param].lower() if isinstance(x[ordering_param], str) else x[ordering_param], reverse=reverse)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how I missed this, thanks for catching it!
| :tooltip="assignCoach$()" | ||
| /> | ||
| </router-link> | ||
| <router-link :to="{ name: PageNames.ENROLL_LEARNERS_SIDE_PANEL }"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of downsides for routing these side panels this way just with the name.
- we will lose the query params when we navigate to the side panels, and this will make the watch to refetch the users.
- We will lose the
facility_idroute param, and activeFacilityId will be undefined.
Screenshare.-.2025-06-30.4_43_53.PM.mp4
I think this is the reason why the facilityPageLinks exists, although in that logic we are not preserving the query params anyways. And im not sure if we want to keep extending more these vuex variables. An alternative could be just to have a method that preserves the params and query and just change the name :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea, I'll update the links to be set using a method instead.
| path: 'trash-panel', | ||
| component: MoveToTrashSidePanel, | ||
| }, | ||
| { | ||
| name: PageNames.FILTER_USERS_SIDE_PANEL, | ||
| path: 'filter-panel', | ||
| component: FilterUsersSidePanel, | ||
| }, | ||
| { | ||
| name: PageNames.ASSIGN_COACHES_SIDE_PANEL, | ||
| path: 'assign-coaches-panel', | ||
| component: AssignCoachesSidePanel, | ||
| }, | ||
| { | ||
| name: PageNames.REMOVE_FROM_CLASSES_SIDE_PANEL, | ||
| path: 'remove-from-classes-panel', | ||
| component: RemoveFromClassSidePanel, | ||
| }, | ||
| { | ||
| name: PageNames.ENROLL_LEARNERS_SIDE_PANEL, | ||
| path: 'enroll-learners-panel', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Nitpick) I think the -panel part of the paths could be dropped
nucleogenesis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LianaHarris360 one thing that came to mind is that it'd help make it clear to whoever takes on the side panel issues where their data comes from if you put the props: { selectedUsers: {...}, classes: {...}} on the SidePanel components
ozer550
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've specifically reviewed the section that deals with sorting Users. Just a small question to think about, other than that changes look good to me!
| ordering: order.value === 'desc' ? `-${ordering.value}` : ordering.value || null, | ||
| user_type: userType.value, | ||
| }), | ||
| force: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that we have force: true is there a specific reason we are doing this? Trying to think this out loud, so if User clicks "Sort by UserName" -> we do an API call, if the User clicks again the same sorting field then this would do the API call again right? is there a way we can prevent this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I updated the changeSortHandler() func to see if the same sorting settings are already being used in the current query params, to prevent duplicate API requests if they haven’t changed. This piece of code was mostly copied from the previous handlers.js file and force: true was already included there, so I didn't remove it.
kolibri/core/auth/api.py
Outdated
|
|
||
| if ordering_param == "username": | ||
| for item in output: | ||
| item["username"] = item["username"].lower() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch here!
| // Fetch learners for all classes | ||
| const learnersPerClass = await Promise.all( | ||
| classList.map(classObj => | ||
| FacilityUserResource.fetchCollection({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should look at optimizing this a bit because if there are 10 classes on the facility we'll be fetching all of the users for every class (like - the data for the users is coming over the wire, even if we just use the ID) - so it might put some low-power devices in a tough spot and in some cases I bet the initial page load time would suffer too.
One option we have is the MembershipResource class that takes a user_ids parameter. So perhaps this logic can/should be deferred to the side panels (ie, pop the panel out in a loading state, then just get the memberships for the selected users then since they're the only users whose memberships we care about).
Over time this will result in more requests as the user opens side panels and closes them, but the overall payloads will be much smaller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't necessarily think this is blocking - but could drop this commit and I'll update the issues to include the need to handle that request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point, I hadn't thought about using the MembershipResource class. I agree, if a facility has a lot of classes, this could make the page load take a lot longer. I will remove this in favor of the side panels handling this logic instead.
…in favor of useUserMangement composable
…and table sorting
…r side panels within UserPage
…ame for bulk user actions
a0bd663 to
b18aa61
Compare
nucleogenesis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes here all LGTM - I spun it up and tested it out again real quick and I think this is good to go!
315e3ae
into
learningequality:develop






Summary
This pull request establishes the foundation for the side panels related to bulk actions. It includes the creation of the primary components and child routes for the side panels, passes the
selectedUsersprop to the<router-view />within the UserPage, and refactors the data architecture of the UserPage by migrating the data fetching logic into a composable./:facility_id?/users/trash-panel/:facility_id?/users/filter-panel/:facility_id?/users/assign-coaches-panel/:facility_id?/users/enroll-learners-panel/:facility_id?/users/remove-from-classes-panelfacility/assets/src/views/UserPage/SidePanels/Screen.Recording.2025-06-30.mov
References
Closes #13484
Reviewer guidance