Migrate the facility plugin to the new resource methods

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__"

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,17 +123,19 @@ 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):
         """
         Filter users related to any of the collections in the provided value. Related through
         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)
 

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', () => {
@@ -546,16 +549,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 +571,37 @@ 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 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);
+      expect(options.baseline).toEqual(snapshot);
+    });
   });
 
   describe('setPin', () => {
@@ -597,7 +621,7 @@ describe('useFacilityEditor', () => {
         method: 'POST',
         data: mockPayload,
       });
-      expect(FacilityDatasetResource.saveModel).toHaveBeenCalled();
+      expect(FacilityDatasetResource.update).toHaveBeenCalled();
     });
   });
 
@@ -616,7 +640,7 @@ describe('useFacilityEditor', () => {
         url: '/api/facility_dataset/update_pin/',
         method: 'PATCH',
       });
-      expect(FacilityDatasetResource.saveModel).toHaveBeenCalled();
+      expect(FacilityDatasetResource.update).toHaveBeenCalled();
     });
   });
 
@@ -636,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: {} });
 

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();
+  });
+});

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);
       },

kolibri/plugins/facility/frontend/composables/useFacilityEditor.js

@@ -196,10 +196,7 @@ export default function useFacilityEditor() {
    * @returns {Promise<object>} 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();
@@ -260,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;
   }