BUM SidePanel: Implement Assign (coaches) to classes functionality

[ozer550]

Summary

This pull request updates the AssignCoachesSidePanel component to carry out the bulk action to assign coaches to one or more classes, with intelligent filtering to handle mixed user selections.

AssignCoaches in Bulk: Uses RoleResource to bulk assign or un-assign (undo) coaches to classes with proper role management and duplicate prevention.

User Filtering: The component now automatically filters out learners from the assignment process, only assigning coaches, admins, and superusers to classes. Users can now select a mix of coaches/admins and learners, with the system automatically handling the filtering and showing appropriate warnings.

Fixed Button Assign Logic: The "Assign coach" button on the main users page is now enabled when at least one eligible user (coach/admin/superuser) is selected, rather than requiring all selected users to be eligible.

Discard functionality: Added a confirmation modal that appears when users try to close the side panel with unsaved class selections, preventing accidental loss of work.

Undo System: After successful assignment, a confirmation modal allows users to undo the coach assignments, with proper API calls to remove the created roles. changes also properly handle reassigning coaches who are already assigned to classes, preventing duplicate assignments and ensuring clean role management.

Assigning coaches using SidePanel:

assigncoach.mp4

Assigning coaches and undoing:

undo.mp4

References

closes #13407

Reviewer guidance

• On the Facility > Users page, select one or more users and click the assign coach icon button.
• Once the side panel opens, confirm that the correct warnings apply based on if the selected users are coaches, learners, and not assigned to any classes.
• Select a class to assign the coaches to and undo the assignment to confirm that the functionality works as intended. Note: Users that are already assigned as coaches to the selected classes will not be affected by selecting assign or undo.

[github-actions]

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.19.0a0.dev0_git.258.gd588ee3e.pex
Windows Installer (EXE)	kolibri-0.19.0a0.dev0+git.258.gd588ee3e-windows-setup-unsigned.exe
Debian Package	kolibri_0.19.0a0.dev0+git.258.gd588ee3e-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.19.0a0.dev0+git.258.gd588ee3e.dmg
Android Package (APK)	kolibri-0.19.0a0.dev0+git.258.gd588ee3e-0.1.5-debug.apk
Raspberry Pi Image	kolibri-pi-image-0.19.0a0.dev0+git.258.gd588ee3e.zip
TAR file	kolibri-0.19.0a0.dev0+git.258.gd588ee3e.tar.gz
WHL file	kolibri-0.19.0a0.dev0+git.258.gd588ee3e-py2.py3-none-any.whl

[LianaHarris360]

I noted a few changes that need to be made on the frontend. In the newest Figma designs, the top the warning messages have been updated:

• Only includes the infoOutline KIcon
• Should contain the two messages:
  - '[N] users are learners and won’t be assigned' (only if learners have been selected)
  - 'Users already in selected classes stay unchanged'
• Should have the gray (palette.grey.v_100) background

Current implementation:

Figma designs:

[nucleogenesis]

I added some notes about a wonky "Cancel" button and some suggestions for the RoleListSerializer. I tested manually and things worked well though.

[nucleogenesis]

This should be flat-button and to determine primariness-vs-secondariness you can use the primary prop set as false.

[nucleogenesis]

A few things to note here.

1. It is a Python anti-pattern to except the Exception class - in part because it will often except on errors you never expected it to. Some well-aged Stack Overflow discussion on the topic.

I don't think we really need to do a try/catch here because the only data you're getting here is already validated. If there was something wrong w/ the data it ought to have failed in the RoleSerializer#validate method below.

2. It was a bit confusing to me seeing the generic class variable ModelClass because I couldn't figure out where/when/why ModelClass would be anything other than the Role class here.

3. You could also avoid looping over each item by using the Django Queryset#bulk_create method w/ the ignore_conflicts=True parameter - this should basically be a bulk version of get_or_create because it'll ignore IntegrityErrors when trying to create something that exists already.

[ozer550]

For 3: I initially started with implementing it using bulk_create. I was not successful in getting the desired results, it bypasses model/serializer validation, which leads to unique constraints still surfacing awkwardly. Even with ignore_conflicts, we hit unique key constraint edge cases and couldn’t distinguish conflicts from real errors cleanly..

[nucleogenesis]

Thanks - if we can get a sense of what the errors are we can investigate further. Optimizing this won't be a blocker for merge.

