refactor: deduplicate pending-scope logic between CourseScope/ContentLibraryScope

efortish · efortish · commit f03d4867432d · 2026-07-29T18:54:22.000-05:00
get_or_create_for_external_key() and link_pending_scope() were nearly identical between the two subclasses, differing only in the FK field name and how to compute external_key from the linked object. Move both methods to the base Scope class, parameterized by a LINKED_OBJECT_FIELD class attribute and an external_key_for_object() hook each subclass implements. Also restores the "scope" variable name (was "row") and fixes the stale "or None if glob pattern" return docstring, which described ScopeManager's dispatch behavior, not this method's. Addresses review comments from BryanttV on openedx#369.
diff --git a/openedx_authz/models/core.py b/openedx_authz/models/core.py
@@ -111,22 +111,95 @@ class Scope(BaseRegistryModel):
     This model can be extended to represent different types of scopes,
     such as courses or content libraries.
 
-    Subclasses should define a NAMESPACE class attribute (e.g., 'lib' for content libraries)
-    and implement get_or_create_for_external_key() classmethod.
+    Subclasses should define a NAMESPACE class attribute (e.g., 'lib' for content libraries),
+    a LINKED_OBJECT_FIELD naming their FK to the backing object (e.g. 'content_library'), and
+    implement external_key_for_object().
     """
 
     objects = ScopeManager()
 
+    # Name of the subclass's FK field pointing at its backing object (e.g. "content_library",
+    # "course_overview"), used by get_or_create_for_external_key()/link_pending_scope() below
+    # to work generically across scope types.
+    LINKED_OBJECT_FIELD: ClassVar[str] = None
+
     # Canonical string form of the scope's key (e.g. a course-v1 course id), set on creation
     # regardless of whether the backing object (CourseOverview, ContentLibrary, ...) exists yet.
     # This is the only way to find a scope back again when its FK to that object is still null
-    # (see get_or_create_for_external_key() in openedx_authz/models/scopes.py) so it can be
-    # linked up once the object is created (see openedx_authz/handlers.py backfill receivers).
+    # so it can be linked up once the object is created (see link_pending_scope() below and the
+    # backfill signal receivers in openedx_authz/handlers.py).
     external_key = models.CharField(max_length=255, null=True, blank=True, unique=True)
 
     class Meta:
         abstract = False
 
+    @classmethod
+    def external_key_for_object(cls, linked_object) -> str:
+        """Return the canonical external_key string for one of this scope's backing objects.
+
+        Subclasses must override this to match how their ScopeData computes external_key
+        (e.g. the course id, or the library key).
+
+        Args:
+            linked_object: An instance of the model named by LINKED_OBJECT_FIELD.
+
+        Returns:
+            str: The canonical external_key for that instance.
+        """
+        raise NotImplementedError
+
+    @classmethod
+    def get_or_create_for_external_key(cls, scope) -> "Scope":
+        """Get or create a scope instance for the given external key.
+
+        The backing object (CourseOverview, ContentLibrary, ...) need not exist yet (e.g.
+        during a course rerun, the destination course id is known before the course is
+        cloned): the scope is created with its LINKED_OBJECT_FIELD left ``None`` in that
+        case, and gets linked up automatically once a matching object is saved (see
+        link_pending_scope() below and the backfill signal receivers in
+        openedx_authz/handlers.py).
+
+        Args:
+            scope: ScopeData object with an external_key attribute.
+
+        Returns:
+            Scope: The (possibly newly created) scope instance for this subclass.
+        """
+        external_key = scope.external_key
+        linked_object = scope.get_object()
+        if linked_object is None:
+            # The object doesn't exist yet: key the row by its external_key so it can be
+            # found and linked up later by link_pending_scope().
+            scope, _ = cls.objects.get_or_create(external_key=external_key)
+            return scope
+
+        # Look up by the FK first (as before the external_key field existed) so scopes
+        # created before this change are reused rather than duplicated.
+        scope, created = cls.objects.get_or_create(
+            **{cls.LINKED_OBJECT_FIELD: linked_object},
+            defaults={"external_key": external_key},
+        )
+        if not created and not scope.external_key:
+            scope.external_key = external_key
+            scope.save(update_fields=["external_key"])
+        return scope
+
+    @classmethod
+    def link_pending_scope(cls, linked_object) -> None:
+        """Link a pending scope to the object that was just created for it.
+
+        Called from the relevant post_save signal receiver in openedx_authz/handlers.py
+        once an object with a matching external_key shows up.
+
+        Args:
+            linked_object: The model instance (CourseOverview, ContentLibrary, ...) that
+                was just saved.
+        """
+        cls.objects.filter(
+            external_key=cls.external_key_for_object(linked_object),
+            **{f"{cls.LINKED_OBJECT_FIELD}__isnull": True},
+        ).update(**{cls.LINKED_OBJECT_FIELD: linked_object})
+
 
 class Subject(BaseRegistryModel):
     """Model representing a subject in the authorization system.
