Skip to content
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.

Sign up for GitHub

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

Reapply "FM2-646: Practitioner: support limiting search to users or p… #551

Closed
wants to merge 4 commits into from
Closed

Reapply "FM2-646: Practitioner: support limiting search to users or p… #551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f85e457
Reapply "FM2-646: Practitioner: support limiting search to users or p…
mogoodrich Sep 26, 2024
6eaa03a
FM2-646: Practitioner: support limiting search to users or providers
mogoodrich Oct 8, 2024
d43f85a
Revert "FM2-646: Practitioner: support limiting search to users or pr…
mogoodrich Oct 8, 2024
59ca983
FM2-646: Practitioner: support limiting search to users or providers
mogoodrich Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions api/src/main/java/org/openmrs/module/fhir2/FhirConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm opening to discussion about this, as I don't really understand what this does (see below)


public static final String OPENMRS_FHIR_EXT_OBSERVATION_REFERENCE_RANGE = OPENMRS_FHIR_EXT_PREFIX
+ "/obs/reference-range";

Expand Down
36 changes: 29 additions & 7 deletions api/src/main/java/org/openmrs/module/fhir2/api/impl/FhirPractitionerServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@

import javax.annotation.Nonnull;

import java.util.Collections;

import ca.uhn.fhir.rest.api.server.IBundleProvider;
import ca.uhn.fhir.rest.param.InternalCodingDt;
import ca.uhn.fhir.rest.param.TokenAndListParam;
import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException;
import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException;
Expand All @@ -21,6 +25,7 @@
import org.hl7.fhir.r4.model.Practitioner;
import org.openmrs.Provider;
import org.openmrs.User;
import org.openmrs.module.fhir2.FhirConstants;
import org.openmrs.module.fhir2.api.FhirGlobalPropertyService;
import org.openmrs.module.fhir2.api.FhirPractitionerService;
import org.openmrs.module.fhir2.api.FhirUserService;
Expand All @@ -30,7 +35,6 @@
import org.openmrs.module.fhir2.api.search.SearchQueryInclude;
import org.openmrs.module.fhir2.api.search.TwoSearchQueryBundleProvider;
import org.openmrs.module.fhir2.api.search.param.PractitionerSearchParams;
import org.openmrs.module.fhir2.api.search.param.SearchParameterMap;
import org.openmrs.module.fhir2.api.translators.PractitionerTranslator;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;
Expand Down Expand Up @@ -92,17 +96,35 @@ public Practitioner create(@Nonnull Practitioner newResource) {
@Override
@Transactional(readOnly = true)
public IBundleProvider searchForPractitioners(PractitionerSearchParams practitionerSearchParams) {
IBundleProvider providerBundle = searchQuery.getQueryResults(practitionerSearchParams.toSearchParameterMap(), dao,
translator, searchQueryInclude);
SearchParameterMap theParams = new SearchParameterMap();
IBundleProvider userBundle = userService.searchForUsers(theParams);

if (!providerBundle.isEmpty() && !userBundle.isEmpty()) {
IBundleProvider providerBundle = null;
IBundleProvider userBundle = null;

if (shouldSearchExplicitlyFor(practitionerSearchParams.getTag(), "provider")) {
providerBundle = searchQuery.getQueryResults(practitionerSearchParams.toSearchParameterMap(), dao, translator,
searchQueryInclude);
}

if (shouldSearchExplicitlyFor(practitionerSearchParams.getTag(), "user")) {
userBundle = userService.searchForUsers(practitionerSearchParams.toSearchParameterMap());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

if (providerBundle != null && userBundle != null) {
return new TwoSearchQueryBundleProvider(providerBundle, userBundle, globalPropertyService);
} else if (providerBundle.isEmpty() && !userBundle.isEmpty()) {
} else if (providerBundle == null && userBundle != null) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I should still test for "isEmpty()" here, but the Encounter search example I am following didn't.

return userBundle;
}

return providerBundle;
}

protected boolean shouldSearchExplicitlyFor(TokenAndListParam tokenAndListParam, @Nonnull String valueToCheck) {
if (tokenAndListParam == null || tokenAndListParam.size() == 0 || valueToCheck.isEmpty()) {
return true;
}

return tokenAndListParam.getValuesAsQueryTokens().stream().anyMatch(
tokenOrListParam -> tokenOrListParam.doesCodingListMatch(Collections
.singletonList(new InternalCodingDt(FhirConstants.OPENMRS_FHIR_EXT_PRACTITIONER_TAG, valueToCheck))));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}
}
9 changes: 7 additions & 2 deletions api/src/main/java/org/openmrs/module/fhir2/api/search/param/PractitionerSearchParams.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,13 @@ public class PractitionerSearchParams extends BaseResourceSearchParams {

private StringAndListParam country;

private TokenAndListParam tag;

@Builder
public PractitionerSearchParams(TokenAndListParam identifier, StringAndListParam name, StringAndListParam given,
StringAndListParam family, StringAndListParam city, StringAndListParam state, StringAndListParam postalCode,
StringAndListParam country, TokenAndListParam id, DateRangeParam lastUpdated, HashSet<Include> revIncludes) {
StringAndListParam country, TokenAndListParam id, TokenAndListParam tag, DateRangeParam lastUpdated,
HashSet<Include> revIncludes) {

super(id, lastUpdated, null, null, revIncludes);

Expand All @@ -57,6 +60,7 @@ public PractitionerSearchParams(TokenAndListParam identifier, StringAndListParam
this.state = state;
this.postalCode = postalCode;
this.country = country;
this.tag = tag;
}

@Override
Expand All @@ -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());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another "I'm not quite sure what this does" addition here that it would be good to add clarification on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
3 changes: 2 additions & 1 deletion ...src/main/java/org/openmrs/module/fhir2/providers/r3/PractitionerFhirResourceProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ public IBundleProvider searchForPractitioners(@OptionalParam(name = Practitioner
@OptionalParam(name = Practitioner.SP_ADDRESS_POSTALCODE) StringAndListParam postalCode,
@OptionalParam(name = Practitioner.SP_ADDRESS_COUNTRY) StringAndListParam country,
@OptionalParam(name = Practitioner.SP_RES_ID) TokenAndListParam id,
@OptionalParam(name = "_tag") TokenAndListParam tag,
@OptionalParam(name = "_lastUpdated") DateRangeParam lastUpdated,
@IncludeParam(reverse = true, allow = { "Encounter:" + Encounter.SP_PARTICIPANT,
"MedicationRequest:" + MedicationRequest.SP_REQUESTER,
Expand All @@ -124,7 +125,7 @@ public IBundleProvider searchForPractitioners(@OptionalParam(name = Practitioner

return new SearchQueryBundleProviderR3Wrapper(
practitionerService.searchForPractitioners(new PractitionerSearchParams(identifier, name, given, family,
city, state, postalCode, country, id, lastUpdated, revIncludes)));
city, state, postalCode, country, id, tag, lastUpdated, revIncludes)));
}

}
3 changes: 2 additions & 1 deletion ...src/main/java/org/openmrs/module/fhir2/providers/r4/PractitionerFhirResourceProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ public IBundleProvider searchForPractitioners(@OptionalParam(name = Practitioner
@OptionalParam(name = Practitioner.SP_ADDRESS_POSTALCODE) StringAndListParam postalCode,
@OptionalParam(name = Practitioner.SP_ADDRESS_COUNTRY) StringAndListParam country,
@OptionalParam(name = Practitioner.SP_RES_ID) TokenAndListParam id,
@OptionalParam(name = "_tag") TokenAndListParam tag,
@OptionalParam(name = "_lastUpdated") DateRangeParam lastUpdated,
@IncludeParam(reverse = true, allow = { "Encounter:" + Encounter.SP_PARTICIPANT,
"MedicationRequest:" + MedicationRequest.SP_REQUESTER, "ServiceRequest:" + ServiceRequest.SP_REQUESTER,
Expand All @@ -131,6 +132,6 @@ public IBundleProvider searchForPractitioners(@OptionalParam(name = Practitioner
}

return practitionerService.searchForPractitioners(new PractitionerSearchParams(identifier, name, given, family, city,
state, postalCode, country, id, lastUpdated, revIncludes));
state, postalCode, country, id, tag, lastUpdated, revIncludes));
}
}
Loading
Loading