Should editing the child obs make its parent dirty?

bharatak · September 22, 2016, 2:35pm

@wyclif the test passes and that is concerning. Can you please try once on your side? It relies on plain BaseModuleContextSensitiveTest. You can copy the testdata and run the test from openmrs-core. Even I am concerned

wyclif · September 22, 2016, 3:07pm

But from that test dataset, encounter with id 7 has no Obs since they are all commented out therefore the first assertion fails anyways, did you tweak the test data? Or do I have to include some other extra test dataset?

bharatak · September 22, 2016, 3:30pm

The test data is added as part of the setup(). You can see here

wyclif · September 22, 2016, 3:48pm

Yes I had seen that and had already included that test dataset but as you can see in that dataset, encounter with id 7 has no obs added to it

bharatak · October 5, 2016, 1:12pm

As part of the fix for this issue, we have done 2 things

Removed setting the dirty flag when obs.setGroupMembers(), obs.addGroupMember() and obs.removeGroupMember() is called. As making the parent dirty on adding children will not have any impact as per our discussion. To clarify, making the obs dirty on updating/deleting its children will technically not really do anything to the parent.
EncounterService.saveEncounter() will use encounter.getObsAtTopLevel() instead of encounter.getAllObs() so that we just pick up the top level obs. Saving a top level obs using obsService.saveObs() will take care of individually saving the leaf obs. If the leaf obs is dirty, then we create a copy of the obs with latest values and then link the old obs to the new obs. This is the as-is behavior as of 2.0

The PR is available here. As @wyclif pointed out in the PR, just want to bring it to discussion here if this approach is fine.

darius · October 5, 2016, 1:59pm

My understanding from our discussions earlier in this thread (i.e. here) we already decided that adding/removing/editing a child should not mark the parent dirty. Is this under discussion again?

wyclif · October 5, 2016, 3:14pm

My understanding was that editing should not mark the parent as dirty but adding/removing should. Did I miss some call? I recall there is a Monday I was out of office may be I missed something.

bharatak · October 5, 2016, 4:30pm

Hi @wyclif There was nothing specifically discussed about it on Monday. My view is that adding/removing an obs will not have any change on the parent side that demands tracking for audit (dirty flag). Its on child side that we update its obs_group_id (either set it on addition or clear it on removal). So, the child becomes dirty but not the parent and it needs audit. This is in conformance to what @burke pointed.

wyclif · October 5, 2016, 5:35pm

That comment from @burke’s says what should happen when a child is edited which I agree with but it’s silent about adding/removing, I’ve heard from Burke before by mouth say that adding/removing changes the meaning of an obs grouping and should mark the parent as dirty, may be we can wait for him to comment. I’m sorry if am being adamant, I just want to be sure and not waste your time to make changes that I think we might end up throwing away.

darius · October 5, 2016, 6:56pm

I remember discussing this, but I can’t find it written down anywhere…

I think that if you add/remove a child obs from an obs group we do not need to void-and-recreate the entire group.

The argument for void-and-recreate in this scenario is to be able to indicate that an obs group “changes meaning” when you add/remove a child member. But in practice doing a void-and-recreate would make the audit trail much harder to follow (you’d have to do a deep comparison of the new group vs the prior voided group to understand what’s going on). If instead we take the approach Bharat has submitted code for, you merely have to compare the timestamps of the parent vs children to see what’s going on.

@burke, can you confirm you are okay with this behavior?

burke · October 6, 2016, 2:03pm

This is my understanding. Given a parent obs (P) and children obs (C1, C2), I show two examples of adding a 3^rd child (+C3) and modifying the 2^nd child (+C2’). Voided obs are faded and stamped.

What we currently have

Adding a child voids & recreates the parent and all its children:

Modifying a child voids & recreates the parent and all its children:

What we would ideally do

Adding a child also makes the parent dirty:

Modifying a child also makes the parent dirty:

But our data model doesn’t allow us to have more than one link from a child to parent obs (i.e., to support the “historic” links demonstrated by the dashed relationships above).

The current compromise (not making the parent dirty)

Adding a child does not make the parent dirty:

