Adding "Order Status" to Orders

thank @darius… that is definitely something we’d have to change…

the issue is that on top of that the saveOrder method fails if you try to change an existing order:

@fruether I would be up for just modifying the failOnExistingOrder as you suggest, but, beyond that the saveOrder method has a lot of other existing business logic that assumes that an order is “new” and I suspect we’d run into other issues.

@darius @burke what do you think about adding a updateOrder(Order order) service method that just calls dao.saveOrder(order) without any of the business logic? We could document that this method should only be used to modify the mutable properties (and will fail hard if you do otherwise).

The Order resource in REST web services would have to be modified to use this service method in the “update” use case, but this seems doable.

Thoughts?

Take care, Mark

If you want a field to be mutable you just add it to this array

I would suggest looking at how dateStopped is implemented, and do something analogous.

(@wyclif could you give a more detailed pointer to something which shows how to extend your design with this new property?)

It’s not much, if you add a new property status to Order class and want it to be mutable, all that needs to be done is add it to this array, if you look at that array, you will notice dateStopped is actually one of them.

@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