OBPIH-2718: Fix error with already existing location during receiving… #1782
Conversation
| @@ -41,6 +47,9 @@ class LocationService { | |||
| String name = getReceivingLocationName(shipmentNumber) | |||
| String[] receivingLocationNames = [name, "Receiving ${shipmentNumber}"] | |||
| Location location = findInternalLocation(parentLocation, receivingLocationNames) | |||
| if (!location) { | |||
| location = findByLocationNumber(locationNumber) | |||
There was a problem hiding this comment.
The underlying issue is that the receiving location is created when we go to the partial receiving page when logged into one location.
https://github.com/openboxes/openboxes/blob/develop/grails-app/controllers/org/pih/warehouse/partialReceiving/PartialReceivingController.groovy#L16
And then the user changes the destination and then goes back to the partial receiving page.
It appears as though this doesn't address the issue as it appear that we are changing the parent location of this receiving location. We're just finding it in a different way and then receiving it into another facilities bin location.
There was a problem hiding this comment.
In addition, this scares me because I assume let's say something created a parent location using the same naming convention that we're using for receiving location. I don't think the receiving and putaway feature would be happy if instead of a bin location, it was given a facility location.
There's also a case where we partially receive into the receiving location in one facility and then change the destination (can we even do that?) and then receive the rest of the items into another facility.
Let's ask Kelsey how she expects this to behave.
awalkowiak
added
the
status: work in progress
|
@jmiranda I fixed it according to the request however, there is one thing that I am not sure about. Parent location can't have empty organization. I am not sure what there should be done if there is a case when user will assign destination that has no organization provided (if this is possible), right now there will be an error (due to failOnError during save) but perhaps there is something that I should do better here? |
awalkowiak
added
awaiting feedback
status: work in progress
awalkowiak
removed
awaiting feedback
status: work in progress
|
@jmiranda done |
| @@ -350,6 +376,20 @@ class StockMovementApiController { | |||
| if (packPageItems) { | |||
| createPackPageItemsFromJson(stockMovement, packPageItems) | |||
| } | |||
|
|
|||
| if (stockMovement.statusCode == StockMovementStatusCode.DISPATCHED.toString()) { | |||
| Location inventoryLocation = Location.findByLocationNumber(stockMovement.identifier) | |||
There was a problem hiding this comment.
I still think we want to look this up by both the parent location (stockMovement.destination) and receiving location. And I believe the identifier is only part of the receiving location name. See LocationService.getReceivingLocationName().
There was a problem hiding this comment.
Also, why did you decide to add the logic to the bindStockMovement() method? It feels like the updateShipment() method would be a better place.
There was a problem hiding this comment.
Reg 1st comment: After binding, I think I won't see the old destination, hence I was looking by the locationNumber which is unique and is assigned during the creation of an internal location while entering receipt. But sure I'll revisit it and try to look for this location with LocationService.getReceivingLocationName() and LocationService.findInternalLocation().
Reg 2nd comment: I do not recall the particular reason, I think that while I was debugging it I stopped there. I think you're right, the updateShipment will be better.
There was a problem hiding this comment.
Reg Reg 1st I see that requisition and shipment still have this data.
| @@ -104,7 +112,13 @@ class StockMovementApiController { | |||
| */ | |||
| def updateShipment = { //StockMovement stockMovement -> | |||
| StockMovement stockMovement = stockMovementService.getStockMovement(params.id) | |||
| bindStockMovement(stockMovement, request.JSON) | |||
| try { | |||
There was a problem hiding this comment.
I don't mind adding these here, but it seems like this would already happen within the ErrorsController since the exception thrown by bindStockMovement would propagate to the ErrorsController and be rendered properly there. Was there something that made you decide to handle the errors within each of the actions?
There was a problem hiding this comment.
You're right. I just automaticaly coverd it with try catch, because I forgot about ErrorController.
… after destination change
|
@jmiranda I got rid of try catches, I am getting the inventory location by parent location and name and I moved logic to StockMovementService. |
Awesome. I'll take a look at the Travis tests. |
… after destination change