[Bulk User]: Handle bad-data errors w/ `handleApiError` (+ better side panel refresh handling) #13769

nucleogenesis · 2025-09-23T04:20:15Z

Summary

In the apiResource.js file, the logError function expected an error object to have a particular shape - but the error that this issue fixes was getting swallowed up by the errors thrown when the error object in logError wasn't the shape expected. I added some existence checks to ensure errors don't get annihilated.
I noticed that refreshing w/ a side panel open would load the page w/ the side panel visible. I added checks in onMounted hooks to redirect to the root page (using the goBack module function) whenever there are no users selected.

This will give the user the ability to refresh the page, which will remove stale data.

References

Fixes #13550

Reviewer guidance

Refresh the page on each of the side panels
Perform the replication steps noted in BUM - Error when trying to enroll a learner in an already deleted class #13550 and see that when you get a bad-data error there, you are shown the API error page and that when you click "Refresh" you are taken to a fresh users table.

github-actions · 2025-09-23T04:58:41Z

Build Artifacts

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

pcenov · 2025-09-23T12:03:57Z

Hi @nucleogenesis - I confirm that now if I try to enrol a user in an already deleted class then I am seeing the "Sorry! Something went wrong" screen and the user can refresh the page.

Will you be handling the following similar edge cases in a similar manner within the scope of this PR or I should report them separately:

Trying to remove a user from a deleted class results in a success message (which is incorrect as the action was never completed) and consequently clicking the UNDO button results in an error message in the snackbar.:

remove.from.deleted.class.mp4

I would expect to see the generic error page in this case too, allowing me to refresh.

Similarly when assigning coaches to a deleted class the user is stuck at the side panel:

assign.coach.mp4

When creating a new user and attempting to assign or enrol the user in the deleted class there's no indication to the user that this is no longer possible:

create.a.new.user.and.assign.or.enrol.in.class.mp4

Actions performed on an already deleted user:

actions.with.deleted.user.mp4

Attempts to interact with a permanently deleted user:

attempts.to.interact.with.permanently.deleted.user.mp4

AlexVelezLl · 2025-09-23T13:52:46Z

kolibri/plugins/facility/assets/src/views/users/sidePanels/EnrollLearnersSidePanel.vue

+      onMounted(() => {
+        // Answering the question "what happens when we refresh?"
+        if (props.selectedUsers.size === 0) {
+          goBack();
+        }
+      });


Just reflecting if we should wait until the component is mounted to do this check? I think we can also prevent the load data API calls like this https://github.com/learningequality/kolibri/pull/13769/files#diff-1004c38374e2196707382df426d82066fb2c3e124faa549273ceff802b59695bR203 if the are no selected users.

I think your link might be pointing to the wrong place it's just bringing me back to this comment.

I did try it w/out the onMounted hook but it didn't work but if there's a better way I'll apply it here.

I've ended up moving this into a beforeRouteEnter guard @AlexVelezLl so that should avoid unneeded API calls.

marcellamaki

the changes you just pushed do seem to simplify things, although having read the code, I'm not sure that I'm the best positioned to be a final approver, since it seems like both @AlexVelezLl and @rtibbles had some concrete recommendations/feedback

marcellamaki · 2025-09-23T21:37:50Z

kolibri/plugins/facility/assets/src/views/users/sidePanels/EnrollLearnersSidePanel.vue

            createdMemberships.value = newMemberships;
          } catch (error) {
-            showErrorWarning.value = true;
+            // Why doesn't this ever get run here?


I don't know what this comment means?

Thanks for catching this lol - it is an artifact of my confusion caused by the errors being swallowed up in the apiResource module 😅

rtibbles

I had far fewer opinions than I thought I would. This is fine to go as long as manual testing of these specific points check out (others will be filed in follow ups).

marcellamaki

Refresh the page on each of the side panels ✅
Perform the replication steps noted in #13550 and see that when you get a bad-data error there, you are shown the API error page and that when you click "Refresh" you are taken to a fresh users table. ✅

manual testing confirms both of these. I will leave more robust edge case finding to peter in our beta0 :) good to go

AlexVelezLl

Just flagging that the implementation of the beforeRouteEnter is partially incorrect, because these side panels can also be opened from the new users page. So redirecting them to the main user management page feel a bit weird? It is an edge case though, so will let you decide if this is a blocker or not!

Screenshare.-.2025-09-23.6_29_37.PM.mp4

AlexVelezLl · 2025-09-23T23:39:15Z

In general I think that it does not makes much sense of having these side panels as route components. IIRC, the reason for having the side panels as route components was to respond to the question of "what happens if the user refresh the page?" and the reason of using routes for displaying the side panels was to keep the side panel open after the user refresh. But in this case this does not apply well because the user selection gets removed when they refresh. And now with this beforeRouteLeave we are basically just removing the reason why we were using routes for displaying the side panels.

Having these side panels coming from different parent routes have made the implementation of the routes definition, the dependencies definition of the components (as all of them are hidden behind the router-view tag), and the close side panel handling a bit more complex, and I think we are not getting anything good from having them as route components.

An option for making it worth could be exploring saving the selected users state in the local storage or route query params, so when the user reloads they keep seeing the side panels with the previously selected users, but I think that this will also add some more complexity to the implementation.

AlexVelezLl

Adding a blocking review until we figure out how to proceed regarding the previous review.

nucleogenesis · 2025-09-24T20:49:15Z

Thanks Alex - I'll definitely apply the changes to ensure the panels all work on the new/removed users pages.

