[BB-5815] Allow delete course content in Studio only for admin users #27857
Conversation
|
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:
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. 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-webhooks
added
open-source-contribution
|
@Skchoudhary Thank you for your contribution. Please let me know once it is ready for our review. |
Skchoudhary
force-pushed
the
skchoudhary/se-4482-allow-delete-in-studio-only-for-admin
branch
from
aafc297 to
fe94b6c
Compare
openedx-webhooks
added
needs triage
waiting on author
|
📣 💥 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 2e33565 as an ancestor. Here's how to see for yourself: 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). |
There was a problem hiding this comment.
Nice work 👍
- I tested this by following the testing instructions you provided
- I read through the code
- I checked for accessibility issues
-
Includes documentation
I do have a slight comment about the testing instructions though. You mention to SSH into the instance. Unfortunately, that's a privilege edX reviewers won't have without some communication.
Accordingly, I updated the [email protected] to have superuser privileges and set that user as an admin to the Demonstration Course. This should allow anyone to set other users as staff on the Demonstration Course.
Can you please update the testing instructions to ask the reviewer to login with the staff user instead?
Also as a side note, I have some doubt that this change will be approved as is, because it affects current course staff in the edX platform. So, there's a slightly chance we'll need to add a waffle switch for this change instead. However, we should wait to hear from someone on edX's side regarding this first.
|
Hi @natabene This PR is ready for edX's review. |
natabene
removed
the
waiting on author
openedx-webhooks
added
product review
|
@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!
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).
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? |
|
@natabene May we ask some help from you regarding this PR? |
|
@gabor-boros Let me check if we get you a response this week. Stay tuned. |
openedx-webhooks
removed
the
product review
|
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 |
|
Thanks for the review @santiagosuarezedunext ! I agree, that this ticket meets the requirements necessary for a product review! The repo product lead is Brad (@cablaa77) @cablaa77 , we're trial-ing a new process to hopefully make it easier for product managers to review and/or approve PRs. I'm creating Product feature tickets for each PR on the Open edX Product Roadmap. The ticket consolidates all the information you need in order to evaluate and approve or deny the PR. The Product feature ticket for this PR is here. Could you please consolidate your comments and decisions there? Thanks! cc @mphilbrick211 |
|
I am approving this feature contingent upon it having a waffle flag defaulted to off. That flag has been created (see comment May 2022). It is clear that the problem is that content that should not be deleted from a course that is deleted. I agree with this statement: for those who face that problem this functionality would solve the problem, for those who do not have the problem they can leave this functionality disabled. |
Tagging @pkulkark here to see this update from Product. Once you confirm, we can do a final Engineering review. Thanks! |
pkulkark
force-pushed
the
skchoudhary/se-4482-allow-delete-in-studio-only-for-admin
branch
from
796ec50 to
d213b4c
Compare
|
@mphilbrick211 This feature is behind a waffle flag that is defaulted to off. c.f. https://github.com/openedx/edx-platform/pull/27857/files#diff-52ac7040db9b061fd0c16c90ac0c15fcb8cb62703ec6703a35e550ca826896c7 Also, the original author is no longer working with OpenCraft, so the CLA check fails. |
|
Thanks, @pkulkark! Will you be pursing this pull request? Looks like some tests need to be re-run and then TNL needs to review. |
pkulkark
force-pushed
the
skchoudhary/se-4482-allow-delete-in-studio-only-for-admin
branch
from
d213b4c to
981ed93
Compare
|
@mphilbrick211 Sorry for the delay. Yes, I will be pursuing this. I've rebased this and started the tests. |
|
Hi @pkulkark - would you mind resolving the branch conflicts here? @openedx/teaching-and-learning - would someone be able to review this? |
The commit for "feat: Allow delete course content in Studio only for admin users" introduces an incorrect import of an lms package from cms. Fixing this requires a larger refactoring that doesn't make sense as part of this rebase. The upsteam version also has this issue at this time: openedx#27857 This can be dropped in Quince if the above is merged, or updated if the above no longer has the issue.
pkulkark
force-pushed
the
skchoudhary/se-4482-allow-delete-in-studio-only-for-admin
branch
from
981ed93 to
cf62233
Compare
|
@mphilbrick211 Sorry for the delay. I've resolved the conflicts. Please proceed with the review. |
pkulkark
force-pushed
the
skchoudhary/se-4482-allow-delete-in-studio-only-for-admin
branch
from
cf62233 to
690652f
Compare
|
Hi @pkulkark - just flagging the failing tests and branch conflicts that have popped up. |
pkulkark
force-pushed
the
skchoudhary/se-4482-allow-delete-in-studio-only-for-admin
branch
3 times, most recently
from
333b57d to
c36e62e
Compare
pkulkark
force-pushed
the
skchoudhary/se-4482-allow-delete-in-studio-only-for-admin
branch
from
5ff4516 to
447ee97
Compare
|
@mphilbrick211 Thanks for the ping. I've fixed the conflicts. |
|
Hi @pkulkark! I checked in on the CLA issue... Because the original author is no longer involved, there's no way around the failing CLA check / changing the author. Would you be able to create a new PR for this item, and we'll take it from there? I believe this would be ready for review if the CLA check wasn't an issue, correct? |
Hi @pkulkark! Just following up on this. |
|
Closing this for now due to inactivity. |
|
@Skchoudhary Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
mphilbrick211
added
the
closed inactivity
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
CourseInstructorrole only and will not be visible to theCourseStaffrole 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
[email protected]onhttps://studio.pr27857.sandbox.opencraft.hosting/. On this sandbox we have given the superuser privileges to[email protected][email protected]as the staff for the course[email protected]on the studio page[email protected]should not be able to see the delete button on the course outline page.Reviewers