GSCoC 2018 / 'Attachments enhancements'

Community GSoC
,
81

@ridmal I think the last bit of Java API that we need is to modify the signature of getAttachments(..) like this:

List<Attachment> getAttachments(Patient patient, boolean includeIsolated, boolean includeRetired);

So that we can toggle whether we want isolated attachments or not.

Could you extend the API as above and update the Javadoc accordingly? Don’t forget the unit test that goes with it!

82

Ok i will start working on this. I have a small question. If user set includeIsolated = true , is it means that API will only fetch the isolated attachments ? ( or fetch both isolated and not isolated attachments ? )

83

You should read it the same as with what includeRetired does. Meaning: “what will be returned should also include the isolated attachments”.

In summary:

attachments with includeIsolated=true = attachments with includeIsolated=false + isolated attachments

By the way you can also keep the existing signature and do this for its implementation:

public List<Attachment> getAttachments(Patient patient, boolean includeRetired) {
  return getAttachments(patient, true, includeRetired);
}

This means that the default behaviour is to return all attachments, and this should be stated in the Javadoc for that signature.

84

I add the changes and updated the PR. :slight_smile:

85

Hi @mksd,

I am getting “Obs.error.trying.write.complex” error in sometimes when i run the test classes. But sometimes it works properly. This error coming from the methods which are used ,

Obs obs = obsSaver.saveOtherAttachment(null, patient, null, “File caption #” + i + 1, multipartRandomFile, ValueComplex.INSTRUCTIONS_DEFAULT);

method. Any suggestions about this error.

86

Can you give me some steps to reproduce this, how do you launch those tests exactly?

87

I launch the test cases using this command .

mvn test -Dtest=AttachmentsServiceTest -pl api-2.0/

88

Did you always experience this, or do you remember when this started happening?

89

No it is not occur in every time. When i run mvn clean install this error is not occur in most times.I think that this error start to occour when i save the attachments without encounters using ComplexObsSaver.

90

Could you try narrowing down the issue the specific test method(s) that might be affected? You could do that by temporarily using the @Ignore annotation on the other methods. And then run the test case class via Maven multiple times to validate that the problem indeed occurs.

I’m trying to pinpoint which exact test method does that, or at least find a scenario that can be replayed to reproduce the issue more or less consistently.

91

@mksd

I tested using @Ignore annotation. But i can’t find exact method which is give above mentioned error.

But all the newly added test class give error in different times. I can’t find exactly which method from that methods. I think all the following methods have common thing for occur this error.

getAttachments_shouldReturnIsolatedAttachments(), getAttachments_shouldNotReturnIsolatedAttachments(), getAttachments_shouldReturnAllAttachments()

But maven clean install will give this error only i run that command several time. It works perfectly in first time. Is this can be a problem in cache of the memory ?

92

@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
93

Ok , I will connect to IRC :slight_smile:

94

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
95

@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:

96

@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]>

97

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
98

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.

99

@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
100

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