Back porting to 1.9.x: Erasing a short name in the edit concepts page

should void it - TRUNK-3445
openmrs · Feb 13, 2013 · bb71cbd · bb71cbd
1 parent 62c5efd
commit bb71cbd
Show file tree

Hide file tree

Showing 6 changed files with 115 additions and 15 deletions.
diff --git a/api/src/main/java/org/openmrs/Concept.java b/api/src/main/java/org/openmrs/Concept.java
@@ -842,20 +842,27 @@ else if (fullySpecifiedName != null && !fullySpecifiedName.isVoided()) {
 	 * @should set the concept name type of the specified name to short
 	 * @should convert the previous shortName if any to a synonym
 	 * @should add the name to the list of names if it not among them before
+	 * @should void old short name if new one is blank (do not save blanks!)
 	 */
 	public void setShortName(ConceptName shortName) {
-		if (shortName.getLocale() == null)
-			throw new APIException("The locale for a concept name cannot be null");
-		else if (shortName != null && !shortName.isVoided()) {
+		if (shortName != null) {
+			if (shortName.getLocale() == null) {
+				throw new APIException("The locale for a concept name cannot be null");
+			}
 			ConceptName oldShortName = getShortNameInLocale(shortName.getLocale());
-			if (oldShortName != null)
+			if (oldShortName != null) {
 				oldShortName.setConceptNameType(null);
+			}
 			shortName.setConceptNameType(ConceptNameType.SHORT);
-			//add this name, if it is new or not among this concept's names
-			if (shortName.getConceptNameId() == null || !getNames().contains(shortName))
-				addName(shortName);
-		} else
-			throw new APIException("Short name cannot be null or voided");
+			if (StringUtils.isNotBlank(shortName.getName())) {
+				//add this name, if it is new or not among this concept's names
+				if (shortName.getConceptNameId() == null || !getNames().contains(shortName)) {
+					addName(shortName);
+				}
+			}
+		} else {
+			throw new APIException("Short name cannot be null");
+		}
 	}
 
 	/**

diff --git a/api/src/main/java/org/openmrs/ConceptName.java b/api/src/main/java/org/openmrs/ConceptName.java
@@ -152,7 +152,12 @@ public String getName() {
 
 	@Element(data = true)
 	public void setName(String name) {
-		this.name = name;
+		if (name != null && StringUtils.isBlank(name) && StringUtils.isNotBlank(this.name)
+		        && this.getConceptNameType().equals(ConceptNameType.SHORT)) {
+			this.setVoided(true);
+		} else {
+			this.name = name;
+		}
 	}
 
 	/**

diff --git a/api/src/test/java/org/openmrs/ConceptTest.java b/api/src/test/java/org/openmrs/ConceptTest.java
@@ -13,8 +13,11 @@
  */
 package org.openmrs;
 
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.nullValue;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThat;
 
 import java.util.ArrayList;
 import java.util.Collection;
@@ -817,6 +820,17 @@ public void setShortName_shouldSetTheConceptNameTypeOfTheSpecifiedNameToShort()
 		Assert.assertEquals(ConceptNameType.SHORT, cn.getConceptNameType());
 	}
 
+	@Test
+	public void setBlankShortName_shouldVoidTheOldOne() {
+		Concept concept = new Concept();
+		ConceptName cn = new ConceptName("some name", Context.getLocale());
+		ConceptName FullySpecName = new ConceptName("fully spec name", Context.getLocale());
+		concept.addName(FullySpecName);
+		concept.setShortName(cn);
+		concept.setShortName(new ConceptName(" ", Context.getLocale()));
+		assertThat(concept.getShortNameInLocale(Context.getLocale()), is(nullValue()));
+	}
+
 	/**
 	 * @see {@link Concept#getShortestName(Locale,Boolean)}
 	 */

