[#10915] Break course CRUD permission into edit, delete and restore permissions

[daongochieu2810]

Part of #10915

Outline of Solution

• FE:
  • Break the checkbox of Edit/Delete/Restore Course into Edit Course, Delete Course and Restore Course checkboxes
  • Disable/Enable Restore, Edit and Delete buttons based on specific permissions
  • Update related tests and update snapshots
• BE:
  • Break the CAN_MODIFY_COURSE permission into CAN_EDIT_COURSE, CAN_DELETE_COURSE, and CAN_RESTORE_COURSE permissions
  • Change modify course permission to more specific permission for UpdateCourseAction, BinCourseAction and RestoreCourseAction
  • Update related tests and json test data

[madanalogy]

This is quite a massive PR, only went through half the files so far hang on for the rest. Preliminary comment below

[madanalogy]

Changes looks good to me! We'll give it a few weeks to see if anyone else has the bandwidth for a second review. Actually, now that I think about it, I'm not really sure how these changes will affect instructors already present in the system. @wkurniawan07 @xpdavid any idea whether we might need a data migration for changes to instructor privileges?

[rrtheonlyone]

Note that this is not a backwards compatible change.

As of now the privileges are just serialised and stored as a JSON string. We have instructors with permission CAN_MODIFY_COURSE at the moment in our database. After this change, those instructors will not be able to update/delete/restore course anymore.

I am guessing our two options are:

• we somehow account for both CAN_MODIFY_COURSE and those fine-grained ones in our actions
• write a script to migrate CAN_MODIFY_COURSE into all of those fine-grained ones above

Also, I am wondering if there is a need for the above change. Yes, fine-grained control is nice to have for courses but the same can be said for sessions/instructors/students etc.

[madanalogy]

Also, I am wondering if there is a need for the above change. Yes, fine-grained control is nice to have for courses but the same can be said for sessions/instructors/students etc.

Yes, the original issue is an epic targeted at all those permissions as well. This PR is "part of", tacking only the portion for courses.

Note that this is not a backwards compatible change.

I suspected as much, thanks for confirming. @daongochieu2810 if you'd rather not have to account for legacy data, we can put this PR on hold until other portions of the epic are resolved. Following which we'll need a data migration script. Alternatively I could set up a feature branch actually.

Regardless, you should be able to claim this PR for 3281 credit since it's already approved. Let me know whether you're able to account for legacy permissions in this PR, and I'll think about where to go from there.

[daongochieu2810]

I think having a script to migrate legacy data is better since we dont need to have both CAN_MODIFY_COURSE and fine-grained permissions in the db. I can work on this script and update the PR accordingly

[daongochieu2810]

@madanalogy I dont think we should delay this until other portions of the PR are done, as there will be a lot of merge conflicts. It is probably better if we maintain a script to migrate legacy data and update it as other portions of the PR are added

[madanalogy]

Merge conflicts are the least of our concerns. Data migration needs to be done in phases. You need new features to be compatible with old data until such a time as a data migration script can be run. Even if you have the script ready, you cannot be certain that it will 100% work the first time you run it on production. This PR will be left on hold until such a time as other aspects are tackled and/or the changes are engineered to be backwards compatible.

[daongochieu2810]

oh I see, then we should leave this on hold for now

[madanalogy]

Closing for now until issue validity is discussed