openmrs · Jan 27, 2014 · Jan 28, 2014
diff --git a/api/src/main/java/org/openmrs/api/db/hibernate/AuditableInterceptor.java b/api/src/main/java/org/openmrs/api/db/hibernate/AuditableInterceptor.java
@@ -24,6 +24,7 @@
 import org.hibernate.type.Type;
 import org.openmrs.Auditable;
 import org.openmrs.OpenmrsObject;
+import org.openmrs.Person;
 import org.openmrs.api.context.Context;
 
 /**
@@ -38,6 +39,7 @@
  * 
  * @since 1.9
  */
+
 public class AuditableInterceptor extends EmptyInterceptor {
 
 	private static final Log log = LogFactory.getLog(AuditableInterceptor.class);
@@ -55,13 +57,20 @@ public class AuditableInterceptor extends EmptyInterceptor {
 	 *      java.lang.Object[], java.lang.String[], org.hibernate.type.Type[])
 	 */
 	@Override
-	public boolean onSave(Object entity, Serializable id, Object[] currentState, String[] propertyNames, Type[] types) {
-		return setCreatorAndDateCreatedIfNull(entity, currentState, propertyNames);
+	public boolean onSave(Object entity, Serializable id, Object[] entityCurrentState, String[] propertyNames, Type[] types) {
+		boolean objectWasChanged;
+		boolean personObjectWasChanged = false;
+
+		if (entity instanceof Person) {
+			personObjectWasChanged = setPersonCreatorAndDateCreatedIfNull(entity, entityCurrentState, propertyNames);
+		}
+		objectWasChanged = setCreatorAndDateCreatedIfNull(entity, entityCurrentState, propertyNames);
+		return objectWasChanged || personObjectWasChanged;
 	}
 
 	/**
 	 * This class method is only called when flushing an updated dirty object, not inserting objects
-	 * 
+	 *
 	 * @return true if the object got the changedBy and dateChanged fields set
 	 * @should set the dateChanged field
 	 * @should set the changedBy field
@@ -81,11 +90,50 @@ public boolean onFlushDirty(Object entity, Serializable id, Object[] currentStat
 			if (log.isDebugEnabled())
 				log.debug("Setting changed by fields on " + entity.getClass());
 
-			if (setValue(currentState, propertyNames, "changedBy", Context.getAuthenticatedUser(), false)) {
+			if (changePropertyValue(currentState, propertyNames, "changedBy", Context.getAuthenticatedUser(), false)) {
+				objectWasChanged = true;
+			}
+
+			if (changePropertyValue(currentState, propertyNames, "dateChanged", new Date(), false)) {
+				objectWasChanged = true;
+			}
+		}
+
+		if (entity instanceof Person && propertyNames != null) {
+			if (log.isDebugEnabled())
+				log.debug("Setting changed by fields on " + entity.getClass());
+
+			if (changePropertyValue(currentState, propertyNames, "personChangedBy", Context.getAuthenticatedUser(), false)) {
+				objectWasChanged = true;
+			}
+
+			if (changePropertyValue(currentState, propertyNames, "personDateChanged", new Date(), false)) {
+				objectWasChanged = true;
+			}
+		}
+
+		return objectWasChanged;
+	}
+
+	/**
+	 * Sets the personCreator and personDateCreated fields to the current user and the current time if they are
+	 * null.
+	 *
+	 * @return true if personCreator and personDateCreated were changed
+	 */
+
+	private Boolean setPersonCreatorAndDateCreatedIfNull(Object entity, Object[] currentState, String[] propertyNames) {
+		boolean objectWasChanged = false;
+
+		if (entity instanceof OpenmrsObject) {
+			if (log.isDebugEnabled())
+				log.debug("Setting personCreator and personDateCreated on " + entity);
+
+			if (changePropertyValue(currentState, propertyNames, "personCreator", Context.getAuthenticatedUser(), true)) {
 				objectWasChanged = true;
 			}
 
-			if (setValue(currentState, propertyNames, "dateChanged", new Date(), false)) {
+			if (changePropertyValue(currentState, propertyNames, "personDateCreated", new Date(), true)) {
 				objectWasChanged = true;
 			}
 		}
@@ -103,17 +151,18 @@ public boolean onFlushDirty(Object entity, Serializable id, Object[] currentStat
 	 * @return true if creator and dateCreated were changed
 	 */
 	private boolean setCreatorAndDateCreatedIfNull(Object entity, Object[] currentState, String[] propertyNames) {
+
 		boolean objectWasChanged = false;
 
 		if (entity instanceof OpenmrsObject) {
 			if (log.isDebugEnabled())
 				log.debug("Setting creator and dateCreated on " + entity);
 
-			if (setValue(currentState, propertyNames, "creator", Context.getAuthenticatedUser(), true)) {
+			if (changePropertyValue(currentState, propertyNames, "creator", Context.getAuthenticatedUser(), true)) {
 				objectWasChanged = true;
 			}
 
-			if (setValue(currentState, propertyNames, "dateCreated", new Date(), true)) {
+			if (changePropertyValue(currentState, propertyNames, "dateCreated", new Date(), true)) {
 				objectWasChanged = true;
 			}
 		}
@@ -131,15 +180,16 @@ private boolean setCreatorAndDateCreatedIfNull(Object entity, Object[] currentSt
 	 * @param setNullOnly
 	 * @return true if the property was changed
 	 */
-	private boolean setValue(Object[] currentState, String[] propertyNames, String propertyToSet, Object value,
+	private boolean changePropertyValue(Object[] currentState, String[] propertyNames, String propertyToSet, Object value,
 	        boolean setNullOnly) {
+
 		int index = Arrays.asList(propertyNames).indexOf(propertyToSet);
 
 		// HACK! When I apply the patch for TRUNK-2588, and then I try to start OpenMRS for the first time during the init wizard
 		// I get something like this:
 		/*
 		java.lang.NullPointerException
-		at org.openmrs.api.db.hibernate.AuditableInterceptor.setValue(AuditableInterceptor.java:140)
+		at org.openmrs.api.db.hibernate.AuditableInterceptor.changePropertyValue(AuditableInterceptor.java:140)
 		at org.openmrs.api.db.hibernate.AuditableInterceptor.onFlushDirty(AuditableInterceptor.java:83)
 		at org.openmrs.api.db.hibernate.ChainingInterceptor.onFlushDirty(ChainingInterceptor.java:77)
 		at org.hibernate.event.def.DefaultFlushEntityEventListener.invokeInterceptor(DefaultFlushEntityEventListener.java:331)
@@ -172,8 +222,10 @@ private boolean setValue(Object[] currentState, String[] propertyNames, String p
 		// END HACK
 
 		if (index >= 0) {
+
 			if (currentState[index] == null || !setNullOnly) {
 				if (!value.equals(currentState[index])) {
+
 					currentState[index] = value;
 					return true;
 				}

diff --git a/api/src/main/java/org/openmrs/api/handler/PersonSaveHandler.java b/api/src/main/java/org/openmrs/api/handler/PersonSaveHandler.java
@@ -43,28 +43,6 @@ public class PersonSaveHandler implements SaveHandler<Person> {
 	 */
 	public void handle(Person person, User creator, Date dateCreated, String other) {
 
-		// only set the creator and date created if they weren't set by the developer already
-		if (person.getPersonCreator() == null) {
-			person.setPersonCreator(creator);
-		}
-
-		if (person.getPersonDateCreated() == null) {
-			person.setPersonDateCreated(dateCreated);
-		}
-
-		// if there is an id already, we assume its been saved before and so set personChanged*
-		boolean hasId;
-		try {
-			hasId = person.getId() != null;
-		}
-		catch (UnsupportedOperationException e) {
-			hasId = true; // if no "id" to check, just go ahead and set them
-		}
-		if (hasId) {
-			person.setPersonChangedBy(creator);
-			person.setPersonDateChanged(dateCreated);
-		}
-
 		// address collection
 		if (person.getAddresses() != null && person.getAddresses().size() > 0) {
 			for (PersonAddress pAddress : person.getAddresses()) {

diff --git a/api/src/test/java/org/openmrs/api/PatientServiceTest.java b/api/src/test/java/org/openmrs/api/PatientServiceTest.java
@@ -3306,7 +3306,7 @@ public void savePatient_shouldSetThePreferredNameAddressAndIdentifierIfNoneIsSpe
 		address.setAddress1("some address");
 		patient.addAddress(address);
 
-		patientService.savePatient(patient);
+		Context.getPatientService().savePatient(patient);
 		Assert.assertTrue(identifier.isPreferred());
 		Assert.assertTrue(name.isPreferred());
 		Assert.assertTrue(address.isPreferred());

diff --git a/api/src/test/java/org/openmrs/api/PersonServiceTest.java b/api/src/test/java/org/openmrs/api/PersonServiceTest.java
@@ -201,6 +201,7 @@ private Patient createTestPatient() {
 		Set<PatientIdentifier> patientIdentifiers = new TreeSet<PatientIdentifier>();
 		patientIdentifiers.add(patientIdentifier);
 		patient.setIdentifiers(patientIdentifiers);
+
 		ps.savePatient(patient);
 		return patient;
 	}

diff --git a/api/src/test/java/org/openmrs/api/db/hibernate/AuditableInterceptorTest.java b/api/src/test/java/org/openmrs/api/db/hibernate/AuditableInterceptorTest.java
@@ -22,6 +22,9 @@
 import org.openmrs.Auditable;
 import org.openmrs.ConceptNumeric;
 import org.openmrs.User;
+import org.openmrs.Person;
+import org.openmrs.PersonAddress;
+import org.openmrs.PersonName;
 import org.openmrs.api.context.Context;
 import org.openmrs.api.context.Daemon;
 import org.openmrs.scheduler.Task;
@@ -91,6 +94,33 @@ public void onFlushDirty_shouldSetTheDateChangedField() throws Exception {
 		Assert.assertNotNull(currentState[1]);
 	}
 
+	@Test
+	public void onFlushDirty_shouldAddPersonChangedByForPerson() throws Exception {
+		AuditableInterceptor interceptor = new AuditableInterceptor();
+
+		Person person = new Person();
+
+		String[] propertyNames = new String[] { "personChangedBy" };
+		Object[] currentState = new Object[] { null };
+
+		interceptor.onFlushDirty(person, null, currentState, null, propertyNames, null);
+		Assert.assertNotNull(currentState[0]);
+	}
+
+	@Test
+	public void onFlushDirty_shouldAddPersonDateChangedForPerson() throws Exception {
+		AuditableInterceptor interceptor = new AuditableInterceptor();
+
+		Person person = new Person();
+
+		String[] propertyNames = new String[] { "personDateChanged" };
+
+		Object[] currentState = new Object[] { null };
+
+		interceptor.onFlushDirty(person, null, currentState, null, propertyNames, null);
+		Assert.assertNotNull(currentState[0]);
+	}
+
 	/**
 	 * @see AuditableInterceptor#onFlushDirty(Object,Serializable,Object[],Object[],String[],Type[])
 	 * @verifies set the dateChanged field
@@ -233,4 +263,45 @@ public void onSave_shouldBeCalledWhenSavingOpenmrsObject() throws Exception {
 		Assert.assertNotNull(u.getDateCreated());
 	}
 
+	@Test
+	public void onSave_shouldPopulateDateCreatedForPersonIfNull() {
+		Person person = createPersonWithNameAndAddress();
+		Context.getPersonService().savePerson(person);
+		Assert.assertNotNull(person.getDateCreated());
+		Assert.assertNotNull(person.getPersonDateCreated());
+	}
+
+	@Test
+	public void onSave_shouldPopulateCreatorForPersonIfNull() {
+		Person person = createPersonWithNameAndAddress();
+		Context.getPersonService().savePerson(person);
+		Assert.assertNotNull(person.getCreator());
+		Assert.assertNotNull(person.getPersonCreator());
+	}
+
+	@Test
+	public void onSave_shouldPopulatePersonChangedByandPersonDateChangedIfPersonAlreadyExists() throws Exception {
+		Person person = Context.getPersonService().getPerson(1);
+
+		Assert.assertNull(person.getPersonChangedBy());
+		Assert.assertNull(person.getPersonDateChanged());
+
+		person.setGender("F");
+		Context.flushSession();
+		Context.getPersonService().savePerson(person);
+
+		Assert.assertNotNull(person.getPersonChangedBy());
+		Assert.assertNotNull(person.getPersonDateChanged());
+	}
+
+	private Person createPersonWithNameAndAddress() {
+		Person person = new Person();
+		person.setGender("M");
+		PersonName name = new PersonName("givenName", "middleName", "familyName");
+		person.addName(name);
+		PersonAddress address = new PersonAddress();
+		address.setAddress1("some address");
+		person.addAddress(address);
+		return person;
+	}
 }
diff --git a/api/src/test/java/org/openmrs/validator/PatientValidatorTest.java b/api/src/test/java/org/openmrs/validator/PatientValidatorTest.java
@@ -78,11 +78,8 @@ public void validate_shouldFailValidationIfAPreferredPatientIdentifierIsNotChose
 		Assert.assertTrue(errors.hasErrors());
 	}
 
-	/**
-	 * @see PatientValidator#validate(Object,Errors)
-	 * @verifies not fail when patient has only one identifier and its not preferred
-	 */
 	@Test
+	@Verifies(value = "should not fail validation if patient that is not preferred only has one identifier", method = "validate(Object,Errors)")
 	public void validate_shouldNotFailWhenPatientHasOnlyOneIdentifierAndItsNotPreferred() throws Exception {
 		PatientIdentifierType patientIdentifierType = Context.getPatientService().getAllPatientIdentifierTypes(false).get(0);
 		Patient patient = new Patient();
@@ -108,7 +105,10 @@ public void validate_shouldNotFailWhenPatientHasOnlyOneIdentifierAndItsNotPrefer
 		patientIdentifier1.setIdentifierType(patientIdentifierType);
 		patient.addIdentifier(patientIdentifier1);
 
-		Context.getPatientService().savePatient(patient);
+		Errors errors = new BindException(patient, "patient");
+		validator.validate(patient, errors);
+
+		Assert.assertFalse(errors.hasErrors());
 	}
 
 }
diff --git a/api/src/test/java/org/openmrs/validator/PersonValidatorTest.java b/api/src/test/java/org/openmrs/validator/PersonValidatorTest.java
@@ -45,6 +45,7 @@ public void setValidator(Validator validator) {
 	 * @see PersonValidator#validate(Object,Errors)
 	 * @verifies fail validation if birthdate is a future date
 	 */
+
 	@Test
 	@Verifies(value = "should fail validation if birthdate is a future date", method = "validate(Object,Errors)")
 	public void validate_shouldFailValidationIfBirthdateIsAFutureDate() throws Exception {