Project: Radiology Reporting Enhancement

Software Add-on Modules
,
42

It is true that if your tests are BaseModuleContextSensitiveTest then there is an in memory DB, only then is your XML dataset used (if you execute it) But that would be an integration/component test. Unit tests are without db, network, file io.

43

@ivange94 I just built and ran your changes. I imported several templates from http://www.radreport.org/ without any issues, great!! :slight_smile:

I have a few comments:

  1. in database table radiology_report_template please move columns uuid,creator,date_created, changed_by, date_changed to the end of the columns in following order creator,date_created,changed_by,date_changed,uuid. Reason: I want to see the more metadata stuff last.
  2. I see that the templates are saved at for example path: /tmp/tomcat7-tomcat7-tmp/mrrtTemplateFile8758726493971092901.html we should not save the templates in /tmp. files in /tmp are typically deleted on reboot on linux. I see that you are using File.createTempFile("mrrtTemplateFile", ".html"); when parsing. Why not use the template home directory you added to the RadiologyProperties ?
  3. The template directory should be configurable under the Settings, Radiology. As I told you please use a global property for this, lets call it GP_MRRT_REPORT_TEMPLATE_DIR. Add a global property entry in config.xml for it with a default value reporttemplates and a description. This GP should be able to handle relative and absolute paths!
  4. Add @Component to your MrrtReportTemplateSearchHandler it should then be accessible
  5. use your great new REST API in your radiologyReportTemplatesTab :blush:
  6. I can import the same template multiple times. We need to think of ways to prevent that.

keep up the good work!!

44

Yeah i noticed that. I’ll fix it. I know where the problem is.[quote=“teleivo, post:43, topic:6171”] The template directory should be configurable under the Settings, Radiology. As I told you please use a global property for this, lets call it GP_MRRT_REPORT_TEMPLATE_DIR. Add a global property entry in config.xml for it with a default value reporttemplates and a description. This GP should be able to handle relative and absolute paths! [/quote]

ok will look into that[quote=“teleivo, post:43, topic:6171”] I can import the same template multiple times. We need to think of ways to prevent that. [/quote]

will look into that too[quote=“teleivo, post:43, topic:6171”] I see that you are using File.createTempFile(“mrrtTemplateFile”, “.html”); when parsing. Why not use the template home directory you added to the RadiologyProperties ? [/quote]

This was not meant as a permanent location for the templates. The problem is that controllers are not allowed to return absolute file paths from browser. I needed that to construct a file object that i can send to Jsoup to parse. But i can’t get that. I only have access to the contents of the file using getInputStream, so i copied to a temporary file and passed that file to Jsoup since now Jsoup will have access to its absolute path. The fix for that is to call template.setPath() with the new path as a parameter. If you look inside ${HOME}.OpenMRS/radiology/reports you will see all templates you have imported, but the thing is the template object is not aware of that yet for reasons explained above so all that is need is to call template.setPath with the new path of templates.

45

fixed.

fixed.[quote=“teleivo, post:43, topic:6171”] The template directory should be configurable under the Settings, Radiology. As I told you please use a global property for this, lets call it GP_MRRT_REPORT_TEMPLATE_DIR. Add a global property entry in config.xml for it with a default value reporttemplates and a description. This GP should be able to handle relative and absolute paths! [/quote]

fixed.[quote=“teleivo, post:43, topic:6171”] handle relative and absolute paths! Add @Component to your MrrtReportTemplateSearchHandler it should then be accessible use your great new REST API in your radiologyReportTemplatesTab :blush: [/quote]

fixed.

Hi @judy, @teleivo

Th templates section is about done. I think it is ready for testing. Right now you can import templates from the UI that will be copied to a folder configurable using Global Properties and meta datas are stored in the database. REST is working fine for MrrtReportTemplate and also you can search for templates through the ui using their title. There is still one problem though(i intensionally left it for last), the templates are not validated, so the current system will import even invalid templates but that is not an issue i just have to provide implementation for the validator(). Also you can import 1 template multiple times. I will look into that.

Previous years students were asked to make mid-term evaluation videos but this year it was not. So should i do a video demoing the state of the project so far?

1 Like
46

Hi @ivange94

good work so far!

I have tested the module in the UI again a few points:

  • major: I tried to upload an invalid template (used a pdf), after hitting save nothing happens in the UI. no error message. bit confusing for the user. Additionally the PDF is stored in the report templates folder and in the database (obviously without dcterms metadata). I guess this is because the validation is not yet implemented. this is a crucial missing piece before we can merge!
  • minor: in the datatables when no records are shown, an empty records message in a blue box appears. however when loading the page it only spans over the first few columns and when hitting save over the entire table. please look into that.

