Openmrs 2.0 - Saving visit saves the encounters under it but not the obs of the encounters

Since openmrs 2.0, VisitService.saveVisit saves the visit and its encounter but does not save the obs in the encounter. The testcase which fails for the above scenario.

    public void saveVisit_shouldPersistNewEncounter() throws Exception {
		
		VisitService vs = Context.getVisitService();
		Visit visit = vs.getVisit(1);
		
		Encounter encounter = new Encounter();
		encounter.setEncounterDatetime(new Date());
		encounter.setEncounterType(Context.getEncounterService().getEncounterType(2));
		encounter.setPatient(visit.getPatient());
		encounter.setLocation(visit.getLocation());
		Obs observation = new Obs();
		observation.setConcept(Context.getConceptService().getConcept(3));
		observation.setValueText("ksjfklshj");
		encounter.addObs(observation);
		visit.addEncounter(encounter);
		
		vs.saveVisit(visit);
		
		Context.flushSession();
		Context.clearSession();
		
		Integer encounterId = encounter.getEncounterId();
		
		// reload the visit
		visit = Context.getVisitService().getVisit(1);

		assertEquals(1, visit.getEncounters().size());
		Encounter savedEncounter = (Encounter) visit.getEncounters().toArray()[0];
		assertFalse(savedEncounter.getAllObs().isEmpty());
		assertEquals(encounterId, savedEncounter.getEncounterId());
	}

We figured out that it was due to removing ‘cascade=all’ in Encounter.hbm.xml and Obs.hbm.xml. Links:

I assume this new behavior was introduced as part of TRUNK-50. (I’m on a call now so I haven’t had a chance to read and figure out if this is intentional or not.)

(Just to check, you’re saying that this test passes against openmrs-core 1.12, but fails against openmrs-core 2.0?)

VisitServiceImpl.saveVisit is what needs to be updated to loop over all encounters in the visit and explicitly invoke saveEncounter() for each, that way the same will happen to all contained obs for each encounter, do you mind creating a ticket to get it fixed? The current work around is to probably add some after advice when saveVisit returns that loops over all the contained encounters and saves them or alternatively do it from a custom VisitSaveHandler in a module but with this approach the save operation cascades to the encounters before rather than after the visit is actually saved.

@darius I modified the test locally in 2.0.x branch to fail for this usecase. cascade=all is present in openmrs 1.12.0 and it works fine. @wyclif yes, I will create the ticket.

@wyclif Also the group members of an obs are not saved when parent obs is saved.

Issue created: https://issues.openmrs.org/browse/TRUNK-4937

Thanks for creating the ticket. Because visit.encounters aren’t getting saved of course group members also don’t get saved but once we fix the encounters to get saved in VisitService.saveVisit, everything else will automatically get saved.

@wyclif What I meant is, when I tried adding cascade=all in Encounter.hbm.xml (just to check whether that is the issue), parent obs got saved but the group members are not saved because cascade=all is changed to cascade=delete in groupMembers field in Obs.hbm.xml. Just pointing it out, so that both the issues can be taken care.

Things changed in 2.0, setting cascade to all isn’t really what you should be using to test if they are getting saved or not, first you need to know how the API treats edited obs, the key thing is not to have save operations to cascade to the group members as such but rather to have the desired behavior happen which is to explicitly save new obs while voiding and replacing existing obs that have been edited. And there is unit tests which ensure that when a parent is saved, new groups members get saved while existing ones get voided and replaced with new copies in case they were edited otherwise these tests would be failing if it wasn’t happening.

@wyclif I went through the issue TRUNK-50. So, from OpenMRS 2.0, any edits on the obs will void it and create a new obs. Is this the model that openmrs suggesting now? Also we are facing some issues around the obsService.saveObs which deals with the ‘dirty’ flag.

We create a new obs like below and attach it to the encounter:

Obs observation = new Obs();
observation.setConcept(concept);
observation.setValueNumeric(10);

now the observation.isDirty() will be true (though it is a new obs). When this obs is saved via encounter, the obs is somehow saved in this line itself (may be due to flush in the session).

Now the obs is dirty and saved as well. When it goes inside the loop, it assumes that obs has to be voided since it is dirty and creating the new obs.

Now how much ever time I call the encounterService.save, the obs will be voided so many times because when you clone a new obs out of the obs to be voided, dirty flag is not reset, it is true in the new obs as well.

It has always been the case that when an Obs is edited it gets voided and replaced with a new one only that it wasn’t caught in some cases, so TRUNK-50 was meant to fix it so that the same behavior is achieved for obs saved along with an encounter. When saveObs() is called, the code checks if an obs is new or existing, so the isDirty() flag is ignored for new a Obs

I wouldn’t expect to be creating or updating encounters and obs by a single operation to a visit. If we want to have a convenience method that, given a visit + encounter, will create the visit and then create the encounter linked the visit, that’s fine. But visits can span days, weeks, even months and comprise hundreds of encounters (each with a hundred observations).

I agree that ‘isDirty()’ flag is ignored for a new obs. But in the above mentioned scenario, the new obs is saved much before it is meant to be saved in the below line:

So when it reaches the line 209, it is considered as editing the obs and since it has dirty flag as true, it is voiding and creating a new obs again.

The test scenario which is failing in Bahmni: https://github.com/Bahmni/bahmni-core/blob/openmrs_2.0/bahmni-emr-api/src/test/java/org/openmrs/module/bahmniemrapi/encountertransaction/impl/BahmniEncounterTransactionServiceImplIT.java#L253

Also shouldn’t the dirty flag be reset to false once the obs is saved to database? Or else when we try to save the encounter again after the obs is saved, it will void and duplicate the obs even though there is no change to the obs.

Bahmni most of the time uses the coarser grained EMR APIs, where the obs constructs are at much higher level. To the provider, the visits and encounters are opaque; all she is doing is capturing diagnosis, complaints or other observations or issuing order etc. So we do create a visit if required, create/update encounter and obs within. (and all that happens with a single API call to respect ACID and user experience)

What I understand, this is the scenario:

  1. Identify a visit if it is found or instantiate a new visit (not saved yet)
  2. Instantiate a new encounter and add to visit
  3. Instantiate new obs and add to encounter
  4. Save visit (which in turn saves the encounter but not the obs)
  5. Save the encounter (now it tries to save the obs)

during 5, we endup having 2 obs (one voided). Seems like when “Context.getMessageSourceService().getMessage()” is called, obs is saved (because hibernate is working on a dirty session and flushes the encounter and associated obs). However, the isDirty flag is still true; now in an persistent obs entity (obs.id not null) and we end up voiding this obs and creating another one.

Thanks for the explanations. I was misled by the title of the topic. So an unintended behavior introduced by TRUNK-50 is saving obs twice when an encounter is saved.

Yes, I agree.

MessageSourceService isn’t transactional therefore I would think the call to Context.getMessageSourceService().getMessage() can’t be the one causing the auto flash, I think step 5 is what may be problematic and you shouldn’t have to do it once we fix saveVisit to call saveEncounter for all encounters that way the flush happens for everything in one single logical outer most transaction after saveVisit returns.

With that said, I do see value in resetting the isDirty flag every time a new Obs is saved to the database, a dev shouldn’t have to know and code around under the hood behavioral complexities like this and besides hibernate auto flushes are always going to happen since we can’t predict how module developers will set up their services or how their custom domain objects associated to Obs are mapped for persistence.

As the code stands now, visit.encounters does cascade saves (see hbm.xml), though as of Platform 2.0 it does so incorrectly. So, while I agree that while in the long run we might want to avoid having visit cascade to encounters, and then to obs, this would be a change, and I suggest we don’t do it now.

Summarizing my understanding of this thread:

  • Bahmni was surprised by the change where saving an encounter or obsgroup doesn’t cascade to the obs inside it.
  • It is an intentional change to OpenMRS in platform 2.0 that saving an encounter/obsgroup doesn’t magically update child obs via Hibernate magic. Instead, editing an obs is supposed to void the original obs and save a new one. (This has always been the intended behavior, and the legacy UI and refapp work this way, as do the HTML Form Entry and XForms modules; it was an oversight that other behavior was allowed by the Java API and the encounter transaction REST API in the emrapi module.)
  • VisitService.saveVisit(Visit) cascades a Hibernate save of visit.encounters, but since this doesn’t call the logic in EncounterService.saveEncounter, the obs are not saved. [This is a bug.]
  • EncounterService.saveEncounter(Encounter):
  • should still save any modified obs (but not via hibernate magic). [This is working right.]
  • should only save each obs one time. [There’s a bug here.]
  • Saving an obs should clear the isDirty flag. [This is a bug.]

If I’m getting this right, I count 3 bugs to be cleaned up in openmrs-core, right? @preethi_s, can you (+/- others on the Bahmni team) do some or all of these fixes? Are you looking to @wyclif to do some, or to ensure that all relevant tickets are ready-for-work?

@darius I would think there is 2 bugs, the last bullet and the second sub bullet of the forth bullet are more less the same, the isDirty() flag should only have to be cleared when an Obs is inserted and not updated since Obs are immutable.

I don’t have an opinion about whether we should consider this 2 or 3 bugs. As long as we fix them. :slight_smile:

Wyclif, there are two branches of code, one saves a new obs, and the other voids the original and saves a copy. I would think that we need to clear the dirty flag in both cases, so that saving an encounter twice doesn’t create a growing number of voided obs.