Added error message in enrollment tab inside CCX coach dashboard

[amir-qayyum-khan]

Background

(copied verbatim from clrux's CCX accessibility feedback https://github.com/edx/edx-platform/pull/9038#issuecomment-131098884 - Aug 14, 2015)

• Enrollment, when adding a student to a list, the student never appears, nor is there confirmation that he or she was added successfully (and no error messages if they weren't)

What is done in this PR

Studio Updates: None.

LMS Updates: Added error message in enrollment tab in case of adding invalid or unregistered user.

• Added error message in case coach enters a invalid student id or email.

Testing

• See the issues added below
• Try to add a student with invalid id
• Try to add a student with invalid email
• Try to add a unregistered student with valid email.

Issue mitocw#59
Issue mitocw#43
Issue jazkarta#119
Issue jazkarta#5
EDX https://github.com/edx/edx-platform/pull/7833

@pdpinch @pwilkins

[openedx-webhooks]

Thanks for the pull request, @amir-qayyum-khan! I've created OSPR-753 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

• supporting documentation
• edx-code email 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 still be done via the Github pull request interface. As a reminder, our process documentation is here.

[downzer0]

Hi @amir-qayyum-khan. Would you please get a sandbox setup with this work for review?

[sarina]

@pdpinch is your team able to provide a reviewer as well?

[sarina]

i18n

[pdpinch]

Sure thing. We'll get another review.

[sarina]

@pdpinch great thanks. Do you guys have the ability to set up a sandbox for Chris to look at?

[pdpinch]

It's not quite as easy for us as it is for you, but we've got one up now at https://sandbox2o.mitx.mit.edu/ https://sandbox2o.mitx.mit.edu/

That sandbox combines Amir's two open PRs.

FYI @clrux

On Sep 1, 2015, at 10:44 AM, Sarina Canelake [email protected] wrote:

@pdpinch https://github.com/pdpinch great thanks. Do you guys have the ability to set up a sandbox for Chris to look at?

—
Reply to this email directly or view it on GitHub https://github.com/edx/edx-platform/pull/9335#issuecomment-136746794.

[downzer0]

@amir-qayyum-khan and @pdpinch, a few more things.

• I get two error messages: one directly under the page title, and another (in red) above the form that manages users. We probably only need one of them. Since we're refreshing the page, we probably want to keep the one higher up in the DOM, under the title. (If you were handling this with AJAX or something, we'd go the opposite).
  • The error message near the top should:
    • Look like an error message. I believe we use .msg .msg-error on the container and .copy on the error message text.
    • This message area also needs a heading. You may use .sr on the heading if you wish to hide it visually (though it might be helpful to indicate that there were errors found).
    • If possible, send focus directly to this container when the page reloads.
    • If possible, make each error (each bullet) a link that links to the field in question for really easy flow using a keyboard.
• When I successfully added myself there was no confirmation that it worked. The page reloaded and the only way I knew that the action worked is if I "navigated" back to the table and listened to the new row with my username in it. Not a showstopper here, but I feel it could be friendlier. You could use the same method as the error message above, but with classes .msg .msg-confirm on the wrapper.

[downzer0]

@amir-qayyum-khan Did you happen to update the sandbox yet?

[amir-qayyum-khan]

No. not yet, I think we will update it in morning.

Thanks

[pwilkins]

@clrux @amir-qayyum-khan This is better. This screenshot is taken from the sandbox2o site.

Is the bullet preceding the message unavoidable? It looks out-of-place for a single line message.

[downzer0]

You should be able to remove the bullet by using the following:

ul {
    @extend %ui-no-list;
}

However, if there will ever be more than one item, it might be best to leave it as a list. Otherwise you don't really need a list at all, just use a p.

[sarina]

Chris, we should have existing style rules if there are existing UI
patterns wer'e following. Can we try to use these existing rules rather
than making new ones?

On Wed, Sep 16, 2015 at 9:03 AM, clrux [email protected] wrote:

You should be able to remove the bullet by using the following:

ul {
margin: 0;
padding: 0;
list-style: none;
}

—
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/9335#issuecomment-140735336.

[amir-qayyum-khan]

@clrux I can follow instructor/membership for this PR

• Do I need to add success messages? Because If we do add it, User can not remove it unless he refresh page.
  So each time he visits ENROLLMENT tab he will see success message and can think he trigged new action.
• If you see the Instructor/membership/COURSE TEAM MANAGEMENT. They are using div to represent error message.

<div class="request-response-error">Could not find a user with username or email address 'sss'.</div>

For success they dont show any message.

@clrux @pwilkins @pdpinch

[downzer0]

@amir-qayyum-khan I've updated my above comment to include an extend we use that I'd forgotten about. Please use it instead of specific styles. Thanks for the heads-up, @sarina.

[amir-qayyum-khan]

@clrux please also give me feedback abt success/confirm messages.

https://github.com/edx/edx-platform/pull/9335#issuecomment-140747738

[downzer0]

@amir-qayyum-khan Sorry for the trouble, here's a bit more info for you.

Please use this markup instead of an unordered list for the error/feedback messages, as I don't think we'll ever need more than one item here. We'll still use the tabindex here so you can send focus to it as you currently are.

<div class="msg msg-error" tabindex="-1">
    <p class="copy">(error message here)</p>
</div>

For success messages, please swap .msg-error for .msg-confirm.

For reference, you can find these styles defined in lms/static/sass/course/instructor/_instructor.scss.

Finally, can you get a sandbox up for this, or provide a link to an existing one?

[amir-qayyum-khan]

• So i dont need to add confirmation message in case CCX coach adds a student successfully or Revoke access?
• Should I not use

<div class="request-response-error">(error message here)</div>

instead of

  <div class="msg msg-error" tabindex="-1">
      <p class="copy">(error message here)</p>
  </div>

Because edx-platform is using `class="request-response-error"`` for Instructor/membership tab which have the same interface (UI) like we have in CCX/ enrollment tab

https://github.com/edx/edx-platform/pull/9335#issuecomment-140747738

[downzer0]

@amir-qayyum-khan That will work. My suggestion was more visual, but I suppose this is more in-line with what's being used. It doesn't need to be an unordered list though.

[amir-qayyum-khan]

Thanks @clrux

[amir-qayyum-khan]

• Done with Changes @clrux @pwilkins
• Need to deploy on sandbox @pdpinch

Thanks

[pdpinch]

Our sandbox has been updated. FYI @clrux @pwilkins

[pwilkins]

This hits the mark. It looks good to me.

[downzer0]

👍

There are additional issues with focus, but they're part of a larger whole in the LMS. This works.

@pwilkins are you and I the only two reviewers on this?

[pdpinch]

LGTM. @sarina, OK if I merge?

[sarina]

Yes, go ahead!