Make UI styling consistent between libraries and courses tabs when empty (SOL-232) - #7059
Conversation
|
Thanks for the pull request, @Kelketek! 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=7059 |
|
@cahrens This PR is intended to also have a resolution for SOL-227 as well. However, I'm a bit confused on what is required. The error message for Libraries is different from the one displayed, and is tailored for it. Libraries also don't have 'course numbers' as in your original comment, and I don't see an error message mentioning libraries and course numbers. Perhaps the issue was resolved elsewhere? Or do I misunderstand? |
|
@Kelketek The issue I raised is that libraries and courses existing within the same namespace. So I couldn't create the new course with org "foo" and course number "bar" because of the already existing library. It was a bit confusing to me because the error message I got only referred to courses, and yet the conflict was actually with a library. https://github.com/edx/edx-platform/pull/6459#issuecomment-69827517 |
There was a problem hiding this comment.
You're calling .format() on a string with no {studio_name} placeholder.
|
Minor issue I just pointed out, and it doesn't yet address SOL-227. Otherwise looks good. BTW This comment may be a little late, but I think it would be awesome to add some copy here that explains what a library is, which is non-obvious. i.e. Add "Content libraries hold problems and content that can be easily re-used across multiple courses." or something like that. |
c2d455f to
4e23c0c
Compare
|
@cahrens I have attempted to reproduce the issue you mentioned, and at first it appeared I had done so. However, upon further examination I discovered I used the same code and organization as a course I had already created out of habit, and didn't notice that because I was looking at the library listing instead of the course listing. When I created a library with an organization and code I don't normally use and then attempt to create a course with those items, it works just fine. Therefore I'm not able to reproduce the issue. |
|
👍 |
Kelketek
changed the title
There was a problem hiding this comment.
@marcotuts @antoviaque @Kelketek Can you please clarify the workflow/use case here? I find it a bit strange that a user would end up at the empty library page and see BOTH the "request access from a library creator or admin" AND the "Create your First Library" messages. If a user does NOT have course author (= library creator) privileges, will they still see the "Create your First Library" button enabled? And if a user does have course author and library creator privileges then it seems confusing to present them with the first message.
There was a problem hiding this comment.
The first message is prefixed with the title 'Expecting to see a library already here?' It's to inform users of the case when they may be in Studio and expect to be able to edit a library, only to find out there isn't one. This is the same behavior that happens for courses.
The permissions check has not changed between this and the previous version-- @bradenmacdonald is there a permission type we should be checking for? I got the impression by the code that 'anyone can create a library', which may not be what we actually want, but if this is the case, the fix for it is probably outside the scope of this task.
There was a problem hiding this comment.
Thanks, @Kelketek I had to check back in documentation for the main feature, to verify our position when we released the original MVP. In the documentation, we have: "In Studio, if you have course creation privileges, you can create a content library..." My understanding is that not all Studio users have permissions to create content libraries. Some might be course team members without course authoring privileges, and also shouldn't have library creation privileges, in which case the "Create First Library" button should ideally be hidden or disabled. @marcotuts your thoughts?
There was a problem hiding this comment.
I don't believe we have different course creation privileges and library creation privileges, we just have creation privileges. @catong I can follow up in person in case I've missed anything, but I think the message is ok to have in either the library or course context, as authors may be expecting one or both of those to be possible.
There was a problem hiding this comment.
@marcotuts Does this mean I should be checking for course authoring privileges when displaying the create button?
There was a problem hiding this comment.
Thanks, @marcotuts Perhaps I'm still missing the point. My original question was about whether a library user should see BOTH of these things:
- a message that says, "Expecting a library here?" You need to ask someone to add you to their library access.
- an active "Create a library" button.
Are you saying that yes, both cases can apply simultaneously, because a course team member who is able to create their own library might not have access privileges to library that someone else created?
Edited: Hmmmm... I might have just explained this to myself. I think I completely misunderstood the situation on first seeing the screenshots.
There was a problem hiding this comment.
Oh, wait... @marcotuts I think I still have an outstanding question. Per what we documented for the MVP and further to @Kelketek's question above, is my understanding correct that only users with course creation privileges also have library creation privileges? If yes, then there might be Studio users who do NOT have library creation privileges, in which case my question is still valid -- in the case of these users, I don't think they should see an active "Create a new library" button.
There was a problem hiding this comment.
Yes, there is a single "creator" privilege that affects the ability to create courses and libraries.
If you look above in this file, you can see the button in the top right checks for if course_creator_status=='granted': before showing the "Create Library" button. The same check should be added to the new button.
antoviaque
changed the title
There was a problem hiding this comment.
Suggested wording: "Were you expecting to see a particular library here?"
4e23c0c to
d0c923f
Compare
|
@catong I've addressed the notes you've written. But were you wanting me to update the course text as well? |
|
@Kelketek At this point I'm not sure whether these templates are the different or same/parameterized so it might be safer :-) for you to do this, if you wouldn't mind. If you would rather not, that's totally OK too, I'll create a backlog story to take care of it. |
|
@catong No problem :) If the other changes look good, I'll go ahead and rebase as well on this next push, then, and we should be good to go once the tests come back green. |
|
Awesome. Thanks! |
d0c923f to
f952a33
Compare
|
Jenkins seems to be hanging. @jzoldak , any idea what's up here? Is it safe to start a new test? |
|
@Kelketek AWS east was down and thus our jenkins infrastructure couldn't start up new workers. The situation has since been resolved. |
|
test this please |
|
@jzoldak Thanks for the heads up! |
|
test this please |
|
@catong @cahrens @marcotuts Tests are green! |
9 participants
Background: Makes the empty library listing look consistent with the empty course listing.
Empty course listing:
Empty library listing:
How it used to look:
Jira ticket number: https://openedx.atlassian.net/browse/SOL-232
Partner information: 3rd party-hosted open edX instance, for an edX solutions client.
Sandbox: http://sandbox4.opencraft.com:18010/