TRUNK-6463: Problem editing existing Complex Obs with the new Local F… (#5458) (#5463)

* TRUNK-6463: Problem editing existing Complex Obs with the new Local File Storage Service
TRUNK-6464: Test Context should not create new application data directory for each test

Author: mogoodrich

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

@@ -105,9 +105,7 @@ public Obs saveObs(Obs obs, String changeMessage) throws APIException {
 		if(obs.getId() != null && changeMessage == null){
 			throw new APIException("Obs.error.ChangeMessage.required", (Object[]) null);
 		}
-
-		handleExistingObsWithComplexConcept(obs);
-
+		
 		ensureRequirePrivilege(obs);
 
 		//Should allow updating a voided Obs, it seems to be pointless to restrict it,
@@ -162,7 +160,7 @@ private Obs saveExistingObs(Obs obs, String changeMessage) {
 		Obs newObs = Obs.newInstance(obs);
 
 		unsetVoidedAndCreationProperties(newObs,obs);
-		
+		handleObsWithComplexConcept(newObs);
 		Obs.Status originalStatus = dao.getSavedStatus(obs);
 		updateStatusIfNecessary(newObs, originalStatus);
 
@@ -220,6 +218,7 @@ private Obs saveObsNotDirty(Obs obs, String changeMessage) {
 	}
 
 	private Obs saveNewOrVoidedObs(Obs obs, String changeMessage) {
+		handleObsWithComplexConcept(obs);
 		Obs ret = dao.saveObs(obs);
 		saveObsGroup(ret,changeMessage);
 		return ret;
@@ -250,7 +249,7 @@ private void saveObsGroup(Obs obs, String changeMessage){
 		}
 	}
 
-	private void handleExistingObsWithComplexConcept(Obs obs) {
+	private void handleObsWithComplexConcept(Obs obs) {
 		ComplexData complexData = obs.getComplexData();
 		Concept concept = obs.getConcept();
 		if (null != concept && concept.isComplex()

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

@@ -694,6 +694,67 @@ public void saveObs_shouldNotOverwriteFileWhenUpdatingAComplexObs() throws IOExc
 		}
 
 	}
+
+	@Test
+	public void updateObs_shouldUpdateAComplexObs() throws IOException {
+		executeDataSet(COMPLEX_OBS_XML);
+		ObsService os = Context.getObsService();
+		ConceptService cs = Context.getConceptService();
+
+		Obs obs = null;
+		Obs updatedObs = null;
+		
+		File obsFile = null;
+		File updatedObsFile = null;
+		
+		try {
+			// the complex data to put onto an obs that will be saved
+			Reader input2 = new CharArrayReader("some string".toCharArray());
+			ComplexData complexData = new ComplexData("nameOfFile", input2);
+
+			// must fetch the concept instead of just new Concept(8473) because the attributes on concept are checked
+			// this is a concept mapped to the text handler
+			Concept questionConcept = cs.getConcept(8474);
+
+			obs = new Obs(new Person(1), questionConcept, new Date(), new Location(1));
+
+			obs.setComplexData(complexData);
+			os.saveObs(obs, null);
+
+			// sanity check, confirm the file exists
+			obs = os.getObs(obs.getObsId());
+			obsFile = new File(OpenmrsUtil.getApplicationDataDirectory() + File.separator + "storage" + File.separator + obs.getValueComplex().split("\\|")[1]);
+			assertTrue(obsFile.exists());
+			
+			// now change the obs
+			// NOTE: this really should change an actual field instead of voidReason; I originally had this change
+			// the comment field but, somewhat disconcertingly, this caused an UnchangeableObjectException because it
+			// tries to flush the original obs (before it is cloned) when it attempts to do a "getGlobalProperty" call.
+			// This exception doesn't occur in real life, and getGlobalProperty is set to transactional=readOnly, so
+			// not sure why it causes a flush when testing.
+			obs.setVoidReason("some comment");
+			updatedObs = os.saveObs(obs, "updating obs");
+			
+			// confirm old file has been removed and new one exists
+			updatedObsFile = new File(OpenmrsUtil.getApplicationDataDirectory() + File.separator + "storage" + File.separator + updatedObs.getValueComplex().split("\\|")[1]);
+			assertTrue(updatedObsFile.exists());
+			assertFalse(obsFile.exists());
+
+		}
+		finally {
+			if (obsFile != null && obsFile.exists()) {
+				obsFile.delete();
+			}
+			if (updatedObsFile != null && updatedObsFile.exists()) {
+				updatedObsFile.delete();
+			}
+		}
+	}
+	
+	private void confirmFileWithNameExists(File[] files, String name) {
+		assertTrue(Arrays.stream(files).anyMatch(f -> f.getName().equals(name)));
+	}
+	
 
 	/**
 	 * @see ObsService#setHandlers(Map)}

api/src/test/java/org/openmrs/test/jupiter/BaseContextSensitiveTest.java

@@ -354,18 +354,20 @@ public Properties getRuntimeProperties() {
 			runtimeProperties.setProperty(Environment.HBM2DDL_AUTO, "update");
 		}
 
-		try {
-			File tempappdir = File.createTempFile("appdir-for-unit-tests-", "");
-			tempappdir.delete(); // so we can make it into a directory
-			tempappdir.mkdir(); // turn it into a directory
-			tempappdir.deleteOnExit(); // clean up when we're done with tests
-			
-			runtimeProperties.setProperty(OpenmrsConstants.APPLICATION_DATA_DIRECTORY_RUNTIME_PROPERTY, tempappdir
-			        .getAbsolutePath());
-			OpenmrsUtil.setApplicationDataDirectory(tempappdir.getAbsolutePath());
-		}
-		catch (IOException e) {
-			log.error("Unable to create temp dir", e);
+		String appDataDir = OpenmrsUtil.getApplicationDataDirectory();
+		if (appDataDir == null || !appDataDir.contains("appdir-for-unit-tests-")) {
+			try {
+				File tempappdir = File.createTempFile("appdir-for-unit-tests-", "");
+				tempappdir.delete(); // so we can make it into a directory
+				tempappdir.mkdir(); // turn it into a directory
+				tempappdir.deleteOnExit(); // clean up when we're done with tests
+
+				runtimeProperties.setProperty(OpenmrsConstants.APPLICATION_DATA_DIRECTORY_RUNTIME_PROPERTY, tempappdir
+					.getAbsolutePath());
+				OpenmrsUtil.setApplicationDataDirectory(tempappdir.getAbsolutePath());
+			} catch (IOException e) {
+				log.error("Unable to create temp dir", e);
+			}
 		}
 
 		return runtimeProperties;