In our first coalition/community spring, we’ve included a ticket to allow OpenMRS Reporting Module reports to be run through the Bahmni Reports app, as long as they are simply based on “start date” and “end date” parameters. (Ticket is BAH-314, but the design discussion should happen here.)
We don’t want to teach our users to do some of their tasks (such as running certain reports) outside of their usual Bahmni Apps screens [and they have an especially complex report that was better to implement with OpenMRS Reporting]
Thankfully a large number of reports are simply based on two parameters ‘start date’ and ‘end date’, and so we thought that we could ensure that such reports, coming from other reporting sources (such as the Reporting module), could also be listed and triggered through Bahmni Apps’ Reports app.
I did not envision this initial “fix” as an ultimate integration of reports into Bahmni Apps. It’s rather a short term workaround.
Configuration-wise, this is fine with me. There should be some sort of mapping of the selected format as well.
But I guess the bigger part of the challenge is to ensure that the underlying implementation is able to handle a ‘switch’ between the various reporting sources.
@binduak as far as you can tell, what sort of effort lies in making sure that down the line the current action buttons connect to the relevant APIs? So in other words, how challenging is it to enable the switch deep down behind clicking on ‘Run now’ or ‘Queue’?
Also I suspect that we may need to leverage the Reporting REST module.
I am planning to keep the exiting Bahmni Reporting functionality (Reports Page) as it is and adding one more link called “Complex Reports” (open for name suggestions) in the Reports app which will have the functionality to make an API call to the OpenMRS reporting module. This page can be enhanced later on to provide more report parameters as per the report definition.
And this can be implemented in either of two ways.
We can create a react component for this new “Complex Reports” page in a separate npm project (like form-controls) and add it as a dependency in bahmniapps. Then use this react-component directive to render component in reports angularjs module
Create openmrs-reporting-owa project with reactjs and build this new report component in this OWA. This is a generic approach. Provide a generic link using angular routes from bahmniapps reports UI to this owa.
I personally like very much the two-step approach that you have described! Note that the OWA naming should be openmrs-owa-reporting.
I don’t want this to be blocking when there is quite some important plumbing to do. But I’m not in favour of a distinction between ‘Reports’ and ‘Complex Reports’. From a user experience perspective this is nothing but misleading. The user doesn’t need to know that reports come from different sources.
This may not be a trivial UI design question though. I guess that we won’t avoid eventually to put a switch in place that invokes the ad-hoc API based on the type of report specified in some JSON configuration.
@darius it seems that such an OWA would basically replace Reporting UI. I know that what the OWA Bindu refers to here is primarily intended for Bahmni EMR, but clearly we should perhaps want to experiment here with a first OWA that could run on multiple distributions.
So, are we definitely wanting to write as much functionality as possible in ReactJS going forwards? I definitely prefer React, and it’s a better future-looking tech than Angular 1, but (a) we don’t have many exemplars of this yes, and (b) are we certain we can plug React code into the current Angular app wherever we need to?
I assume you’ve thought this through already, and we’re ready to invest in React as the way of the future, but I just wanted to check.
I agree that we should take advantage of this opportunity. Maybe the best way is for Bindu to make a first pass at writing this as she naturally would for Bahmni, and partway through this we can do a review and think about how it would look different if shared with the OpenMRS Reference Application.
We have couple of exemplars where we plugged React code into Angular app. Those are new medication tab feature and form-builder. You can find the commit details below.
But OWA for Bahmni is something we haven’t tried before. I will do a quick spike on it and let you know the updates.
As part of the BAH-314 card, we are developing a reporting UI in Bahmni which will use the existing OpenMRS reporting module(will be updating repo url shortly). Its a OWA application. The UI looks like below.
So when user clicks on Run Now button then he should be able to create a report request using POST call support of ReportRequestResource. But this feature is unavailable yet. And also, the reportRequest has complex data structure like Mapped which becomes difficult to serialize/deserialize. So we started implementing POST for the reportRequest resource in a simple way using setConvertedProperties override.
Commit details for the same can be found [here] (bindu | BAH-314 | Added functionlaity to create a report request usin… · binduak/openmrs-module-reportingrest@3dcc509 · GitHub) (Please note that its a WIP code). The below is the sample JSON body that we are sending as part of the post call.
@binduak, great to see you building in support for reports exposed by the reporting module. I’d add a few comments:
A “Mapped” structure shouldn’t be that hard to serialize/deserialize. It is basically just a reference to the report definition and then a Map of parameter names → serialized values. Something like:
Alternatively, you could do something like what is done in the “EvaluatedDefinition” base resource.
I suppose you could do this, but I don’t really see why you would do that. There is really no value in limiting this to CSV in the UI. I would simply expose whatever rendering modes that are supported by the ReportDefinition via it’s configured ReportDesigns, and make these available to choose from in a select list.
(Edit: I wrote this without noticing Mike’s reply, but it’s still relevant.)
I would look at the existing EvaluatedXyzResource classes for how we’ve handled parameters. It won’t be exactly analogous, but see org.openmrs.module.reportingrest.web.resource.EvaluatedCohortResource#getEvaluatedCohort
Specifically, you should reuse the utility method org.openmrs.module.reportingrest.web.resource.EvaluatedResource#getEvaluationContextWithParameters which allows the user to specify parameters in a POST body or as query parameters. (You may need to refactor it out of this abstract class…)
For ReportRequest, I do agree that we should simplify the REST API and hide the complexity of Mapped. I would think that we just let the reportDefinition parameter be a UUID (like you show in your example) and then use the utility method I mentioned above for getting the parameters that you’d use with this report definition.
I could create a ReportRequest through post call by overriding setConvertedProperties as mentioned in the my previous post. But I wanted to try creating the ReportRequest with Mapped JSON to check if it works out of box.
But it was failing here as it couldn’t find the MappedConverterClass. So I have implemented a new MappedReportDefinitionConverter class.
But its throwing the below exception when setProperty method is being called and because of this the UUID of the ReportDefinition is coming as null.
java.lang.IllegalArgumentException: Cannot invoke org.openmrs.module.reporting.evaluation.parameter.Mapped.setParameterizable - argument type mismatch
Please let me know your inputs.
yes, I will use RenderingModeConverter class to handle any type of renderer.
@darius This method evaluating the input parameters(like startDate, endDate and uuid in this case) for the report to run. Not handling Status and Priority parameters that we send as part of the POST body. If we are going head with overriding the setConvertedProperties method then I will try to make use of this method.
Second, in the converter, the value for "parameterizable" is coming as a LinkedHashMap (that’s because we’re coming from the JSON in the first place) ; but in fact what we want to set to the Mapped<ReportDefinition> as its "parameterizable" field is a ReportDefinition instance.
Hi @darius@mseaton, As mentioned in the comments for the PR, I tried making the MappedReportDefinitionConverter class generic to handle all the mapped types but not only the ReportDefinition class.
But as I don’t have handle to the type of class at runtime, I couldn’t create the template class(T.class)
@shruthipitta and I paired on it and implemented setProperty method in ReportRequestResource class which overrides the setProperty method in BaseDelegatingResource class. Here we have the handle to the class type. This will convert the Mapped class to object of ReportRequest type. The following is the code for the same.
you are doing a hacky approach in the setProperty method for only this
one resource, by telling it how to handle the two specific Mapped fields
that the resource has
you’re doing this because it’s too hard to do a generic Mapped<?>
handler that will work correctly at runtime with our abstract class
hierarchy and @Handler annotations.
That seems fine to me, so yes, go ahead and raise the PR if you haven’t
already done this.
Nitpicking: reportDefinitionCls is the wrong variable name, and I think
that Mapped<ReportDefinition> is wrong also because it might be a
Mapped<CohortDefinition>.
I generally agree with @darius and also noticed the same “nitpick” issues. For me the most important thing to start with is whether this actually works for all of the expected use cases, so if you can demonstrate this with tests and other examples then that will be helpful.
When we go to Report Administration page, We can see the below 3 report types.
These are all of ReportDefintion class type(subtypes are different though). I tried creating the ReportRequest for all the above 3 report types from POST MAN and they are working fine. We don’t get to know the report class type either from RequestBody or ResponseBody of the ReportRequest. One test case works for all the 3 report types. This is the reason why I couldn’t demonstrate much of the test cases here.
In the same Report Administration page, We can see the below 5 Report Definition types.
These are all of different class types(not of ReportDefinition class type). But we can create these report definitions(eg: Cohort Queries)from UI and add them as mappings to Period Indicator, Row-per-patient and Custom Report (I could see few of the combinations are not working, following this documentation helps). After adding them as mappings to any of the Period Indicator, Row-per-patient and Custom Report, I could successfully run them from POST MAN.
Now my questions are,
Do we have to run these 5 Report Definition types from POSTMAN ??
I could see ReportRequest class allows only 2 types of Mapped classes. I.e ReportDefiniton and ChortDefinition.
What is exactly Mapped<CohortDefinition> baseCohort in ReportRequest class ??
Should it be only of “Cohort Query” type or any of different Cohort Queries( I could see Person Query and Encounter Query along with Cohort Query under Cohort Queries tab). I could see this is optional when sending the RequestBody for the POST call. How does sending the baseCohort value changes the ReportRequest output ??. Sorry I am unable to get one use case for this.
I could see there are lot of possibilities with the OpenMRS reporting Module. Every time I go and try something out, I am learning new stuff. How about once the above questions are clarified, we release a MVP(minimum viable product) in the first release (obiviously which will not have support for all the types of report types) and improving in the subsequent release by taking inputs from community. This is just my opinion. Others opinions are always welcome.
@darius Please let me know we can have a call sometime today or tomorrow to clarify the above questions.
@binduak I think you are being too comprehensive. We’re just talking about the ReportRequest resource, which is used to request the execution of an existing report, which should already exist.
You don’t need to test all of the different report types – that’s a concern of the reporting module.
For this case I would expect to see tests like:
POST { reportDefinition: { uuid: “uuid-of-simple-def” } } => expect ReportService.saveReportRequest to have been called with expected ReportRequest
POST { reportDefinition: { uuid: “uuid-of-def-with-params”, mappings: {“startDate”: “2017-01-01”, “endDate”: “2017-12-31”}}} => expect ReportService.saveReportRequest to have been called with expected ReportRequest
Below is the screenshot of the OpenMRS Reports UI which aligns with Bahmni Reporting Module UI. Please provide your valuable feedback (a small amount of styling part is yet to be done). Thanks !
