… because the saveOrder method explicitly fails when trying to operate on an order already being persisted:
@darius there’s not a great analogy for stopOrder… there’s a utility method for changing the stopOrder, but it’s private and it looks like there’s no way to just individually change the stop date… the stopOrder method is only called via other workflow methods like discontinueOrder, etc…
I could make something called "updateOrderFulfillerStatus(Order, OrderStatus, String comment), but our main use case will be to access/update this property RESTfully… so I guess I’d need make a new endpoint for this… seems like it would be simpler/cleaner to just create a new updateOrder method that could, in the end, could additionally be used for any other fields we add that we want to be mutable.
Peeking at the current code I think the addition that’s inline with the current design, would be to add a new method like you suggest (e.g. updateOrderFulfillerStatus), which behind the scenes calls saveOrderInternal.
Personally, for REST I like the idea of making this appear like a subresource of Order (because it’s meant to be set completely independently of the order itself, e.g. you shouldn’t set it at order creation time).
Is that method updateOrderFulfillerStatus supposed to have a check implemented that only the Order was changed in regards to the OrderStatus and not any other attribute changed it values compared to the previous version @mogoodrich@darius ?
Otherwise, this feature could be used to also change other attributes that are supposed to not change.
If this is specified I would be able to implement it.
Yes, @fruether let’s go with updateFulfillerStatus(Order order, OrderFulfillerStatus status, String fulfillerComment)… and, yes, if you can enforce that none of the those fields have changed, that would be best… (though if that becomes too complicated the ImmutableOrderInterceptor should also handle this).
Thanks for staying with this through all the design questions! I’ve updated the ticket with the new specifications.
As you guessed that’s because the setter for dateStopped isn’t public because it up to the api to set it internally when discontinueOrder and other related methods are called, not sure if it’s the same pattern you are following for status too.
Based on the proposed solution, I’m guess status is going to have no public setter too, right? If yes, then I agree that you probably need the new proposed service method that sets it under the hood.
You are welcome! But I will try to have it done during the next 2 weeks.
I am nor sure how the magic behind ImmutableOrderInterceptor works. Therefore, i am not completly sure if more functionality would be needed or if this use case is already fully supported by adding the field to the array
I was implementing it accordingly. But my original question regarding the business logic is still unanswered. Is there any logic regarding status change that should be applied e.g.
(1) is is allowed to set a fulfillerStatus to null that previously had a non null value e. g. received
(2) is it allowed to change a status as a person wishes to. E. g. can a fulfillerStatus that is completed get back to a status earlier in the state machine e.g. received
Currently I did not implemented any of this logic but I feel like it could be useful or necessary? @mogoodrich
To make clearer what I did have a look to the according changes:
This is a good question; however, since the “truth” of fulfiller status is expected to come from an external system, the business rules do not belong to OpenMRS. It’s probably more important to ensure that order status can only be changed by a fulfiller.
I think it’s fine to allow this status to be changed from any value to any other value by a process with appropriate privileges.
Personally, for REST I like the idea of making this appear like a subresource of Order (because it’s meant to be set completely independently of the order itself, e.g. you shouldn’t set it at order creation time).
@darius (cc @wyclif@mogoodrich) since I am not experienced with the REST framework of OpenMRS yet, I would like to discuss a bit of the design/implementation for this model extension in this thread. If this is fine with you. Issue: RESTWS-714
A subresource of Order was created. Since it is taking as argument the fulfillerComment and fulfillerStatus I created a new Representation that just contains these two arguments and a reference to the according order. Basically I was taking CustomDatatypeHandlerRepresentation as an example. Code of class is below
Only this Subresource should be able to set the FulfillerStatus and Comment. That’s why I used a method of this subresource to actually call updateOrderFulfillerStatus. This is one of the two really implemented methods in the subresource.
The subresource itself is referencing the Order and the just created object. The latter one will be constructed in newDelegate() method
According to 1:
public class FulfillerDetailsRepresentation {
private Order.FulfillerStatus fulfillerStatus;
private String fulfillerComment;
private Order parent;
According to 2:
@SubResource(parent = OrderResource2_2.class, path = "fulfillerstatus",..)
public class FulfillerStatusResource2_2 extends DelegatingSubResource<FulfillerDetailsRepresentation, Order, OrderResource2_2> {
@Override
public FulfillerDetailsRepresentation save(FulfillerDetailsRepresentation delegate) {
Order order = Context.getOrderService().updateOrderFulfillerStatus(delegate.getParent(), delegate.getFulfillerStatus(), delegate.getFulfillerComment());
delegate.setParent(order);
return delegate;
}
public DelegatingResourceDescription getCreatableProperties() {
DelegatingResourceDescription d = new DelegatingResourceDescription();
d.addProperty("fulfillerStatus");
d.addProperty("fulfillerComment");
return d;
}
Does this look correct or the way it was intended? Did I miss the implementation of an important method? Is there anything I should take into account which I did not do so far?
@fruether it has been years since I have created one of these subresources, so I can’t comment on whether the code is exactly right or not.
I suggest that you start by proposing the expected behavior, and ask if people agree with this. Based on that behavior, you can ensure that your implementation fulfills it.
(In other words I’m suggesting that you approach this with a test-driven-development style, and that you share the tests for comment. It would make sense for these to be web layer tests, like this one.)
Currently I am getting an error that seems to be related to:
REPORT-819 since I am getting a similar error 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**
Does someone has an idea how to approach this error? In the referenced ticket they created a Converter but I do not see anything like this in the REST module. How can we fix this error here?
To get a patient orders both the patient and care setting uuid must be passed to the request, however in openmrs 2.1.2 only passing patient uuid works and all the orders are returned however in 2.1.4 passing only patient uuid only returns one order, the most recent patient order. To retrieve all the orders in openmrs 2.1.4 one must indicate care-setting uuid and status=any and patient uuid
This may reflect an error in our transition to supporting care settings. A care setting is inpatient, outpatient, or emergency room; orders are managed relatively independently in these different settings; and, it would be rare (perhaps for auditing tools) that one would want to look at orders across all settings.
Returning a single order from a call for all orders definitely sounds like a bug.
Resurrecting this thread to discuss a possible enhancement… we are not 100% set on this design yet, but will likely want to move quickly on this so wanted to throw it out soon rather than later.
Summary of where we are now:
The Lab Workflow OWA serves as a simplied LIMS system that allows for entering of lab results
Our lab team (users of the Lab Workflow OWA) have asked for the ability to associate a “Lab ID” with an order as well
Questions
From my understanding, accession_number seems like the correct place for this, as it’s usually added by a downstream “fulfiller” LIMS (which is this case in the Lab Workflow OWA). Does this make sense?
If so, would people support expanding the “Fulfiller Details” endpoint to also allow updating the accession number?