TRUNK-3949: Do not commit with validation errors

openmrs · Apr 2, 2013 · 09948bb · 09948bb
1 parent 8896b02
commit 09948bb
Show file tree

Hide file tree

Showing 5 changed files with 53 additions and 8 deletions.
diff --git a/api/src/main/java/org/openmrs/api/UserService.java b/api/src/main/java/org/openmrs/api/UserService.java
@@ -22,7 +22,6 @@
 import org.openmrs.User;
 import org.openmrs.annotation.Authorized;
 import org.openmrs.annotation.Logging;
-import org.openmrs.api.context.Context;
 import org.openmrs.api.context.UserContext;
 import org.openmrs.util.PersonByNameComparator;
 import org.openmrs.util.PrivilegeConstants;
@@ -642,5 +641,6 @@ public List<User> getUsers(String name, List<Role> roles, boolean includeRetired
 	 * @param hasPrivilege <code>true</code> if the authenticated user has the required privilege or if it is a proxy privilege
 	 * @since 1.8.4, 1.9.1, 1.10
 	 */
+	@Transactional(readOnly = true)
 	public void notifyPrivilegeListeners(User user, String privilege, boolean hasPrivilege);
 }
diff --git a/api/src/main/java/org/openmrs/api/impl/UserServiceImpl.java b/api/src/main/java/org/openmrs/api/impl/UserServiceImpl.java
@@ -629,7 +629,6 @@ public List<User> getUsers(String name, List<Role> roles, boolean includeRetired
 	 * @see UserService#notifyPrivilegeListeners(User, String, boolean)
 	 */
 	@Override
-	@Transactional(readOnly = true)
 	public void notifyPrivilegeListeners(User user, String privilege, boolean hasPrivilege) {
 		if (privilegeListeners != null) {
 			for (PrivilegeListener privilegeListener : privilegeListeners) {

diff --git a/web/src/main/java/org/openmrs/web/controller/ConceptFormController.java b/web/src/main/java/org/openmrs/web/controller/ConceptFormController.java
@@ -62,6 +62,7 @@
 import org.openmrs.util.OpenmrsUtil;
 import org.openmrs.util.PrivilegeConstants;
 import org.openmrs.validator.ConceptValidator;
+import org.openmrs.validator.ValidateUtil;
 import org.openmrs.web.WebConstants;
 import org.springframework.beans.propertyeditors.CustomDateEditor;
 import org.springframework.beans.propertyeditors.CustomNumberEditor;
@@ -160,6 +161,7 @@ else if ("next".equals(jumpAction))
 	 * @should remove a concept map from an existing concept
 	 * @should ignore new concept map row if the user did not select a term
 	 * @should add a new Concept map when creating a concept
+	 * @should not save changes if there are validation errors
 	 */
 	@Override
 	protected ModelAndView onSubmit(HttpServletRequest request, HttpServletResponse response, Object obj,
@@ -245,7 +247,7 @@ protected ModelAndView onSubmit(HttpServletRequest request, HttpServletResponse
 					concept.getCreator().getPersonName();
 
 				try {
-					new ConceptValidator().validate(concept, errors);
+					ValidateUtil.validate(concept, errors);
 					if (!errors.hasErrors()) {
 						cs.saveConcept(concept);
 						httpSession.setAttribute(WebConstants.OPENMRS_MSG_ATTR, "Concept.saved");

diff --git a/web/src/test/java/org/openmrs/web/controller/ConceptFormControllerTest.java b/web/src/test/java/org/openmrs/web/controller/ConceptFormControllerTest.java
@@ -13,9 +13,14 @@
  */
 package org.openmrs.web.controller;
 
+import static org.hamcrest.Matchers.greaterThan;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.nullValue;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 
 import java.util.Collection;
@@ -37,20 +42,28 @@
 import org.openmrs.test.Verifies;
 import org.openmrs.web.controller.ConceptFormController.ConceptFormBackingObject;
 import org.openmrs.web.test.BaseWebContextSensitiveTest;
+import org.openmrs.web.test.WebTestHelper;
+import org.openmrs.web.test.WebTestHelper.Response;
+import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.mock.web.MockHttpServletResponse;
 import org.springframework.mock.web.MockHttpSession;
+import org.springframework.test.annotation.NotTransactional;
 import org.springframework.validation.BindException;
+import org.springframework.web.bind.annotation.RequestMethod;
 import org.springframework.web.servlet.ModelAndView;
 
-import static org.hamcrest.Matchers.*;
-import static org.junit.Assert.*;
-
 /**
  * Unit testing for the ConceptFormController.
  */
 public class ConceptFormControllerTest extends BaseWebContextSensitiveTest {
 
+	@Autowired
+	WebTestHelper webTestHelper;
+
+	@Autowired
+	ConceptService conceptService;
+
 	/**
 	 * Checks that the conceptId query param gets a concept from the database
 	 * 
@@ -952,4 +965,27 @@ public void onSubmit_shouldRemoveAConceptMapFromAnExistingConcept() throws Excep
 
 		assertEquals(initialConceptMappingCount - 1, cs.getConcept(conceptId).getConceptMappings().size());
 	}
+
+	/**
+     * @see ConceptFormController#onSubmit(HttpServletRequest,HttpServletResponse,Object,BindException)
+     * @verifies not save changes if there are validation errors
+     */
+    @Test
+    @NotTransactional
+    public void onSubmit_shouldNotSaveChangesIfThereAreValidationErrors() throws Exception {
+    	Integer conceptId = 792;
+
+	    MockHttpServletRequest request = new MockHttpServletRequest("POST", "/dictionary/concept.form");
+		request.setParameter("conceptId", conceptId.toString());
+		request.setParameter("namesByLocale[en].name", "should not change");
+		request.setParameter("preferredNamesByLocale[en]", "should not change");
+		request.setParameter("synonymsByLocale[en][1].name", ""); //empty name is invalid
+		request.setParameter("synonymsByLocale[en][1].voided", "false");
+
+		Response response = webTestHelper.handle(request);
+		assertThat(response.getErrors().hasFieldErrors("synonymsByLocale[en][1].name"), is(true));
+
+		Concept concept = conceptService.getConcept(conceptId);
+		assertThat(concept.getPreferredName(Locale.ENGLISH).getName(), is("STAVUDINE LAMIVUDINE AND NEVIRAPINE"));
+    }
 }
diff --git a/web/src/test/java/org/openmrs/web/test/WebTestHelper.java b/web/src/test/java/org/openmrs/web/test/WebTestHelper.java
@@ -24,6 +24,8 @@
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.mock.web.MockHttpServletResponse;
 import org.springframework.stereotype.Component;
+import org.springframework.validation.BindException;
+import org.springframework.validation.Errors;
 import org.springframework.web.servlet.HandlerAdapter;
 import org.springframework.web.servlet.HandlerExecutionChain;
 import org.springframework.web.servlet.HandlerMapping;
@@ -133,8 +135,6 @@ public Response handle(final HttpServletRequest request) throws Exception {
 
 		Assert.assertTrue("The requested URI has no handlers: " + request.getRequestURI(), supported);
 
-		//Complete the request by flushing and clearing the Hibernate session
-		Context.flushSession();
 		Context.clearSession();
 
 		return new Response(response, request.getSession(), modelAndView);
@@ -153,5 +153,13 @@ public Response(MockHttpServletResponse http, HttpSession session, ModelAndView
 			this.session = session;
 			this.modelAndView = modelAndView;
 		}
+
+		public Errors getErrors(String model) {
+			return (Errors) modelAndView.getModel().get(BindException.MODEL_KEY_PREFIX + model);
+		}
+
+		public Errors getErrors() {
+			return getErrors("command");
+		}
 	}
 }