diff --git a/api/src/test/java/org/openmrs/api/db/hibernate/HibernateProviderDAOTest.java b/api/src/test/java/org/openmrs/api/db/hibernate/HibernateProviderDAOTest.java
@@ -55,9 +55,9 @@ public void runBeforeEachTest() throws Exception {
 	 */
 	@Test
 	public void getProvidersByPerson_shouldNotReturnRetiredProvidersIfIncludeRetiredFalse() throws Exception {
-        Collection<Provider> providers = service.getProvidersByPerson(personDao.getPerson(2), false);
+		Collection<Provider> providers = service.getProvidersByPerson(personDao.getPerson(2), false);
 		Assert.assertEquals(1, providers.size());
-        Assert.assertFalse(providers.iterator().next().isRetired());
+		Assert.assertFalse(providers.iterator().next().isRetired());
 	}
 
 	/**

diff --git a/web/src/main/java/org/openmrs/web/controller/ConceptFormController.java b/web/src/main/java/org/openmrs/web/controller/ConceptFormController.java
@@ -472,9 +472,7 @@ public Concept getConceptFromFormData() {
 				}
 
 				ConceptName shortNameInLocale = shortNamesByLocale.get(locale);
-				if (StringUtils.hasText(shortNameInLocale.getName())) {
-					concept.setShortName(shortNameInLocale);
-				}
+				concept.setShortName(shortNameInLocale);
 
 				for (ConceptName synonym : synonymsByLocale.get(locale)) {
 					if (synonym != null && StringUtils.hasText(synonym.getName())) {

diff --git a/web/src/test/java/org/openmrs/web/controller/ConceptFormControllerTest.java b/web/src/test/java/org/openmrs/web/controller/ConceptFormControllerTest.java
@@ -43,6 +43,9 @@
 import org.springframework.validation.BindException;
 import org.springframework.web.servlet.ModelAndView;
 
+import static org.hamcrest.Matchers.*;
+import static org.junit.Assert.*;
+
 /**
  * Unit testing for the ConceptFormController.
  */
@@ -426,6 +429,79 @@ public void shouldUpdateConceptWithShortNameAlreadyInSynonymList() throws Except
 
 	}
 
+	/**
+	 * Test updating a concept by adding a name
+	 * 
+	 * @throws Exception
+	 */
+	@Test
+	public void shouldUpdateConceptByAddingName() throws Exception {
+		ConceptService cs = Context.getConceptService();
+
+		// make sure the concept already exists
+		Concept concept = cs.getConcept(3);
+		assertNotNull(concept);
+
+		ConceptFormController conceptFormController = (ConceptFormController) applicationContext.getBean("conceptForm");
+
+		MockHttpServletRequest mockRequest = new MockHttpServletRequest();
+		MockHttpServletResponse response = new MockHttpServletResponse();
+
+		mockRequest.setMethod("POST");
+		mockRequest.setParameter("action", "");
+		mockRequest.setParameter("conceptId", concept.getConceptId().toString());
+		mockRequest.setParameter("namesByLocale[en].name", "new name");
+
+		ModelAndView mav = conceptFormController.handleRequest(mockRequest, response);
+		assertNotNull(mav);
+		assertTrue(mav.getModel().isEmpty());
+
+		Concept actualConcept = cs.getConceptByName("new name");
+		assertNotNull(actualConcept);
+		assertEquals(concept.getConceptId(), actualConcept.getConceptId());
+	}
+
+	/**
+	 * Test removing short name by adding a blank short name
+	 *
+	 * @throws Exception
+	 */
+	@Test
+	public void shouldVoidShortName() throws Exception {
+		final String CONCEPT_NAME = "default concept name";
+
+		ConceptService cs = Context.getConceptService();
+
+		final Concept concept = new Concept();
+		concept.addName(new ConceptName(CONCEPT_NAME, Locale.ENGLISH));
+		concept.setShortName(new ConceptName("shortname", Locale.ENGLISH));
+		cs.saveConcept(concept);
+
+		Concept actualConcept = cs.getConceptByName(CONCEPT_NAME);
+		assertThat(actualConcept.getShortNameInLocale(Locale.ENGLISH), is(notNullValue()));
+		assertThat(actualConcept.getShortNames().size(), greaterThan(0));
+		assertThat(actualConcept.getNames().size(), is(2));
+
+		ConceptFormController conceptFormController = (ConceptFormController) applicationContext.getBean("conceptForm");
+
+		MockHttpServletRequest mockRequest = new MockHttpServletRequest();
+		MockHttpServletResponse response = new MockHttpServletResponse();
+
+		mockRequest.setMethod("POST");
+		mockRequest.setParameter("action", "");
+		mockRequest.setParameter("conceptId", concept.getConceptId().toString());
+		mockRequest.setParameter("shortNamesByLocale[en].name", " ");
+		mockRequest.setParameter("concept.datatype", "1");
+
+		ModelAndView mav = conceptFormController.handleRequest(mockRequest, response);
+		assertNotNull(mav);
+
+		actualConcept = cs.getConceptByName(CONCEPT_NAME);
+		assertThat(actualConcept.getShortNameInLocale(Locale.ENGLISH), is(nullValue()));
+		assertThat(actualConcept.getShortNames().size(), is(0));
+		assertThat(actualConcept.getNames().size(), is(1));
+	}
+
 	/**
 	 * Test adding a concept with a preferred name, short name, description and synonyms.
 	 *