I agree re: the routing - but I may put that into a follow-up issue.

…esh)

…ion this way

…e about the class The problem with showing an error here is that the removal can be partially successful. The error doesn't show up when removing, but rather when undoing the removal - these changes simply proceed along as if everything went okay because ... you're removing users from the class and, technically, they've already been removed from the class :)

AlexVelezLl

Just some thoughts!

AlexVelezLl · 2025-10-20T15:59:57Z

kolibri/plugins/facility/assets/src/views/users/sidePanels/EnrollLearnersSidePanel.vue

            createdMemberships.value = newMemberships;
          } catch (error) {
-            showErrorWarning.value = true;
+            store.dispatch('handleApiError', { error });


Should this specific side panel have a different error handling on its main action? The other side panels handle this by setting showErrorWarning to true.

I understood the issue Peter reported was that the error warning was unhelpful in this case because knowing something went wrong is half the battle - the other issue is that the most likely solution is to refresh the page & try again so dispatching the API error here ensures that happens.

To be honest, though, I think a more robust errors reporting would be ideal here to let the user know specifically what actually happened (ie, "The class you selected 'Class Name' no longer exists"). I'll think on this a bit more when I spin this up again in a bit. Thanks Alex

AlexVelezLl · 2025-10-20T16:07:22Z

kolibri/plugins/facility/assets/src/views/users/sidePanels/RemoveFromClassSidePanel.vue

+        try {
+          await Promise.all([
+            enrollments.length
+              ? MembershipResource.saveCollection({ data: enrollments })
+              : Promise.resolve(),
+            assignments.length
+              ? RoleResource.saveCollection({ data: assignments })
+              : Promise.resolve(),
+          ]);
+        } catch (_) {
+          showErrorWarning.value = true;
+        } finally {
+          loading.value = false;
+        }


The undoUserRemoval function is executed after the RemoveFromClassSidePanel has been unmounted, since this callback is executed in the undo snackbar, after the modal has been closed. Therefore, setting the showErrorWarning or loading refs here does not have any effect. Perhaps it would be a better idea to show this error in another snackbar?

Pushed a change using a Snackbar - great catch Alex thank you!

AlexVelezLl

Code changes look good to me! Thanks @nucleogenesis!

nucleogenesis added this to the Kolibri 0.19: Bulk User Management milestone Sep 23, 2025

nucleogenesis requested review from AllanOXDi and LianaHarris360 September 23, 2025 04:20

nucleogenesis added the P0 - critical Priority: Release blocker or regression label Sep 23, 2025

github-actions bot added APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) DEV: frontend SIZE: small labels Sep 23, 2025

AlexVelezLl reviewed Sep 23, 2025

View reviewed changes

rtibbles self-assigned this Sep 23, 2025

marcellamaki reviewed Sep 23, 2025

View reviewed changes

rtibbles reviewed Sep 23, 2025

View reviewed changes

marcellamaki approved these changes Sep 23, 2025

View reviewed changes

AlexVelezLl reviewed Sep 23, 2025

View reviewed changes

AlexVelezLl requested changes Sep 24, 2025

View reviewed changes

rtibbles assigned AlexVelezLl and unassigned rtibbles Oct 7, 2025

nucleogenesis added 6 commits October 13, 2025 12:41

more safely handle err.config properties in logError

5e7bafd

bulk user panels go to root table when no users selected (ie, on refr…

e353b29

…esh)

handle refresh in nav guard, not onMounted, avoid whole setup() funct…

08bca7f

…ion this way

errant comment

627600e

oops duped in rebase

6a89e60

nucleogenesis force-pushed the fix--bulk-action-to-deleted-class branch from 6d9f450 to 1316f1e Compare October 13, 2025 22:43

AlexVelezLl self-requested a review October 14, 2025 19:53

marcellamaki requested a review from akolson October 15, 2025 14:19

AlexVelezLl reviewed Oct 20, 2025

View reviewed changes

show undoing error message in SnackBar

f82d020

AlexVelezLl approved these changes Oct 20, 2025

View reviewed changes

nucleogenesis merged commit 413d0eb into learningequality:develop Oct 20, 2025
51 checks passed

pcenov mentioned this pull request Oct 23, 2025

Follow-up to "Handle bad-data errors w/ handleApiError" #13859

Open

[Bulk User]: Handle bad-data errors w/ handleApiError (+ better side panel refresh handling) #13769

[Bulk User]: Handle bad-data errors w/ handleApiError (+ better side panel refresh handling) #13769

Uh oh!

Conversation

nucleogenesis commented Sep 23, 2025

Summary

References

Reviewer guidance

Uh oh!

github-actions bot commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pcenov commented Sep 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcellamaki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibbles left a comment

Choose a reason for hiding this comment

Uh oh!

marcellamaki left a comment

Choose a reason for hiding this comment

Uh oh!

AlexVelezLl left a comment

Choose a reason for hiding this comment

Uh oh!

AlexVelezLl commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexVelezLl left a comment

Choose a reason for hiding this comment

Uh oh!

nucleogenesis commented Sep 24, 2025

Uh oh!

AlexVelezLl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexVelezLl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[Bulk User]: Handle bad-data errors w/ `handleApiError` (+ better side panel refresh handling) #13769

[Bulk User]: Handle bad-data errors w/ `handleApiError` (+ better side panel refresh handling) #13769

github-actions bot commented Sep 23, 2025 •

edited

Loading

AlexVelezLl commented Sep 23, 2025 •

edited

Loading