Content library permissions & collaboration (SOL-81)

[bradenmacdonald]

Title: Content library permissions & collaboration (SOL-81)

Background: This PR adds a new page to studio where the user can manage who else has permission to access a content library. This PR also introduces a new read-only role used only for libraries.

Jira tickets: SOL-81

Discussions: Various offline discussions and the internal PRs below.

Dependencies: None - all dependencies are already merged to the feature branch.

Sandbox URL: CMS http://sandbox4.opencraft.com:18010/ - LMS http://sandbox4.opencraft.com/

Internal code reviews PRs open-craft#8 open-craft#11

Partner information: 3rd party-hosted open edX instance, for an edX solutions client.

Author concerns: Studio's roles ("Instructor", "Staff") are vague (is there any documentation in the code?) and with a third role in the mix ("User"), it was unclear which roles were allowed to do what, so I added the concept of permissions (EDIT_ROLES, VIEW_USERS, EDIT_CONTENT, VIEW_CONTENT) defined via roles... Would like feedback on this approach.

[openedx-webhooks]

Thanks for the pull request, @bradenmacdonald! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=6283

[antoviaque]

Added a sandbox:

• CMS http://sandbox4.opencraft.com:18010/
• LMS http://sandbox4.opencraft.com/

[bradenmacdonald]

@explorerleslie @cahrens @frrrances @cptvitamin @antoviaque Can you please review this PR?

Please let me know if there are other reviewers I should ping for the permissions changes - I'm not sure whose domain that falls under.

n.b. Jenkins is showing some build errors but I believe they are irrelevant. Quality is 100% despite the failure message, and bok choy errors seem unrelated.

[antoviaque]

Updated the sandbox: http://sandbox4.opencraft.com:18010/

[cahrens]

@bradenmacdonald, @antoviaque, and @smagoun This new Instructor Access settings page for Content Libraries looks just like the existing Course Team settings page. I assume that is intentional. We should not be copying all that code and HTML again-- you need to refactor the code and HTML so that it is shared between the 2 settings pages. At the same time, it would be very, very good to switch it to Backbone and client-side rendering. As @andy-armstrong and I have both mentioned, we vastly prefer using Backbone for new JS, and you will find it much easier to share and test the code.

After you have done that, please have someone from the Solutions team review the PR before T&L reviews.

[antoviaque]

@cahrens FYI this code was written before we started getting feedback on the PRs - it had been reviewed then (we always review PRs internally before opening upstream PRs), but that was before knowing how you want these features to be approached. We should have done another internal review before pushing this PR, sorry about that oversight -- my bad.

For the refactor, sharing the code between the two pages is a fair request. For converting it to Backbone, given the looming deadlines for the libraries MVP and the fact that the same page in courses doesn't currently use backbone, we wouldn't have the time to complete this now. This is a large refactor that is beyond the scope of content librairies given that it involves rewriting another, existing feature - I understand and appreciate the fact that you are trying to improve the quality of the codebase, and I would love to have the time to do this, but sadly we are under obligation to deliver within a fixed schedule over which we don't have control. That's not the ideal circumstances for going after technical debt that is already there - we do try to improve things as much as we can there (I hope the amount of work done on all our PRs is a proof of commitment on that!), but we do need your help to be able to merge all the pending PRs within the next two weeks. Remember that is the condition that allowed to have resources to have content libraries implemented at all - it's a tradeoff.

Also, while Braden is working on refactoring the client side, would it be possible to get a review of the backend/python changes? We will also do another internal review once Braden's changes are in, as requested.

Thank you for the help and understanding Christina!

[smagoun]

On refactoring: after discussion with @antoviaque and @cahrens we will go with the current structure for now, without either refactoring to use Backbone or the proposed intermediate refactoring. @antoviaque will create a development story in our backlog for refactoring to use backbone; we'll address after the deadline pressure has passed. There is agreement that Backbone is the right approach; the concern is purely schedule.

[bradenmacdonald]

@chrisndodge @martynjames can one or both of you please take a look at this? Thanks :)

