Fix for editing a child obs makes the parent obs dirty and creates a …

…new instance.

api/src/main/java/org/openmrs/Obs.java

@@ -440,13 +440,6 @@ public Set<Obs> getGroupMembers(boolean includeVoided) {
 	 * @should not mark the obs as dirty when the set is replaced with another with same members
 	 */
 	public void setGroupMembers(Set<Obs> groupMembers) {
-		if (CollectionUtils.isNotEmpty(this.groupMembers) && CollectionUtils.isNotEmpty(groupMembers)) {
-			setDirty(!CollectionUtils.disjunction(this.groupMembers, groupMembers).isEmpty());
-		} else if (CollectionUtils.isEmpty(this.groupMembers) && CollectionUtils.isNotEmpty(groupMembers)) {
-			setDirty(true);
-		} else if (CollectionUtils.isNotEmpty(this.groupMembers) && CollectionUtils.isEmpty(groupMembers)) {
-			setDirty(true);
-		}
 		this.groupMembers = new HashSet<Obs>(groupMembers); //Copy over the entire list
 
 	}
@@ -477,9 +470,7 @@ public void addGroupMember(Obs member) {
 		}
 
 		member.setObsGroup(this);
-		if (groupMembers.add(member)) {
-			setDirty(true);
-		}
+		groupMembers.add(member);
 	}
 
 	/**
@@ -499,7 +490,6 @@ public void removeGroupMember(Obs member) {
 
 		if (groupMembers.remove(member)) {
 			member.setObsGroup(null);
-			setDirty(true);
 		}
 	}
 
@@ -1279,10 +1269,4 @@ private void markAsDirty(Object oldValue, Object newValue) {
 			dirty = true;
 		}
 	}
-
-	private void setDirty(Boolean dirty){
-		if(obsId != null){
-			this.dirty = dirty;
-		}
-	}
 }

api/src/main/java/org/openmrs/api/impl/EncounterServiceImpl.java

@@ -9,15 +9,6 @@
  */
 package org.openmrs.api.impl;
 
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Date;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-import java.util.Vector;
-
 import org.apache.commons.lang.StringUtils;
 import org.openmrs.Cohort;
 import org.openmrs.Encounter;
@@ -51,6 +42,15 @@
 import org.openmrs.util.PrivilegeConstants;
 import org.springframework.transaction.annotation.Transactional;
 
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Date;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.Vector;
+
 /**
  * Default implementation of the {@link EncounterService}
  * <p>
@@ -202,7 +202,7 @@ public Encounter saveEncounter(Encounter encounter) throws APIException {
 		ObsService os = Context.getObsService();
 		List<Obs> toRemove = new ArrayList<>();
 		List<Obs> toAdd = new ArrayList<>();
-		for (Obs o : encounter.getAllObs(true)) {
+		for (Obs o : encounter.getObsAtTopLevel(true)) {
 			if (o.getId() == null) {
 				os.saveObs(o, null);
 			} else {

api/src/main/java/org/openmrs/api/impl/ObsServiceImpl.java

@@ -187,11 +187,11 @@ private Obs saveObsNotDirty(Obs obs, String changeMessage) {
 				os.saveObs(o, null);
 			} else {
 				Obs replacement = os.saveObs(o, changeMessage);
-				//The logic in saveObs evicts the old obs instance, so we need to update
-				//the collection with the newly loaded and voided instance
-				toRemove.add(o);
-				toAdd.add(os.getObs(o.getId()));
-				toAdd.add(replacement);
+				if(!replacement.equals(o)){
+					toRemove.add(o);
+					toAdd.add(os.getObs(o.getId()));
+					toAdd.add(replacement);
+				}
 			}
 		}
 

api/src/test/java/org/openmrs/ObsTest.java

@@ -858,7 +858,7 @@ public void addGroupMember_shouldReturnFalseWhenADuplicateObsIsAddedAsAMember()
 		Obs obs = new Obs(2);
 		Obs member = new Obs();
 		obs.addGroupMember(member);
-		assertTrue(obs.isDirty());
+		assertFalse(obs.isDirty());
 		resetObs(obs);
 		obs.addGroupMember(member);
 		assertFalse(obs.isDirty());
@@ -881,18 +881,18 @@ public void addGroupMember_shouldReturnFalseWhenADuplicateObsIsAddedAsAMemberToN
 
 	/**
 	 * @see Obs#addGroupMember(Obs)
-	 * @verifies return true when a new obs is added as a member
+	 * @verifies return isDirty false when a new obs is added as a member
 	 */
 	@Test