diff --git a/openedx_authz/models/scopes.py b/openedx_authz/models/scopes.py
@@ -59,6 +59,7 @@ class ContentLibraryScope(Scope):
     """
 
     NAMESPACE = "lib"
+    LINKED_OBJECT_FIELD = "content_library"
 
     # Link to the actual content library, if applicable. In other cases, this could be null.
     # Piggybacking on the existing ContentLibrary model to keep the ExtendedCasbinRule up to date
@@ -79,54 +80,9 @@ class ContentLibraryScope(Scope):
     )
 
     @classmethod
-    def get_or_create_for_external_key(cls, scope) -> "ContentLibraryScope":
-        """Get or create a ContentLibraryScope for the given external key.
-
-        The backing ContentLibrary need not exist yet (e.g. it may be created
-        later as part of an in-progress operation): the scope is created with
-        ``content_library=None`` in that case and gets linked up automatically
-        once a ContentLibrary with a matching key is saved (see link_pending_scope()
-        and the backfill signal receiver in openedx_authz/handlers.py).
-
-        Args:
-            scope: ScopeData object with an external_key attribute containing
-                a LibraryLocatorV2-compatible string.
-
-        Returns:
-            ContentLibraryScope: The Scope instance for the given ContentLibrary,
-                or None if the scope is a glob pattern (contains wildcard).
-        """
-        content_library = scope.get_object()
-        if content_library is None:
-            # The library doesn't exist yet: key the row by its external_key so it can be
-            # found and linked up later by link_pending_scope().
-            row, _ = cls.objects.get_or_create(external_key=scope.external_key)
-            return row
-
-        # Look up by the FK first (as before this change) so scopes created before the
-        # external_key field existed are reused rather than duplicated.
-        row, created = cls.objects.get_or_create(
-            content_library=content_library,
-            defaults={"external_key": scope.external_key},
-        )
-        if not created and not row.external_key:
-            row.external_key = scope.external_key
-            row.save(update_fields=["external_key"])
-        return row
-
-    @classmethod
-    def link_pending_scope(cls, content_library) -> None:
-        """Link a pending ContentLibraryScope to the ContentLibrary that was just created for it.
-
-        Called from the ContentLibrary post_save signal receiver in openedx_authz/handlers.py
-        once a library with a matching external_key shows up.
-
-        Args:
-            content_library: The ContentLibrary instance that was just saved.
-        """
-        cls.objects.filter(
-            external_key=str(content_library.library_key), content_library__isnull=True
-        ).update(content_library=content_library)
+    def external_key_for_object(cls, linked_object) -> str:
+        """Return the canonical external_key string for a ContentLibrary instance."""
+        return str(linked_object.library_key)
 
 
 class CourseScope(Scope):
@@ -136,6 +92,7 @@ class CourseScope(Scope):
     """
 
     NAMESPACE = "course-v1"
+    LINKED_OBJECT_FIELD = "course_overview"
 
     # Link to the actual course, if applicable. In other cases, this could be null.
     # Piggybacking on the existing CourseOverview model to keep the ExtendedCasbinRule up to date
@@ -156,52 +113,6 @@ class CourseScope(Scope):
     )
 
     @classmethod
-    def get_or_create_for_external_key(cls, scope) -> "CourseScope":
-        """Get or create a CourseScope for the given external key.
-
-        The backing CourseOverview need not exist yet (e.g. during a course
-        rerun, the destination course id is known before the course is
-        cloned): the scope is created with ``course_overview=None`` in that
-        case and gets linked up automatically once a CourseOverview with a
-        matching id is saved (see link_pending_scope() and the backfill
-        signal receiver in openedx_authz/handlers.py).
-
-        Args:
-            scope: ScopeData object with an external_key attribute containing
-                a CourseKey string.
-
-        Returns:
-            CourseScope: The Scope instance for the given CourseOverview,
-                or None if the scope is a glob pattern (contains wildcard).
-        """
-        course_overview = scope.get_object()
-        if course_overview is None:
-            # The course doesn't exist yet: key the row by its external_key so it can be
-            # found and linked up later by link_pending_scope().
-            row, _ = cls.objects.get_or_create(external_key=scope.external_key)
-            return row
-
-        # Look up by the FK first (as before this change) so scopes created before the
-        # external_key field existed are reused rather than duplicated.
-        row, created = cls.objects.get_or_create(
-            course_overview=course_overview,
-            defaults={"external_key": scope.external_key},
-        )
-        if not created and not row.external_key:
-            row.external_key = scope.external_key
-            row.save(update_fields=["external_key"])
-        return row
-
-    @classmethod
-    def link_pending_scope(cls, course_overview) -> None:
-        """Link a pending CourseScope to the CourseOverview that was just created for it.
-
-        Called from the CourseOverview post_save signal receiver in openedx_authz/handlers.py
-        once a course with a matching external_key shows up.
-
-        Args:
-            course_overview: The CourseOverview instance that was just saved.
-        """
-        cls.objects.filter(
-            external_key=str(course_overview.id), course_overview__isnull=True
-        ).update(course_overview=course_overview)
+    def external_key_for_object(cls, linked_object) -> str:
+        """Return the canonical external_key string for a CourseOverview instance."""
+        return str(linked_object.id)
