bindu | BAH-314 | Added functionality to create a report request using RequestRequest resource #24
Conversation
…g RequestRequest resource
|
For detailed discussion on this PR, please check this Bahmni talk. |
| @@ -0,0 +1,79 @@ | |||
| /** | |||
| * The contents of this file are subject to the OpenMRS Public License | |||
There was a problem hiding this comment.
This is the wrong license. We now use MPLv2. See http://openmrs.org/license/
(You can probably copy this from some existing file elsewhere into your IntelliJ settings.)
|
|
||
| @Override | ||
| public SimpleObject asRepresentation(Mapped<ReportDefinition> instance, Representation rep) throws ConversionException { | ||
| System.out.println("In MappedReportDefinitionConverter asRepresentation()"); |
There was a problem hiding this comment.
Don't leave a println here.
I assume that return(null) isn't right either, is it?
| /** | ||
| * {@link Converter} for {@link RenderingMode}s | ||
| */ | ||
| @Handler(supports = Mapped.class, order = 1) |
There was a problem hiding this comment.
I think this is going to handle all Mapped, not just Mapped so I think you may need to generalize this to handle more than just Mapped.
There was a problem hiding this comment.
@djazayeri do you mean that this class should be extending Converter<Mapped<T extends Parameterizable>> rather than being a special case for ReportDefinition (aka Converter<Mapped<ReportDefinition>>)?
There was a problem hiding this comment.
@mks-d yes I think that is right. The converter says it handles Mapped, so we need to handle Mapped. If we want to code specifically for a Mapped we can do that in a switch and throw an UnsupportedOperationException if it is not a ReportDefinition I suppose, but otherwise we should handle it generically
There was a problem hiding this comment.
@mseaton @djazayeri The Mapped class is created by the Handler dynamically. The type associated with the relevant Mapped class will not be available at runtime. So I couldn't make it generalised.
| public void setProperty(Object mapped, String propertyName, Object value) throws ConversionException { | ||
| try { | ||
| if ("parameterizable".equals(propertyName)) { | ||
| value = ConversionUtil.convert(value, ReportDefinition.class); |
There was a problem hiding this comment.
I think we'd want to generalize this line to handle all Mapped...
|
|
||
| /** | ||
| * {@link Converter} for {@link RenderingMode}s | ||
| */ |
|
|
||
| @Override | ||
| public SimpleObject asRepresentation(Mapped<ReportDefinition> instance, Representation rep) throws ConversionException { | ||
| System.out.println("In MappedReportDefinitionConverter asRepresentation()"); |
| /** | ||
| * {@link Converter} for {@link RenderingMode}s | ||
| */ | ||
| @Handler(supports = Mapped.class, order = 1) |
There was a problem hiding this comment.
@mks-d yes I think that is right. The converter says it handles Mapped, so we need to handle Mapped. If we want to code specifically for a Mapped we can do that in a switch and throw an UnsupportedOperationException if it is not a ReportDefinition I suppose, but otherwise we should handle it generically
| assertEquals(response.get("status"), ReportRequest.Status.REQUESTED); | ||
| assertEquals(response.get("priority"), ReportRequest.Priority.NORMAL); | ||
| SimpleObject resultObject = (SimpleObject) response.get("renderingMode"); | ||
| String rendererType = (String) resultObject.get("rendererType"); |
There was a problem hiding this comment.
This doesn't seem to be testing the Mapped is coming back correctly. Is this tested and found to work?
There was a problem hiding this comment.
yes @mseaton, this test case is not covering the different Mapped types. I will update in the next PR.
|
As I mentioned in the above comments, I couldn't make the Mapped class generalised. So I had to come up with other approach where I am overriding the setProperty method in ReportRequestResource class. Here I have handle to the particular class type. I will be posting the code block in this talk. @djazayeri @mseaton |
…g RequestRequest resource
…eportingrest into BAH-314 # Conflicts: # omod/src/main/java/org/openmrs/module/reportingrest/web/resource/ReportRequestResource.java # omod/src/test/java/org/openmrs/module/reportingrest/web/resource/ReportRequestResourceTest.java
|
Closing this PR. Will be raising a new PR with the changes in sometime. |
No description provided.