Reapply "FM2-646: Practitioner: support limiting search to users or p… #551
Conversation
…roviders" This reverts commit 1bad7e3.
| return new TwoSearchQueryBundleProvider(providerBundle, userBundle, globalPropertyService); | ||
| } else if (providerBundle.isEmpty() && !userBundle.isEmpty()) { | ||
| } else if (providerBundle == null && userBundle != null) { |
There was a problem hiding this comment.
Not sure if I should still test for "isEmpty()" here, but the Encounter search example I am following didn't.
|
|
||
| return tokenAndListParam.getValuesAsQueryTokens().stream().anyMatch( | ||
| tokenOrListParam -> tokenOrListParam.doesCodingListMatch(Collections | ||
| .singletonList(new InternalCodingDt(FhirConstants.OPENMRS_FHIR_EXT_PRACTITIONER_TAG, valueToCheck)))); |
There was a problem hiding this comment.
So I copied this method bring much verbatim from a similar method in the encounter service, but, honestly, I don't exactly understand what the this internal coding and this new constant I defined does... we don't include this coding in our actual request, but it still seems to match like it should. @ibacher ?
There was a problem hiding this comment.
The thing you copied and pasted from was intended to support this:
/Encounter?_tag=http://fhir.openmrs.org/ext/encounter-tag|encounter
So, in this case:
/Provider?_tag=http://fhir.openmrs.org/ext/provider-tag|provider
If this works, it's kind of by mistake or at least for reasons I don't fully understand... (FHIR Tags are coded elements so ideally they'd have both a system and a code).
| @@ -68,6 +72,7 @@ public SearchParameterMap toSearchParameterMap() { | |||
| .addParameter(FhirConstants.CITY_SEARCH_HANDLER, getCity()) | |||
| .addParameter(FhirConstants.STATE_SEARCH_HANDLER, getState()) | |||
| .addParameter(FhirConstants.POSTALCODE_SEARCH_HANDLER, getPostalCode()) | |||
| .addParameter(FhirConstants.COUNTRY_SEARCH_HANDLER, getCountry()); | |||
| .addParameter(FhirConstants.COUNTRY_SEARCH_HANDLER, getCountry()) | |||
| .addParameter(FhirConstants.TAG_SEARCH_HANDLER, getTag()); | |||
There was a problem hiding this comment.
Another "I'm not quite sure what this does" addition here that it would be good to add clarification on.
There was a problem hiding this comment.
By itself, I think this is meaningless or maybe a vestige of a previous version that passed the tag down the the DAO layer? In any case, it's harmless because there's nothing in the DAO layer that does anything with this.
| @@ -109,6 +109,8 @@ private FhirConstants() { | |||
|
|
|||
| public static final String OPENMRS_FHIR_EXT_ENCOUNTER_TAG = OPENMRS_FHIR_EXT_PREFIX + "/encounter-tag"; | |||
|
|
|||
| public static final String OPENMRS_FHIR_EXT_PRACTITIONER_TAG = OPENMRS_FHIR_EXT_PREFIX + "/practitioner-tag"; | |||
There was a problem hiding this comment.
I'm opening to discussion about this, as I don't really understand what this does (see below)
| @@ -68,6 +72,7 @@ public SearchParameterMap toSearchParameterMap() { | |||
| .addParameter(FhirConstants.CITY_SEARCH_HANDLER, getCity()) | |||
| .addParameter(FhirConstants.STATE_SEARCH_HANDLER, getState()) | |||
| .addParameter(FhirConstants.POSTALCODE_SEARCH_HANDLER, getPostalCode()) | |||
| .addParameter(FhirConstants.COUNTRY_SEARCH_HANDLER, getCountry()); | |||
| .addParameter(FhirConstants.COUNTRY_SEARCH_HANDLER, getCountry()) | |||
| .addParameter(FhirConstants.TAG_SEARCH_HANDLER, getTag()); | |||
There was a problem hiding this comment.
By itself, I think this is meaningless or maybe a vestige of a previous version that passed the tag down the the DAO layer? In any case, it's harmless because there's nothing in the DAO layer that does anything with this.
|
|
||
| return tokenAndListParam.getValuesAsQueryTokens().stream().anyMatch( | ||
| tokenOrListParam -> tokenOrListParam.doesCodingListMatch(Collections | ||
| .singletonList(new InternalCodingDt(FhirConstants.OPENMRS_FHIR_EXT_PRACTITIONER_TAG, valueToCheck)))); |
There was a problem hiding this comment.
The thing you copied and pasted from was intended to support this:
/Encounter?_tag=http://fhir.openmrs.org/ext/encounter-tag|encounter
So, in this case:
/Provider?_tag=http://fhir.openmrs.org/ext/provider-tag|provider
If this works, it's kind of by mistake or at least for reasons I don't fully understand... (FHIR Tags are coded elements so ideally they'd have both a system and a code).
| } | ||
|
|
||
| if (shouldSearchExplicitlyFor(practitionerSearchParams.getTag(), "user")) { | ||
| SearchParameterMap theParams = new SearchParameterMap(); |
There was a problem hiding this comment.
The search parameter map should definitely be copied here or else additional filtering won't work...
| } | ||
|
|
||
| if (shouldSearchExplicitlyFor(practitionerSearchParams.getTag(), "user")) { | ||
| userBundle = userService.searchForUsers(practitionerSearchParams.toSearchParameterMap()); |
There was a problem hiding this comment.
Note that formerly the search parameters were not passed into the search for users method, which seemed incorrect, but these parameters may not currently be used in a user context?
…roviders"
This reverts commit 1bad7e3.
Description of what I changed
Issue I worked on
see https://issues.openmrs.org/browse/FM2-
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amendI ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master