for the code:

  • service: please update javadocs add tests taking the existing services as blueprint (service methods need to be tested if given null, id’s for templates that do not exist, all edge cases).
  • tests: test all classes you havent tested. for example test your new RadiologyProperties method. add javadocs. what should happen if the user enters an absolute path, relative path, relative path with subdirectories that do not exist, are parent directories created. global property is empty or null, …
  • implement validation of templates and prevent non valid ones from being imported
  • add a method to the template service so the client does not need to fiddle with the parser himself. he just uses the method to import a template from a file or stream (see github comments).

I added comments on github, please incorporate them. Squash your commits into one after you are done, close and reopen the PR so we start fresh and work on getting your changes merged :slight_smile:

keep up the good work :sunny:

47

Hi @teleivo, @judy

I have responded to comments on last pull request. I have done some code clean up, added more tests, updated javadoc, added method in service to import template and remove that functionality from controller. And also started with implementation of validator. But at the moment only file extensions are being validated. I have also created a new pull request with one commit.

I would like to request for an exam break. I started last week but i was writing just two courses so i tried to do the little i could. But this week i am writing 8 courses in a row skipping only sunday. I write my last paper Wednesday 13 of July. I know that is a lot of timeout but considering that I’ve made it this far while i was still going to school i believe after my exams i will still be able to complete my objectives before the deadline. I will be working 12+ hours a day as my only priorities will be eat, code and sleep. But this is a polite request I also prioritize this project, if you feel this will be bad for the project, i can try to multitask.

48

Hi @ivange94 and @judy,

am ok with you taking an exam break but you really need to work hard after the 13th of July since there is still a lot to do. Please focus on your exams now dont worry about whats left you’ll manage but I hope there are no distractions after the 13th.

Best of luck for your exams :mortar_board:

49

We use balsamiq and @michael is not involved with OpenMRS outside of GSoC – I have admin access to confluence so can answer any questions you have @ivange94/@teleivo

1 Like
50

Hi @ivange94,

I added some fixes to your code on my branch + a list of TODOs you would need to address so I can merge your PR.

You can see the diff here: https://github.com/ivange94/openmrs-module-radiology/compare/RAD-272-part1...teleivo:RAD-272-ivo

51

@ivange94 and @judy

I think we should work on a “release-test” or “smoke-test” subproject of the radiology module. it will be in the same project on github just a maven submodule. goal would be to test for example the MRRT report template functionality (import templates, query imported templates) from real world report templates like in an IHE connectathon. this test can be triggered manually by a developer from the command line with maybe some settings passed in via a properties file like the folder where the report templates are stored that should be imported. then all templates are imported into an in-memory db (“h2”) and some queries done against it.

examples: http://ihewiki.wustl.edu/wiki/index.php/MESA/Report_Template_Manager

52

This sounds great ! Even more we need to document this process

53

@ivange94 whats the status? build of your PR is failing and coverage is low. we really need to get going.

54

@teleivo i saw the build failure. But i don’t think that is my fault. The travic-ci build messages says some files do not have the license header but all the files I added do have license headers and more over “mvn clean install” locally passed fine. I have resolved all comments on pull request, I have also rebase on latest master and your fork and resolved all conflicts. ON coverage I am working on it, already added to test cases and still added some.

55

We use https://github.com/mycila/license-maven-plugin/blob/master/README.md now to ensure all files have a proper license header. Check out its readme for the commands, this way you can find out which files are not ok. I think it should also be able to insert the header for you. Format is important so that might be why its failing.

Also if you still have issues with the setup of your VM. I am switching to docker. Have just merged it. See the updated readme in our repo for instructions. Its soon much faster!!!

56

Hi. I have fixed the build issues, I am currently working on the test coverage. I have looked at the coveralls message and seen what is bring coverage so low and I’ve already started addressing it. I’ll be travelling in the next 2 hours and I’ll be in the bus for up to 7 hours. In fact the reason I didn’t work yesterday at night is because I was travelling to my village and its about a 7 hours journey. Am returning back today. Please exercise some more patience and I appreciate how patient you have been all this time. :slight_smile:

57

At the moment, i think the fastest for me is manual setup as I only have to install openmrs and not the pacs server.

1 Like
58

@ivange94 as you travel to your village, please send my regards to your grand parents and other relatives there. Must be lots of fun spending some time with them! :smile:

3 Likes
59

Docker setup is not installing the pacs. I suggests you try it, you will not want to go back :wink: its less typing of commands and takes < 1min to freshly start openmrs already with your built rad module deployed :slight_smile: :slight_smile: :slight_smile:

1 Like
60

will give a short at it then

61
  • low memory footprint also good for us :slight_smile: and our hdds :slight_smile: