Adding "Order Status" to Orders

@wyclif… right, but you can’t do something like this:

order.setDateStopped(new Date())
orderService.saveOrder(order)

… 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.

Take care, Mark

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

So far I implemented a public setter(). At some point a user has to be able to change the status manually

Then in that case in my opinion, adding status to the array of mutable fields in ImmutableOrderInterceptor should be enough.

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:

@darius @wyclif My pull request is ready to review in case you want to have a look to my changes regarding this discussion: https://github.com/openmrs/openmrs-core/pull/2694

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.

@burke thank you for the clarification :slight_smile:

Then the code should be fine as I have written it and in my opinion my PR could be reviewed then :smile:

Yes, I agree with @burke… I should be able to take a look at the pull request today.

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

  1. 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
  2. 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.)

@fruether you can actually create a PR with the initial implementation and we can comment on that.

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?

PR for discussion as suggested by @wyclif: https://github.com/openmrs/openmrs-module-webservices.rest/pull/340

Solved thanks to @wyclif New PR here: https://github.com/openmrs/openmrs-module-webservices.rest/pull/341

Not sure if this is the right thread but according to this code https://github.com/openmrs/openmrs-core/blob/6082fd6dc6c3c0c2b3ea78aab7143f0dff855215/api/src/main/java/org/openmrs/api/impl/OrderServiceImpl.java#L509

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

Is this the expected behaviour?

@dkayiwa @mogoodrich @wyclif

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:

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?

@burke particularly interested in your thoughts.

fyi @mseaton

Take care, Mark