[martynjames]

@bradenmacdonald - looking at your changes, but right now there seem to be a number of test failures (see above).

[martynjames]

Are we sure that role being blank here is the correct change? Did you change something to alter that behaviour? Are there other instances in code where we'd need to change role set to None => ""

[bradenmacdonald]

I changed it slightly... either None or "" works if you're submitting JSON. The issue is that this tests was submitting the data via POST, and the only thing that doesn't work anymore is submitting a POST request with role=None, because that actually results in the string value "None" appearing as the value in the POST data. The code now requires that the role field be present one way or another, and it's value is None or "".

So this would also work fine:

         resp = self.client.post(
             self.detail_url,
            data=json.dumps({"role": None}),
            content_type="application/json",
            HTTP_ACCEPT="application/json",
)

[bradenmacdonald]

Also, no, as far as I can tell, no existing code is affected by this, other than this one test.

[cahrens]

What does it mean for role to be "" or None?

[bradenmacdonald]

It means remove the user from the team (ensure they have no assigned role in this course/library). It's the same as sending a RESTful DELETE request.

[bradenmacdonald]

@martynjames The test failures are unrelated. If you click on the details you'll see that the quality is 100%. The bok choy test failures are timeouts - not sure why, but those tests are passing on my devstack.

[martynjames]

ok - I'm going to run them again manually, and see if we get an improvement.

[martynjames]

I love the change to think of these as permissions, and roles can have permissions!

[martynjames]

@antoviaque @bradenmacdonald - These are the changes that I think we need to run by others at edx - I like the change, but we want to ensure that everyone else is ready to go with the change.

[bradenmacdonald]

@andy-armstrong Can you please give us your thoughts on this and advise on who else should comment?

[cahrens]

I think @dmitchell would be a good person to review the auth changes.

[martynjames]

Overall code looks great! Some of the merge issues are going to cause some difficulty... Although I think a merge will also address some of the test failures. So, I think we need to rebase this branch in order to get it mergable, and then run the tests again.

[antoviaque]

@martynjames Thanks! The tests now seem to be passing after @bradenmacdonald 's latest rebase.

@explorerleslie @cahrens @frrrances @cptvitamin I believe this should thus be ready for review.

@dmitchell Can you review the approach discussed in https://github.com/edx/edx-platform/pull/6283#discussion_r22216164 ?

Thank you! And Merry Christmas : )

[antoviaque]

Also updated the sandbox to the latest version of the code: http://sandbox4.opencraft.com:18010/

[frrrances]

If this is a disabled state, it would be nice to remove the hover state, use the %ui-disabled extend, or add the is-disabled class.

[frrrances]

Oh now I see you just moved it so you can ignore my comment above!

[frrrances]

Overall looks good, just a few small UX comments:

1. The "instructor" wording is awkward here as we use "instructor" elsewhere to mean people that don't use Studio … maybe "collaborator" is a better term for the general library users (user, staff, admin)?
2. The user role is told they can't edit (and they see "read-only" in the library meta-data) but when they enter the library, there is no reminder there, and if they try to edit, they get a generic 403 Studio error. Ideally, we would hide edit button on components if a user is read-only level. If that is beyond the scope of this PR, it would be nice to have a notification at the top of the page, something like "You have read-only access. You will not be able to save changes."

Neither is a blocker, but would be nice to have. Otherwise 👍

[cptvitamin]

Hi @antoviaque , @bradenmacdonald This is looking pretty good from an accessibility perspective. I had a chance to take a little closer look and I have a couple of comments:

1. The "Create" button on the Create a New Library form is disabled until all of the required fields have a value. While it is visually disabled with styles, and physically disabled with javascript, there is nothing to indicate to the screen reader user that the button is disabled. toggling the state of the attribute aria-disabled should do the trick. It's critical here that if you set aria-disabled="true" by default make sure to toggle it to false when conditions are met, otherwise the button becomes physically disabled to screen reader users and they would never be able to submit the form.
2. On the Add an Instructor form, the above technique is not used. If I try to submit the form, an error message appears (modal dialog). focus is not moved to the dialog, and a screen reader user has no idea that the message is on screen. There is also no indication that an error has occurred. The form itself looks accessible, so moving focus should do the trick. To do so, you will need to add an ID to the modal and also a tabindex="-1" so that it can be focused on with javascript. Also, there is a <nav> element used in the modal. I think this is an inappropriate use of the nav element and would remove it all together. The SR text "prompt Actions" is also not very helpful. Suggest "Error Message - Action Required"

I have one small comment regarding the help text displayed below each form field in the Create a New Library form i.e. "The public display name for your library." Because screen reader users use practically every key on the keyboard to perform one action or another, they have to go into "forms processing mode" when they start filling out a form, which means they stop navigating the page the way they normally would and use tab to move from one form field to the next. This means they do not encounter any text displayed between form controls. A nice touch to add aria-describedby to reference the associated help text, programmatically linking it to the form control. This takes a space separated list of IDs so you can include the Required field tip as well. I also recommend just using html5 required attribute instead of aria-required="true", which is implied when html5 required is present e.g.

<label for="new-library-name">Library Name</label>
<input class="new-library-name" id="new-library-name" name="new-library-name" required placeholder="e.g. Computer Science Problems" type="text" aria-describedby="tip-new-library-name tip-required">
<span class="tip" id="tip-new-library-name">The public display name for your library.</span>
<span class="tip tip-error is-showing" id="tip-required">Required field.</span>

Let me know if you have any questions.

[cahrens]

Related to @cptvitamin's feedback-- for any changes you make for accessibility, please also make the same changes to the duplicated Studio course admin HTML. And then of course ensure that they make it into the shared HTML/JS when you do the refactoring later in January.

[cahrens]

I don't understand how/why this is linked to ENABLE_CREATOR_GROUP (which controls whether or not we expose the course creator permissions workflow-- this is True for Edge, False for edx.org, where only edX staff can create new courses).

[bradenmacdonald]

When ENABLE_CREATOR_GROUP is True, users without course creator permissions cannot create courses.

This test makes sure that when ENABLE_CREATOR_GROUP is True, users without course creator permissions likewise cannot create libraries. We have to patch the setting because it's false by default, so we can't test it.

[cahrens]

OK, it was not clear to me that the two things should be linked together (permission to create a course vs. permission to create a library). But I suppose they can always be split apart in the future if necessary.

Note though that users will not be able to create libraries on edx.org.

[cahrens]

When does "Help text here" get replaced with something real?

[catong]

@cahrens There is a separate PR for this, in review: https://github.com/edx/edx-platform/pull/6426

[catong]

Sorry for the last minute doc comments. I've finished doc review on this PR. Thanks!

[bradenmacdonald]

@catong Thanks for the documentation edits! I have just pushed b879f8b33b2f6369927c6554756f8f09256166ab which includes all those changes. Please click the link to view my exact edits if you'd like to double-check.

Screenshot:

[catong]

@bradenmacdonald Everything looks great!

[frrrances]

@bradenmacdonald Thanks for making the changes to the edit for users with read-only access. Much better experience! 👍

[bradenmacdonald]

Build should be passing now but Jenkins is timing out on the unit tests - can someone please re-run the tests for me? Penultimate and antepenultimate commits are green.

[antoviaque]

@bradenmacdonald Done - this now seem to be passing. (Note: to re-trigger the tests, I usually amend the last commit.)

[bradenmacdonald]

Thanks Xavier. I did amend the commit once but the new build had the same issue. Seems to be fixed now.

[bradenmacdonald]

I think this PR is good to go now?

[cahrens]

Good from my point of view, but is there a story on some team's backlog to resolve the StudioPermissionsService deprecation issue?

[bradenmacdonald]

I don't think there is yet. I'm not sure which team that would be? Platform?

[cpennington]

It's not on our backlog. Honestly, I think it's part of a larger story to figure out what we're doing with authentication/authorization, and I'm not sure who that'll fall to.