feat: support role assignment for scopes that don't exist yet by efortish · Pull Request #369 · openedx/openedx-authz

efortish · 2026-07-28T00:26:26Z

Description

During course rerun, edx-platform needs to grant a user the instructor/staff role on the destination course key before that course's CourseOverview exists yet (the clone happens after role assignment would ideally occur). Today openedx-authz crashes in that case, so edx-platform (PR #38840) works around it with authz-specific branching that defers role assignment until after the course is cloned, plus a fallback check elsewhere in the meantime.

The actual crash: CourseScope.get_or_create_for_external_key() / ContentLibraryScope.get_or_create_for_external_key() call CourseOverview.get_from_id() / ContentLibrary.objects.get_by_key(), both of which raise DoesNotExist when the course/library isn't created yet.

This PR makes role assignment tolerant of that: the Scope row is created with its object FK left None, and gets linked up automatically once the real CourseOverview/ContentLibrary is saved.

Out of scope

Reverting the edx-platform PR #38840 workaround is explicit follow-up called out in the issue thread, not part of this change. The REST API's scope.exists() validation (rest_api/v1/serializers.py) is untouched — it's an intentional, separate guard for untrusted HTTP callers; this fix only affects the internal Python API path used directly by trusted platform code (e.g. student/roles.py's authz_add_role).

Solves #352

openedx-webhooks · 2026-07-28T00:26:32Z

Thanks for the pull request, @efortish!

This repository is currently maintained by @openedx/committers-openedx-authz.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

mariajgrimaldi

Can we deploy this into our sandbox that already has data to test? Thanks!

mariajgrimaldi · 2026-07-29T18:38:46Z

+    CourseScope.objects.filter(
+        external_key=str(instance.id), course_overview__isnull=True
+    ).update(course_overview=instance)


Don't we have a method for this so we don't use the model directly?

mariajgrimaldi · 2026-07-29T18:38:59Z

+    ContentLibraryScope.objects.filter(
+        external_key=str(instance.library_key), content_library__isnull=True
+    ).update(content_library=instance)


Same question here: don't we have a method for this so we don't use the model directly?

mariajgrimaldi · 2026-07-29T18:45:36Z

+    # 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).
+    external_key = models.CharField(max_length=255, null=True, blank=True, unique=True, db_index=True)


Should it be indexed? Is it needed? Also, should it be unique?

mariajgrimaldi · 2026-07-29T18:46:06Z

-        content_library = ContentLibrary.objects.get_by_key(library_key)
-        scope, _ = cls.objects.get_or_create(content_library=content_library)
+        try:
+            content_library = ContentLibrary.objects.get_by_key(library_key)


Don't we have a method for checking this?

mariajgrimaldi · 2026-07-29T18:46:41Z

+        try:
+            course_overview = CourseOverview.get_from_id(course_key)
+        except CourseOverview.DoesNotExist:
+            # The course doesn't exist yet: key the row by its external_key so it can be
+            # found and linked up later by the backfill signal in openedx_authz/handlers.py.


Same question here about having a method to check this, if I remember correctly it's called exists()

- Drop redundant db_index on Scope.external_key (unique already indexes it). - Reuse ScopeData.get_object() instead of duplicating the DoesNotExist handling in CourseScope/ContentLibraryScope.get_or_create_for_external_key(). - Add CourseScope.link_pending_scope()/ContentLibraryScope.link_pending_scope() so the handlers.py signal receivers no longer touch the model querysets directly. Addresses review comments from mariajgrimaldi on openedx#369.

BryanttV · 2026-07-29T22:38:17Z

@@ -98,25 +96,37 @@ def get_or_create_for_external_key(cls, scope) -> "ContentLibraryScope":
            ContentLibraryScope: The Scope instance for the given ContentLibrary,


Please update this return description

BryanttV · 2026-07-29T22:44:39Z

-            scope, _ = cls.objects.get_or_create(external_key=str(library_key))
-            return scope
+            # found and linked up later by link_pending_scope().
+            row, _ = cls.objects.get_or_create(external_key=scope.external_key)


I don't think row says much. I prefer the previous name, scope

BryanttV · 2026-07-29T22:52:46Z

+        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)


BryanttV · 2026-07-29T23:10:51Z

@@ -84,6 +82,12 @@ class ContentLibraryScope(Scope):
    def get_or_create_for_external_key(cls, scope) -> "ContentLibraryScope":


The code in get_or_create_for_external_key and link_pending_scope is very similar to both clases. Do you think we can reduce the code duplication in any way?

…LibraryScope 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.

Every merged PR in this repo bumps __version__ and adds a CHANGELOG.rst entry (feat -> minor bump, per semver); this PR was missing both. Also update the "extending Scope" section of the architecture ADR (0005), which still described the old per-subclass get_or_create_for_external_key() contract instead of the current LINKED_OBJECT_FIELD/external_key_for_object() one.

