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.

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

FM2-347:Add support for _has and _has:...:not for ServiceRequest and … #335

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

gitcliff
Copy link
Contributor

@gitcliff gitcliff marked this pull request as draft March 20, 2021 18:17
@codecov
Copy link

codecov bot commented Mar 21, 2021

Codecov Report

Merging #335 (b0c8a88) into master (6abb746) will decrease coverage by 0.48%.
The diff coverage is 32.14%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #335      +/-   ##
============================================
- Coverage     80.22%   79.74%   -0.48%     
- Complexity     2214     2219       +5     
============================================
  Files           204      204              
  Lines          5941     5993      +52     
  Branches        704      707       +3     
============================================
+ Hits           4766     4779      +13     
- Misses          768      806      +38     
- Partials        407      408       +1     
Impacted Files Coverage Δ
.../fhir2/api/dao/impl/FhirServiceRequestDaoImpl.java 43.55% <0.00%> (-43.95%) ⬇️
.../providers/r3/ObservationFhirResourceProvider.java 96.43% <ø> (+7.14%) ⬆️
...iders/r3/ProcedureRequestFhirResourceProvider.java 91.67% <ø> (ø)
...oviders/r4/ServiceRequestFhirResourceProvider.java 85.71% <ø> (ø)
...org/openmrs/module/fhir2/api/dao/impl/BaseDao.java 65.52% <42.86%> (-0.81%) ⬇️
...ule/fhir2/api/dao/impl/FhirObservationDaoImpl.java 86.09% <71.43%> (-0.95%) ⬇️
...ule/fhir2/api/impl/FhirObservationServiceImpl.java 94.29% <100.00%> (+0.17%) ⬆️
.../fhir2/api/impl/FhirServiceRequestServiceImpl.java 87.50% <100.00%> (ø)
...rs/module/fhir2/api/search/SearchQueryInclude.java 87.98% <100.00%> (ø)
.../providers/r4/ObservationFhirResourceProvider.java 96.55% <100.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6abb746...b0c8a88. Read the comment docs.

@gitcliff gitcliff force-pushed the FM2-347 branch 2 times, most recently from ca1f34e to 453e3cd Compare March 29, 2021 16:57
@gitcliff
Copy link
Contributor Author

@ibacher kindly review

@ibacher
Copy link
Member

ibacher commented Mar 29, 2021

PS I think the handling of the _has parameter should look something like this:

private void handleHasAndParam(Criteria criteria, HasAndListParam hasAndListParam) {
	if (hasAndListParam == null) {
		return;
	}

	DetachedCriteria detachedCriteria = DetachedCriteria.forClass(Obs.class);

	hasAndListParam.getValuesAsQueryTokens().stream().map(this::handleHasOrParam).filter(Optional::isPresent)
			.flatMap(Optional::get).forEach(detachedCriteria::add);
		
		
	criteria.add(Subqueries.exists(detachedCriteria));
}

private Optional<Stream<Criterion>> handleHasOrParam(HasOrListParam hasOrListParam) {
	if (hasOrListParam == null) {
		return Optional.empty();
	}

	return Optional.of(hasOrListParam.getValuesAsQueryTokens().stream().map(this::handleHasParam).filter(
			Optional::isPresent).map(Optional::get));
}

private Optional<Criterion> handleHasParam(HasParam hasParam) {
	if (hasParam == null) {
		return Optional.empty();
	}
		
	if (!FhirConstants.OBSERVATION.equals(hasParam.getTargetResourceType())) {
		return Optional.empty();
	}
}

(I've left out the mapping of the fields to the right criteria, but hopefully this outline is useful).

@gitcliff
Copy link
Contributor Author

@ibacher thanks for the outline ,,,
i have added the field mapping

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

@gitcliff Sorry it took me so long to add some comments here. I've tried to do so, if you're willing to take another look. One thing that's clearly missing here is some tests for the methods in question, which would be ideal.

@gitcliff gitcliff force-pushed the FM2-347 branch 2 times, most recently from 44ae398 to 02ffea8 Compare June 8, 2021 08:12
@gitcliff
Copy link
Contributor Author

gitcliff commented Jun 8, 2021

hello @ibacher,,, i added some unit tests

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

@gitcliff Thanks for the update on this. I've provided some updated thoughts on the central function to this account. Weirdly, the unit tests still don't seem to be actually using this new code. Maybe a good idea to look into why that is. Anyway, thanks a lot! 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants