From 514c83034b8b9325c4a35196d567592605d75e56 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Wed, 12 Aug 2026 08:29:17 -0700 Subject: [PATCH 1/8] Move facility resource calls onto the new Resource methods The Model/Collection cache goes with them, so every read that did not pass `force: true` now hits the network. Co-Authored-By: Claude Opus 5 (1M context) --- .../__tests__/useFacilityEditor.spec.js | 29 +++++++---- .../frontend/composables/useDeleteClass.js | 2 +- .../frontend/composables/useFacilityEditor.js | 11 ++--- .../modules/classAssignMembers/actions.js | 18 +++---- .../modules/classAssignMembers/handlers.js | 30 +++++------- .../modules/classEditManagement/actions.js | 19 ++++---- .../modules/classEditManagement/handlers.js | 6 +-- .../modules/classManagement/actions.js | 8 ++-- .../modules/classManagement/handlers.js | 5 +- .../frontend/modules/importCSV/index.js | 4 +- .../frontend/modules/manageCSV/actions.js | 15 ++---- .../modules/userManagement/actions.js | 48 +++++++++++-------- .../frontend/modules/userManagement/utils.js | 16 +++---- .../views/DataPage/SyncInterface/index.vue | 4 +- .../frontend/views/DataPage/index.vue | 2 +- .../facility/frontend/views/UserEditPage.vue | 4 +- .../views/__tests__/UserEditPage.spec.js | 2 +- .../frontend/views/common/ClassCopyModal.vue | 29 +++++------ .../views/common/ClassRenameModal.vue | 2 +- .../sidePanels/AssignCoachesSidePanel.vue | 12 ++--- .../sidePanels/EnrollLearnersSidePanel.vue | 15 +++--- .../sidePanels/RemoveFromClassSidePanel.vue | 23 ++------- .../__tests__/UserCreateSidePanel.spec.js | 38 +++++++-------- .../users/sidePanels/UserCreate/index.vue | 42 ++++++++-------- 24 files changed, 167 insertions(+), 217 deletions(-) diff --git a/kolibri/plugins/facility/frontend/composables/__tests__/useFacilityEditor.spec.js b/kolibri/plugins/facility/frontend/composables/__tests__/useFacilityEditor.spec.js index fbb8a2bb568..8e562a363cd 100644 --- a/kolibri/plugins/facility/frontend/composables/__tests__/useFacilityEditor.spec.js +++ b/kolibri/plugins/facility/frontend/composables/__tests__/useFacilityEditor.spec.js @@ -546,16 +546,13 @@ describe('useFacilityEditor', () => { describe('saveFacilityName', () => { it('saves facility name and updates facilities list', async () => { const newName = 'New Facility Name'; - FacilityResource.saveModel.mockResolvedValue({ id: mockFacilityId, name: newName }); + FacilityResource.update.mockResolvedValue({ id: mockFacilityId, name: newName }); const { saveFacilityName, facilityName } = useFacilityEditor(); await saveFacilityName(newName); - expect(FacilityResource.saveModel).toHaveBeenCalledWith({ - id: mockFacilityId, - data: { name: newName }, - }); + expect(FacilityResource.update).toHaveBeenCalledWith(mockFacilityId, { name: newName }); expect(facilityName.value).toBe(newName); }); }); @@ -571,13 +568,29 @@ describe('useFacilityEditor', () => { await saveFacilityConfig(); - const savedData = FacilityDatasetResource.saveModel.mock.calls[0][0].data; + const [savedId, savedData] = FacilityDatasetResource.update.mock.calls[0]; + expect(savedId).toBe(mockDatasetId); expect(savedData).not.toHaveProperty('picture_password_settings'); expect(savedData).not.toHaveProperty('learner_can_login_with_no_password'); expect(savedData).not.toHaveProperty('learner_can_edit_password'); expect(savedData).toHaveProperty('learner_can_edit_username'); expect(savedData).toHaveProperty('id'); }); + + it('diffs the config against the snapshot last synced with the server', async () => { + const { saveFacilityConfig, copySettings, settings, settingsCopy, facilityDatasetId } = + useFacilityEditor(); + settings.value = { ...mockFacilityConfig }; + facilityDatasetId.value = mockDatasetId; + copySettings(); + const snapshot = { ...settingsCopy.value }; + settings.value = { ...settings.value, learner_can_edit_username: false }; + + await saveFacilityConfig(); + + const [, , options] = FacilityDatasetResource.update.mock.calls.at(-1); + expect(options.baseline).toEqual(snapshot); + }); }); describe('setPin', () => { @@ -597,7 +610,7 @@ describe('useFacilityEditor', () => { method: 'POST', data: mockPayload, }); - expect(FacilityDatasetResource.saveModel).toHaveBeenCalled(); + expect(FacilityDatasetResource.update).toHaveBeenCalled(); }); }); @@ -616,7 +629,7 @@ describe('useFacilityEditor', () => { url: '/api/facility_dataset/update_pin/', method: 'PATCH', }); - expect(FacilityDatasetResource.saveModel).toHaveBeenCalled(); + expect(FacilityDatasetResource.update).toHaveBeenCalled(); }); }); diff --git a/kolibri/plugins/facility/frontend/composables/useDeleteClass.js b/kolibri/plugins/facility/frontend/composables/useDeleteClass.js index ff8d2aeea28..08e668634e5 100644 --- a/kolibri/plugins/facility/frontend/composables/useDeleteClass.js +++ b/kolibri/plugins/facility/frontend/composables/useDeleteClass.js @@ -25,7 +25,7 @@ export default function useDeleteClass(classroomProp) { return Promise.reject('No classId was provided'); } - return ClassroomResource.deleteModel({ id: deleteId }).then( + return ClassroomResource.delete(deleteId).then( () => { $store.commit('classManagement/DELETE_CLASS', deleteId); }, diff --git a/kolibri/plugins/facility/frontend/composables/useFacilityEditor.js b/kolibri/plugins/facility/frontend/composables/useFacilityEditor.js index 6edba7c0a0f..7d947475fe0 100644 --- a/kolibri/plugins/facility/frontend/composables/useFacilityEditor.js +++ b/kolibri/plugins/facility/frontend/composables/useFacilityEditor.js @@ -196,10 +196,7 @@ export default function useFacilityEditor() { * @returns {Promise} Resolves with the updated facility model. */ async function saveFacilityName(name) { - const facility = await FacilityResource.saveModel({ - id: facilityId.value, - data: { name }, - }); + const facility = await FacilityResource.update(facilityId.value, { name }); // Update facilities list await fetchFacility(); @@ -219,9 +216,9 @@ export default function useFacilityEditor() { for (const field of LOGIN_SETTINGS_FIELDS) { delete data[field]; } - await FacilityDatasetResource.saveModel({ - id: facilityDatasetId.value, - data, + // Diffing against the last server-synced snapshot sends only the edited settings. + await FacilityDatasetResource.update(facilityDatasetId.value, data, { + baseline: settingsCopy.value, }); await fetchFacilityConfig(); copySettings(); diff --git a/kolibri/plugins/facility/frontend/modules/classAssignMembers/actions.js b/kolibri/plugins/facility/frontend/modules/classAssignMembers/actions.js index 069c631ae70..654c1287e82 100644 --- a/kolibri/plugins/facility/frontend/modules/classAssignMembers/actions.js +++ b/kolibri/plugins/facility/frontend/modules/classAssignMembers/actions.js @@ -4,26 +4,20 @@ import { UserKinds } from 'kolibri/constants'; import uniq from 'lodash/uniq'; export function enrollLearnersInClass(store, { classId, users }) { - return MembershipResource.saveCollection({ - getParams: { - collection: classId, - }, - data: uniq(users).map(userId => ({ + return MembershipResource.bulkCreate( + uniq(users).map(userId => ({ collection: classId, user: userId, })), - }); + ); } export function assignCoachesToClass(store, { classId, coaches }) { - return RoleResource.saveCollection({ - getParams: { - collection: classId, - }, - data: uniq(coaches).map(userId => ({ + return RoleResource.bulkCreate( + uniq(coaches).map(userId => ({ collection: classId, user: userId, kind: UserKinds.COACH, })), - }); + ); } diff --git a/kolibri/plugins/facility/frontend/modules/classAssignMembers/handlers.js b/kolibri/plugins/facility/frontend/modules/classAssignMembers/handlers.js index d7ffb3f12ba..a540ab2e68d 100644 --- a/kolibri/plugins/facility/frontend/modules/classAssignMembers/handlers.js +++ b/kolibri/plugins/facility/frontend/modules/classAssignMembers/handlers.js @@ -15,8 +15,8 @@ export function showLearnerClassEnrollmentPage(store, toRoute, fromRoute) { } // facility users that are not enrolled in this class - const userPromise = FacilityUserResource.fetchCollection({ - getParams: pickBy({ + const userPromise = FacilityUserResource.list( + pickBy({ member_of: facilityId.value, page: toRoute.query.page || 1, page_size: toRoute.query.page_size || 30, @@ -24,10 +24,9 @@ export function showLearnerClassEnrollmentPage(store, toRoute, fromRoute) { exclude_member_of: id, exclude_coach_for: id, }), - force: true, - }); + ); // current class - const classPromise = ClassroomResource.fetchModel({ id }); + const classPromise = ClassroomResource.retrieve(id); const shouldResolve = samePageCheckGenerator(toRoute); return Promise.all([userPromise, classPromise]).then( ([facilityUsers, classroom]) => { @@ -58,20 +57,17 @@ export function showCoachClassAssignmentPage(store, toRoute, fromRoute) { pageLoading.value = true; } // all users in facility eligible to be a coach that is not already a coach - const userPromise = FacilityUserResource.fetchCollection({ - getParams: { - member_of: facilityId.value, - exclude_member_of: id, - exclude_user_type: 'learner', - exclude_coach_for: id, - page: toRoute.query.page || 1, - page_size: toRoute.query.page_size || 30, - search: toRoute.query.search && toRoute.query.search.trim(), - }, - force: true, + const userPromise = FacilityUserResource.list({ + member_of: facilityId.value, + exclude_member_of: id, + exclude_user_type: 'learner', + exclude_coach_for: id, + page: toRoute.query.page || 1, + page_size: toRoute.query.page_size || 30, + search: toRoute.query.search && toRoute.query.search.trim(), }); // current class - const classPromise = ClassroomResource.fetchModel({ id, force: true }); + const classPromise = ClassroomResource.retrieve(id); const shouldResolve = samePageCheckGenerator(toRoute); return Promise.all([userPromise, classPromise]).then( ([facilityUsers, classroom]) => { diff --git a/kolibri/plugins/facility/frontend/modules/classEditManagement/actions.js b/kolibri/plugins/facility/frontend/modules/classEditManagement/actions.js index 94f5fa3bf3e..bdc92628bc2 100644 --- a/kolibri/plugins/facility/frontend/modules/classEditManagement/actions.js +++ b/kolibri/plugins/facility/frontend/modules/classEditManagement/actions.js @@ -9,7 +9,7 @@ export function removeClassLearner(store, { classId, userId }) { return; } // fetch the membership model with this classId and userId. - return MembershipResource.deleteCollection({ + return MembershipResource.bulkDelete({ user: userId, collection: classId, }).then( @@ -31,7 +31,7 @@ export function removeClassCoach(store, { classId, userId }) { } // TODO use a getModel with role id? should be available. Might have to undo mappers // fetch the membership model with this classId and userId. - return RoleResource.deleteCollection({ + return RoleResource.bulkDelete({ user: userId, collection: classId, }).then( @@ -46,22 +46,19 @@ export function removeClassCoach(store, { classId, userId }) { } /** - * Updates a class with the given data and commits the change to the store. + * Renames a class and commits the change to the store. + * `name` is the only class field this action can change, so the payload is already the diff. * @param {object} store - The Vuex store instance. * @param {object} payload - Payload object. * @param {string} payload.id - The ID of the class to update. - * @param {object} payload.updateData - The data to update on the class. + * @param {string} payload.name - The new class name. * @returns {Promise|void} Resolves when the class has been updated. */ -export function updateClass(store, { id, updateData }) { - if (!id || Object.keys(updateData).length === 0) { - // if no id or empty updateData passed, abort the function +export function updateClass(store, { id, name }) { + if (!id || !name) { return; } - return ClassroomResource.saveModel({ - id, - data: updateData, - }).then( + return ClassroomResource.update(id, { name }).then( updatedClass => { store.commit('UPDATE_CLASS', { id, updatedClass }); store.dispatch('displayModal', false); diff --git a/kolibri/plugins/facility/frontend/modules/classEditManagement/handlers.js b/kolibri/plugins/facility/frontend/modules/classEditManagement/handlers.js index 0158201289d..0770c35eac5 100644 --- a/kolibri/plugins/facility/frontend/modules/classEditManagement/handlers.js +++ b/kolibri/plugins/facility/frontend/modules/classEditManagement/handlers.js @@ -17,9 +17,9 @@ export function showClassEditPage(store, classId) { const { facilityId } = useFacility(); const promises = [ - FacilityUserResource.fetchCollection({ getParams: { member_of: classId }, force: true }), - ClassroomResource.fetchModel({ id: classId, force: true }), - ClassroomResource.fetchCollection({ getParams: { parent: facilityId.value }, force: true }), + FacilityUserResource.list({ member_of: classId }), + ClassroomResource.retrieve(classId), + ClassroomResource.list({ parent: facilityId.value }), ]; store.commit('classEditManagement/SET_DATA_LOADING', true); Promise.all(promises) diff --git a/kolibri/plugins/facility/frontend/modules/classManagement/actions.js b/kolibri/plugins/facility/frontend/modules/classManagement/actions.js index 46200e857ab..b7d35cec57a 100644 --- a/kolibri/plugins/facility/frontend/modules/classManagement/actions.js +++ b/kolibri/plugins/facility/frontend/modules/classManagement/actions.js @@ -9,11 +9,9 @@ import { selectedFacilityId } from 'kolibri-common/composables/useFacility'; * @returns {Promise} Resolves when the class has been created. */ export function createClass(store, name) { - return ClassroomResource.saveModel({ - data: { - name, - parent: selectedFacilityId.value, - }, + return ClassroomResource.create({ + name, + parent: selectedFacilityId.value, }).then( classroom => { store.commit('ADD_CLASS', classroom); diff --git a/kolibri/plugins/facility/frontend/modules/classManagement/handlers.js b/kolibri/plugins/facility/frontend/modules/classManagement/handlers.js index 760678e7142..ef2cea615dc 100644 --- a/kolibri/plugins/facility/frontend/modules/classManagement/handlers.js +++ b/kolibri/plugins/facility/frontend/modules/classManagement/handlers.js @@ -8,10 +8,7 @@ export function showClassesPage(store) { store.commit('classManagement/SET_STATE', { dataLoading: true }); const { facilityId } = useFacility(); - return ClassroomResource.fetchCollection({ - getParams: { parent: facilityId.value }, - force: true, - }) + return ClassroomResource.list({ parent: facilityId.value }) .then(classrooms => { store.commit('classManagement/SET_STATE', { modalShown: false, diff --git a/kolibri/plugins/facility/frontend/modules/importCSV/index.js b/kolibri/plugins/facility/frontend/modules/importCSV/index.js index 415232d1e98..7f5d864f36e 100644 --- a/kolibri/plugins/facility/frontend/modules/importCSV/index.js +++ b/kolibri/plugins/facility/frontend/modules/importCSV/index.js @@ -86,14 +86,14 @@ export default { state.filename = task.extra_metadata.filename; state.users_report = task.extra_metadata.users; state.classes_report = task.extra_metadata.classes; - TaskResource.clear(state.taskId); + TaskResource.clear_v2(state.taskId); state.taskId = ''; }, SET_FAILED(state, task) { state.status = CSVImportStatuses.ERRORS; set(state, 'overall_error', task.extra_metadata.overall_error); set(state, 'per_line_errors', []); - TaskResource.clear(state.taskId); + TaskResource.clear_v2(state.taskId); state.taskId = ''; }, }, diff --git a/kolibri/plugins/facility/frontend/modules/manageCSV/actions.js b/kolibri/plugins/facility/frontend/modules/manageCSV/actions.js index 8eec7535f6d..aa8dc08d5d2 100644 --- a/kolibri/plugins/facility/frontend/modules/manageCSV/actions.js +++ b/kolibri/plugins/facility/frontend/modules/manageCSV/actions.js @@ -18,10 +18,7 @@ function getFirstLogDate(store) { } function getCSVLogRequest(store, logType, facility) { - return GenerateCSVLogRequestResource.fetchCollection({ - getParams: { log_type: logType, facility: facility }, - force: true, - }) + return GenerateCSVLogRequestResource.list({ log_type: logType, facility }) .then(csvlogrequest => { if (logType == 'summary') { store.commit('SET_SUMMARY_LOG_REQUEST', csvlogrequest[0]); @@ -110,7 +107,7 @@ function checkTaskStatus(store, newTasks, taskType, taskId, commitStart, commitF store.commit(commitFinish, new Date()); getExportedCSVsInfo(store); } - TaskResource.clear(taskId); + TaskResource.clear_v2(taskId); } } else { const running = myNewTasks.filter(task => { @@ -139,12 +136,8 @@ function startExportUsers(store) { } function refreshTaskList(store) { - return Promise.all([ - TaskResource.fetchCollection({ - force: true, - }), - ]) - .then(([newTasks]) => { + return TaskResource.list() + .then(newTasks => { checkTaskStatus( store, newTasks, diff --git a/kolibri/plugins/facility/frontend/modules/userManagement/actions.js b/kolibri/plugins/facility/frontend/modules/userManagement/actions.js index d5b166c8ccb..2599fb12655 100644 --- a/kolibri/plugins/facility/frontend/modules/userManagement/actions.js +++ b/kolibri/plugins/facility/frontend/modules/userManagement/actions.js @@ -14,7 +14,7 @@ import { updateFacilityLevelRoles } from './utils'; function setUserRole(user, role) { return updateFacilityLevelRoles(user, role.kind).then(() => { // Force refresh the User to get updated roles - return FacilityUserResource.fetchModel({ id: user.id, force: true }); + return FacilityUserResource.retrieve(user.id); }); } @@ -26,17 +26,15 @@ function setUserRole(user, role) { * @returns {Promise} Resolves when the user has been created. */ export function createFacilityUser(store, payload) { - return FacilityUserResource.saveModel({ - data: { - facility: selectedFacilityId.value, - username: payload.username, - full_name: payload.full_name, - password: payload.password, - id_number: payload.id_number, - gender: payload.gender, - birth_year: payload.birth_year, - extra_demographics: payload.extra_demographics, - }, + return FacilityUserResource.create({ + facility: selectedFacilityId.value, + username: payload.username, + full_name: payload.full_name, + password: payload.password, + id_number: payload.id_number, + gender: payload.gender, + birth_year: payload.birth_year, + extra_demographics: payload.extra_demographics, }).then(facilityUser => { if (payload.role.kind !== UserKinds.LEARNER) { return setUserRole(facilityUser, payload.role); @@ -44,24 +42,32 @@ export function createFacilityUser(store, payload) { }); } +/** + * Updates a facility user's details, and their facility-level role when it changed. + * `updates.facilityUserUpdates` is already diff-only — `UserEditPage.getUpdates` picks just the + * fields that differ from the fetched user — so no baseline is passed to `update`. + * @param {object} store - The Vuex store instance. + * @param {object} payload - Payload object. + * @param {string} payload.userId - The ID of the user to update. + * @param {object} payload.updates - `{ facilityUserUpdates, roleUpdates }`. + * @returns {Promise} Resolves when the user has been updated. + */ export function updateFacilityUserDetails(store, { userId, updates }) { const { facilityUserUpdates, roleUpdates } = updates; if (isEmpty(facilityUserUpdates) && !roleUpdates) { return Promise.resolve(); } - return FacilityUserResource.saveModel({ id: userId, data: { ...facilityUserUpdates } }).then( - user => { - if (roleUpdates) { - return updateFacilityLevelRoles(user, roleUpdates.kind); - } - }, - ); + return FacilityUserResource.update(userId, facilityUserUpdates).then(user => { + if (roleUpdates) { + return updateFacilityLevelRoles(user, roleUpdates.kind); + } + }); } export function updateFacilityUserPassword(store, { userId, password }) { - return FacilityUserResource.saveModel({ id: userId, data: { password } }); + return FacilityUserResource.update(userId, { password }); } export function deleteFacilityUser(store, { userId }) { - return FacilityUserResource.deleteModel({ id: userId }); + return FacilityUserResource.delete(userId); } diff --git a/kolibri/plugins/facility/frontend/modules/userManagement/utils.js b/kolibri/plugins/facility/frontend/modules/userManagement/utils.js index 308713e0a71..b9cb5d2a93c 100644 --- a/kolibri/plugins/facility/frontend/modules/userManagement/utils.js +++ b/kolibri/plugins/facility/frontend/modules/userManagement/utils.js @@ -20,12 +20,10 @@ export function updateFacilityLevelRoles(facilityUser, newRoleKind) { // Currently, we assume only ONE Facility-Level Role per user const currentFacilityRole = find(roles, { collection: facility }); const createFacilityRole = () => - RoleResource.saveModel({ - data: { - user: id, - collection: facility, - kind: newRoleKind, - }, + RoleResource.create({ + user: id, + collection: facility, + kind: newRoleKind, }); // When FacilityUser is only a Learner or New User (i.e. no current Role) @@ -43,14 +41,12 @@ export function updateFacilityLevelRoles(facilityUser, newRoleKind) { // Downgrading Role to LEARNER if (newRoleKind === UserKinds.LEARNER) { - return RoleResource.deleteCollection({ user: id }); + return RoleResource.bulkDelete({ user: id }); } // Changing from one Facility-Level Role to another. Any Classroom-Level Roles // are left untouched if (FACILITY_ROLES.includes(newRoleKind)) { - return createFacilityRole().then(() => - RoleResource.deleteModel({ id: currentFacilityRole.id }), - ); + return createFacilityRole().then(() => RoleResource.delete(currentFacilityRole.id)); } } diff --git a/kolibri/plugins/facility/frontend/views/DataPage/SyncInterface/index.vue b/kolibri/plugins/facility/frontend/views/DataPage/SyncInterface/index.vue index 2650778bcfa..36ed7a4b225 100644 --- a/kolibri/plugins/facility/frontend/views/DataPage/SyncInterface/index.vue +++ b/kolibri/plugins/facility/frontend/views/DataPage/SyncInterface/index.vue @@ -223,14 +223,14 @@ }, pollSyncTask() { // Like facilityTaskQueue, just keep polling until component is destroyed - TaskResource.get(this.syncTaskId).then(task => { + TaskResource.retrieve(this.syncTaskId).then(task => { if (runEndedSince(task, this.syncTaskLastFinished)) { this.isSyncing = false; // Clearing a repeating row would delete the facility's sync // schedule, and it is briefly clearable between the run ending and // the re-schedule that requeues it. if (task.clearable && task.repeat === 0) { - TaskResource.clear(this.syncTaskId); + TaskResource.clear_v2(this.syncTaskId); } this.syncTaskId = ''; const status = taskDisplayStatus(task); diff --git a/kolibri/plugins/facility/frontend/views/DataPage/index.vue b/kolibri/plugins/facility/frontend/views/DataPage/index.vue index 7bd25b72252..5208210f98d 100644 --- a/kolibri/plugins/facility/frontend/views/DataPage/index.vue +++ b/kolibri/plugins/facility/frontend/views/DataPage/index.vue @@ -297,7 +297,7 @@ }, mounted() { // fetch task list after fetching facilities, to ensure proper syncing state - FacilityResource.fetchCollection({ force: true }).then(facilities => { + FacilityResource.list().then(facilities => { this.$store.commit('manageCSV/RESET_STATE'); this.$store.commit('manageCSV/SET_STATE', { facilities }); if (this.pollForTasks) { diff --git a/kolibri/plugins/facility/frontend/views/UserEditPage.vue b/kolibri/plugins/facility/frontend/views/UserEditPage.vue index 40dadd0e96d..6029e903099 100644 --- a/kolibri/plugins/facility/frontend/views/UserEditPage.vue +++ b/kolibri/plugins/facility/frontend/views/UserEditPage.vue @@ -339,9 +339,7 @@ }, created() { const facilityConfigPromise = this.updateFacilityConfig(); - const facilityUserPromise = FacilityUserResource.fetchModel({ - id: this.$route.params.id, - }).then(user => { + const facilityUserPromise = FacilityUserResource.retrieve(this.userId).then(user => { this.username = user.username; this.fullName = user.full_name; this.idNumber = user.id_number; diff --git a/kolibri/plugins/facility/frontend/views/__tests__/UserEditPage.spec.js b/kolibri/plugins/facility/frontend/views/__tests__/UserEditPage.spec.js index f318211d5ae..247dfe472dc 100644 --- a/kolibri/plugins/facility/frontend/views/__tests__/UserEditPage.spec.js +++ b/kolibri/plugins/facility/frontend/views/__tests__/UserEditPage.spec.js @@ -88,7 +88,7 @@ async function renderPage({ }), ); - FacilityUserResource.fetchModel.mockResolvedValue(user); + FacilityUserResource.retrieve.mockResolvedValue(user); const router = createRouter(); const store = makeStore(); diff --git a/kolibri/plugins/facility/frontend/views/common/ClassCopyModal.vue b/kolibri/plugins/facility/frontend/views/common/ClassCopyModal.vue index 6430e1d1de8..02d580f252e 100644 --- a/kolibri/plugins/facility/frontend/views/common/ClassCopyModal.vue +++ b/kolibri/plugins/facility/frontend/views/common/ClassCopyModal.vue @@ -112,34 +112,32 @@ } async function createClass() { - const classroom = await ClassroomResource.saveModel({ - data: { - name: copiedClassName.value.trim(), - parent: classToCopy.parent, - }, + const classroom = await ClassroomResource.create({ + name: copiedClassName.value.trim(), + parent: classToCopy.parent, }); createdClass.value = classroom; } function assignCoachesToClass() { if (!copyAllCoaches.value || !classCoachesIds.value.length) return Promise.resolve(); - return RoleResource.saveCollection({ - data: classCoachesIds.value.map(coachId => ({ + return RoleResource.bulkCreate( + classCoachesIds.value.map(coachId => ({ user: coachId, kind: UserKinds.COACH, collection: createdClass.value.id, })), - }); + ); } function assignLearnersToClass() { if (!copyAllLearners.value || !classLearnerIds.value.length) return Promise.resolve(); - return MembershipResource.saveCollection({ - data: classLearnerIds.value.map(learnerId => ({ + return MembershipResource.bulkCreate( + classLearnerIds.value.map(learnerId => ({ user: learnerId, collection: createdClass.value.id, })), - }); + ); } async function copyClass() { @@ -177,12 +175,9 @@ try { copiedClassName.value = copyOfClass$({ class: classToCopy.name }); classCoachesIds.value = classToCopy.coaches.map(coach => coach.id); - const classLearners = await FacilityUserResource.fetchCollection({ - getParams: { - member_of: classToCopy.id, - exclude_coach_for: classToCopy.id, - }, - force: true, + const classLearners = await FacilityUserResource.list({ + member_of: classToCopy.id, + exclude_coach_for: classToCopy.id, }); classLearnerIds.value = classLearners.map(learner => learner.id); } catch (error) { diff --git a/kolibri/plugins/facility/frontend/views/common/ClassRenameModal.vue b/kolibri/plugins/facility/frontend/views/common/ClassRenameModal.vue index cb6e42217a5..2f196e27973 100644 --- a/kolibri/plugins/facility/frontend/views/common/ClassRenameModal.vue +++ b/kolibri/plugins/facility/frontend/views/common/ClassRenameModal.vue @@ -94,7 +94,7 @@ if (this.formIsValid) { try { this.submitting = true; - await this.updateClass({ id: this.classid, updateData: { name: this.name } }); + await this.updateClass({ id: this.classid, name: this.name }); const updatedClasses = this.classes.map(c => { if (c.id === this.classid) { diff --git a/kolibri/plugins/facility/frontend/views/users/sidePanels/AssignCoachesSidePanel.vue b/kolibri/plugins/facility/frontend/views/users/sidePanels/AssignCoachesSidePanel.vue index 01479937653..8de3d4d743b 100644 --- a/kolibri/plugins/facility/frontend/views/users/sidePanels/AssignCoachesSidePanel.vue +++ b/kolibri/plugins/facility/frontend/views/users/sidePanels/AssignCoachesSidePanel.vue @@ -175,10 +175,8 @@ return; } isLoading.value = true; - const users = await FacilityUserResource.fetchCollection({ - getParams: { - by_ids: Array.from(props.selectedUsers).join(','), - }, + const users = await FacilityUserResource.list({ + by_ids: Array.from(props.selectedUsers).join(','), }); facilityUsers.value = users.map(_userState); isLoading.value = false; @@ -257,9 +255,7 @@ })), ); - const newRoles = await RoleResource.saveCollection({ - data: roleData, - }); + const newRoles = await RoleResource.bulkCreate(roleData); // Only add roles that were actually created (have an id) const actuallyCreatedRoles = newRoles.filter(role => role.id); @@ -269,7 +265,7 @@ async function handleUndoAssignments() { if (createdRoles.value.length > 0) { const roleIds = createdRoles.value.map(role => role.id); - await RoleResource.deleteCollection({ by_ids: roleIds }); + await RoleResource.bulkDelete({ by_ids: roleIds }); props.onChange({ affectedClasses: selectedClasses.value, }); diff --git a/kolibri/plugins/facility/frontend/views/users/sidePanels/EnrollLearnersSidePanel.vue b/kolibri/plugins/facility/frontend/views/users/sidePanels/EnrollLearnersSidePanel.vue index dd85faab960..7259ac426fc 100644 --- a/kolibri/plugins/facility/frontend/views/users/sidePanels/EnrollLearnersSidePanel.vue +++ b/kolibri/plugins/facility/frontend/views/users/sidePanels/EnrollLearnersSidePanel.vue @@ -173,10 +173,8 @@ return; } loading.value = true; - const users = await FacilityUserResource.fetchCollection({ - getParams: { - by_ids: Array.from(props.selectedUsers).join(','), - }, + const users = await FacilityUserResource.list({ + by_ids: Array.from(props.selectedUsers).join(','), }); facilityUsers.value = users.map(_userState); loading.value = false; @@ -220,9 +218,8 @@ async function setClassUsers() { loading.value = true; try { - const classMemberships = await MembershipResource.fetchCollection({ - getParams: { user_ids: Array.from(props.selectedUsers).join(',') }, - force: true, + const classMemberships = await MembershipResource.list({ + user_ids: Array.from(props.selectedUsers).join(','), }); classMembershipsByUser.value = groupBy(classMemberships, 'user'); classLearners.value = Object.keys(classMembershipsByUser.value); @@ -244,7 +241,7 @@ }); if (enrollments.length > 0) { try { - const newMemberships = await MembershipResource.saveCollection({ data: enrollments }); + const newMemberships = await MembershipResource.bulkCreate(enrollments); createdMemberships.value = newMemberships; } catch (error) { handleApiError({ error }); @@ -279,7 +276,7 @@ async function handleUndoEnrollments() { if (createdMemberships.value?.length > 0) { const ids = createdMemberships.value.map(m => m.id).join(','); - await MembershipResource.deleteCollection({ by_ids: ids }); + await MembershipResource.bulkDelete({ by_ids: ids }); props.onChange({ affectedClasses: selectedOptions.value, }); diff --git a/kolibri/plugins/facility/frontend/views/users/sidePanels/RemoveFromClassSidePanel.vue b/kolibri/plugins/facility/frontend/views/users/sidePanels/RemoveFromClassSidePanel.vue index 566bdb68ba9..2fb4ae8de8a 100644 --- a/kolibri/plugins/facility/frontend/views/users/sidePanels/RemoveFromClassSidePanel.vue +++ b/kolibri/plugins/facility/frontend/views/users/sidePanels/RemoveFromClassSidePanel.vue @@ -209,17 +209,8 @@ const userIdsStr = userIds.join(','); const [membershipsData, coachRoles] = await Promise.all([ - MembershipResource.fetchCollection({ - getParams: { user_ids: userIdsStr }, - force: true, - }), - RoleResource.fetchCollection({ - getParams: { - user_ids: userIdsStr, - kind: UserKinds.COACH, - }, - force: true, - }), + MembershipResource.list({ user_ids: userIdsStr }), + RoleResource.list({ user_ids: userIdsStr, kind: UserKinds.COACH }), ]); membershipsByUser.value = groupBy(membershipsData, 'user'); @@ -249,12 +240,8 @@ try { await Promise.all([ - enrollments.length - ? MembershipResource.saveCollection({ data: enrollments }) - : Promise.resolve(), - assignments.length - ? RoleResource.saveCollection({ data: assignments }) - : Promise.resolve(), + enrollments.length ? MembershipResource.bulkCreate(enrollments) : Promise.resolve(), + assignments.length ? RoleResource.bulkCreate(assignments) : Promise.resolve(), ]); } catch (_) { createSnackbar(defaultErrorMessage$()); @@ -282,7 +269,7 @@ async function removeItems(resource, items) { if (items.length) { const ids = items.map(item => item.id).join(','); - await resource.deleteCollection({ by_ids: ids }); + await resource.bulkDelete({ by_ids: ids }); } } try { diff --git a/kolibri/plugins/facility/frontend/views/users/sidePanels/UserCreate/__tests__/UserCreateSidePanel.spec.js b/kolibri/plugins/facility/frontend/views/users/sidePanels/UserCreate/__tests__/UserCreateSidePanel.spec.js index 4abecfa258b..4f1c9fc6914 100644 --- a/kolibri/plugins/facility/frontend/views/users/sidePanels/UserCreate/__tests__/UserCreateSidePanel.spec.js +++ b/kolibri/plugins/facility/frontend/views/users/sidePanels/UserCreate/__tests__/UserCreateSidePanel.spec.js @@ -229,11 +229,11 @@ describe('UserCreateSidePanel', () => { ])( 'creates $name with the correct role and password', async ({ setupOpts, kindLabel, facilityCoach, password, expectedRole }) => { - FacilityUserResource.saveModel.mockResolvedValue({ + FacilityUserResource.create.mockResolvedValue({ id: 'new-user-id', facility: 'fac-1', }); - RoleResource.saveModel.mockResolvedValue({}); + RoleResource.create.mockResolvedValue({}); setup(setupOpts); await waitForFormReady(); if (kindLabel) { @@ -251,25 +251,21 @@ describe('UserCreateSidePanel', () => { await fireEvent.click(saveAndCloseButton()); await waitFor(() => { - expect(FacilityUserResource.saveModel).toHaveBeenCalledTimes(1); + expect(FacilityUserResource.create).toHaveBeenCalledTimes(1); }); - expect(FacilityUserResource.saveModel).toHaveBeenCalledWith( + expect(FacilityUserResource.create).toHaveBeenCalledWith( expect.objectContaining({ - data: expect.objectContaining({ - username: 'testuser', - full_name: 'Test User', - password, - }), + username: 'testuser', + full_name: 'Test User', + password, }), ); if (expectedRole === null) { - expect(RoleResource.saveModel).not.toHaveBeenCalled(); + expect(RoleResource.create).not.toHaveBeenCalled(); } else { await waitFor(() => { - expect(RoleResource.saveModel).toHaveBeenCalledWith( - expect.objectContaining({ - data: expect.objectContaining({ kind: expectedRole }), - }), + expect(RoleResource.create).toHaveBeenCalledWith( + expect.objectContaining({ kind: expectedRole }), ); }); } @@ -280,7 +276,7 @@ describe('UserCreateSidePanel', () => { setup(); await waitForFormReady(); await fireEvent.click(saveAndCloseButton()); - expect(FacilityUserResource.saveModel).not.toHaveBeenCalled(); + expect(FacilityUserResource.create).not.toHaveBeenCalled(); }); it('does not call FacilityUserResource when picture passwords are exhausted', async () => { @@ -288,7 +284,7 @@ describe('UserCreateSidePanel', () => { await waitForFormReady(); await fillRequired(); await fireEvent.click(saveAndCloseButton()); - expect(FacilityUserResource.saveModel).not.toHaveBeenCalled(); + expect(FacilityUserResource.create).not.toHaveBeenCalled(); }); }); @@ -319,7 +315,7 @@ describe('UserCreateSidePanel', () => { }); it('submits the form with the user-supplied password when the facility has picture passwords configured', async () => { - FacilityUserResource.saveModel.mockResolvedValue({ id: 'new-user-id', facility: 'fac-1' }); + FacilityUserResource.create.mockResolvedValue({ id: 'new-user-id', facility: 'fac-1' }); setup({ pictureLogin: true, pictureLoginFeatureEnabled: false }); await waitForFormReady(); await fillRequired(); @@ -327,12 +323,10 @@ describe('UserCreateSidePanel', () => { await fireEvent.click(saveAndCloseButton()); await waitFor(() => { - expect(FacilityUserResource.saveModel).toHaveBeenCalledTimes(1); + expect(FacilityUserResource.create).toHaveBeenCalledTimes(1); }); - expect(FacilityUserResource.saveModel).toHaveBeenCalledWith( - expect.objectContaining({ - data: expect.objectContaining({ password: 'secret123' }), - }), + expect(FacilityUserResource.create).toHaveBeenCalledWith( + expect.objectContaining({ password: 'secret123' }), ); }); }); diff --git a/kolibri/plugins/facility/frontend/views/users/sidePanels/UserCreate/index.vue b/kolibri/plugins/facility/frontend/views/users/sidePanels/UserCreate/index.vue index 0d6843378aa..b287c66a3f2 100644 --- a/kolibri/plugins/facility/frontend/views/users/sidePanels/UserCreate/index.vue +++ b/kolibri/plugins/facility/frontend/views/users/sidePanels/UserCreate/index.vue @@ -412,32 +412,30 @@ const saveUserRole = (facilityUser, newRoleKind) => { const { id, facility } = facilityUser; - return RoleResource.saveModel({ - data: { - user: id, - collection: facility, - kind: newRoleKind, - }, + return RoleResource.create({ + user: id, + collection: facility, + kind: newRoleKind, }); }; const enrollLearnerInClasses = (userId, classIds) => { - return MembershipResource.saveCollection({ - data: classIds.map(classId => ({ + return MembershipResource.bulkCreate( + classIds.map(classId => ({ collection: classId, user: userId, })), - }); + ); }; const assignCoachToClasses = (userId, classIds) => { - return RoleResource.saveCollection({ - data: classIds.map(classId => ({ + return RoleResource.bulkCreate( + classIds.map(classId => ({ collection: classId, user: userId, kind: UserKinds.COACH, })), - }); + ); }; const createFacilityUser = async () => { @@ -445,17 +443,15 @@ if (!showPasswordInput.value) { passwordValue = NOT_SPECIFIED; } - const facilityUser = await FacilityUserResource.saveModel({ - data: { - facility: facilityId.value, - username: username.value, - full_name: fullName.value, - password: passwordValue, - id_number: idNumber.value, - gender: gender.value, - birth_year: birthYear.value, - extra_demographics: extraDemographics.value, - }, + const facilityUser = await FacilityUserResource.create({ + facility: facilityId.value, + username: username.value, + full_name: fullName.value, + password: passwordValue, + id_number: idNumber.value, + gender: gender.value, + birth_year: birthYear.value, + extra_demographics: extraDemographics.value, }); if (newUserRole.value !== UserKinds.LEARNER) { await saveUserRole(facilityUser, newUserRole.value); From e0e43026d897e3dffa858648f18e0de4af7e6617 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Wed, 12 Aug 2026 08:29:46 -0700 Subject: [PATCH 2/8] Rewrite DeletedFacilityUserResource.restoreCollection onto request The resource is imported only by facility, so the recipe's in-place rewrite applies rather than a `_v2` sibling. It now resolves with the payload instead of the axios response, and failures are logged by `request`. Co-Authored-By: Claude Opus 5 (1M context) --- .../UsersTrashPage/PermanentDeleteModal.vue | 2 +- .../views/users/common/MoveToTrashModal.vue | 8 ++-- .../DeletedFacilityUserResource.js | 14 +++---- .../DeletedFacilityUserResource.spec.js | 39 +++++++++++++++++++ 4 files changed, 48 insertions(+), 15 deletions(-) create mode 100644 packages/kolibri-common/apiResources/__tests__/DeletedFacilityUserResource.spec.js diff --git a/kolibri/plugins/facility/frontend/views/users/UsersTrashPage/PermanentDeleteModal.vue b/kolibri/plugins/facility/frontend/views/users/UsersTrashPage/PermanentDeleteModal.vue index 2ef594e89cc..12575bea3d7 100644 --- a/kolibri/plugins/facility/frontend/views/users/UsersTrashPage/PermanentDeleteModal.vue +++ b/kolibri/plugins/facility/frontend/views/users/UsersTrashPage/PermanentDeleteModal.vue @@ -59,7 +59,7 @@ loading.value = true; sendPoliteMessage(deletingLabel$()); try { - await DeletedFacilityUserResource.deleteCollection({ + await DeletedFacilityUserResource.bulkDelete({ by_ids: Array.from(props.selectedUsers).join(','), }); createSnackbar(usersDeletedNotice$()); diff --git a/kolibri/plugins/facility/frontend/views/users/common/MoveToTrashModal.vue b/kolibri/plugins/facility/frontend/views/users/common/MoveToTrashModal.vue index e8df757120f..5ec295d71eb 100644 --- a/kolibri/plugins/facility/frontend/views/users/common/MoveToTrashModal.vue +++ b/kolibri/plugins/facility/frontend/views/users/common/MoveToTrashModal.vue @@ -87,10 +87,8 @@ const loadData = async () => { loading.value = true; try { - const userModels = await FacilityUserResource.fetchCollection({ - getParams: { - by_ids: Array.from(props.selectedUsers), - }, + const userModels = await FacilityUserResource.list({ + by_ids: Array.from(props.selectedUsers), }); users.value = userModels.map(_userState); } finally { @@ -106,7 +104,7 @@ loading.value = true; sendPoliteMessage(movingToTrash$()); try { - await FacilityUserResource.deleteCollection({ + await FacilityUserResource.bulkDelete({ by_ids: Array.from(props.selectedUsers).join(','), }); createSnackbar(usersTrashedNotice$()); diff --git a/packages/kolibri-common/apiResources/DeletedFacilityUserResource.js b/packages/kolibri-common/apiResources/DeletedFacilityUserResource.js index a8a7c47faa4..7540c64fed9 100644 --- a/packages/kolibri-common/apiResources/DeletedFacilityUserResource.js +++ b/packages/kolibri-common/apiResources/DeletedFacilityUserResource.js @@ -1,16 +1,12 @@ import { Resource } from 'kolibri/apiResource'; -import client from 'kolibri/client'; export default new Resource({ name: 'deletedfacilityuser', - restoreCollection(getParams) { - if (!getParams) { - throw new Error('You must provide a getParams object to restore deleted users.'); + async restoreCollection(params = {}) { + if (!Object.keys(params).length) { + throw TypeError('Params must be specified to narrow what is being restored'); } - return client({ - url: this.getUrlFunction('restore')(), - method: 'POST', - params: getParams, - }); + const response = await this.request({ method: 'POST', action: 'restore', params }); + return response.data; }, }); diff --git a/packages/kolibri-common/apiResources/__tests__/DeletedFacilityUserResource.spec.js b/packages/kolibri-common/apiResources/__tests__/DeletedFacilityUserResource.spec.js new file mode 100644 index 00000000000..085971e23fc --- /dev/null +++ b/packages/kolibri-common/apiResources/__tests__/DeletedFacilityUserResource.spec.js @@ -0,0 +1,39 @@ +import client from 'kolibri/client'; +import DeletedFacilityUserResource from '../DeletedFacilityUserResource'; + +jest.mock('kolibri/client'); +jest.mock('kolibri/urls'); + +describe('DeletedFacilityUserResource', () => { + beforeEach(() => { + client.__reset(); + }); + + describe('restoreCollection', () => { + it('resolves with the response payload, not the response envelope', async () => { + client.__setPayload({ restored: 2 }); + + await expect( + DeletedFacilityUserResource.restoreCollection({ by_ids: 'a,b' }), + ).resolves.toEqual({ restored: 2 }); + }); + + it('POSTs the filter as query parameters', async () => { + await DeletedFacilityUserResource.restoreCollection({ by_ids: 'a,b' }); + + expect(client).toHaveBeenCalledTimes(1); + expect(client).toHaveBeenCalledWith( + expect.objectContaining({ method: 'POST', params: { by_ids: 'a,b' } }), + ); + }); + + it.each([[undefined], [{}]])( + 'refuses %p, which would restore every deleted user', + async params => { + await expect(DeletedFacilityUserResource.restoreCollection(params)).rejects.toThrow(); + + expect(client).not.toHaveBeenCalled(); + }, + ); + }); +}); From d4e3e4f0890d10350054707ef98c8897a73e0c38 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Wed, 12 Aug 2026 08:30:05 -0700 Subject: [PATCH 3/8] Read facility users through useList MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four behaviour changes come with useFetch: `dataLoading` clears on failure rather than spinning forever, `usersCount`/`totalPages` normalise to 0 instead of undefined, a superseded fetch's response is discarded, and failure handling runs from a watcher a tick later instead of a catch. `fetchClasses` stays a plain `list()` — `useUsersFilters` dereferences the `classes` ref before any fetch runs, and `useFetch` initialises `data` to null. Co-Authored-By: Claude Opus 5 (1M context) --- .../__tests__/useUserManagement.spec.js | 72 ++++++++++++++++ .../frontend/composables/useUserManagement.js | 83 ++++++++++--------- 2 files changed, 118 insertions(+), 37 deletions(-) create mode 100644 kolibri/plugins/facility/frontend/composables/__tests__/useUserManagement.spec.js diff --git a/kolibri/plugins/facility/frontend/composables/__tests__/useUserManagement.spec.js b/kolibri/plugins/facility/frontend/composables/__tests__/useUserManagement.spec.js new file mode 100644 index 00000000000..90017741e39 --- /dev/null +++ b/kolibri/plugins/facility/frontend/composables/__tests__/useUserManagement.spec.js @@ -0,0 +1,72 @@ +import { nextTick } from 'vue'; +import client from 'kolibri/client'; +import { error as appError, clearError } from 'kolibri/utils/appError'; +import { useRoute, useRouter } from 'vue-router/composables'; +import useUserManagement from '../useUserManagement'; + +jest.mock('kolibri/client'); +jest.mock('kolibri/urls'); +jest.mock('vue-router/composables'); + +const SERVER_ERROR_MESSAGE = 'Request failed with status code 500'; + +describe('useUserManagement', () => { + let router; + + function setup({ query = {} } = {}) { + router = { push: jest.fn() }; + useRoute.mockReturnValue({ query }); + useRouter.mockReturnValue(router); + return useUserManagement({ activeFacilityId: 'facility-1' }); + } + + beforeEach(() => { + client.__reset(); + clearError(); + }); + + it('exposes the paginated response as mapped users and page counts', async () => { + client.__setPayload({ + results: [{ id: 'user-1', full_name: 'Test User', facility: 'facility-1', roles: [] }], + count: 1, + total_pages: 3, + }); + + const { fetchUsers, facilityUsers, usersCount, totalPages } = setup(); + await fetchUsers(); + + expect(facilityUsers.value).toEqual([ + expect.objectContaining({ id: 'user-1', full_name: 'Test User', facility_id: 'facility-1' }), + ]); + expect(usersCount.value).toBe(1); + expect(totalPages.value).toBe(3); + }); + + it('falls back to the first page when a stale page number 404s', async () => { + // A bare `status`, not an axios-shaped error: the composable branches on `error.status`, and + // `logError` only stays quiet while `config` and `response` are both absent. + client.mockRejectedValue({ status: 404 }); + + const { fetchUsers } = setup({ query: { page: '2' } }); + await fetchUsers(); + // The handler is a watcher on the `error` ref, so the awaited fetch alone does not reach it. + await nextTick(); + + expect(router.push).toHaveBeenCalledWith( + expect.objectContaining({ query: expect.objectContaining({ page: 1 }) }), + ); + expect(appError.value).toBeNull(); + }); + + it('reports any other failure without throwing out of the watcher', async () => { + client.mockRejectedValue({ status: 500, message: SERVER_ERROR_MESSAGE }); + + const { fetchUsers } = setup(); + await fetchUsers(); + await nextTick(); + + // A throw from the watcher hits Vue's error handler, which the suite's console rules fail on. + expect(appError.value).toContain(SERVER_ERROR_MESSAGE); + expect(router.push).not.toHaveBeenCalled(); + }); +}); diff --git a/kolibri/plugins/facility/frontend/composables/useUserManagement.js b/kolibri/plugins/facility/frontend/composables/useUserManagement.js index d5113551307..7044878f4cd 100644 --- a/kolibri/plugins/facility/frontend/composables/useUserManagement.js +++ b/kolibri/plugins/facility/frontend/composables/useUserManagement.js @@ -16,10 +16,6 @@ export default function useUserManagement({ softDeletedUsers = false, } = {}) { const selectedUsers = ref(new Set()); - const facilityUsers = ref([]); - const totalPages = ref(0); - const usersCount = ref(0); - const dataLoading = ref(false); const classes = ref([]); const router = useRouter(); const route = useRoute(); @@ -38,44 +34,57 @@ export default function useUserManagement({ selectedUsers.value = new Set(); }; - const fetchUsers = async () => { - dataLoading.value = true; - try { - const fetchResource = softDeletedUsers ? DeletedFacilityUserResource : FacilityUserResource; - const resp = await fetchResource.fetchCollection({ - getParams: pickBy({ - member_of: activeFacilityId, - date_joined__gte: dateJoinedGt?.toISOString(), - page: page.value, - page_size: pageSize.value, - search: search.value?.trim() || null, - ordering: order.value === 'desc' ? `-${ordering.value}` : ordering.value || null, - ...getBackendFilters(), - }), - force: true, - }); - facilityUsers.value = resp.results.map(_userState); - totalPages.value = resp.total_pages; - usersCount.value = resp.count; - dataLoading.value = false; - pageLoading.value = false; - } catch (error) { + const userResource = softDeletedUsers ? DeletedFacilityUserResource : FacilityUserResource; + + // `useList` reads these at fetch time rather than watching them, so refetching is driven by + // the query-param watcher further down. + const userParams = () => + pickBy({ + member_of: activeFacilityId, + date_joined__gte: dateJoinedGt?.toISOString(), + page: page.value, + page_size: pageSize.value, + search: search.value?.trim() || null, + ordering: order.value === 'desc' ? `-${ordering.value}` : ordering.value || null, + ...getBackendFilters(), + }); + + const { + data: users, + loading: dataLoading, + error: usersError, + count, + totalPages: responseTotalPages, + fetchData: fetchUsers, + } = userResource.useList(userParams); + + const facilityUsers = computed(() => (users.value || []).map(_userState)); + const usersCount = computed(() => count.value ?? 0); + const totalPages = computed(() => responseTotalPages.value ?? 0); + + // `useFetch` clears `loading` on success and on failure, and leaves it set for a superseded + // fetch, so this covers every path the page's spinner should stop for. + watch(dataLoading, isLoading => { + if (!isLoading) { pageLoading.value = false; - // In case of 404 error because of stale pagination try loading users of page 1 - if (error.status === 404 && page.value > 1) { - router.push({ ...route, query: { ...route.query, page: 1 } }); - } else { - handleApiError({ error, reloadOnReconnect: true }); - } } - }; + }); + + watch(usersError, error => { + if (!error) { + return; + } + // A 404 here is a stale page number, outliving the filter change that shrank the result set. + if (error.status === 404 && page.value > 1) { + router.push({ ...route, query: { ...route.query, page: 1 } }); + } else { + handleApiError({ error, reloadOnReconnect: true, shouldThrow: false }); + } + }); const fetchClasses = async () => { try { - const classList = await ClassroomResource.fetchCollection({ - getParams: { parent: activeFacilityId }, - force: true, - }); + const classList = await ClassroomResource.list({ parent: activeFacilityId }); classes.value = classList; } catch (error) { handleApiError({ error, reloadOnReconnect: true }); From c3d6507f70a35d61b5066c6782634e58b0bd7365 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Wed, 12 Aug 2026 08:30:12 -0700 Subject: [PATCH 4/8] Delete the unused plugin-local PortalResource It duplicates packages/kolibri-common/apiResources/PortalResource.js and has no importers. Co-Authored-By: Claude Opus 5 (1M context) --- .../plugins/facility/frontend/apiResources.js | 22 ------------------- 1 file changed, 22 deletions(-) delete mode 100644 kolibri/plugins/facility/frontend/apiResources.js diff --git a/kolibri/plugins/facility/frontend/apiResources.js b/kolibri/plugins/facility/frontend/apiResources.js deleted file mode 100644 index 071560a6e5a..00000000000 --- a/kolibri/plugins/facility/frontend/apiResources.js +++ /dev/null @@ -1,22 +0,0 @@ -import { Resource } from 'kolibri/apiResource'; -import urls from 'kolibri/urls'; - -export const PortalResource = new Resource({ - name: 'portal', - validateToken(token) { - const url = urls['kolibri:core:portal_validate_token'](); - return this.client({ - url, - method: 'get', - params: { token }, - }); - }, - registerFacility({ facility_id, token }) { - const url = urls['kolibri:core:portal_register'](); - return this.client({ - url, - method: 'post', - data: { facility_id, token }, - }); - }, -}); From 87999a9a4cc040c1a91934878265a46066df2c52 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Wed, 12 Aug 2026 09:10:04 -0700 Subject: [PATCH 5/8] Return each user once from the facility user relation filters member_of, related_to__in and user_type joined memberships and roles, returning a user once per matching row - a learner in two classes appeared twice. The legacy Collection cache deduplicated by id client-side, hiding it until the resource migration. Co-Authored-By: Claude Opus 5 (1M context) --- kolibri/core/auth/test/test_api.py | 33 ++++++++++++++++ kolibri/core/auth/viewsets/facility_user.py | 43 +++++++++++++-------- 2 files changed, 60 insertions(+), 16 deletions(-) diff --git a/kolibri/core/auth/test/test_api.py b/kolibri/core/auth/test/test_api.py index 410a9debf1a..84c09ba70be 100644 --- a/kolibri/core/auth/test/test_api.py +++ b/kolibri/core/auth/test/test_api.py @@ -1821,6 +1821,39 @@ def test_user_member_of_filter(self): self.assertEqual(data[1]["id"], self.admin_1.id) self.assertEqual(data[2]["id"], self.user_1.id) + def test_user_member_of_filter_returns_multiply_enrolled_user_once(self): + for _ in range(2): + ClassroomFactory.create(parent=self.facility_1).add_member(self.user_1) + + response = self.client.get( + reverse("kolibri:core:facilityuser-list"), {"member_of": self.facility_1.id} + ) + ids = [user["id"] for user in response.data] + self.assertEqual(ids.count(self.user_1.id), 1) + + def test_user_related_to_in_filter_returns_multiply_enrolled_user_once(self): + classrooms = [ClassroomFactory.create(parent=self.facility_1) for _ in range(2)] + for classroom in classrooms: + classroom.add_member(self.user_1) + + response = self.client.get( + reverse("kolibri:core:facilityuser-list"), + {"related_to__in": ",".join(str(c.id) for c in classrooms)}, + ) + ids = [user["id"] for user in response.data] + self.assertEqual(ids.count(self.user_1.id), 1) + + def test_user_type_in_filter_returns_each_matching_user_once(self): + for _ in range(2): + ClassroomFactory.create(parent=self.facility_2).add_coach(self.admin_2) + + response = self.client.get( + reverse("kolibri:core:facilityuser-list"), + {"user_type__in": "learner,coach", "member_of": self.facility_2.id}, + ) + ids = [user["id"] for user in response.data] + self.assertCountEqual(ids, [self.user_2.id, self.admin_2.id]) + class LoginLogoutTestCase(APITestCase): databases = "__all__" diff --git a/kolibri/core/auth/viewsets/facility_user.py b/kolibri/core/auth/viewsets/facility_user.py index c3b03c4a975..8a7b1cd6c85 100644 --- a/kolibri/core/auth/viewsets/facility_user.py +++ b/kolibri/core/auth/viewsets/facility_user.py @@ -5,6 +5,8 @@ from django.core.exceptions import PermissionDenied from django.core.exceptions import ValidationError as DjangoValidationError from django.db import transaction +from django.db.models import Exists +from django.db.models import OuterRef from django.db.models import Q from django.http import Http404 from django.utils.timezone import now @@ -42,6 +44,7 @@ from ..models import Collection from ..models import Facility from ..models import FacilityUser +from ..models import Membership from ..models import Role from ..models import validate_username_allowed_chars from ..models import validate_username_max_length @@ -65,6 +68,17 @@ class ChoiceInFilter(BaseInFilter, ChoiceFilter): pass +def _user_has(model, **filters): + """ + Q matching users with a related `model` row, as a correlated subquery. + + Memberships and roles are multi-valued, so OR-ing one into a `filter()` LEFT JOINs the + relation and returns a user once per related row. `exclude()` already subqueries, so the + `filter_exclude_*` methods need no wrapper. + """ + return Q(Exists(model.objects.filter(user_id=OuterRef("id"), **filters))) + + class FacilityUserFilter(FilterSet): USER_TYPE_CHOICES = ( ("learner", "learner"), @@ -109,7 +123,9 @@ class FacilityUserFilter(FilterSet): by_ids = UUIDInFilter(field_name="id") def filter_member_of(self, queryset, name, value): - return queryset.filter(Q(memberships__collection=value) | Q(facility=value)) + return queryset.filter( + _user_has(Membership, collection=value) | Q(facility=value) + ) def filter_related_to__in(self, queryset, name, value): """ @@ -117,9 +133,9 @@ def filter_related_to__in(self, queryset, name, value): memberships, facility, or roles. """ return queryset.filter( - Q(memberships__collection__in=value) + _user_has(Membership, collection__in=value) | Q(facility__in=value) - | Q(roles__collection__in=value) + | _user_has(Role, collection__in=value) ) def filter_user_type(self, queryset, name, value): @@ -129,24 +145,19 @@ def filter_user_type(self, queryset, name, value): user_type_filter = Q() if "learner" in value: - user_type_filter |= Q(roles__isnull=True) + user_type_filter |= ~_user_has(Role) - if "coach" in value: - # Return users with either coach or classroom assignable coach roles - user_type_filter |= Q(roles__kind=role_kinds.COACH) | Q( - roles__kind=role_kinds.ASSIGNABLE_COACH - ) if "superuser" in value: user_type_filter |= Q(devicepermissions__is_superuser=True) - rest_filters = [ - user_type_value - for user_type_value in value - if user_type_value not in ["learner", "coach", "superuser"] - ] + # "coach" covers both the coach and the classroom-assignable coach role; every other + # value is already a role kind. + kinds = set(value) - {"learner", "coach", "superuser"} + if "coach" in value: + kinds |= {role_kinds.COACH, role_kinds.ASSIGNABLE_COACH} - if rest_filters: - user_type_filter |= Q(roles__kind__in=rest_filters) + if kinds: + user_type_filter |= _user_has(Role, kind__in=sorted(kinds)) return queryset.filter(user_type_filter) From dd68192680a6b41008e876513468518e1b873249 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Fri, 14 Aug 2026 10:21:33 -0700 Subject: [PATCH 6/8] Read the user back instead of patching nothing on a role-only change Legacy saveModel dirty-checked and skipped the request when the payload was empty; update() always PATCHes. A user-type change with no edited details sent an empty PATCH purely to obtain roles for updateFacilityLevelRoles. Co-Authored-By: Claude Opus 5 (1M context) --- .../facility/frontend/modules/userManagement/actions.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/kolibri/plugins/facility/frontend/modules/userManagement/actions.js b/kolibri/plugins/facility/frontend/modules/userManagement/actions.js index 2599fb12655..8f98ba47b39 100644 --- a/kolibri/plugins/facility/frontend/modules/userManagement/actions.js +++ b/kolibri/plugins/facility/frontend/modules/userManagement/actions.js @@ -46,6 +46,8 @@ export function createFacilityUser(store, payload) { * Updates a facility user's details, and their facility-level role when it changed. * `updates.facilityUserUpdates` is already diff-only — `UserEditPage.getUpdates` picks just the * fields that differ from the fetched user — so no baseline is passed to `update`. + * `updateFacilityLevelRoles` needs the user's current roles, so a role-only change reads the user + * back rather than writing an empty patch. * @param {object} store - The Vuex store instance. * @param {object} payload - Payload object. * @param {string} payload.userId - The ID of the user to update. @@ -57,7 +59,10 @@ export function updateFacilityUserDetails(store, { userId, updates }) { if (isEmpty(facilityUserUpdates) && !roleUpdates) { return Promise.resolve(); } - return FacilityUserResource.update(userId, facilityUserUpdates).then(user => { + const userPromise = isEmpty(facilityUserUpdates) + ? FacilityUserResource.retrieve(userId) + : FacilityUserResource.update(userId, facilityUserUpdates); + return userPromise.then(user => { if (roleUpdates) { return updateFacilityLevelRoles(user, roleUpdates.kind); } From 1d17217a9dd3fe732a171219da5a46d05dde48fa Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Fri, 14 Aug 2026 11:35:06 -0700 Subject: [PATCH 7/8] Keep the settings snapshot until both halves of the save are done MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `saveConfig` saves login settings and then the rest of the config. `saveFacilityLoginSettings` re-snapshotted `settingsCopy` as it finished, so the config diff that follows saw no changes and `update` sent no request — every edited setting was silently dropped behind a "saved" snackbar. `saveFacilityConfig` already takes the snapshot once both halves are saved. Dropping the early one also fixes the failure path, where `undoSettingsChange` restored the edited state rather than the original. Co-Authored-By: Claude Opus 5 (1M context) --- .../__tests__/useFacilityEditor.spec.js | 25 +++++++++++-------- .../frontend/composables/useFacilityEditor.js | 4 ++- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/kolibri/plugins/facility/frontend/composables/__tests__/useFacilityEditor.spec.js b/kolibri/plugins/facility/frontend/composables/__tests__/useFacilityEditor.spec.js index 8e562a363cd..6b2c62a985c 100644 --- a/kolibri/plugins/facility/frontend/composables/__tests__/useFacilityEditor.spec.js +++ b/kolibri/plugins/facility/frontend/composables/__tests__/useFacilityEditor.spec.js @@ -82,6 +82,9 @@ describe('useFacilityEditor', () => { urls['kolibri:core:facilitydataset_update_pin'] = jest .fn() .mockReturnValue('/api/facility_dataset/update_pin/'); + urls['kolibri:core:facilitydataset_save_facility_login_settings'] = jest + .fn() + .mockReturnValue('/api/facility_dataset/save_facility_login_settings/'); }); describe('initialization', () => { @@ -577,15 +580,23 @@ describe('useFacilityEditor', () => { expect(savedData).toHaveProperty('id'); }); - it('diffs the config against the snapshot last synced with the server', async () => { - const { saveFacilityConfig, copySettings, settings, settingsCopy, facilityDatasetId } = - useFacilityEditor(); - settings.value = { ...mockFacilityConfig }; + it('diffs against the pre-save snapshot, which saving the login settings leaves alone', async () => { + client.mockResolvedValue({ status: 200, data: {} }); + const { + saveFacilityConfig, + saveFacilityLoginSettings, + copySettings, + settings, + settingsCopy, + facilityDatasetId, + } = useFacilityEditor(); + settings.value = { ...mockFacilityConfig, learner_can_edit_username: true }; facilityDatasetId.value = mockDatasetId; copySettings(); const snapshot = { ...settingsCopy.value }; settings.value = { ...settings.value, learner_can_edit_username: false }; + await saveFacilityLoginSettings(); await saveFacilityConfig(); const [, , options] = FacilityDatasetResource.update.mock.calls.at(-1); @@ -649,12 +660,6 @@ describe('useFacilityEditor', () => { }); describe('saveFacilityLoginSettings', () => { - beforeEach(() => { - urls['kolibri:core:facilitydataset_save_facility_login_settings'] = jest - .fn() - .mockReturnValue('/api/facility_dataset/save_facility_login_settings/'); - }); - it('calls the save-facility-login-settings endpoint via PATCH with login fields', async () => { client.mockResolvedValue({ data: {} }); diff --git a/kolibri/plugins/facility/frontend/composables/useFacilityEditor.js b/kolibri/plugins/facility/frontend/composables/useFacilityEditor.js index 7d947475fe0..0e8a862ea17 100644 --- a/kolibri/plugins/facility/frontend/composables/useFacilityEditor.js +++ b/kolibri/plugins/facility/frontend/composables/useFacilityEditor.js @@ -257,7 +257,9 @@ export default function useFacilityEditor() { if (response.status === 202 && response.data.task?.id) { pictureLoginTaskId.value = response.data.task.id; } - copySettings(); + // No `copySettings()` here: `saveFacilityConfig` runs straight after and diffs against + // `settingsCopy`, so re-snapshotting now would make every other edited setting look + // unchanged and be dropped. It takes the snapshot once both halves are saved. return response.data; } From ae364097c7ce84bd9fb105554a5c5f3cf8052576 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Fri, 14 Aug 2026 12:00:17 -0700 Subject: [PATCH 8/8] Apply review-cycle changes with no owning commit --- .../userManagement/__tests__/actions.spec.js | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 kolibri/plugins/facility/frontend/modules/userManagement/__tests__/actions.spec.js diff --git a/kolibri/plugins/facility/frontend/modules/userManagement/__tests__/actions.spec.js b/kolibri/plugins/facility/frontend/modules/userManagement/__tests__/actions.spec.js new file mode 100644 index 00000000000..d29b8defe48 --- /dev/null +++ b/kolibri/plugins/facility/frontend/modules/userManagement/__tests__/actions.spec.js @@ -0,0 +1,48 @@ +import client from 'kolibri/client'; +import { UserKinds } from 'kolibri/constants'; +import { updateFacilityUserDetails } from '../actions'; + +jest.mock('kolibri/client'); +jest.mock('kolibri/urls'); + +const userId = 'user-1'; + +describe('updateFacilityUserDetails', () => { + beforeEach(() => { + client.__reset(); + }); + + it('reads the user back instead of patching nothing when only the role changed', async () => { + client.__setPayload({ id: userId, facility: 'facility-1', roles: [] }); + + await updateFacilityUserDetails( + {}, + { + userId, + updates: { facilityUserUpdates: {}, roleUpdates: { kind: UserKinds.ADMIN } }, + }, + ); + + // The GET is the read-back that gives `updateFacilityLevelRoles` the user's current roles; + // the POST is the new facility role. An empty PATCH in between would be the bug. + expect(client.mock.calls.map(([{ method }]) => method)).toEqual(['GET', 'POST']); + }); + + it('patches the edited fields when details changed alongside the role', async () => { + client.__setPayload({ id: userId, facility: 'facility-1', roles: [] }); + + await updateFacilityUserDetails( + {}, + { + userId, + updates: { + facilityUserUpdates: { full_name: 'Edited Name' }, + roleUpdates: { kind: UserKinds.ADMIN }, + }, + }, + ); + + expect(client.mock.calls.map(([{ method }]) => method)).toEqual(['PATCH', 'POST']); + expect(client.mock.calls[0][0].data).toEqual({ full_name: 'Edited Name' }); + }); +});