Prevent dual enrollment 13847

[nucleogenesis]

Summary

Work planned interactively with Claude Code (+superpowers), some (mostly backend) code updates performed by Claude code.

Disallows in the API the ability to enroll a user to a class if they're already assigned to that class as a coach (or vice
versa).

This maintains the same behavior of enrolling/assigning users through the Class Details page, where we are filtering out the users which are not permitted.

References

Fixes #13847

Reviewer guidance

Known issues

• You should not see any error when you try to add a user who would not be added - the snackbar "users enrolled successfully" may not be 100% true, but it's not in scope here as UX writing review can help us poilsh this UX
• Currently there is just a message like "Users already in selected classes will not be affected" - we may update some of this through the UX writing reviwe

Fixes the related issue

Basically, you should be able to use the bulk user management flows without issue such that if you try to assign/enroll a user who is not permitted to be assigned/enrolled because they're already one or the other. Users included in the selection are basically ignored/filtered out during the request to assign/enroll them - so there should be basically no effect on that user's relation to the class(es) selected.

• Test one user who should be ignored
• Test a selection with a mix of users - some who will certainly be enrolled/assigned and others who won't
• Test both of the above with 1 selected class, and when selecting multiple classes to assign/enroll them to
• Be sure to test both directions (ie, an admin enrolled as a learner cannot be then assigned as a coach, and vice versa).

[nucleogenesis]

Not sure this is necessary but Claude seemed concerned during planning about how this could/would impact learnergroups -- but really there isn't a path for this to be an issue in the UI

[nucleogenesis]

I kept it because it's something that should pass so long as we're not making huge changes to the UI and it's probably a good place to see a failure if those changes touch this API anyway

[rtibbles]

My only thoughts are:

1. Silently 201ing feels slightly icky, but I get it. As it's an edge case, the alternative would be to just raise a 400 and let the frontend try again.
2. For both roles and membership, it's doing an exists query for every item that's being created, so you will have an N queries issue. Might be better to collect user lists by collection and then do a check for each collection and cross referencing the user_id lists. Alternatively, if you're just doing a 400 for the entire request, you could do an exists check for each colelction and raise the ValidationError there and then.
3. Are we ever using the individual model serializer on its own, or are we always handling even a single create/update using the ListSerializer? If not, then this will still be possible one user at a time.

[github-actions]

Build Artifacts

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

[nucleogenesis]

This was the tree I started barking up by hand exploring - there's obviously a lot more code here and a lot of iterating over things in general @rtibbles - but it does address the N queries problem I think. As I read through it (thanks Claude) my main negative thought was "seems like a lot of code for what feels like it should be simpler to do".

