Contributing to module radiologydcm4chee

Hello everyone!

I am very interested in contributing to the openmrs-module-radiologydcm4chee. This module is great!

What is the best way to get in touch with the main developers/maintainers? On github I can see the names of @akhilrv and @sunbiz :slight_smile: Are they maybe around?

I could start with contributing unit tests, adapt it to include translations from transifexā€¦

Would be great to talk about the maintainers plans for the future of this module :smile:

cheers ivo

1 Like

Ivo, Thank you for your interest in contributing to the module. Feel free to fork - https://github.com/openmrs/openmrs-module-radiologydcm4chee and send pull requests

Hello @sunbiz

great! I am on it, thank you :slight_smile:

As I understand, the radiologydcm4chee is based of the module radiology so thats why the roots pom.xml says artifactId = radiology

But should the artifactId be changed to radiologydcm4chee, since its another module or will this be left since radiologydcm4chee is replacing the old module?

It would be great to get the module regularly built by the bamboo ci and up to https://modules.openmrs.org So I am wondering what the next steps would be to do that.

Thank you!

Right, this module is replacing the other radiology module with dcm4chee connection. I am not sure how many implementations continue to use the other module, but if you ask that question on the impl list, you should get an answer. Due to the change in the Order API in OpenMRS, this module will not work with the latest platform release. Making that change in the radiologydcm4chee module will be an important contribution

thanks I will ask on the impl list.

I started to make some minor changes I thought are useful and added pull requests. would be great if you could give me some feedback on them. (I might need to redo them since I just read that you use eclipses auto-formatting and my vim settings might be different :smile: )

I will learn my way through to module to see how to adapt it to the new Order API.

thanks a lot! have a great day ivo

Dear @sunbiz

regarding the https://wiki.openmrs.org/display/docs/Coding+Conventions

I imported the OpenMRS Formatter and Code Template and ran control-shift-f on the radiologydcm4chee modules files and it seems that it would affect almost all the files, since the used style is not very consistent (mix of tabs and spaces, ā€¦).

I would like to apply the OpenMRS formatting on all files and commit this before making further changes. And from then on always check with the auto-formatting before committing to get a consistent style.

Do you agree?

Thank a lot, ivo

I prefer the auto formatting done by the maven formatter plugin. Look at the webservices.rest module for example. Instead of using the IDE, whenever you build code, the maven plugin formats all Java code.

Dear @sunbiz

thanks for your tip! I added the maven formatter plugin as you suggested and as stated here

mvn java-formatter:format mvn clean package

both run successfully.

Made a pull request of the changes

Could you please have a look and merge it if it looks good to you :smiley:

I would then make another commit with the auto-formated files before I start at changing any of the code.

cheers ivo

1 Like

Dear all and @sunbiz,

I wrote a new PR for the radiology module :smile:

Changes in short:

  • it introduces the modules first unit tests!
  • uses the hl7api to generate the HL7 order message to create the worklist entries in dcm4chee (and by that fixes some issues)

please have a look at my PR, merge it if you like it or lets discuss it :smile:


Looking forward I have a question regarding the design of the class Study: I want to reduce the uses of OrderService (this module uses the deprecated OrderType which I want to replace in the future)

Study has the member orderId. So whenever I want to get the order of a study I have to use the orderService.getOrder(study.getOrderId())

I am thinking of replacing Study.orderId by Study.Order and creating a simple getter like getOrder() which returns the Study.order

This way I can definitely reduce the uses of OrderService.

Do you have any opinions on that? The openmrs-core does similar thing with Order which has a field of type Patient with a getter for it: getPatient()

Thanks a lot!

@teleivo, I agree with this approach, similar to what is used in the core. For me the module runs only on OpenMRS 1.9.x and that is because of the change in the Order API in openmrs in 1.10 and above. Are you also making the change that the module uses the new order API?

@sunbiz, great! This pr does not yet include any change regarding the order api, it only touches the DicomUtils class and how the HL7 order message is created. The OrderType/Order is one of the next things I want to work on.

In your opinion should I choose the approach of

that has for 1.9 and 1.10? So that I keep compatibility for OpenMRS 1.9.x and write another api which relies on the newer openmrs-core?

I think it will be some work to support both, unless you/implementation really wants to. You may want to just support 1.10 and higher with the new releases of the module, instead of having to maintain for the 1.9.x and lower.

@sunbiz youā€™re right. Personally I just want to support 1.10 and higher. Since this modules is still in a development stage with version is 0.1.0-dev and nobody replied to my posts about real implementations I dont think breaking compatibility with 1.9.x will bother anyone. If somebody has objections please let us know.

1 Like

Iā€™m trying to implement thar but I hope a OpenMRS 2.x version of module will be available soon!!!

@ipadawan great to hear from someone :smile:

are you going to implement it in production? or just starting for research purposes?

I cant promise a date when it will support 1.10 and higher, am still getting to know the code, trying to understand it and clean it up a bit.

let me know if you need any help to get started!

dear @sunbiz, do you have time to check out my pull request?

I added a new one (#11) which builds on the older one #10

I changed Study.modality from int to type Modality to enforce type safety.

  • Store Study.modality with its enum name in the database instead of the enum ordinal to be independent of the enum order
  • Added StudyValidator to ensure a modality is selected when an order is placed
  • Fixed RadiologyOrderFormController to not store an order/study in the database if the Validators return errors

Thanks!

Now I am going to work on the Order/OrderType :smile:

Sorry @teleivo for the delayā€¦ Iā€™m in the middle of a life event and will need some more time to review. I will get back to it sometime next weekend

Hi. Iā€™d like to implement in production but there are several issue to solve firstā€¦ For now, I need to add ā€œmachine nameā€ other than ā€œmodalityā€ but I canā€™t find doc that explains how to add new DICOM tags :ā€™( Thank you for support.

dear @ipadawan do you mean ā€œScheduled Station AE Titleā€ with ā€œMachine Nameā€ ? Unfortunately thats not possible at the moment.

In my opinion there are quite some things that need to be done before adding any new features (such as writing unit tests to cover most of the application, upgrading the openmrs-core version). I am not sure how long it will take.

would you like to join development? testing would also be great :slight_smile: or do you have experience with puppet? I wrote some puppet modules to install openmrs+radiologymodule with dcm4chee to test the module while developing. These puppet modules are not yet polished and I could need some help :wink:

Dear Ivo, Iā€™m an humble craftsman of programming but in my experience I know that ā€œImpossibleā€ and ā€œProgrammingā€ canā€™t be linked :smile:. What do you mean with ā€œat momentā€? Where can I find code that write data from order creation html form to dicom file in /var/lib/tomcat6/mpps ? Iā€™d like to join development or testing (without deadline, of course!). I never heard about puppetā€¦

1 Like