GSCoC 2018 / 'Attachments enhancements'

Tags: #<Tag:0x00007f0f21eb4da8> #<Tag:0x00007f0f21ebb450>

I sent a message to dileka to asking about ATT -24. I haven’t get any response yet. I will clone his repo when i get his permission.

I got a message from the Dileka and he gave his approval to use his work and continue with ATT-24. I will clone his repo and get familiar with the existing implementation. :slight_smile:

1 Like

hi @mksd , Did you review the last changes related to the ATT-24 issue ? ( Which is implementing a AttachmentService inside the Attachments module done by dieka. )

And there is many comments with that PR and I am little bit confuse about that. So can please help me to sort it out that what are the further implementation need to be done for the ATT-24-1. ( adding AttachmentService to Api ) :slight_smile:

The newly introduced service provides an API to retrieve, for a given patient, the list of attachments

  • within a given encounter
  • within a given visit

What I was asking Dileka was to put together unit tests that show that it all works as expected, and he was almost there. The bottom line is: attachments are not new entities, they are just wrappers around complex obs, so they are just a special kind of obs that are mixed with the others in encounters and visits.

So writing a unit test that demonstrates that the new API does indeed list the attachments of an encounter or a visit means setting up encounters and visits that mix up complex obs with other obs ; to then show that only the expected ones are returned: the complex obs (but wrapped as attachments).

Does that make sense? Here is how you can start off from where Dileka left it. After you have cloned Attachments, you can do this:

git remote add dileka
git fetch dileka
git checkout -b ATT-24 dileka/ATT-24-1

And at that point you will be on a local branch ATT-24 that carries all latest commits from Dileka.

Yes i got some idea , and I already follow the git commands and setup my local machine. Ok i will check about the unit test above you mentioned. :slight_smile:

SO i read the AttachmentsService ahead of REST API work (ATT-24) thread and here is the first question come to my mind when i check the implementation. Please correct if i am wrong.

There are 3 methods implemented in AttachmentsServiceImpl for get the attachments based on the encounters and visits. So inside that Dileka used getConceptComplexList() method for only fetch the complex obs , which coded by the concepts configured through the module. So that means from the API we can get only the obs relevant to this module. Am i correct? Is there anything to implement or modify inside that methods?



I want you to focus on the unit tests, not the implementation. Get a bunch of solid unit tests that show that when there is a mix of obs that are saved, fetching the attachments for a given encounter or a given visit produces the expected results. As soon as you have all those tests in place, we can revisit Dileka’s implementation.

Makes sense?

Ok , But there are two existing unit tests implemented in AttachmentsServiceTest for above mentioned purpose. Am I right? So Inside the getAttachments_shouldReturnEncounterAttachments() method, Dileka try to create 3 complex obs and 1 non-complex obs based on encounter and verify with AttachementService API that it only get the complex obs. Seems right thing to do. Is there any modifications need to be done in there?

Imagine that you had two complex obs saved within an encounter: one being managed by Attachments and the other one not. Would the service return only the expected one out of the two?

In order to make it more collection-minded, I would check this test case:

  • Save in the same encounter: 2 obs, 2 non-Attachments managed complex obs and 2 Attachments-managed complex obs.
  • Verify that the service only returns the two Attachments-managed ones.
  • Edit the configuration and encompass one (or two) of the other complex obs to be also Attachments-managed.
  • Verify that the service returns the two original Attachments-managed ones plus the one (or two) that were encompassed through the configuration change.

That would be a solid test for the encounter-based query. Then we can expand on that one to test that visit-based queries also produce the expected results.

As soon as we have solid tests in place, I would like to streamline the implementation as I felt that almost the same code was copy/pasted from a method to the other, instead of introducing reusable blocks. But tests first.

1 Like

Ok , thank you very much for your clarifications.I will modify the unit test as you mentioned . :slight_smile:

I have been stuck a little bit with creating a non-attachments managed complex Obs. I tried to create Complex Obs using following method ,

But It failed i used this complex Concept UUID and Its only work with concept UUIDs relevant to the attachment module. ({“7cac8397-53cd-4f00-a6fe-028e8d743f8e” and “42ed45fd-f3f6-44b6-bfc2-8bde1bb41e00”}).

Any suggestions about this ?

Are you trying to save some other complex obs? If yes you may just load this test data set. That should give you two of them, see ObsServiceTest-complex.xml in Core.

As it is they are not associated with any visits or encounters, so you will have edit and re-save them as part of your test’s setup.

1 Like

Thank you for you guidance. When i try to save the modified Obs it gives me this error.

getAttachments_shouldReturnEncounterAttachments(org.openmrs.module.attachments.AttachmentsServiceTest): OBS.STATUS - (Non-uppercase input column: status) in ColumnNameToIndexes ca che map. Note that the map’s column names are NOT case sensitive.

I see that you have already brought a copy of this file as ObsTest.xml. This is not what I meant initially but this is what we will have to do now.

The reason being that Obs.status was introduced in Core 2.1.x and Attachments depends on Core 2.0.0. So in your above XML file, just remove status="FINAL" from both obs.

Also please rename this file as ComplexObsTest.xml.

I did that in first place and tried to re save without status column. But then there is a NullPointerException . I will change name of the XML file. :slight_smile:

Ok so after digging a bit more into it I think it will be better to just add a method to TestHelper that does this via code. It’s a bit too intricate to wire this together using XML test data set files.

Something like

public Obs saveNonSupportedComplexObs(Encounter encounter) {

Could you work in making the above method work as expected? So this test helper method would enable to easily save another complex obs to any give encounter.

1 Like

Ok , Actually I tried this way too. But the problem is there are no other concept-complex IDs for creating complex obs which are not relevant to the attachment module.

But I will check that again. Can I used both ways.Which means for creating the additional complex-concepts we can used XML file and for creating obs we can used above method. :slight_smile:

You could just add in TestHelper.init() something like what’s done in the activator, see here.

This kind of stuff should suffice:

ConceptComplex conceptComplex = new ConceptComplex();
ConceptName conceptName = new ConceptName("Out-of-Attachments test concept complex", Locale.ENGLISH);

You will need to autowire ConceptService. Above I made sure to choose a handler from the Core, not from Attachments, just to make it look completely foreign (but the handler doesn’t affect its belonging to Attachments or not).

Thank You very much for your help. I will use this. :slight_smile:

@mksd This is an additional work. And sorry if this is a bad thing to do.

Like i mentioned before, i tried to create a concept complex using XML ( ComplexObsTest.xml ) and relevant Obs using method in TestHelper

So inside the AttachmentsServiceTest class I execute the XML file and created the Complex Concepts.

Then create a separate objects for out - of - attachments concept complex and attachment concept complex and test with the AttachmentServiceAPI. I think it work properly.:slight_smile:

Can You please review it and say your suggestions.