GSCoC 2018 / 'Attachments enhancements'

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();
conceptComplex.setHandler(BinaryDataHandler.class.getSimpleName());
ConceptName conceptName = new ConceptName("Out-of-Attachments test concept complex", Locale.ENGLISH);
conceptComplex.setFullySpecifiedName(conceptName);
conceptComplex.setPreferredName(conceptName);
conceptComplex.setConceptClass(conceptService.getConceptClassByUuid(ConceptClass.QUESTION_UUID));
conceptComplex.setDatatype(conceptService.getConceptDatatypeByUuid(ConceptDatatype.COMPLEX_UUID));
...
conceptService.saveConcept(conceptComplex);

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.

.

Hi @mksd , i haven’t heard from you since last friday. Is everything alright?

Hi @zouchine , @ahmed14 can you review the above method i used to create a unit test and give me your suggestions ?

Hi @ridmal,

Yes, i’ll review the unit tests and get in touch with you once i have a suggestion :slight_smile: . i just need to catch up with what I missed.

1 Like

Ok :slight_smile: Thank you very much . Actually I used XML based data loading only for creating a concept complex which is not relevant to the attachment module. ( But it can also be done using a method like this).It can be done both ways successfully. So please suggest a way to do that too . :slight_smile:

@ridmal apologies, I had said that I would be quite unavailable somewhere around May 15th because of a baby delivery :wink: Anyway, he was delivered on May 22nd in the end. I am now back home from the maternity ward which will allow me to spend some time reviewing your work.

It’s great that @zouchine got involved anyway, thanks for that Zoheir!

I would just say the following: please open a PR, even if the work is just only in progress for now. Using GitHub’s PR review mechanism will be the easiest way to provide feedback commit after commit. Do not force push future commits, simply just keep adding commits on your branch, they will be reviewed one by one until the PR is ready to be merged. I will take care in the end of squashing all commits and setting a summarising commit message that will enter Attachments commits history.

As soon as I see a PR I will be in a position to review. @zouchine, please also review the PR as soon as it is available.

Hi @mksd It is ok and congratulation for your baby :blush:

Ok I will send a PR for the 1st part we discussed. Please review and give suggestions . :slight_smile:

I add a new method which can create both non-attachment and attachment complex obs. non-attachment complex concept is created in here.

I think that we can merge the saveComplexObsForEncounter and saveComplexObsForEncounterDefault methods together. Is it ok ?

@mksd , @zouchine for creating the attachments ( Complex obs relevant to attachment module) current test is used ComplexObsSaver inside the TestHelper. Can we used general complex obs saving method for that?

@zouchine thank you for you feedbacks. . i modified the PR with some changes. Please give me your suggestions about that. :slight_smile:

I think that we can merge the saveComplexObsForEncounter and saveComplexObsForEncounterDefault methods together. Is it ok ?

On the principle I would be in favour of improving TestHelper so that it’s a better/easier helper class to setup unit tests. I don’t know where this saveComplexObsForEncounterDefault was intended to be used. If it is not used then you can get rid of it.


About ComplexObsSaver. It is good to use it for reproducing the saving of complex obs as Attachments would do it. Because in fact that’s the class being used behind the scenes. See here in the main controller AttachmentResource1_10. As you can see the modules does in fact use that class to save its attachments/complex obs. So whenever you want to mimic such setup within unit tests, you are well better off just reusing that API.

For any other purposes you feel free to use any other API such as using ObsService directly, as long as you manage to setup your tests the desired way.

1 Like

Hi @mksd ,

You mentioned in PR that two TestHelper classes are not necessary. What can we do for that? ( There are methods we used only in the the /api-2.0. )

If you look at the master branch you will see that there is only one TestHelper in /api, I wonder why this was duplicated. Why simply not extend and use that one class?