Revert: Cannot rerun courses - authz role assignment expects rerun to already exist (#38840) - #38973
Revert: Cannot rerun courses - authz role assignment expects rerun to already exist (#38840)#38973efortish wants to merge 8 commits into
Conversation
…un to already exist (openedx#38840) (openedx#38858)" This reverts commit ceb6a18.
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @efortish! This repository is currently maintained by 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 approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
test_create_course_disabled_by_flag and its 'authorized_staff_client' fixture belonged to the old TestCourseHandlerAuthz class, which an unrelated refactor (not openedx#38840) had already replaced with the leaner TestCourseHandlerStaffAccess before openedx#38840 was even merged. The revert's merge conflict was between that refactor and openedx#38840's insertion point, not a real need to restore this method.
1.22.0 ships the fix for openedx-authz#352 (role assignment on a course/library scope before its CourseOverview/ContentLibrary exists), which this revert depends on.
…revert-38840-issue381
BryanttV
left a comment
BryanttV
left a comment
There was a problem hiding this comment.
LGTM! I tested these changes on my local and it works as expected with the new openedx-authz version
| assert dest_course.display_name == 'Legacy Rerun Success' | ||
|
|
||
| # Verify author access | ||
| assert has_course_author_access(self.user, dest_course_key) |
There was a problem hiding this comment.
Wouldn't these test cases work with the current implementation? Why would we want to remove it? Or can we even replace them with a different kind of test? I just want to make sure these code paths are thoroughly tested.
There was a problem hiding this comment.
Good catch, you're right — I deleted them mechanically as part of the literal revert, but they don't actually test the removed workaround, they test that rerun still works correctly with authz enabled/disabled. Verified locally against a real devstack with openedx-authz 1.22.0+ installed: all 7 tests pass unmodified against the reverted code (that's the whole point of the revert — #352 makes the workaround unnecessary). Restored them in e667620, and cleaned up the docstrings that referenced the now-gone conditional add_instructor skip / created_user fallback.
Per review feedback from @mariajgrimaldi: these test classes were removed as part of the literal revert of openedx#38840, but they don't actually test the removed workaround itself — they test that rerun succeeds and grants correct access with authz enabled/disabled, which is exactly the behavior this revert needs to keep working now that openedx-authz#352 is implemented. Verified locally against the real devstack (openedx-authz 1.22.0+): all 7 tests pass unmodified against the reverted code. Cleaned up the docstrings that referenced the now-removed conditional add_instructor skip and created_user fallback, since add_instructor runs unconditionally pre-task again.
4 participants
Description
This is a clean revert of #38840, prepared ahead of time per openedx-authz#381
Background
#38840 worked around a limitation in
openedx-authz: role assignment required the destinationCourseOverview/ContentLibraryto already exist. This is not the case for a course rerun, where the role needs to be granted before the clone happens.The workaround therefore:
add_instructoruntil afterclone_course().created_userfallback inget_in_process_course_actions()so the initiating user could still see the in-process rerun status in the meantime.Long-term fix
The long-term fix is to allow role assignment on a scope key before its backing object exists, and backfill the link once the object is created.
This is being implemented in [openedx-authz#352], specifically [openedx-authz#369].
Once that fix ships, this workaround will no longer be necessary, and
add_instructorcan run unconditionally at its original call site again.Testing Instructions
Once openedx-authz#369 is released and pinned:
authz.enable_course_authoringflag globally.CourseOverview.DoesNotExisterror.Manual verification
This was manually verified in a sandbox with openedx-authz#369 installed and this revert applied:
See openedx-authz#381 for tracking.