-	public void addGroupMember_shouldReturnTrueWhenANewObsIsAddedAsAMember() throws Exception {
+	public void addGroupMember_shouldReturnFalseWhenANewObsIsAddedAsAMember() throws Exception {
 		Obs obs = new Obs(2);
 		Obs member1 = new Obs();
 		obs.addGroupMember(member1);
-		assertTrue(obs.isDirty());
+		assertFalse(obs.isDirty());
 		resetObs(obs);
 		Obs member2 = new Obs();
 		obs.addGroupMember(member2);
-		assertTrue(obs.isDirty());
+		assertFalse(obs.isDirty());
 	}
 
 	/**
@@ -908,22 +908,22 @@ public void removeGroupMember_shouldReturnFalseWhenANonExistentObsIsRemoved() th
 
 	/**
 	 * @see Obs#removeGroupMember(Obs)
-	 * @verifies return true when an existing obs is removed from the group
+	 * @verifies return isDirty as false when an existing obs is removed from the group
 	 */
 	@Test
-	public void removeGroupMember_shouldReturnTrueWhenAnObsIsRemoved() throws Exception {
+	public void removeGroupMember_shouldReturnDirtyFalseWhenAnObsIsRemoved() throws Exception {
 		Obs obs = new Obs(2);
 		Obs member = new Obs();
 		obs.addGroupMember(member);
-		assertTrue(obs.isDirty());
+		assertFalse(obs.isDirty());
 		resetObs(obs);
 		obs.removeGroupMember(member);
-		assertTrue(obs.isDirty());
+		assertFalse(obs.isDirty());
 	}
 
 	/**
 	 * @see Obs#removeGroupMember(Obs)
-	 * @verifies return false when an new obs is removed from the group
+	 * @verifies return isDirty false when an new obs is removed from the group
 	 */
 	@Test
 	public void removeGroupMember_shouldReturnFalseForDirtyFlagWhenAnObsIsRemovedFromGroup() throws Exception {
@@ -938,16 +938,16 @@ public void removeGroupMember_shouldReturnFalseForDirtyFlagWhenAnObsIsRemovedFro
 
 	/**
 	 * @see Obs#setGroupMembers(Set)
-	 * @verifies mark the existing obs as dirty when the set is changed from null to a non empty one
+	 * @verifies do not mark the existing obs as dirty when the set is changed from null to a non empty one
 	 */
 	@Test
-	public void setGroupMembers_shouldMarkTheExistingObsAsDirtyWhenTheSetIsChangedFromNullToANonEmptyOne() throws Exception {
+	public void setGroupMembers_shouldNotMarkTheExistingObsAsDirtyWhenTheSetIsChangedFromNullToANonEmptyOne() throws Exception {
 		Obs obs = new Obs(5);
 		assertNull(Obs.class.getDeclaredField("groupMembers").get(obs));
 		Set members = new HashSet<>();
 		members.add(new Obs());
 		obs.setGroupMembers(members);
-		assertTrue(obs.isDirty());
+		assertFalse(obs.isDirty());
 	}
 
 	/**
@@ -966,10 +966,10 @@ public void setGroupMembers_shouldNotMarkNewObsAsDirtyWhenTheSetIsChangedFromNul
 
 	/**
 	 * @see Obs#setGroupMembers(Set)
-	 * @verifies mark the existing obs as dirty when the set is replaced with another with different members
+	 * @verifies do not mark the existing obs as dirty when the set is replaced with another with different members
 	 */
 	@Test
-	public void setGroupMembers_shouldMarkTheExistingObsAsDirtyWhenTheSetIsReplacedWithAnotherWithDifferentMembers()
+	public void setGroupMembers_shouldNotMarkTheExistingObsAsDirtyWhenTheSetIsReplacedWithAnotherWithDifferentMembers()
 	    throws Exception {
 		Obs obs = new Obs(2);
 		Set members1 = new HashSet<>();
@@ -979,7 +979,7 @@ public void setGroupMembers_shouldMarkTheExistingObsAsDirtyWhenTheSetIsReplacedW
 		Set members2 = new HashSet<>();
 		members2.add(new Obs());
 		obs.setGroupMembers(members2);
-		assertTrue(obs.isDirty());
+		assertFalse(obs.isDirty());
 	}
 
 	/**