ObsService.saveObs - the obs and its encounter must point to same person

aman · February 19, 2020, 3:49am

Hello everyone. I am working on the issue -

I had a doubt. Is it that always the person in Obs and its encounter must be same before being persisted to the database?. If so then why don’t we always make the person in encounter same as the person in obs just before persisting the obs. Looking at the saveObs() method of ObsSevice it appears that there are 3 cases being checked before persisting and in 2 of them we are are making the person in encounter to be equal to person in obs.

Reason why the test given in issue fails is that the 3rd case is being called where we do not make the person in obs same as person in its encounter.

@dkayiwa

dkayiwa · February 19, 2020, 7:46pm

Yes.

Can you point us to where we make the person in obs different from its encounter?

aman · February 20, 2020, 9:51am

Well, its not being made different. It is just that it is not being checked either.

Here is the link to the code snippet taken from core repo of ObsServiceImpl saveObs method - https://github.com/openmrs/openmrs-core/blob/8d540f18f374473904641efce397efb3e4a93f11/api/src/main/java/org/openmrs/api/impl/ObsServiceImpl.java#L106-L114

Look at the highlighted code snippet, In the very first case no check is being made and this is the case being called with the test given in description of the ticket. This case gets called when ObsId is null.

Here is the link to the draft PR of how I have approached this issue : https://github.com/openmrs/openmrs-core/pull/3125.

Now the test passes.

dkayiwa · February 20, 2020, 9:47pm

After reading the ticket comments, it looks like the ticket description was edited to require a validation error when the obs person and that of the encounter do not match. The unit test should also change accordingly.

aman · February 21, 2020, 5:12am

I got your point. But wouldn’t this require changing the datasets used in other tests to ensure that obs person and that of encounter match?

aman · February 21, 2020, 7:23am

@dkayiwa I wrote the validation test and added the validation code in ObsValidator class . As expected now there are validation exceptions in other ObsServiceTests. There are two ways that come to my mind to handle this :

Using Try Catch block

2.Correct the datasets being used for tests

Whats your opinion?

dkayiwa · February 21, 2020, 7:42am

Correct the datasets.

aman · February 21, 2020, 12:17pm

Making the slightest change in datasets is leading to a large increase in errors( from 3 to 80 ). Can there be any other approach?

@dkayiwa

dkayiwa · February 21, 2020, 1:40pm

What kind of change did you make?

aman · February 21, 2020, 3:21pm

Wherever validation errors were being thrown, I changed the the person id in obs to that of encounters patient id. But due to these changes other tests are starting to fail. Test failure increased from 3 to 80.

aman · February 21, 2020, 7:42pm

Rather then changing the person in encounter in dataset, changing the person in Obs in the dataset to that of encounter reduced the errors drastically. Then there were minor changes in some tests and dataset that I did. Now the validation error will be thrown as given in ticket description.

Here is the link to my PR - https://github.com/openmrs/openmrs-core/pull/3125