As per the current code voiding an order doesn’t mark it as dirty, this was intentional with the assumption that developers should call voidObs() instead
It seems like we need to decide on our desired API behavior as far as trying to edit an obs inside an encounter.
@preethi_s and @angshuonline, can you please share in a bit more detail what Bahmni needs to be able to do via the encounter transaction REST API? Angshu comments [quote=“angshuonline, post:15, topic:7925”] 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) [/quote]
and this makes me think you’d like to be able to do the equivalent of this JavaScript:
var encounter = getJson(".../encounter/123");
encounter.obs[3].value = "this is a changed value";
postJson(encounter);
Is that right? Wyclif is suggesting we shouldn’t allow this, and I guess that would lead to needing to do something like
var encounter = getJson(".../encounter/123");
var newObs = copyObsForEditing(encounter.obs[3]);
newObs.value = "this is a changed value";
encounter.obs.push(newObs);
encounter.obs[3].voided = true;
postJson(encounter);
Ideally this can be discussed briefly at the start of tomorrow’s design forum (which will be very late for India, so best if we already know what you think!).
What I’m saying is to allow this and it is already supported
var encounter = getJson(".../encounter/123");
encounter.obs[3].value = "this is a changed value";
postJson(encounter);
From the code above, the obs will get voided and replaced with a new one with the new value.
We should discourage this
var encounter = getJson(".../encounter/123");
encounter.obs[3].voided = true;// This applies for voidedBy, dateVoided, voidReason
postJosn(encounter);How would we support it then, especially if the client wants to make multiple changes to an encounter atomically via REST? If we support changing an obs as above, we should support one or both of these as well:
var encounter = getJson(".../encounter/123");
encounter.obs[3].value = "changed value"; // should void obs 3 and create a new one
// option 1
encounter.obs[5].voided = true;
// option 2
encounter.obs.splice(5, 1);
// end options
postJson(encounter);
In Bahmni, we will have scenarios to add new obs, edit obs and void obs (can be multiple obs) via encounter transaction.
Encounter encounter = findOrCreateEncounter();
//create obs
Obs obs1 = new Obs();
obs1.setValue("new obs");
encounter.addObs(obs1);
//edit obs
Obs obs2 = encounter.getObs().iterator().next();
obs2.setValue("changed value");
//void existing obs
Obs obs3 = encounter.getObs().iterator().next().next();
obs.setVoided(true); // if it has group members, will explicitly void them as well
encounterService.saveEncounter(encounter);
We dont call voidObs() method, instead we set the voided flag and expect them to get saved while saving enocunter.
I would think irrespective of whether it is create, edit or void scenario, we should be clearing the dirty flag once the obs is saved to the database.
I do believe our current logic saves voided Obs in an encounter. As I mentioned earlier setting Obs.voided to true doesn’t make it dirty because anyways it’s getting ‘deleted’, I think we might want to tweak HibernateObsDao.save to discard any other changes to a voided Obs so that in the event of unvoiding it any other changes that were made to it when getting voided don’t take effect otherwise it would effectively by pass our logic that ensures that Obs never get changed.
Adding to what I said in my previous comment, actually changing this to return false would resolve the issue where an Obs is voided with other changes and then unvoided later
Hi,
@preethi_s or @angshuonline can one of you please write up and share a unit test of the issue you’re running into? I.e Where you create a new Obs, associate it to an encounter but when you save the encounter the Obs gets voided and replaced with a new copy. I can’t seem to reproduce this in a unit test. Thanks!
What I can reproduce is when you load an existing Obs from the DB, edit and then call saveObs(), the newObs with the changes you get back is marked as dirty meaning that if you call saveObs(newObs) the new Obs gets voided and replaced even if you made no changes to it which is a bug.
@wyclif We tried reproducing that issue (obs being auto flushed before it is meant to save) by creating a test in openmrs-core but we were not able to succeed. This was reproducible only in Bahmni tests as I mentioned in one of the previous replies. https://github.com/Bahmni/bahmni-core/blob/openmrs_2.0/bahmni-emr-api/src/test/java/org/openmrs/module/bahmniemrapi/encountertransaction/impl/BahmniEncounterTransactionServiceImplIT.java#L261
Also this would get solved once we clear the dirty flag after saving the obs to the database.
When I look at the pom file I see that both the bahmni and emrapi module are setup to compile and run against older platform versions and not 2.0.
My proposed fix is actually to only reset the dirty flag if an Obs has an ObsId i.e an existing one that has already been saved to the DB
@wyclif, why are you opposed to resetting the dirty flag in other situations? I think that dirty=true means “has changes that have not yet been written to the DB”, so it makes sense to always clear that flag after whatever saving logic has happened.
I think that resetting the flag for an updated Obs promotes bad practice, see the snippet below:
1 ObsService os = Context.getObsService();
2 Obs originalObs = os.getObs(1)
3 originalObs.setComment("new comment");
4 Obs savedObs1 = os.saveObs(originalObs, "some reason");
//this will actually result in an contraint violation because obs.previousObs has a unique key constraing
5 //originalObs.setComment("Another different comment");
6 Obs savedObs2 = os.saveObs(originalObs, "another reason");
Working with the originalObs after line 4 above is miss using the API because it’s stale and detached from the hibernate session, at that point there’s another updated instance that is already saved to the DB and is voided which is not the case for the stale copy, infact the stale copy has the wrong values and should be discarded by the client code. If you uncomment line 5, the call to saveObs() on line 6 will still fail anyways because of some unique key constraint Obs.previousObs since you can’t have a single Obs referenced by more than one Obs to be the previous.
As for new Obs, updating Obs.markDirty() to skip obs with a null obsId solves any issues even if hibernate auto flushes it since it’s dirty flag would still be false anyways.