At a high level, though (and to your point # 1 re: the ickiness of silent 201s), hindsight being 20/20 I think that we might have done well to deliberately make for a partial success API call that includes some info in the response about what did/didn't happen. We never intended to display such granular errors to the user, necessarily, but it could be used to answer certain questions such as "did anything happen at all?" because in this case you could select 2 coaches and try to enroll them in a class they're filtered out of and still we'll say "Users enrolled successfully".

[pcenov]

No issues observed while manually testing - implemented as specified!

[rtibbles]

It's an edge case (but so is the dual enrollment prevention), but telling a user that the coach has been assigned, when it definitely hasn't, seems pretty misleading (and is a result of our silently swallowing errors).

Screencast.From.2025-10-27.11-41-24.mp4

[pcenov]

Hi @rtibbles and @nucleogenesis - yesterday while testing I saw the edge case based on which Richard is requesting changes here, but I thought that this is corresponding to the general idea of this fix. I don't see how this edge case is different from the entirely realistic scenario where if I go to the Users table, select exactly the same coach user and assign that user as a coach (getting a misleading/entirely wrong success message) since the coach user is actually already enrolled as a learner?

[nucleogenesis]

@pcenov This PR needs some additional review as we've opted to include some substantial changes that handle edge cases around partial success/failure.

Please test the following scenarios in bulk user management workflow on the users table:

1. When all of your selected users can successfully be enrolled/assigned (ie, they're not assigned/enrolled and should succeed)
2. When some, but not all, of your users can successfully be enrolled/assigned (ie, assign some users who are enrolled and some who are not -- test in both directions)
3. When none of your selected users would successfully be enrolled/assigned (ie, assign a selection of users who are enrolled -- test in both directions)

In the "Class Details" page, please test the edge case of multi-tab scenarios where one tab opens to see a list of users who are valid, then make it so they cannot be assigned/enrolled (based on the circumstances) in another tab, go back and try to assign the users who should no longer be allowed to be assigned/enrolled in that situation.

Generally, you should see correct snackbar messages.

---

Things to expect to see:

1. When there are NO failures at all: the workflow should be basically the same as before - we close the side panel and go back to a unfiltered table.
2. When there are ANY failures:

• the side panel will close
• the snackbar message will indicate the partial success
• the user will see the UsersTable pre-filtered to only include the users who failed to be assigned/enrolled
• there should also be a correct message alert at the top of the table about the failure
• clearing the filters should result in the alert message disappearing

---

[pcenov]

Hi @nucleogenesis,

Users page:

1. If I have already assigned a user as a coach, then when I attempt to enroll the same user as a learner I am seeing (only for a split second) a yellow banner "The users shown were unable to be enrolled", and I am seeing the table with all available users, plus the following error in the console: ReferenceError: invalidRoles is not defined

the.users.shown.were.unable.to.be.enrolled.mp4

Similar behavior is observed when attempting to enroll several coaches as learners while some of them are already enrolled as such:

coaches.enrolled.as.learners.mp4

2. If I have already enrolled some of my coaches to a class as learners, then when I attempt to assign all of the coaches to all available classes I am seeing the following snackbar message Some coaches assigned successfully, but some were not. (which is correct by itself) and I am seeing only the table rows for the coaches which were not assigned to a class (since they are enrolled as learners) BUT while partially helpful, this can still be very confusing from a user's point of view, as the snackbar message is shown only for 5 seconds and also I am still seeing the text 11 users selected <ins>Clear selection</ins> while there are now only 3 coaches in the table... Nothing happens when I attempt to click on the Clear selection link and I am seeing a bunch of TypeError: someFailedToAssign$ is not a function errors in the console.

some.coaches.not.assigned.mp4

3. If a user (regardless of the role) is not enrolled in a class, then when I try to enroll that user (or multiple users) in a class, I am always brought back to the page I was before completing the action:

enroll-brought-back-to-previous-page.mp4

4. At the New users page things are working better, but I'm not seeing the yellow banner at all, and when trying to enroll already enrolled learners, the list with users is not filtered:

new.users.mp4

Classes page:

All looks good to me, except the following potentially confusing edge cases:

1. An edge case where I am at the Classes page and I am keeping the Assign coaches tab open separately, while at the same time I have already enrolled all coaches as learners, results in not showing the snackbar message at all:

edge.case.with.coaches.enrolled.mp4

Trying the same scenario with some coaches enrolled as learners and others not, results in seeing the following snackbar message Some coaches assigned successfully, but some were not., which while being true is potentially not very helpful in that particular scenario especially if they have multiple coaches and they don't know which were assigned and which were not:

some.were.some.were.not.mp4

[AlexVelezLl]

Thanks @nucleogenesis! Just one little regression I found on the enroll learners side panel. Everything else are more nitpicky comments!

[AlexVelezLl]

(Nitpick) Initially, it was not evident to me why an invalid_items property would be set if the is_valid method is setting raise_exception=True and in theory it would raise an exception if the data is invalid, but this is true for bulk creations where some items may be invalid but not all of them, and therefore it wouldn't raise the exception. It would be great if we could have some comments around here explaining this.

[AlexVelezLl]

Not sure how relevant it is, but just noting that if we open the filters, we can still see the previously applied filters, which may or may not be confusing:

[AlexVelezLl]

It's just an idea, but I feel we are adding too many edgy handlers for the by_ids filters that may be easily overseen if we update these modules in the future. I wonder if it would be better just to explicitly reset all filters when applying the by_ids filter, and let this filter co-exist with others without too many edge case handlers.

So, in the side panel modals, we may call the resetWorkingFilter and apply filters with just a by_ids filter, so this would override the filters on the route query leaving the by_ids filter always isolated, but isolated by on purposely removing all other filters from its source of truth and not by adding conditionals to cut the filters execution in the composables.

[nucleogenesis]

I need to come back to this comment tomorrow with a fresher mind but I like your suggestion here . I also think that taking pass at what you're talking about here will help me better work out if or how we should handle your comment just above re: filters still being visible

[pcenov]

Hi @nucleogenesis, I confirm that the issues I reported are now fixed and that sorting is working when displaying the list of unassigned users.

Critical regression issue:

The new user creation flow is broken now when directly enrolling or assigning the newly created user (the user is actually created but the side panel doesn't reflect that fact):

new.user.broken.mp4

New issue:

1. When I am presented with the list of unassigned users, it feels natural to select them, remove them from all classes and then attempt to assign them to all or some of the classes. However in that case the banner persistently remains visible along with the now obsolete list of unassigned users, since they are now assigned:

clear.the.unassigned.filter.mp4

A couple of usability issues that should be further discussed:

1. When I am assigning/enrolling users in multiple classes and some were not assigned/enrolled, then we do show the yellow banner and we do list the users, but it's not clear for which class or classes that applies. So it's probably best to add that info in the banner text otherwise an unsuspecting user might keep trying to assign or enroll the users wondering why it's not working (I guess they should be advised to go to the Classes page and start looking at what's going on):

not.clear.for.which.class.that.applies.mp4

Here's an example of me searching for a specific user, trying to assign that user to several classes and not being sure what's failing. Note that the banner text doesn't reflect the fact that we are dealing with a single user, and that the snackbar message text Some coaches assigned successfully, but some were not. is really confusing:

working.with.a.single.user.mp4

3. When we are presented with the short list of users who were not assigned or enrolled then I can see the 1 filter link and if I click on it, none of the filters are selected leaving me puzzled. So maybe in that case the 1 filter link should be hidden.

says.1.filter.applied.but.no.filters.selected.mp4

Note that it should also be considered that one can have applied some filters prior to assigning/enrolling users, in which case currently if everything is OK we are keeping the filters intact after completing the operation, while if some of the users were not assigned/enrolled we are again showing the 1 filter link (while I can clearly see that there are multiple filters applied):

several.filters.applied.mp4

[pcenov]

Hi @nucleogenesis,

1. The critical regression is fixed for users of type learner, facility coach and admin, however when I try to create a class coach user, select a class and click either the Save and close or Save and add another button then I am getting a 500 error in the console plus TypeError: this.errors.includes is not a function:

coach.error.mp4

2. After creating a new user the user is not immediately listed in the New users table, have to manually refresh the page to see the user:

new.user.not.shown.in.the.table.mp4

3. When a user cannot be assigned then at the Users page you are showing the yellow banner, while at the New users page you are not:

users.vs.new.users.mp4

From the list with the usability issues in my previous comment I can see that only the last point has been fixed, so lets not forget to discuss the rest as they are important. :)

[pcenov]

Hi @nucleogenesis,

I'm happy to let you know that I was able to identify just the following minor edge case at the Users table:

1. The message 'The users shown were unable to be assigned.' doesn't disappear when I decide to instead enroll the users which I was previously unable to assign:

banner.not.disappearing.after.successful.enrollment.mp4

This scenario is working correctly with the other message ("The users shown were unable to be enrolled.").

Everything else seems to be working properly.

At the Classes page I noticed the following regression - when I remove some or all of the coaches and try to copy the class then in the modal the count of the coaches is not updated correctly and that results in an error when copying the class:

class.removed.users.count.mp4

[rtibbles]

There are some significant bugs hiding here - obfuscated by an extra wrapper of verbose claude generated code in some cases.

[rtibbles]

Is this meant to be an else if with the created?.length check? Otherwise, this is just overriding the someLearnersEnrolledNotice above with noLearnersEnrolled.

[nucleogenesis]

This logic made more sense when I was returning early in the side panels like this where no else-if is needed to avoid double-snackbarring

      const snackbarMessage$ = () => {
        if (createdRoles.value?.length) {
          if (invalidRoles.value?.length) {
            return someCoachesAssignedNotice$();
          } else {
            return coachesAllAssignedNotice$();
          }
        }
        if (invalidRoles.value?.length) {
          return someFailedToAssign$();
        }
      };

Will push updates here shortly

[rtibbles]

Yeah, I saw the other instances where this was a return early pattern instead, and saw where this came from.

[rtibbles]

Again, shouldn't this be an else if?

[rtibbles]

I am a bit confused by this - why are we only preventing classroom assignable coaches from being assigned and enrolled in a class, but not putting similar restrictions on facility coaches and admins?

[rtibbles]

A user cannot have an "assignable_coach" role wrt a classroom, only wrt a facility.