-
Notifications
You must be signed in to change notification settings - Fork 867
BUM SidePanel: Implement remove users from classes side panel functionality #13651
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
BUM SidePanel: Implement remove users from classes side panel functionality #13651
Conversation
Build Artifacts
|
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.
Great work overall. I left a few questions and code review will pass when they're resolved. Otherwise, this is ready for QA
| classCoaches.value = Object.keys(rolesByUser.value); | ||
| } finally { |
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.
Is this a place where we'd want to catch and show the default error similar to the undoUserRemoval function below?
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, that would be good to include. I will update it to include a catch.
| })); | ||
| await MembershipResource.saveCollection({ data: enrollments }); | ||
| } | ||
| if (hasCoachRoles) { |
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.
If the learner membership save throws an error, should we still try to save the coach roles in a separate try/catch?
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.
Hmm, in this function we're undoing the user removals. So if undoing the learner memberships fails and throws an error, and we undo the coach removals in a separate try/catch statement, should we include a different error message informing the user which undo removal (coach or learners) failed?
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.
Or should the saveCollection for the memberships and roles be wrapped into one promise, similar to lines 177-187?
| membershipsByUser.value = groupBy(membershipsData, 'user'); | ||
| rolesByUser.value = groupBy(coachRoles, 'user'); |
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 really like this approach to have the two separated but structured the same way so that other operations can be used on both of them (ie, getItemsToRemove, removeItems) - it's a little thing but it's the kind of little thing that makes this file easy to read and reason about.
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.
Just a couple of thoughts!
| <SidePanelModal | ||
| alignment="right" | ||
| sidePanelWidth="700px" | ||
| @closePanel="$router.back()" |
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.
Just noting that here we should use the useGoBack composable to prevent the 4th point that Peter mentioned in this PR #13608 (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.
This is a good point, I will update the side panel to use this.
| @closePanel="$router.back()" | ||
| > | ||
| <template #header> | ||
| <h1>{{ removeUsersFromClassesHeading$({ numUsers: selectedUsers.size }) }}</h1> |
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 very small deviation from the figma specs. In the figma this font-size is 20px, and with this h1 we are getting a 32px font-size.
| :style="{ color: $themeTokens.error }" | ||
| class="warning-text" | ||
| > | ||
| <span>{{ defaultErrorMessage$() }}</span> |
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.
In general I think we should make consistent the error handling for the api requests 😅. In other side panels I think we just show a snackback error, but I think showing the api error in the side panel is slightly better. Some decisions need to be made around there to have a consistent behavior across side panels.
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.
Hmm, I agree with this. Depending on the preferred approach, perhaps we should open an issue to update all of the side panels to have the same error handling approach? @nucleogenesis
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 opening a follow up for a consistent approach is a great idea
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.
Definitely - I'll be putting together some notes tomorrow re: some inconsistencies such as these and will make a follow-up.
| </template> | ||
| </div> | ||
| </div> | ||
| <h2 id="remove-from-selected-classes">{{ SelectClassesLabel$() }}</h2> |
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.
Here there is a little difference from the designs, this font-size should be 16px.
| <KButton | ||
| :text="coreString('cancelAction')" | ||
| :disabled="loading" | ||
| @click="closeSidePanel(selectedOptions.length > 0 ? true : false)" |
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.
If the users closes the side panel by clicking outside the side panel, should it also show the confirmation message? Should it show it if the user hits the browser back button?
If so, we have this CloseConfirmationGuard component. We can add some props and a content slot to match the specs for this use case, but in general it has the common guard checks before the route leave happens, and even if the user closes the tab.
Here is an example of how we can use it. (Just noticed the beforeRouteLeave link that needs to be done is not well docummented in the component 😓).
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.
That is a good point I hadn't thought of. I'll update it so that the CloseConfirmationGuard is displayed when the user hits the browser back button, the cancel button, or clicking outside the side panel when changes have been made.
| // Combine and deduplicate class IDs | ||
| const uniqueClassIds = new Set([...learnerClassIds, ...coachClassIds]); | ||
| return props.classes |
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'll get ahead of Peter's comment 😅. Could we add a sort here to have the classes ordered alphabetically?
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, thanks for looking ahead.
| membershipsByUser.value = groupBy(membershipsData, 'user'); | ||
| rolesByUser.value = groupBy(coachRoles, 'user'); | ||
| classLearners.value = Object.keys(membershipsByUser.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.
It seems like we are not using this variable.
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, I'll remove it.
| user, | ||
| kind: UserKinds.COACH, | ||
| })); | ||
| await RoleResource.saveCollection({ data: assignments }); |
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.
Since these two save requests are independent. Perhaps a good idea would be to use the Promise.all to save then concurrently?
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, this is what I suggested doing in response to Jacob's earlier comment, I'll move forward with that approach.
| } | ||
| } | ||
| try { | ||
| await removeItems(MembershipResource, learnerMembershipsToRemove); |
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.
idem, perhaps we can use Promise.all here too.
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 know if I can for these two items specifically. I initially tried to put them into one Promise.all call but a "Database is not available for write operations" error is always returned from the MembershipResource.deleteCollection, I haven't been able to figure out why.
| autoDismiss: true, | ||
| duration: 6000, | ||
| actionText: undoAction$(), | ||
| actionCallback: () => undoUserRemoval(), |
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.
Nice!
…lk remove coaches from classes
…ulk user action side panels
…GoBack; move removal undos into a single promise
ccda004 to
369bc83
Compare
pcenov
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 - LGTM, implemented as specified.
| await Promise.all([ | ||
| enrollments.length | ||
| ? MembershipResource.saveCollection({ data: enrollments }) | ||
| : Promise.resolve(), | ||
| assignments.length | ||
| ? RoleResource.saveCollection({ data: assignments }) | ||
| : Promise.resolve(), | ||
| ]); |
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.
👍
195e66f
into
learningequality:develop
Summary
New Functionality:
MembershipResourceis used to bulk un-enroll or re-enroll (undo) learners.RoleResourceis used to bulk un-assign or re-assign (undo) coaches.Screen.Recording.2025-08-13.at.3.34.35.PM.mov
References
Closes #13388
Figma
Reviewer guidance