openedx
[BB-5815] Allow delete course content in Studio only for admin users
Description
This PR and a check to show/hide the delete button on the course content in Studio. With these changes, the delete button is only visible to the admin user one with the CourseInstructor role only and will not be visible to the CourseStaff role users.
Course Outline view for the user when it has admin access to the course
Course Outline view for the user when it has staff access to the course
Supporting information
JIRA Ticket: BB-5815
Testing instructions
- Login with
[email protected]onhttps://studio.pr27857.sandbox.opencraft.hosting/. On this sandbox we have given the superuser privileges to[email protected] - Go to the setting > Course team of the Demonstration Course
- Add
[email protected]as the staff for the course - Login from the
[email protected]on the studio page [email protected]should not be able to see the delete button on the course outline page.
Reviewers
- [ ] @nizarmah
Thanks for the pull request, @Skchoudhary! Please note that it may take us up to several weeks or months to complete a review and merge your PR.
Feel free to add as much of the following information to the ticket as you can:
- supporting documentation
- Open edX discussion forum threads
- timeline information ("this must be merged by XX date", and why that is)
- partner information ("this is a course on edx.org")
- any other information that can help Product understand the context for the PR
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.
Please let us know once your PR is ready for our review and all tests are green.
:warning: We can't start reviewing your pull request until you've submitted a signed contributor agreement or indicated your institutional affiliation. Please see the CONTRIBUTING file for more information. If you've signed an agreement in the past, you may need to re-sign. See The New Home of the Open edX Codebase for details.
Once you've signed the CLA, please allow 1 business day for it to be processed. After this time, you can re-run the CLA check by adding a comment here that you have signed it. If the problem persists, you can tag the @openedx/cla-problems team in a comment on your PR for further assistance.
@Skchoudhary Thank you for your contribution. Please let me know once it is ready for our review.
📣 💥 Heads-up: You must either rebase onto master or merge master into your branch to avoid breaking the build.
We recently removed diff-quality and introduced lint-amnesty. This means that the automated quality check that has run on your branch doesn't work the same way it will on master. If you have introduced any quality failures, they might pass on the PR but then break the build on master.
This branch has been detected to not have commit 2e335653 as an ancestor. Here's how to see for yourself:
git merge-base --is-ancestor 2e335653 skchoudhary/se-4482-allow-delete-in-studio-only-for-admin && echo "You're all set" || echo "Please rebase onto master or merge master to your branch"
If you have any questions, please reach out to the Architecture team (either #edx-shared-architecture on Open edX Slack or #architecture on edX internal).
@Skchoudhary - Thanks for the contribution!
Can you provide more context here on why this role differentiation is critical for you / your clients? Splitting the experience between studio authors and admins is something we have generally avoided (the admin role only differs in its ability to add new team members or remove team members.)
In this part of the platform removing deletion feels like an issue to instructors - imagine accidentally adding a section to a live course and now need an admin to remove it for you. The rationale for removing quick editing capabilities on the studio outline page feels unclear.
Currently based on context we have I would recommend closing this PR and not merging. Your input would be helpful here prior to doing so in case we have missed additional rationale for adding this.
Hello @marcotuts!
imagine accidentally adding a section to a live course and now need an admin to remove it for you.
According to our client's request, the staff member would hide the section from learners and mark it for deletion (for example by changing the section's name).
Splitting the experience between studio authors and admins is something we have generally avoided (the admin role only differs in its ability to add new team members or remove team members.)
Thanks for clarifying 👍🏼
I suppose, then, adding a new role wouldn't be appropriate. However, would such a change possibly be upstreamed if it were a Waffle Switch or Flag? Let us know please.
Hello @marcotuts! Just a friendly reminder for your inputs on putting this change behind a waffle flag. Do you suppose it would make it more upstreamable this way?
I would be ok considering this for merge / upstream as a waffle flag defaulted to off.
I remain wary of introducing role differentiation in studio like this but I can understand some course teams would find it difficult to recover from a situation where content was deleted that shouldn't have been. Apologies fo the delay on this!
FTR: the implementation has a UI loophole that should be fixed before merging. I'll post more details on Monday
@natabene , I have fixed the UI snag. It occurred because the user object can be None at create_xblock_info for ajax requests.
Here is a video of the bug in action recorded by one of our clients. https://drive.google.com/file/d/1DZs0CPLMJ16sL0ebXz2G6bwnJ0ftFn1j/view?us
Please review the pull request at your convenience.
Hello @joidegn! Before moving the pull request to edX's review, can you please check the failing builds, identify the ones that aren't caused by the changes from this pull request, and fix the ones that are?
Fixing the failing builds doesn't have to be done directly, it can be scheduled for a future sprint. But it would be best to go through these before moving it back to edX's review.
Hey @natabene, I uploaded some fixes and Jenkins/tests are passing now, but some workflows are on hold yet. If you have to run those workflows, so I think is ready for your review. If not, let me know and I will work on it.
I'm supportive of having this feature on the platform. It's been a roadmap request for some time, for example https://github.com/openedx/platform-roadmap/issues/59
Historically, we've found that user's have an optimistic view of destructive actions -- account deletion being an example -- and that warning text isn't typically read or full groked.
Would it be possible to use the pattern you see numerous place that requires the name of thing being deleted to be typed into a modal text box?
@e0d Sorry for the delay in responding here. You raise a good point, but I think it would be better suited in more destructive actions (like the account deletion example you mentioned). In the case of course creation/updation, creators will want to delete sections or units more often and having them go through an extra step of typing into a text box each time might not be a very good user experience (just my opinion). The use case that this PR targets is, that the admins still have the access to freely delete course content but non-admin members will only be able to update it.
If the above PR can be included as a waffle flag setting, then why not. @Skchoudhary please let me know if it's been merged already and the waffle name?
A similar issue we are asked by our clients is "Does Open edX® have a role for content reviewer/publisher?", i.e. someone who can publish content. In this scenario Course Staff cannot publish content through to the LMS without the help of a Course Reviewer/Publisher. A waffle flag solution for this would be great too.
In Moodle they do not have this User Role either, and even though you can create custom roles, you can't create a role like Reviewer/Publisher as far as I can see.
Sorry for the delay in replying here @DeanJayMathew. The original author of this PR is no longer working with us, so i'll be taking over the ownership of it.
If the above PR can be included as a waffle flag setting, then why not. @Skchoudhary please let me know if it's been merged already and the waffle name?
This is the PR that introduces the waffle flag. Once this is merged you can use the flag name PREVENT_STAFF_STRUCTURE_DELETION.
@natabene Can we get this reviewed soon?
@natabene Hi Natalia - just looking to see if you had any updated info on this PR so we can help move it along. We can't view the Jira ticket you linked here.
@jmakowski1123 anything additional needed for you for Product review?
@mphilbrick211 Last Product WG meeting, Ryan indicated that his team was working through the backlog. They ran into a GH permissions issue, not being able to change statuses once they reviewed tickets to "complete". It's possible this issue (and others) has been reviewed but the status hasn't changed. I think @sarina was going to look into possible solutions on the GH permissions end? In the meantime, I've asked Ryan to have PMs comment in lieu of changing the statuses directly.
They ran into a GH permissions issue, not being able to change statuses once they reviewed tickets to "complete".
I know the fix, I just need @ProductRyan to accept my invite to the org, and provide me with the GH usernames of the other PMs so I can invite them as well.
@ProductRyan In the event this one is still in queue, flagging as priority, given how long it's been open. Reading through the history, Marco effectively approved this back in Oct 2021, contingent on it being implemented as a waffle auto toggled to off. That flag has been created (see comment May 2022). Are there any outstanding reservations on this one?
@jmakowski1123 @ProductRyan - flagging this. Is this still being pursued?