Copying an Obs doesn't copy over an associated Order

mogoodrich · February 7, 2019, 8:45pm

Hello all–

When copying an Obs, any order associated with that Obs is not copied over. This seems incorrect to me, particularly because since Obs are immutable, whenever an Obs is modified it is “copied”, resulting in the loss of any linkage to an Order.

However, there is one unit test that expects this behavior:

github.com

openmrs/openmrs-core/blob/master/api/src/test/java/org/openmrs/api/EncounterServiceTest.java#L2796


		
		assertNotNull("valid EncounterROle object should be returned", encounterRoles);
		assertEquals(encounterRoles.size(), 1);
		assertEquals(encounterRoles.get(0).getName(), name);
	}
	
	/**
	 * @see EncounterService#transferEncounter(Encounter,Patient)
	 */
	@Test
	public void transferEncounter_shouldTransferAnEncounterWithObservationsButNotOrdersToGivenPatient() {
		executeDataSet(TRANSFER_ENC_DATA_XML);
		Patient targetPatient = Context.getPatientService().getPatient(201);
		// encounter has 2 obs which are connected with the same order
		Encounter sourceEncounter = Context.getEncounterService().getEncounter(201);
		
		Assert.assertEquals(2, sourceEncounter.getOrders().size());
		Assert.assertEquals(2, sourceEncounter.getObs().size());
		
		//transfer
		Encounter transferredEncounter = Context.getEncounterService().transferEncounter(sourceEncounter, targetPatient);

Do we need to special case this? It seems like this test is assuring that when an Encounter is transferred from one patient to another, we don’t copy over any Orders associated with that Encounter, which seems fine to me. But do we really need to make sure any Obs on that Encounter that were linked to an order no longer are? I assume the point of this ticket was to make sure that Orders aren’t inadvertently transferred from one patient to another. But the “Order” on an Obs refers to the the Order that resulted in this Obs (as I understand it). I don’t think we want to lose this information?

fyi @dkayiwa @mseaton @burke

Take care, Mark

mseaton · February 8, 2019, 3:30pm

I’d agree with you @mogoodrich, it would seem that this connection to the order should be preserved.

dkayiwa · February 11, 2019, 9:01pm

This commit https://github.com/openmrs/openmrs-core/commit/3a960636ce970b03b4709cf4b17ca7e84f3a9946 was done as part of this ticket https://issues.openmrs.org/browse/TRUNK-257

After having looked at it again, i believe that @mogoodrich you are correct. We should have fixed it in a way that does not require the removal of this line from Obs.java newObs.setOrder(obsToCopy.getOrder());

mogoodrich · February 11, 2019, 10:27pm

Thanks @mseaton @dkayiwa… in the coming days I will add back in the line that copies over the Orders when copying an Obs and then also change the “transferEncounter” test so that it doesn’t expect the Orders to be removed.