Adding test for Obsgroup. #565

parthfloyd · 2025-05-27T14:20:22Z

Description of what I changed

Added test case & data for Observation Group (FHIR Observation with hasMember)

Issue I worked on

see https://openmrs.atlassian.net/browse/FM2-661

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 --amend
I 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 --amend
[x ] I ran mvn clean package right before creating this pull request and
added 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

ibacher · 2025-05-27T19:56:02Z

So the failure I see here isn't related to locale but to this:

Error: org.openmrs.module.fhir2.providers.r4.ObservationFhirResourceProviderIntegrationTest.shouldCreateNewObservationGroupAsJson -- Time elapsed: 0.185 s <<< FAILURE!
java.lang.AssertionError:

Expected: response with HTTP status <201>
but: response with status code <500> with message "HAPI-0389: Failed to call access method: org.openmrs.api.UnchangeableObjectException: editing.fields.not.allowed"

ibacher · 2025-05-27T20:06:38Z

I kind of suspect that the issue here is that the code is trying to change the existing obs (to make it a group member) rather than the usual pattern of void-and-duplicate the obs and that the loop its stuck in (I can reproduce that) has nothing to do with "locales", but more to do with continuous throwing the UnchangeableObjectException, which tries to translate the message, triggering the getGlobalProperty() call, triggering an attempt to flush the obs and so on and so on until you get the stack overflow.

However, no amount of jiggering with locales will fix that.

- for child observation voided and added new obs as group member. - draft for saving child obsv.

parthfloyd · 2025-06-02T06:34:05Z

Thanks @ibacher I tried the approach of voiding and creating new instance of the child observation & adding it to observation group. Which allowed me to create Obs Group.

However I faced difficulty when trying to save the new child observation (threw same stack overflow error) & since the child observation is not created or saved It is not in the database, this is not working as intended.

I have updated test case to show the behavior (Trying to get the child observation it is not found).

Do you've any suggestion on how we can save the new child observation w/o failing?

api/src/main/java/org/openmrs/module/fhir2/api/util/ObsGroupHelper.java

ibacher · 2025-06-09T18:40:45Z

api/src/main/java/org/openmrs/module/fhir2/api/util/ObsGroupHelper.java

+//      @TODO: save new observation, currently this is failing
+//		observationDao.createOrUpdate(newObservation);
+//      obsService.saveObs(newObservation, "created while translating observation after adding to Obs Group");


Right, you can't save the child observation by itself. Don't do this. Save the obsGroup itself.

Thanks Ian, however when I'm just saving the Obsgroup, it is not saving the newObervation (the new instance(with obsGroup) of the old child observation)

ibacher · 2025-06-09T18:42:46Z

api/src/main/java/org/openmrs/module/fhir2/api/translators/impl/ObservationTranslatorImpl.java

 		for (Reference reference : observation.getHasMember()) {
-			existingObs.addGroupMember(observationReferenceTranslator.toOpenmrsType(reference));
+			Obs childObservation = observationReferenceTranslator.toOpenmrsType(reference);
+			if (childObservation.getObsGroup() == null) {


Either way, if the observation exists, we need to void it and add it to the obs group. I'm not sure that we need this if... else here. I also think moving things into the ObsGroupHelper probably isn't helping....

The real question, though, is what happens if childObservation is null (because it couldn't be converted?

You're right, I mistakenly thought about creation vs retrieval but that won't be case when we're translating to OpenMRS

This is a good question.
In that case shall we:

Not create the Obsgroup (In this case user can re send the request once they've the child observation fixed/created on server).

Create Obsgroup without the null childObservation (This could result in inconsistent data, think It'd make it tricky to handle as user then needs to update this obsgroup once child observation is fixed).

Let me know which option makes more sense or if we need more input, can send this to Slack or on OpenMRS Talk.

ibacher · 2025-06-09T18:53:46Z

...ources/org/openmrs/module/fhir2/api/dao/impl/FhirObservationDaoImplTest_initial_data_2.1.xml

 	<concept_numeric concept_id="5089" hi_normal="250.0" low_normal="0.0" units="kg" allow_decimal="false" display_precision="10" />
 	<concept_numeric concept_id="5090" hi_normal="272.0" low_normal="10.0" units="cm" allow_decimal="false" display_precision="10" />
-	<concept_numeric concept_id="5085" hi_normal="250.0" low_normal="0.0" units="kg" allow_decimal="false" display_precision="10" />
+	<concept_numeric concept_id="5085" hi_normal="250.0" low_normal="0.0" units="mmHg" allow_decimal="false" display_precision="10" />


I don't really understand the unit change here?

This was part of this commit (37d4684), i assume it fixed something during testing?

ibacher · 2025-06-09T18:54:01Z

...ources/org/openmrs/module/fhir2/api/dao/impl/FhirObservationDaoImplTest_initial_data_2.1.xml

 	<fhir_concept_source fhir_concept_source_id="3" name="LOINC" url="http://loinc.org" concept_source_id="6" creator="1" date_created="2005-01-01 00:00:00.0" retired="0" uuid="30a5aa84-2df5-46da-aed7-451bafe5593b" />
 	<fhir_observation_category_map observation_category_map_id="1" concept_class_id="1" observation_category="laboratory" creator="1" date_created="2005-01-01 00:00:00.0" retired="0" uuid="7117a470-0d6c-4bae-b708-c951bccf1133"/>
 	<fhir_observation_category_map observation_category_map_id="2" concept_class_id="8" observation_category="laboratory" creator="1" date_created="2005-01-01 00:00:00.0" retired="0" uuid="d2cbb346-9404-477f-b3bd-f099a6ce41c1"/>
-	<concept concept_id="5085" retired="false" datatype_id="1" class_id="1" is_set="false" creator="1" date_created="2004-08-12 00:00:00.0" version="" changed_by="1" date_changed="2005-02-25 11:43:43.0" uuid="5085AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"/>


I'm also unsure why we're dropping the datatype?

gracepotma · 2025-09-12T14:07:25Z

Hi @parthfloyd I see this ticket is marked as "Draft" - If it's ready for review, please kindly update to "Ready for review" :)

Thank you for your continued contributions to FHIR on OpenMRS! 🥇

parthfloyd · 2025-09-15T01:38:24Z

Hi @parthfloyd I see this ticket is marked as "Draft" - If it's ready for review, please kindly update to "Ready for review" :)

Thank you for your continued contributions to FHIR on OpenMRS! 🥇

Thanks @gracepotma for your time on this! It is still WIP.
To continue development on this I need some feedback/answers on the conversation above & then push changes for this PR to be merged.

dkayiwa · 2025-09-22T22:04:29Z

@parthfloyd did you also get a chance to look at the build failure?

- created test to find issue for obsgroup

372d2f0

Fix some minor issues

37d4684

removed value for obs group

455546d

- for child observation voided and added new obs as group member. - draft for saving child obsv.

ibacher reviewed Jun 9, 2025

View reviewed changes

Adding test for Obsgroup. #565

Are you sure you want to change the base?

Adding test for Obsgroup. #565

Uh oh!

Conversation

parthfloyd commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

Uh oh!

ibacher commented May 27, 2025

Uh oh!

ibacher commented May 27, 2025

Uh oh!

parthfloyd commented Jun 2, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gracepotma commented Sep 12, 2025

Uh oh!

parthfloyd commented Sep 15, 2025

Uh oh!

dkayiwa commented Sep 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

parthfloyd commented May 27, 2025 •

edited

Loading