Modifying a child does not make the parent dirty:

I believe @wyclif is asking for the ideal approach (that I’d love to have, but I don’t think our model supports) and ThoughtWorks has submitted a PR for the compromise.

If I’m understanding this correctly, then I support accepting the compromise, since our current model can’t support the ideal approach. Let me know if my understanding is incorrect.

wyclif · October 6, 2016, 3:37pm

Thanks @burke, I’ll go ahead and merge the PR if everything is fine

bharatak · October 6, 2016, 4:37pm

Thanks @burke for clarifying.

preethi.s · October 9, 2016, 4:58pm

@wyclif Few doubts in code related to saving group members. Can you throw some light on this?

Once a dirty groupmember is saved, we remove the group member from parent collection and add the new group member to the collection via code. Here Why not refresh the parent entity from db instead of updating the parent in hibernate session cache via removeGroupMember and addGroupMember?

github.com

openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/api/impl/ObsServiceImpl.java#L199




	private Obs saveObsNotDirty(Obs obs, String changeMessage) {
		if(!obs.isObsGrouping()){
			return obs;
		}


		ObsService os = Context.getObsService();
		boolean refreshNeeded = false;
		for (Obs o : obs.getGroupMembers(true)) {
			if (o.getId() == null) {
				os.saveObs(o, null);
			} else {
				Obs newObs = os.saveObs(o, changeMessage);
				refreshNeeded = !newObs.equals(o) || refreshNeeded;
			}
		}


		if(refreshNeeded) {
			Context.refreshEntity(obs);
		}
		return obs;

One problem due to this behaviour is, when the groupmember is not really dirty, both replacement and old obs are same and in the process of removing and adding the groupmember to parent, groupmember becomes dirty since we unset and set the obs group in it.

wyclif · October 10, 2016, 2:11pm

Yes, it seems to make sense to evict the parent and reload it

wyclif · October 10, 2016, 6:00pm

Just to clarify, yes we can get rid of the logic that removes the voided child obs and adds their replacements by reloading the parent obs instance but the only downside is that we will then be returning a new instance of the parent obs and the caller has a stale one, I would also assume we have to evict the parent from the session before we re-read it.

bharatak · October 11, 2016, 12:03am

Hi, Just wanted to clarify on the behavior of removing a child from parent obs. Following Burke’s style of images for clarity.

When a child C2 is removed from P using P.removeGroupMember(C2), C2’s obs_group is set to null
While saving this obs, we clone it to a new version of this obs as here. But as C2 obs_group is null, the C2’ is not associated to the parent. But it still has its encounter association. So, it will now become a top level obs.
Now, while voiding C2 as part of the last step, we evict the C2 from session to get a fresh copy and then void. Because of the eviction, we are loosing the change of obs_group being set to null on C2. The resultant behavior is that it is still associated to P but it is voided.

Is this an accepted behavior?

darius · October 11, 2016, 12:29am

This sounds like the correct behavior to me.

I.e. the newly created one that results from removing an OBS from a group (I.e. voiding it) is now a top level OBS on the encounter.

(Do we know where this method is used, actually? I don’t like it, conceptually.)

burke · October 11, 2016, 1:26am

Overall this sounds right, but I’m trying to get my head around the use case(s). Technically speaking, I’d expect a child obs removed from a parent to move one step up the tree (become a sibling of the parent). 90% of the time, this will behave as @bharatak describes; however, if the parent itself has a parent, then I’d expect its removed child to become a child of that parent (it’s grandparent) rather than skip all the way to the top level. In other words, if P has a parent, then C2' becomes a child of that parent; otherwise, C2' has no parent. Probably an edge case, but thought’s I’d mention it.

bharatak · October 11, 2016, 1:36am

@burke trying to understand in what scenarios this might be useful. This behavior was not implemented in openmrs till now. All we do is mark the obsGroup of C2 as null instead of pointing it to C2’s grand parent. Its technically possible to do, but curious on what scenarios this might be useful?

Also, is it a good idea to take it outside the scope of the current PR and play as a separate jira issue just to avoid the creep. But I agree that it is a good discussion item.