GSCoC 2018 / 'Attachments enhancements'

Tags: #<Tag:0x00007fe6da824128> #<Tag:0x00007fe6da824010>

@ridmal could you add me as a collaborator (see here how) on your fork of Attachments. I will push a commit to unlock the situation.

I have found the exact reason why this is happening, but in the meantime I took the time to refactor quite a few things so better let me insert a full commit and take it from there.

I’m busy now, going into a meeting, but I will explain what happened in my next message.

P.S. I would be great if you could connect on IRC Freenode #openmrs.

1 Like

Ok , I will connect to IRC :slight_smile:

As part of my commit I also changed the actual implementation a little bit. In particular it does not get upset right away if no concepts complex are configured, a simple warning is then given (see here).

However it will get upset if, for some reason (such as a misconfiguration), non-complex obs are returned, see

Could you add a unit test that verifies that an API exception is thrown when the module is misconfigured?

So the test scenario would be:

  • Configure a concept complex UUID through the ad-hoc global property that in fact points to a non-complex concept.
  • Choose a concept UUID for which some obs are already saved (as per the standard test data set).
  • Attempt to fetch the attachments for the relevant encounter(s) or visit(s).
  • Verify that an API exception is thrown.
1 Like

@mksd ,

I am really grateful for your positive feedback and advises given in the 1st evaluation and thankful for all the support and guidance you have given me to complete my tasks. :slight_smile:

@mksd ,

I update the PR with required changes. There is a error occurred in sometimes ( very rarely occured ) . I think this is because order of the fetching attachments.What Can we about that ?

getAttachments_shouldReturnAllAttachments(org.openmrs.module.attachments.AttachmentsServiceTest): arrays first differed at element [0]; expected:<[0be9476d-17dc-4e01- be71-51b81e3ec594]> but was:<[1186503c-74c2-43c3-b771-6035ccbe439a]>

Could it be because of this?

Each routine of the service implementation ensures that the obs are returned in descending order of updated timestamps. However in this very specific place the properly sorted obs from two method calls might get mixed up when the two arrays are added. On the other hand the actual attachments are sorted correctly and the assert fails on the first mismatching element (element 0). That’s my guess of what might be happening. Can you @Ignore that test routine and launch the test case multiple times to confirm that that’s where the problem come from?

If you can confirm that, the solution would be to sort expectedAllAttachments by update date prior to asserting against actualAllAttachments.

Does that make sense?

1 Like

I run test cases by putting @Ignore in every test cases individually. Like you said testIncludeIsolatedParameter() unit test occurred previous mentioned error many times. But when i ignore that method and run the test cases , sometimes

getAttachments_shouldReturnAllAttachments() , getAttachments_shouldNotReturnIsolatedAttachments() , getAttachments_shouldReturnIsolatedAttachments() tests also give the same error ( ration of occurring is 1 for 10 times.) I used mvn test -Dtest=AttachmentsServiceTest -pl api-2.0/ for testings.

And for overcome this error can i used this method ,

use Collections.sort(expectedAttachments, Comparator.comparing(Attachment::getUuid)); which will sort the expectedAttachments list based on the Uuid of attachements.( i didn’t able to get getDateCreated() for sorting because it return null vlaues ). And aslo sort the actualAttachments list using same method and used the Assert.assertArrayEquals method for both sorted lists.

@ridmal I added this commit to your branch. This ensures that you can now obtain the attachment/obs date-time out of any Attachment instance. I also added some other minor changes.

Can you try to sort expected attachments by descending date-time in a test method where you know that the problem occurs?

1 Like

ok i will try that. Thank you. :slight_smile:

I update the PR. I sort the expectedAttachments using getDateTime() in every methods i mentioned above. :slight_smile:

@mksd ,

Did you review the latest PR?. :slight_smile: What is our next step ?

Ok first of all I added a final little commit and merged all the current work here:

  • (fb128fe) ATT-24: Foundation of the Java API.

We need to move onto finalising the REST API. I need to give it some thoughts, we should catchup on Monday or Tuesday about it.

1 Like

Ok @mksd ,

Until then is there anything i need to do or look ?

Hi @mksd ,

Is there any task i need to do ?

Connect to IRC! We’ll setup a call then.

@ridmal are you ok with ATT-24?

4 posts were split to a new topic: Attachments: REST end point for file upload should allow to specify encounter (ATT-27)

I updated the PR on Friday by changing the all occurrence of includeRetired to includeVoided. :slight_smile:

Merged, we can move on to the next task! A new AttachmentResourceTest that is not context sensitive.

A post was merged into an existing topic: AttachmentsService ahead of REST API work (ATT-24)