Location hierarchy should not allow loops, description should not be …

…required - TRUNK-2707 Removed stray comment in code - TRUNK-2707
openmrs · May 6, 2013 · 6c26027 · 6c26027
1 parent 2a23a2e
commit 6c26027
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 1 deletion.
diff --git a/api/src/main/java/org/openmrs/validator/LocationValidator.java b/api/src/main/java/org/openmrs/validator/LocationValidator.java
@@ -54,6 +54,7 @@ public boolean supports(Class c) {
 	 * @should set retired to false if retireReason is null or empty
 	 * @should pass validation if all fields are correct
 	 * @should pass validation if retired location is given retired reason
+	 * @should fail validation if parent location creates a loop
 	 */
 	public void validate(Object obj, Errors errors) {
 		Location location = (Location) obj;
@@ -64,10 +65,21 @@ public void validate(Object obj, Errors errors) {
 
 			if (location.isRetired()) {
 				if (!StringUtils.hasLength(location.getRetireReason())) {
-					location.setRetired(false); // so that the jsp page displays properly again
+					location.setRetired(false); // so that the jsp page displays
+					// properly again
 					errors.rejectValue("retireReason", "error.null");
 				}
 			}
+			// Traverse all the way up (down?) to the root and check if it
+			// equals the root.
+			Location root = location;
+			while (root.getParentLocation() != null) {
+				root = root.getParentLocation();
+				if (root.equals(location)) { // Have gone in a circle
+					errors.rejectValue("parentLocation", "Location.parentLocation.error");
+					break;
+				}
+			}
 		}
 
 		super.validateAttributes(location, errors, Context.getLocationService().getAllLocationAttributeTypes());

diff --git a/api/src/test/java/org/openmrs/validator/LocationValidatorTest.java b/api/src/test/java/org/openmrs/validator/LocationValidatorTest.java
@@ -107,4 +107,26 @@ public void validate_shouldPassValidationIfRetiredLocationIsGivenRetiredReason()
 
 		Assert.assertFalse(errors.hasErrors());
 	}
+
+	/**
+	 * @see {@link LocationValidator#validate(Object,Errors)}
+	 */
+	@Test
+	@Verifies(value = "should fail validation if parent location creates a loop", method = "validate(Object,Errors)")
+	public void validate_shouldFailValidationIfParentLocationCreatesALoop() throws Exception {
+		Location location1 = new Location();
+		Location location2 = new Location();
+		Location location3 = new Location();
+		location1.setName("County General");
+		location2.setName("Radiology");
+		location3.setName("Radiology Lab");
+		location3.setParentLocation(location2);
+		location2.setParentLocation(location1);
+		location1.setParentLocation(location3);
+
+		Errors errors = new BindException(location1, "location");
+		new LocationValidator().validate(location1, errors);
+
+		Assert.assertTrue(errors.hasFieldErrors("parentLocation"));
+	}
 }
diff --git a/webapp/src/main/webapp/WEB-INF/messages.properties b/webapp/src/main/webapp/WEB-INF/messages.properties
@@ -1164,6 +1164,7 @@ Location.unretireLocation=Unretire this Location
 Location.retired=Location retired successfully
 Location.unretired=Location unretired successfully
 Location.parentLocation=Parent Location
+Location.parentLocation.error=Can't set parent to a child! As this would create a loop in the hierarchy.
 Location.childLocations=Child Locations
 Location.tags=Tags
 Location.search.error=Error while attempting to find locations