[Bulk User]: Handle bad-data errors w/ handleApiError (+ better side panel refresh handling)
#13769
Conversation
github-actions
bot
added
APP: Facility
Build Artifacts
|
|
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:
remove.from.deleted.class.mp4I would expect to see the generic error page in this case too, allowing me to refresh.
assign.coach.mp4
create.a.new.user.and.assign.or.enrol.in.class.mp4
actions.with.deleted.user.mp4
attempts.to.interact.with.permanently.deleted.user.mp4 |
| onMounted(() => { | ||
| // Answering the question "what happens when we refresh?" | ||
| if (props.selectedUsers.size === 0) { | ||
| goBack(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've ended up moving this into a beforeRouteEnter guard @AlexVelezLl so that should avoid unneeded API calls.
There was a problem hiding this comment.
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
| createdMemberships.value = newMemberships; | ||
| } catch (error) { | ||
| showErrorWarning.value = true; | ||
| // Why doesn't this ever get run here? |
There was a problem hiding this comment.
I don't know what this comment means?
There was a problem hiding this comment.
Thanks for catching this lol - it is an artifact of my confusion caused by the errors being swallowed up in the apiResource module 😅
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
|
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 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 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. |
|
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. |
Summary
apiResource.jsfile, thelogErrorfunction 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 inlogErrorwasn't the shape expected. I added some existence checks to ensure errors don't get annihilated.onMountedhooks to redirect to the root page (using thegoBackmodule 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