The requested changes have been addressed.

[nucleogenesis]

Noting for posterity here that the bulk_create method did not work out here because among the caveats for that method includes that it doesn't work with many-to-many relationships, so the IntegrityErrors persisted despite using the ignore_conflicts kwarg. (cc @rtibbles)

[rtibbles]

But, the Role object only has direct ForeignKey relationships, not ManyToMany fields? So I'm not sure why that caveat would apply here: https://github.com/learningequality/kolibri/blob/develop/kolibri/core/auth/models.py#L1343

[nucleogenesis]

I trusted my intuition and didn't consult the model 🤦‍♂️

I wonder if it's because save() isn't called then (or one of the other caveats). This part of the save() method in particular seems like it'd avoid the error @ozer550 was getting:

[python-devserver]     return Database.Cursor.execute(self, query, params)
[python-devserver] django.db.utils.IntegrityError: UNIQUE constraint failed: kolibriauth_role.user_id, kolibriauth_role.collection_id, kolibriauth_role.kind

Curious what you think @rtibbles - if the save() method not being called is the culprit, seems we're stuck iterating rather than batching the creates here?

[rtibbles]

I agree that this may be something specific for morango models. My guess is that the id is not being set properly during bulk create. I have no objections with us not doing the bulk create at this juncture.

It's an optimization, and we can dig in more if it seems necessary during testing - as long as we encourage Peter to try with large numbers of users!

[nucleogenesis]

Thanks @rtibbles - for when we come back to it

[pcenov]

Hi @ozer550, as always I'm listing here all of my testing observations for consideration, anything out of scope for this PR can be filed as a separate issue:

1. Assigning a coach from the 'New users' table is not working at all:

2. There are several unhandled edge cases, such as assigning a coach to a deleted class, assigning a deleted user to a class and assigning 0 selected users by directly going to http://127.0.0.1:8080/en/facility/#/users/assign-coaches

assign.to.deleted.class.mp4

assign.0.users.mp4

assign.deleted.coach.mp4

3. I'm not seeing the "Discard changes?" message when accidentally clicking outside of the side panel or attempting to close it after having already selected classes.
4. If I select several users and assign them then after dismissing the confirmation modal I am back at the Users table and all of the selected users remain selected. Typically after completion of the action the table should be displayed in its default state.

users.remain.selected.mp4

5. I'm not sure what are the final approved texts for the notice section and the modal so posting a screenshot of what I am observing. In my opinion the N users value should be only for the admins/coaches that can actually be assigned, and not for the total of selected users. Then in the notice/info section it seems that mentioning that N learner's can't be assigned as coaches is sufficient ("They won't be added" is just repeating the same info and "Users already in selected classes will not be affected" seems unnecessary as I am not deleting users so I'm not scared that users will be ''affected".
   The modal almost repeats the same text twice, so both should be updated.

6. Same goes for the snackbar messages which currently are "Selected coaches have been assigned" and "Assign action has been undone" - both should be updated.

7. The classes in the "Select classes" table are not sorted.

8. I have previously mentioned somewhere that the main button and course of action at the success modal should not be UNDO as we are intentionally assigning users.

[nucleogenesis]

@pcenov -- @ozer550 has pushed changes addressing 1, 3, 4, and 7.

2, 5, 6 and 8 are being tracked elsewhere - once this is rebased it'll be ready for re-review

[ozer550]

@pcenov -- @ozer550 has pushed changes addressing 1, 3, 4, and 7.

2, 5, 6 and 8 are being tracked elsewhere - once this is rebased it'll be ready for re-review

Done! should be ready for QA @pcenov .

[pcenov]

Thanks @ozer550 - I confirm that 1, 3, 4, and 7 are fixed now, and since the other comments will be addressed separately this PR is good to go!

[AlexVelezLl]

Just one important note: We usually should try not to make api requests inside fors as much as possible. The rest of the comments are non blocking :), in general there are some things we can standarize across these side panels, but it's probably a better idea to handle it separately.

[AlexVelezLl]

We have this CloseConfirmationGuard component to abstract the confirmation modal logic.

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 😅).

[AlexVelezLl]

Code changes look good to me!

[ozer550]

Hi @pcenov! Due to some latest changes structurally and logically this would require re-QA.

[pcenov]

Thanks @ozer550 - no new issues observed while testing again, LGTM!

The requested changes have been addressed.