[BB-5815] Allow delete course content in Studio only for admin users

[Skchoudhary]

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 staff@example.com on https://studio.pr27857.sandbox.opencraft.hosting/. On this sandbox we have given the superuser privileges to staff@example.com
• Go to the setting > Course team of the Demonstration Course
• Add verified@example.com as the staff for the course
• Login from the verified@example.com on the studio page
• verified@example.com should not be able to see the delete button on the course outline page.

Reviewers

• @nizarmah

[openedx-webhooks]

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.

⚠️ 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.

[natabene]

@Skchoudhary Thank you for your contribution. Please let me know once it is ready for our review.

[arch-bom-gocd-alerts]

📣 💥 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:

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).

[nizarmah]

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 staff@example.com 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.

[Skchoudhary]

Hi @natabene

This PR is ready for edX's review.

[marcotuts]

@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.

[nizarmah]

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.

[nizarmah]

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?

[gabor-boros]

@natabene May we ask some help from you regarding this PR?

[natabene]

@gabor-boros Let me check if we get you a response this week. Stay tuned.

[marcotuts]

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!

[gabor-boros]

FTR: the implementation has a UI loophole that should be fixed before merging. I'll post more details on Monday

[jmakowski1123]

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

[cablaa77]

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.

[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]

@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.

[mphilbrick211]

Thanks, @pkulkark! Will you be pursing this pull request? Looks like some tests need to be re-run and then TNL needs to review.

[mphilbrick211]

Thanks, @pkulkark! Will you be pursing this pull request? Looks like some tests need to be re-run and then TNL needs to review.

Friendly ping on this @pkulkark!

[pkulkark]

@mphilbrick211 Sorry for the delay. Yes, I will be pursuing this. I've rebased this and started the tests.

[mphilbrick211]

Hi @pkulkark - would you mind resolving the branch conflicts here?

@openedx/teaching-and-learning - would someone be able to review this?

[pkulkark]

@mphilbrick211 Sorry for the delay. I've resolved the conflicts. Please proceed with the review.

[mphilbrick211]

Hi @pkulkark - just flagging the failing tests and branch conflicts that have popped up.

[pkulkark]

@mphilbrick211 Thanks for the ping. I've fixed the conflicts.

[mphilbrick211]

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?

[mphilbrick211]

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.

[mphilbrick211]

Closing this for now due to inactivity.

[openedx-webhooks]

@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.