Conversation
|
|
||
| List<Provider> providerList = HtmlFormEntryUtil.getProviders(providerRoleIds, true); | ||
|
|
||
| if (matchMode == null) { |
There was a problem hiding this comment.
By default, the <obs style="provider_autocomplete" /> element is rendered with <input type='hidden' id='*_matchMode_hid' value='ANYWHERE' /> to indicate that the matchMode should be ANYWHERE. However, if the <obs> is conditionally rendered with <controls><when></controls>, the value of matchMode is reset to "". This gets around it by always defaulting matchMode to ANYWHERE if it's not specified.
There was a problem hiding this comment.
So if I'm understanding it correctly, you discovered that match mode doesn't work when hidden within a controls/when?
Now, if you specific another match mode rather than "ANYWHERE" but within a controls/when, does it revert to "ANYWHERE", or is it only a problem in the "default" case?
If you've confirmed there's no easy way to fix this, I'm okay with this workaround, but let's add some of the notes above as a comment in the code (and add a quick note to the text on the limitations--ie don't use match mode within a when/then).
There was a problem hiding this comment.
Yes. If I put <encounterProviderAndRole autocompleteProvider="true" providerMatchMode="START" /> inside a controls/when, the providerMatchMode value is erased. Without the providerMatchMode value, the server does not do provider filtering at all, and just returns the entire list of providers regardless of what's typed in the text box. So, the providerMatchMode value erasure aside, I think we should add this default value anyway.
I haven't actually implemented providerMatchMode for <obs style="provider_autocomplete">, so it always defaults to ANYWHERE.
I will:
- document the providerMatchMode defaulting to ANYWHERE for provider_autocomplete
- Add a comment explaining for why we need to default the matchMode value if it's not set
- file a ticket for fixing the
_matchMode_hidvalue being erased when rendering inside a controls/when. (this might be a bigger issue for<input type='hidden'>in general)
mogoodrich
left a comment
mogoodrich
left a comment
There was a problem hiding this comment.
@chibongho generally looks good, but one question
|
|
||
| List<Provider> providerList = HtmlFormEntryUtil.getProviders(providerRoleIds, true); | ||
|
|
||
| if (matchMode == null) { |
There was a problem hiding this comment.
So if I'm understanding it correctly, you discovered that match mode doesn't work when hidden within a controls/when?
Now, if you specific another match mode rather than "ANYWHERE" but within a controls/when, does it revert to "ANYWHERE", or is it only a problem in the "default" case?
If you've confirmed there's no easy way to fix this, I'm okay with this workaround, but let's add some of the notes above as a comment in the code (and add a quick note to the text on the limitations--ie don't use match mode within a when/then).
mseaton
left a comment
mseaton
left a comment
There was a problem hiding this comment.
Generally looks good- just really one comment regarding the Exceptions
| try { | ||
| matchMode = MatchMode.valueOf(parameters.get("providerMatchMode").toUpperCase()); | ||
| } | ||
| catch (IllegalArgumentException e) { |
There was a problem hiding this comment.
Why are we catching an IllegalArgumentException here? Shouldn't this catch "Exception? And shouldn't it throw something like a BadFormDesignException (I forget the exact name)
There was a problem hiding this comment.
Changed to catching Exception and throwing BadFormDesignException. I also had to add throws BadFormDesignException in various functions (or throws Exception for test functions), as it's a checked exception.
| Person person = (Person) value; | ||
| obs.setValueText(person.getId().toString() + " - " + person.getPersonName().toString()); | ||
| } else if (value instanceof Provider) { | ||
| obs.setValueText(((Provider) value).getId().toString()); |
There was a problem hiding this comment.
I don't love setting the primary key id to the value - it would be much better if this were the uuid - but if this is the pre-existing convention, I guess it is fine. Can you confirm?
There was a problem hiding this comment.
So we have "<id> - <name>" format for Location and Person when saving them as obs values, but reading / parsing the value is more relaxed and can accept that, the id or the uuid.
However, Provider is stored as just <id>. We've been doing that for <obs style="provider_dropdown"> and <obs style="provider_radio"> already.
None of them are stored as uuid.
There was a problem hiding this comment.
Yeah, sounds like legacy behavior--if it's straightforward to change all of them to use uuids (while being backwards compatible) it would likely make sense to fix, though I my gut would be to do it in a quick follow-on ticket and commit so we can track it separately in case there are any issues.
There was a problem hiding this comment.
Filed https://openmrs.atlassian.net/browse/HTML-888 for this.
3 participants
See: https://openmrs.atlassian.net/browse/HTML-885
The
ProviderAjaxAutoCompleteWidgetis already created, but only used for the<EncounterProviderAndRole>tag. This PR adds function to input provider as obs with an autocomplete search.