GSCoC 2018 / 'Attachments enhancements'

Community GSoC
,
61

Is there a way to directly get the TestHelper class ( inside the /api ) for the AttachmentServiceTest class inside the /api-2.0.

Or may I create a new testHelper class inside the /api-2.0 and extend with the above mentioned TestHelper class and add the additional methods required for the /api-2.0 ?

62

Ah ok I understand the problem.

Just add this dependency to api-2.0/pom.xml:

<dependency>
  <groupId>${project.parent.groupId}</groupId>
  <artifactId>${project.parent.artifactId}-api</artifactId>
  <version>${project.parent.version}</version>
  <classifier>tests</classifier>
  <type>test-jar</type>
  <scope>test</scope>
</dependency>

This will expose apiā€™s test classes to the test scope of api-2.0, and in particular the much wanted TestHelper. It is to be noted that apiā€™s test-jar is already being built, see here.

More on all this here ā€˜How to create a jar containing test classesā€™.

1 Like
63

Thank you very much for your help . I will get rid of the additional TestHelper.:slight_smile:

64

Hi @mksd,

Inside the AttachmentServiceTest class @dileka used

Assert.assertArrayEquals(expectedAttachments.stream().map(Attachment::getUuid).collect(Collectors.toList()).toArray(),actualAttachments.stream().map(Attachment::getUuid).collect(Collectors.toList()).toArray()); method

Instead of that can i use Assert.assertEquals(expectedAttachments.size(), actualAttachments.size()); ( Because sometimes Array values can be in different orders.)

65

Comparing the sizes is nowhere near as good as verifying that the contents match.

A correct design would be to ensure that the API returns attachments in an order that is anticipated. Such as ā€˜most recent firstā€™, and then arrange the expected array so that is respects that order ahead of the comparison.

66

Thank you for your suggestions. I updated the PR. It is only focus on

saveComplexObs(Encounter encounter, int count, int otherCount) method. And i add additional parameter ( allComplexRelatedToAttach , I just put this name .if it is not relevent please suggest a new name ) for method which can config the Complex obs creation for used non-attachment concepts and attachment concepts.

67

@mksd , For testing the fetch attachment based on the visit ,

  1. Do i need to create a separate method in TestHelper ? or
  2. Can I used the methods create for encounters saveComplexObs(Encounter encounter, int count, int otherCount) ? ( So like you mentioned earlier we can get all the encounters for particular visit and using above method we can fetch all the attachments )
68

I would not add yet another method to TestHelper, I would just have a test setup that is a bit longer/more complex and that takes care of populating - say - two or three encounters of a given visit with saveComplexObs(Encounter, int, int), with a slightly different configuration for each encounter.

1 Like
69

Hi , @mksd I updated the PR. Please review and give me your feedback. :slight_smile:

70

I just did.

I also would like you to write an exhaustive Javadoc for the API, so on the interface AttachmentsService (not AttachmentsServiceImpl). Thatā€™s where other developers will go to find the documentation as to what this API exactly does.

Also I just finally had a look at the implementation, so AttachmentsServiceImpl, and itā€™s great! Reusability :slight_smile: Very good.

Now here is what weā€™ve got, for a given patient:

  • a method that can return attachments of a given encounter ;
  • a method that can return attachments of a given visit ;
  • a method that can return attachments of all encounters.

There is a fourth category of attachments that are not associated with any encounters (and hence any visits either). We need a way to fetch those as well, letā€™s introduce:

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

(We might rename it, Iā€™m not sure yet about the best terminology here.)

1 Like
71

Thank you for your feed backs. I will start to work with above mentioned tasks. :slight_smile:

72

Hi @mksd ,

I updated the PR with changes you mentioned. And can you give me a example about how to write a java doc for APIs in OpenMRS ( Is there any specific standard in OpenMRS.? )

73

Mmm, maybe have a look at this. Nothing specific to OpenMRS though, just enough information for an outsider to understand what an API is about:

  • A (short) description of what the method does.
  • A short description for each argument (using @param).
  • A short description of what is returned (using @return).

Note that most IDEs (at least Eclipse and most likely IntelliJ) will pre-generate everything for you as soon as you type ā€˜slash star star ENTERā€™, so /** + ENTER.

1 Like
74

@mksd I added the Javadoc for the API and update the PR. :slight_smile:

75

Just commented on your two last commits.

Thanks for the Javadoc, in the end we will keep it simpler than what you submitted. In general the OpenMRS code base is poorly Javadocā€™d, so we wonā€™t go over the top here. But believe me that sometimes you will pull your hair because the original developer didnā€™t take the necessary 3 min of his life to tell others what some API method is doing exactly.

For instance in the context of OpenMRS, when fetching data, a Patient parameter is quite obvious. So thatā€™s a case where there is no need to detail why itā€™s thereā€¦ etc.

76

Hi @mksd,

I was little bit stuck with getIsolatedAttachments(ā€¦) unit test implementation. I tried to create Attachments without configure any encounter and visit.

	// setup
	Patient patient = ps.getPatient(2);
	Concept concept = context.getConceptComplex(AttachmentsConstants.ContentFamily.OTHER);
	List<Attachment> expectedAttachments = new ArrayList<>();
	
	
	//
	for (int i = 0; i < 2; i++) {
		Obs obs = new Obs();
		obs.setConcept(concept);
		obs.setObsDatetime(new Date());
		obs.setPerson(patient);
		obs.setValueText("Some text value for a test obs.");
		obs = os.saveObs(obs, null);
		expectedAttachments.add(new Attachment(obs));
	}

But there was an error Configuration required: attachments.defaultConceptComplexUuid. I canā€™t find a way to solve this. Is there any other way to create obs without configuring encounter or visit ?

77

Hi @mksd

I solved the problem by using different approach and update the PR. Please review and give me your suggestions. :slight_smile:

78

Thanks @ridmal, I just reviewed.

79

Thank you for you suggestions and modifications. I add those modifications and update the PR. :slight_smile: