feat: add course authoring migration and rollback scripts#218
Conversation
|
Thanks for the pull request, @dwong2708! 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. |
rodmgwgu
left a comment
There was a problem hiding this comment.
We should make sure to only apply the migrations on courses that are covered by the feature flag.
If we cannot access the feature flag from here (as it is defined in edx-platform), we should add a parameter to the migration commands and functions to specify a list of courses to work on, so edx-platform can specify which courses to migrate.
b605933 to
0b8547b
Compare
- Add `authz_migrate_course_authoring` command to migrate legacy CourseAccessRole data to the new Authz (Casbin-based) system - Add `authz_rollback_course_authoring` command to rollback Authz roles back to legacy CourseAccessRole - Support optional `--delete` flag for controlled cleanup of source permissions after successful migration - Add `migrate_legacy_course_roles_to_authz` and `migrate_authz_to_legacy_course_roles` service functions - Add unit tests to verify migration and command behavior
b0463fa to
c8f1c73
Compare
bmtcril
left a comment
There was a problem hiding this comment.
Overall this is looking good. I think there is some tension between the need to use these commands for testing and for production migrations. For instance being able to migrate permissions when the waffle flag is not set which can cause perms to get out of sync and might cause confusion when an operator runs the command but nothing changes on the frontend.
Maybe we can add a flag that makes sure the waffle flag for the course(s) is set correctly for the direction that the migration is happening so the state stays locked to the permissions? I think I would make the operator version the default (change the waffle flag and delete) and make testing the exceptional case. What do you think?
Hello @rodmgwgu and @bmtcril , Based on what Rodrigo and I discussed, we are not using waffle flags here because doing so would require us to simulate waffle functionality, which currently lives in Open edX Platform. Bringing it here would likely require workarounds or introduce unnecessary complexity. That’s why we decided to exclude it. The next step is to place this tool somewhere that listens to waffle flag events and triggers it accordingly. This will likely require changes in the Open edX Platform, since that’s where waffle functionality resides. I also agree with running the operator version by default. What do you think? |
rodmgwgu
left a comment
There was a problem hiding this comment.
Just some minor corrections.
openedx_authz/management/commands/authz_migrate_course_authoring.py
Outdated
Show resolved
Hide resolved
openedx_authz/management/commands/authz_migrate_course_authoring.py
Outdated
Show resolved
Hide resolved
openedx_authz/management/commands/authz_rollback_course_authoring.py
Outdated
Show resolved
Hide resolved
rodmgwgu
left a comment
There was a problem hiding this comment.
I tested it in my local and it's working great.
Just please remove the things added to the Django admin and make sure to create a follow-up issue to handle org assignments later before merging.
wgu-taylor-payne
left a comment
There was a problem hiding this comment.
Overall looks good. I didn't go too deep, but here are some suggestions.
openedx_authz/engine/utils.py
Outdated
|
|
||
| # If we successfully created the legacy role, we can unassign the new role | ||
| if delete_after_migration: | ||
| batch_unassign_role_from_users( |
There was a problem hiding this comment.
Calling a batch function with only one user works, but is also unexpected. Are there ways to batch or is there another function that just expects one user?
There was a problem hiding this comment.
I added a way to do it on batch. Thanks
openedx_authz/management/commands/authz_migrate_course_authoring.py
Outdated
Show resolved
Hide resolved
Ah right, the course/org flags were never broken out as a proper library. Ok. That might be an argument to move these scripts into edx-platform eventually but I think this is fine for now. |
bmtcril
left a comment
There was a problem hiding this comment.
This is good for me when the other feedback is addressed, thanks!
Resolves: #179
Course Authoring Roles Migration to Authz
Overview
This PR introduces migration and rollback support for course authoring roles between the legacy
CourseAccessRolemodel and the new Authz (Casbin-based) authorization system.It provides:
Automated data migration(The Django migration will be added once it is released)What’s Included
1️⃣ Management Commands
authz_migrate_course_authoringMigrates legacy
CourseAccessRoleentries to the new Authz system../manage.py authz_migrate_course_authoring (--course-id-list <id> [<id> ...] | --org-id <org>) [--delete]authz_rollback_course_authoringRolls back Authz course authoring roles into legacy CourseAccessRole.
./manage.py cms authz_rollback_course_authoring (--course-id-list <id> [<id> ...] | --org-id <org>) [--delete]2️⃣ Service Layer
Added two migration service functions:
migrate_legacy_course_roles_to_authzmigrate_authz_to_legacy_course_rolesThese encapsulate the migration logic and are reusable by:
3️⃣
Data MigrationA Django data migration has been added to automatically trigger the forward migration.This ensures legacy course authoring roles are migrated during deployment without requiring manual intervention.4️⃣ Test Coverage
Added comprehensive unit tests that verify:
🧠 Design Considerations
Data migration runs automatically during deployment.✅ Result
This PR provides a safe, reversible, and production-ready migration path from legacy course authoring roles to the new Authz system.
Merge checklist:
Check off if complete or not applicable: