Solving RESTWS-714 - Add fulfillerStatus and Orders.fulfillerComment to Order resource #340
Conversation
|
Typically you get that error if you have a compound field that has no representation which I think in this case is FulfillerDetailsRepresentation |
| /** | ||
| * {@link Resource} for Order, supporting standard CRUD operations on fulfiller_comment and fulfiller_status | ||
| */ | ||
| @SubResource(parent = OrderResource2_2.class, path = "fulfillerstatus", supportedClass = Order.class, supportedOpenmrsVersions = { |
There was a problem hiding this comment.
I think supported class is supposed to be FulfillerDetailsRepresentation which should get you past the current error you're getting.
There was a problem hiding this comment.
It would be nice to have some consistency in the naming of the resource and these classes, some places you are using FulfillerStatus while others you are using FulfillerDetailsRepresentation, can you use FulfillerDetails or FulfillerStatus, am more inclined towards FulfillerDetails
|
|
||
| import org.openmrs.Order; | ||
|
|
||
| public class FulfillerDetailsRepresentation { |
There was a problem hiding this comment.
Is this really a representation? How about just FulfillerDetails?
| @SubResource(parent = OrderResource2_2.class, path = "fulfillerstatus", supportedClass = Order.class, supportedOpenmrsVersions = { | ||
| "2.2.*" }) | ||
|
|
||
| public class FulfillerStatusResource2_2 extends DelegatingSubResource<FulfillerDetailsRepresentation, Order, OrderResource2_2> { |
There was a problem hiding this comment.
By convention the sub resource name should match closely the supported class by appending Resource and minimum supported version, i.e. if the resource is named FulfillerDetails, then this should be named FulfillerDetailsResource2_2
| @Override | ||
| public FulfillerDetailsRepresentation save(FulfillerDetailsRepresentation delegate) { | ||
| Order order = Context.getOrderService().updateOrderFulfillerStatus(delegate.getParent(), delegate.getFulfillerStatus(), delegate.getFulfillerComment()); | ||
| delegate.setParent(order); |
There was a problem hiding this comment.
Isn't this redundant? Because what value would you have got back on line before it?
There was a problem hiding this comment.
I am not sure if the created order is somehow different and hence a new parent must be set?
There was a problem hiding this comment.
I thought based on the logic you implemented in the OrderService.updateOrderFulfillerStatus, it updates and returns the same order object.
There was a problem hiding this comment.
You call saveOrderInternal after setting fullfiller details which returns the same order object.
| } | ||
|
|
||
| @Override | ||
| public Model getUPDATEModel(Representation rep) { |
There was a problem hiding this comment.
I think the resource only supports create and not update
There was a problem hiding this comment.
Are you sure? Because the filler details like comment and status are may updated during the order process. And I am not sure if in this scenario always the Create method would be called by the users of the the REST service.
Or if maybe at some point some modules would Update these details by submitting it through PUT...?
There was a problem hiding this comment.
This is a unique resource because the supported class is not persistent so differentiating save vs update doesn't make much sense.
|
|
||
| private Order.FulfillerStatus fulfillerStatus; | ||
| private String fulfillerComment; | ||
| private Order parent; |
There was a problem hiding this comment.
Can you please rename this to order for clarity?
There was a problem hiding this comment.
I am not sure what you mean? Should the order not be called parent?
There was a problem hiding this comment.
The field value is an Order object, therefore it's more intuitive to name it order
| } | ||
|
|
||
| @Override | ||
| protected void delete(FulfillerDetailsRepresentation delegate, String reason, RequestContext context) |
There was a problem hiding this comment.
Do we actually want to support this?
| } | ||
|
|
||
| @Test | ||
| public void shouldCreateNew() throws Exception { |
There was a problem hiding this comment.
This test should be named along the lines of what it actually does, i.e. set fulfiller details.
|
FYI, you don't have to create a new PR when you make changes, just commit and push changes to your remote branch and then github will automatically update the original PR. |
|
Yeah it is more like happening by accident @wluyima. Sometimes I squash the commits and then somehow force an empty push to the repository. That's leads to the closing of the PR :( |
|
Actually the dev merging the PR will squash the commits in github, it's a feature that didn't exist at the time we wrote the documentation for PRs |
This PR is not yet working but I added it to give a discussion base for the Talk thread:
The error is basically is the following message:
org.springframework.http.converter.HttpMessageNotWritableException: Could not write JSON: Direct self-reference leading to cycle (through reference chain: org.openmrs.module.webservices.rest.web.v1_0.resource.openmrs2_2.FulfillerDetailsRepresentation["parent"]->org.openmrs.TestOrder["creator"]->org.openmrs.User_$$_jvstbb0_55["creator"]); nested exception is org.codehaus.jackson.map.JsonMappingException: Direct self-reference leading to cycle