- Drop redundant db_index on Scope.external_key (unique already indexes it). - Reuse ScopeData.get_object() instead of duplicating the DoesNotExist handling in CourseScope/ContentLibraryScope.get_or_create_for_external_key(). - Add CourseScope.link_pending_scope()/ContentLibraryScope.link_pending_scope() so the handlers.py signal receivers no longer touch the model querysets directly. Addresses review comments from mariajgrimaldi on openedx#369.

…LibraryScope 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.

Every merged PR in this repo bumps __version__ and adds a CHANGELOG.rst entry (feat -> minor bump, per semver); this PR was missing both. Also update the "extending Scope" section of the architecture ADR (0005), which still described the old per-subclass get_or_create_for_external_key() contract instead of the current LINKED_OBJECT_FIELD/external_key_for_object() one.

The backfill_course_scope/backfill_content_library_scope post_save receivers ran unconditionally on every CourseOverview/ContentLibrary save, including unrelated fixtures elsewhere in the suite. Wrap them in a try/except plus their own savepoint (transaction.atomic()) so a failure to link a pending scope never breaks the caller's save() or poisons its transaction. Also fix the ContentLibrary test stub's get_by_key(): it only set locator, leaving org/slug unset, so library_key crashed with AttributeError as soon as anything (now including ScopeData.get_object()'s canonical-key check) touched it. It now populates org/slug like the real manager does. Fixes the 6 test failures from CI on this PR. refactor: deduplicate pending-scope logic between CourseScope/ContentLibraryScope 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.

Every merged PR in this repo bumps __version__ and adds a CHANGELOG.rst entry (feat -> minor bump, per semver); this PR was missing both.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 28, 2026

openedx-webhooks added this to Contributions Jul 28, 2026

github-project-automation Bot moved this to Needs Triage in Contributions Jul 28, 2026

mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Jul 28, 2026

efortish marked this pull request as ready for review July 29, 2026 17:39

efortish mentioned this pull request Jul 29, 2026

Support role assignments for keys (courses, and others) that do not exist yet #352

Open

mariajgrimaldi requested review from BryanttV and mariajgrimaldi July 29, 2026 18:11

mariajgrimaldi requested changes Jul 29, 2026

View reviewed changes

BryanttV requested changes Jul 29, 2026

View reviewed changes

BryanttV linked an issue Jul 30, 2026 that may be closed by this pull request

Support role assignments for keys (courses, and others) that do not exist yet #352

Open

efortish added 4 commits July 30, 2026 13:59

feat: support role assignment for scopes that don't exist yet

1cd2e93

fix: ci linting warning

e861670

fix: last ci check

c2960cb

efortish force-pushed the ks/issue#352 branch from 6e00b02 to 3f18e80 Compare July 30, 2026 19:02

efortish added 2 commits July 30, 2026 14:07

docs: bump version and update changelog for openedx#369

f1c7d0a

Every merged PR in this repo bumps __version__ and adds a CHANGELOG.rst entry (feat -> minor bump, per semver); this PR was missing both.

efortish force-pushed the ks/issue#352 branch from 3f18e80 to f1c7d0a Compare July 30, 2026 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: support role assignment for scopes that don't exist yet - #369

feat: support role assignment for scopes that don't exist yet#369
efortish wants to merge 6 commits into
openedx:mainfrom
eduNEXT:ks/issue#352

efortish commented Jul 28, 2026

Uh oh!

openedx-webhooks commented Jul 28, 2026 •

edited

Loading

Uh oh!

mariajgrimaldi left a comment

Uh oh!

mariajgrimaldi Jul 29, 2026

Uh oh!

mariajgrimaldi Jul 29, 2026

Uh oh!

mariajgrimaldi Jul 29, 2026

Uh oh!

mariajgrimaldi Jul 29, 2026

Uh oh!

mariajgrimaldi Jul 29, 2026

Uh oh!

BryanttV Jul 29, 2026

Uh oh!

BryanttV Jul 29, 2026

Uh oh!

BryanttV Jul 29, 2026

Uh oh!

BryanttV Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		@@ -98,25 +96,37 @@ def get_or_create_for_external_key(cls, scope) -> "ContentLibraryScope":
		ContentLibraryScope: The Scope instance for the given ContentLibrary,

		@@ -84,6 +82,12 @@ class ContentLibraryScope(Scope):
		def get_or_create_for_external_key(cls, scope) -> "ContentLibraryScope":

Uh oh!

Conversation

efortish commented Jul 28, 2026

Description

Out of scope

Uh oh!

openedx-webhooks commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariajgrimaldi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

openedx-webhooks commented Jul 28, 2026 •

edited

Loading