squash!: Apply suggestions and improve testing

Author: rodmgwgu

openedx_authz/rest_api/v1/views.py

@@ -677,7 +677,11 @@ def _get_courses_queryset(
         if allowed_ids is not None or allowed_orgs is not None:
             org_filter = Q(org__in=allowed_orgs) if allowed_orgs else Q()
             id_filter = Q(id__in=allowed_ids) if allowed_ids else Q()
-            qs = qs.filter(org_filter | id_filter)
+            combined_filter = org_filter | id_filter
+            if not combined_filter:
+                qs = qs.none()
+            else:
+                qs = qs.filter(combined_filter)
         if search:
             qs = qs.filter(display_name__icontains=search)
         return qs.annotate(
@@ -718,11 +722,16 @@ def _get_libraries_queryset(
         ).values("scope_id", "display_name_col", "org_name", "scope_type")
 
     def _build_queryset(self, courses_qs: QuerySet | None, libraries_qs: QuerySet | None) -> QuerySet:
-        """Union the provided querysets and sort by org. If only one is provided, return it sorted directly."""
+        """Union the provided querysets and sort deterministically.
+
+        Orders by org_name first (satisfying the 'ordered by org' requirement), then by
+        scope_type, display_name_col, and scope_id as tiebreakers to ensure stable pagination.
+        """
+        ordering = ("org_name", "scope_type", "display_name_col", "scope_id")
         if courses_qs is not None and libraries_qs is not None:
-            return courses_qs.union(libraries_qs).order_by("org_name")
+            return courses_qs.union(libraries_qs).order_by(*ordering)
         qs = courses_qs if courses_qs is not None else libraries_qs
-        return qs.order_by("org_name")
+        return qs.order_by(*ordering)
 
     def get_queryset(self) -> QuerySet:
         """Return scopes ordered by org, filtered by the user's permissions."""

openedx_authz/tests/rest_api/test_views.py

@@ -5,15 +5,11 @@
 including permission validation, user-role management, and role listing capabilities.
 """
 
-import operator
-from functools import reduce
 from unittest.mock import patch
 from urllib.parse import urlencode
 
 from ddt import data, ddt, unpack
 from django.contrib.auth import get_user_model
-from django.db.models import CharField, Q, Value
-from django.db.models.functions import Cast
 from django.urls import reverse
 from organizations.models import Organization
 from rest_framework import status
@@ -31,6 +27,7 @@
 from openedx_authz.rest_api.v1.permissions import AnyScopePermission, DynamicScopePermission
 from openedx_authz.rest_api.v1.views import UserValidationAPIView
 from openedx_authz.tests.api.test_roles import BaseRolesTestCase
+from openedx_authz.tests.stubs.models import LearningPackage
 
 ContentLibrary = get_content_library_model()
 CourseOverview = get_course_overview_model()
@@ -924,14 +921,24 @@ def setUpTestData(cls):
             id=cls.COURSE_ORG2, defaults={"org": "Org2", "display_name": "Course Org2"}
         )
 
+        lp1, _ = LearningPackage.objects.get_or_create(title="Library Org1")
+        lp2, _ = LearningPackage.objects.get_or_create(title="Library Org2")
+        lp3, _ = LearningPackage.objects.get_or_create(title="Library Org3")
+
         ContentLibrary.objects.get_or_create(
-            slug="LIB1", org=org1, defaults={"locator": "lib:Org1:LIB1", "title": "Library Org1"}
+            slug="LIB1",
+            org=org1,
+            defaults={"locator": "lib:Org1:LIB1", "title": "Library Org1", "learning_package": lp1},
         )
         ContentLibrary.objects.get_or_create(
-            slug="LIB2", org=org2, defaults={"locator": "lib:Org2:LIB2", "title": "Library Org2"}
+            slug="LIB2",
+            org=org2,
+            defaults={"locator": "lib:Org2:LIB2", "title": "Library Org2", "learning_package": lp2},
         )
         ContentLibrary.objects.get_or_create(
-            slug="LIB3", org=org3, defaults={"locator": "lib:Org3:LIB3", "title": "Library Org3"}
+            slug="LIB3",
+            org=org3,
+            defaults={"locator": "lib:Org3:LIB3", "title": "Library Org3", "learning_package": lp3},
         )
 
     def setUp(self):
@@ -966,38 +973,6 @@ def setUp(self):
         self.build_qs_patcher.start()
         self.addCleanup(self.build_qs_patcher.stop)
 
-        # The stub ContentLibrary uses 'title' directly instead of 'learning_package__title'.
-        # Patch _get_libraries_queryset to use the stub-compatible field, aliased as 'display_name'
-        # to match the column name the union and serializer expect.
-        def stub_get_libraries_queryset(_, allowed_pairs=None, allowed_orgs=None, search=""):  # pylint: disable=unused-argument
-            qs = ContentLibrary.objects
-            if allowed_pairs is not None or allowed_orgs is not None:
-                org_filter = Q(org__short_name__in=allowed_orgs) if allowed_orgs else Q()
-                pair_filter = (
-                    reduce(operator.or_, (Q(org__short_name=org, slug=slug) for org, slug in allowed_pairs))
-                    if allowed_pairs
-                    else Q()
-                )
-                combined = org_filter | pair_filter
-                if not combined:
-                    qs = qs.none()
-                else:
-                    qs = qs.filter(combined)
-            return qs.annotate(
-                scope_id=Cast("slug", output_field=CharField()),
-                display_name_col=Cast("title", output_field=CharField()),
-                org_name=Cast("org__short_name", output_field=CharField()),
-                scope_type=Value("library", output_field=CharField()),
-            ).values("scope_id", "display_name_col", "org_name", "scope_type")
-
-        self.libraries_qs_patcher = patch.object(
-            views.AdminConsoleScopesAPIView,
-            "_get_libraries_queryset",
-            stub_get_libraries_queryset,
-        )
-        self.libraries_qs_patcher.start()
-        self.addCleanup(self.libraries_qs_patcher.stop)
-
     # ------------------------------------------------------------------ #
     # Authentication                                                      #
     # ------------------------------------------------------------------ #
@@ -1175,11 +1150,9 @@ def test_view_permission_filters_courses_for_non_staff(self):
         user = User.objects.get(username="regular_9")
         self.client.force_authenticate(user=user)
         self.build_qs_patcher.stop()
-        self.libraries_qs_patcher.stop()
 
         response = self.client.get(self.url, {"type": "course"})
 
-        self.libraries_qs_patcher.start()
         self.build_qs_patcher.start()
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         external_keys = [item["external_key"] for item in response.data["results"]]
@@ -1210,21 +1183,13 @@ def test_library_display_name_populated_in_standalone_path(self):
         Regression test: without aliasing learning_package__title as display_name,
         the standalone library queryset returns 'title' as the key and the serializer
         silently produces empty strings since it only reads 'display_name'.
-
-        Skipped when the stub ContentLibrary model is in use (no learning_package relation).
         """
-        if not hasattr(ContentLibrary, "learning_package"):
-            self.skipTest("Stub ContentLibrary has no learning_package relation")
-
         user = User.objects.get(username="regular_1")
         self.client.force_authenticate(user=user)
-        # Stop both patchers so the real _get_libraries_queryset runs without union.
         self.build_qs_patcher.stop()
-        self.libraries_qs_patcher.stop()
 
         response = self.client.get(self.url, {"type": "library"})
 
-        self.libraries_qs_patcher.start()
         self.build_qs_patcher.start()
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.assertGreater(response.data["count"], 0)
@@ -1241,11 +1206,9 @@ def test_manage_permission_filters_courses_for_non_staff(self):
         user = User.objects.get(username="regular_10")
         self.client.force_authenticate(user=user)
         self.build_qs_patcher.stop()
-        self.libraries_qs_patcher.stop()
 
         response = self.client.get(self.url, {"type": "course", "management_permission_only": "true"})
 
-        self.libraries_qs_patcher.start()
         self.build_qs_patcher.start()
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         external_keys = [item["external_key"] for item in response.data["results"]]
@@ -1285,6 +1248,42 @@ def test_empty_allowed_library_pairs_returns_no_libraries(self):
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.assertEqual(response.data["count"], 0)
 
+    def test_empty_allowed_course_ids_returns_no_courses(self):
+        """When a non-staff user has no allowed courses, no courses are returned.
+
+        Regression test: an empty allowed_ids/allowed_orgs set must not bypass the filter
+        and return all courses (empty Q() | empty Q() was a no-op).
+        """
+        # regular_1 has only library permissions, no course permissions.
+        user = User.objects.get(username="regular_1")
+        self.client.force_authenticate(user=user)
+        self.build_qs_patcher.stop()
+
+        response = self.client.get(self.url, {"type": "course"})
+
+        self.build_qs_patcher.start()
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertEqual(response.data["count"], 0)
+
+    def test_library_only_user_sees_no_courses_in_mixed_listing(self):
+        """A user with only library permissions sees no courses in the default mixed listing.
+
+        Regression test: without the empty-set guard, a user with library access but no
+        course permissions would see all courses in the combined results.
+        """
+        # regular_1 has only library permissions, no course permissions.
+        user = User.objects.get(username="regular_1")
+        self.client.force_authenticate(user=user)
+        self.build_qs_patcher.stop()
+
+        response = self.client.get(self.url)
+
+        self.build_qs_patcher.start()
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        scope_types = {item["external_key"].split(":")[0] for item in response.data["results"]}
+        self.assertNotIn("course-v1", scope_types)
+        self.assertIn("lib", scope_types)
+
     def test_org_glob_scope_returns_all_org_libraries(self):
         """A user with an org-level glob permission (lib:ORG:*) sees all libraries in that org."""
         user = User.objects.get(username="regular_1")
@@ -1310,7 +1309,6 @@ def test_org_glob_scope_returns_all_org_courses(self):
         user = User.objects.get(username="regular_9")
         self.client.force_authenticate(user=user)
         self.build_qs_patcher.stop()
-        self.libraries_qs_patcher.stop()
 
         # Simulate get_scopes_for_user_and_permission returning an org-level glob.
         glob_scope = OrgCourseOverviewGlobData(external_key="course-v1:Org1+*")
@@ -1320,7 +1318,6 @@ def test_org_glob_scope_returns_all_org_courses(self):
         ):
             response = self.client.get(self.url, {"type": "course"})
 
-        self.libraries_qs_patcher.start()
         self.build_qs_patcher.start()
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         external_keys = [item["external_key"] for item in response.data["results"]]

openedx_authz/tests/stubs/migrations/0002_learningpackage_contentlibrary_learning_package.py

@@ -0,0 +1,28 @@
+import django.db.models.deletion
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("stubs", "0001_initial"),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name="LearningPackage",
+            fields=[
+                ("id", models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")),
+                ("title", models.CharField(max_length=255)),
+            ],
+        ),
+        migrations.AddField(
+            model_name="contentlibrary",
+            name="learning_package",
+            field=models.OneToOneField(
+                blank=True,
+                null=True,
+                on_delete=django.db.models.deletion.CASCADE,
+                to="stubs.learningpackage",
+            ),
+        ),
+    ]

openedx_authz/tests/stubs/models.py

@@ -35,6 +35,18 @@ def get_by_key(self, library_key):
         return obj
 
 
+class LearningPackage(models.Model):
+    """Stub model representing a learning package for testing purposes.
+
+    .. no_pii:
+    """
+
+    title = models.CharField(max_length=255)
+
+    def __str__(self):
+        return self.title
+
+
 class ContentLibrary(models.Model):
     """Stub model representing a content library for testing purposes.
 
@@ -45,10 +57,16 @@ class ContentLibrary(models.Model):
     title = models.CharField(max_length=255, blank=True, null=True)
     slug = models.SlugField(allow_unicode=True)
     org = models.ForeignKey(Organization, on_delete=models.PROTECT, null=True)
+    learning_package = models.OneToOneField(LearningPackage, on_delete=models.CASCADE, null=True, blank=True)
     created_at = models.DateTimeField(auto_now_add=True)
 
     objects = ContentLibraryManager()
 
+    @property
+    def library_key(self):
+        """Get the LibraryLocatorV2 opaque key for this library."""
+        return LibraryLocatorV2(org=self.org.short_name, slug=self.slug)
+
     def __str__(self):
         return str(self.locator)