New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MH-12976, custom role patterns not working #389

Merged
merged 6 commits into from Sep 5, 2018

Conversation

Projects
None yet
4 participants
@slampunk
Copy link
Contributor

slampunk commented Aug 20, 2018

The new chosen library we use introduced line breaks to chosen-type generated elements. These line breaks pick up as a textNodes; the DOM selection query in the custom function getMoreRoles thus returns the textNode instead of the input element.

The fix simply updates the DOM selection query to search for an input element with class chosen-search-input. Note that, if the chosenjs maintainers remove the class chosen-search-input from the input, this change will need to be updated again.

@lkiesow

This comment has been minimized.

Copy link
Member

lkiesow commented Aug 20, 2018

Would it be possible to push this fix upstream? Otherwise, this is bound to break again and again each time we update angular-chosen.

@lkiesow lkiesow changed the title T/mh 12976 custom role patterns not working MH-12976, custom role patterns not working Aug 20, 2018

@gregorydlogan gregorydlogan self-assigned this Aug 20, 2018

@gregorydlogan

This comment has been minimized.

Copy link
Member

gregorydlogan commented Aug 20, 2018

Normally I wouldn't take this since I was starting work on it, but I haven't actually done anything with it yet so I'll review since I'm familiar with the code already :)

+1 for pushing upstream, will test this afternoon.

@gregorydlogan

This comment has been minimized.

Copy link
Member

gregorydlogan commented Aug 21, 2018

Ok, still seeing issues. Well, two really:

  1. The default build does not ship with enough roles showing up in the ACL list to trigger the text input. If you're seeing the text box then you're not looking at strictly default roles. I have a patch set for this here
  2. Once you do have enough roles, this does't work right (at least for me). The input allows you to type, once you type something that matches the text you just typed disappears, and the dropdown reappears with the text you typed as a new item at the bottom. With enough roles this new item may not be visible on screen, and even if it is it's very confusing. This leads to be user confusion (what happened?) and bad data.

STR for bad data:
Target role is abc123_Learner, custom pattern is ^[0-9a-f-]+_(Learner|Instructor)$

  • Type asdbc123_Learner
  • Move cursor to beginning of text, remove s
  • New role is entered as adbc123_Learner

IMO, if the role does not match an existing role, and matches the custom pattern it should replace the no search results found text with something like "hit enter to create a new role", or something.

@slampunk

This comment has been minimized.

Copy link
Contributor

slampunk commented Aug 22, 2018

Thanks for first pass.

  1. I'll cherry-pick your commits from there (i.e. 451ac10)
  2. yes, that's irritating in the least. Most (if not all) of the code for this specific functionality is largely unchanged on our side, so I suspect there may be further changes on the chosen lib side which need to be adapted for our use case.
    Give me a day or two to go thru the angular-chosen plumbing to make it work (at least) as it was in 3.x*. Thereafter, I can look at implementing your idea for explicitly opting to add the external role.

* 3.x had it that the new role automatically popped up in the dropdown as soon as it passed the regex match + without the text in the input disappearing.

MH-12976: Enabling search on all dropdowns. By default we were requir…
…ing 8 items before rendering the search, this sets that requirement to 0 so that we always display the search element.

@gregorydlogan gregorydlogan added the bug label Aug 22, 2018

@slampunk

This comment has been minimized.

Copy link
Contributor

slampunk commented Aug 25, 2018

Hi @gregorydlogan

I added an earlier customisation that was skipped during the angular-chosen update. The fix restores the functionality to how it was in 3.x/4.x, where the textbox is not cleared on a successful custom pattern match.

There's a slight discrepancy though in that the newly entered custom role is not always highlighted, once the list of roles is updated as a result of a successful search.
To see this discrepancy, try:

  1. Enter "abc_Learner" (this will be highlighted: expected).
  2. Move cursor to beginning of text, and add a single "a" to make the text read as "aabc_Learner".
  3. Although "aabc_Learner" is the text in the textbox, the element in the dropdown list with role "aabc_Learner" is not highlighted (unexpected behaviour).

I'll resolve this discrepancy with a follow-on commit.

@slampunk

This comment has been minimized.

Copy link
Contributor

slampunk commented Sep 3, 2018

Hi @gregorydlogan

The latest commit highlights the newly entered custom role (see my previous comment).

Would you still prefer confirmation text for new roles, i.e. "hit enter to use role [new role here]"?

@gregorydlogan

This comment has been minimized.

Copy link
Member

gregorydlogan commented Sep 3, 2018

Ideally yes, but that would require new translation strings. If you want to do that we'll have to file a separate PR against develop. Let me look at this in the next couple of days and get back to you.

@mtneug

This comment has been minimized.

Copy link
Member

mtneug commented Sep 5, 2018

IMO, if the role does not match an existing role, and matches the custom pattern it should replace the no search results found text with something like "hit enter to create a new role", or something.

@gregorydlogan I'm not sure if this is relevant, but note that there might be other role provider (e.g. for Sakai or Moodle) that provide non-existing roles. Here the "matches the custom pattern"-check is not possible. Why do we need that check anyway? Additionally, these role provider could potentially return multiple roles. E.g. given the input 123 the Moodle role provider returns both 123_Learner and 123_Instructor. If I recall correctly, the Sakai role provider does the same.

@gregorydlogan

This comment has been minimized.

Copy link
Member

gregorydlogan commented Sep 5, 2018

@mtneug you bring up good points, and I was unable to actually look at the result when I made my previous comment. Having built it and actually tested now, the current implementation is acceptable, if a little jarring.

@slampunk I'm going to merge this as is, thanks for the work!

@gregorydlogan gregorydlogan merged commit 120ce5f into opencast:r/5.x Sep 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment