Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Oct 20, 2025
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5e7bafd
more safely handle err.config properties in logError
nucleogenesis Sep 23, 2025
e353b29
bulk user panels go to root table when no users selected (ie, on refr…
nucleogenesis Sep 23, 2025
08bca7f
handle refresh in nav guard, not onMounted, avoid whole setup() funct…
nucleogenesis Sep 23, 2025
627600e
errant comment
nucleogenesis Sep 23, 2025
6a89e60
oops duped in rebase
nucleogenesis Oct 13, 2025
1316f1e
Removing users from deleted class - no need to show error message her…
nucleogenesis Oct 13, 2025
f82d020
show undoing error message in SnackBar
nucleogenesis Oct 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions kolibri/plugins/facility/assets/src/views/users/sidePanels/AssignCoachesSidePanel.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@
import FacilityUserResource from 'kolibri-common/apiResources/FacilityUserResource';
import flatMap from 'lodash/flatMap';
import CloseConfirmationGuard from '../common/CloseConfirmationGuard.vue';
import { PageNames } from '../../../constants.js';
import { getRootRouteName, overrideRoute } from '../../../utils';
import SelectableList from '../../common/SelectableList.vue';
import { _userState } from '../../../modules/mappers';
Expand Down Expand Up @@ -335,6 +336,19 @@
default: () => {},
},
},
beforeRouteEnter(to, from, next) {
// We can't land here without having navigated to here from the users root page - we can't
// have selected any users if we load into this page, so go to the users table.
if (from.name === null) {
next(
// Override to to keep params like facility_id in place
overrideRoute(to, {
name: PageNames.USER_MGMT_PAGE,
}),
);
}
next();
},
beforeRouteLeave(to, from, next) {
this.$refs.closeConfirmationGuardRef?.beforeRouteLeave(to, from, next);
},
Expand Down
37 changes: 26 additions & 11 deletions kolibri/plugins/facility/assets/src/views/users/sidePanels/EnrollLearnersSidePanel.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@
<script>
import { useRoute } from 'vue-router/composables';
import { ref, computed } from 'vue';
import { getCurrentInstance, ref, computed } from 'vue';
import SidePanelModal from 'kolibri-common/components/SidePanelModal';
import commonCoreStrings, { coreStrings } from 'kolibri/uiText/commonCoreStrings';
import { useGoBack } from 'kolibri-common/composables/usePreviousRoute';
Expand All @@ -123,6 +123,7 @@
import groupBy from 'lodash/groupBy';
import SelectableList from '../../common/SelectableList.vue';
import useActionWithUndo from '../../../composables/useActionWithUndo';
import { PageNames } from '../../../constants.js';
import { getRootRouteName, overrideRoute } from '../../../utils';
import CloseConfirmationGuard from '../common/CloseConfirmationGuard.vue';
Expand All @@ -135,6 +136,16 @@
},
mixins: [commonCoreStrings],
setup(props) {
const store = getCurrentInstance().proxy.$store;
const route = useRoute();
const goBack = useGoBack({
getFallbackRoute: () => {
return overrideRoute(route, {
name: getRootRouteName(route),
});
},
});
const loading = ref(false);
const showErrorWarning = ref(false);
const selectedOptions = ref([]);
Expand All @@ -161,15 +172,6 @@
const { classesLabel$ } = coreStrings;
const route = useRoute();
const goBack = useGoBack({
getFallbackRoute: () => {
return overrideRoute(route, {
name: getRootRouteName(route),
});
},
});
// Computed properties
const classList = computed(() =>
props.classes
Expand Down Expand Up @@ -223,7 +225,7 @@
const newMemberships = await MembershipResource.saveCollection({ data: enrollments });
createdMemberships.value = newMemberships;
} catch (error) {
showErrorWarning.value = true;
store.dispatch('handleApiError', { error });
Copy link
Member

@AlexVelezLl AlexVelezLl Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@nucleogenesis nucleogenesis Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

loading.value = false;
return false;
}
Expand Down Expand Up @@ -310,6 +312,19 @@
default: () => {},
},
},
beforeRouteEnter(to, from, next) {
// We can't land here without having navigated to here from the users root page - we can't
// have selected any users if we load into this page, so go to the users table.
if (from.name === null) {
next(
// Override to to keep params like facility_id in place
overrideRoute(to, {
name: PageNames.USER_MGMT_PAGE,
}),
);
}
next();
},
beforeRouteLeave(to, from, next) {
this.$refs.closeConfirmationGuardRef?.beforeRouteLeave(to, from, next);
},
Expand Down
36 changes: 28 additions & 8 deletions kolibri/plugins/facility/assets/src/views/users/sidePanels/RemoveFromClassSidePanel.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@
import { UserKinds } from 'kolibri/constants';
import groupBy from 'lodash/groupBy';
import { useGoBack } from 'kolibri-common/composables/usePreviousRoute';
import { PageNames } from '../../../constants.js';
import SelectableList from '../../common/SelectableList.vue';
import { getRootRouteName, overrideRoute } from '../../../utils';
import useActionWithUndo from '../../../composables/useActionWithUndo';
Expand Down Expand Up @@ -267,14 +268,20 @@
kind: UserKinds.COACH,
}))
: [];
await Promise.all([
enrollments.length
? MembershipResource.saveCollection({ data: enrollments })
: Promise.resolve(),
assignments.length
? RoleResource.saveCollection({ data: assignments })
: Promise.resolve(),
]);
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;
}
Comment on lines 275 to 286
Copy link
Member

@AlexVelezLl AlexVelezLl Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@nucleogenesis nucleogenesis Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

props.onChange({
affectedClasses: selectedOptions.value,
resetSelection: true,
Expand Down Expand Up @@ -379,6 +386,19 @@
default: () => {},
},
},
beforeRouteEnter(to, from, next) {
// We can't land here without having navigated to here from the users root page - we can't
// have selected any users if we load into this page, so go to the users table.
if (from.name === null) {
next(
// Override to to keep params like facility_id in place
overrideRoute(to, {
name: PageNames.USER_MGMT_PAGE,
}),
);
}
next();
},
beforeRouteLeave(to, from, next) {
this.$refs.closeConfirmationGuardRef?.beforeRouteLeave(to, from, next);
},
Expand Down
4 changes: 2 additions & 2 deletions packages/kolibri/apiResource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1054,7 +1054,7 @@ export class Resource {
}
console.groupEnd();
}
if (Object.keys(err.config.params).length) {
if (err.config?.params && Object.keys(err.config.params).length) {
console.group('Query parameters');
for (const [k, v] of Object.entries(err.config.params)) {
console.log(`${k}: ${v}`);
Expand All @@ -1073,7 +1073,7 @@ export class Resource {
}
} catch (e) {} // eslint-disable-line no-empty
}
if (Object.keys(err.config.headers).length) {
if (err.config?.headers && Object.keys(err.config.headers).length) {
console.group('Headers');
for (const [k, v] of Object.entries(err.config.headers)) {
console.log(`${k}: ${v}`);
Expand Down