bhima
bhima copied to clipboard
Fix stock exit via shipment action menu
I asked @jniles to take a look at this problem. He suggested the fix in this PR. Thanks @jniles !
Note that this test could be tried on a production server by inserting this new line at client/src/modules/stock/exit/exit.js at line 231:
vm.validate();
If this does not work, We will need to try doing some debugging on the server and client sides to see why all items are not getting from the server to the client.
Closes https://github.com/IMA-WorldHealth/bhima/issues/6809
TESTING
- Ideally do this with a server on a separate computer from the client
- Create a shipment with many items and mark it ready for shipping
- In the Shipments Registry, use the action menu for the new shipment and do the "Exit stock for this shipment" command
- Verify that all the items in the shipment appear in the stock exit
So, my suggestion to improving this code is to move the following lines:
https://github.com/IMA-WorldHealth/bhima/blob/9f6430e2d23d0bdba046a4c8212e9a96078cecba/client/src/modules/stock/exit/exit.js#L217-L235
Into the StockExitFormService
. Typically in AngularJS, we like to think of the controller as just the "glue" that binds the view (our templates) to the model (in this case, our services). The design of the StockExitFormService
was to try and mimic this, removing as much of the state out of the controller as possible, leaving it to update/configure the view.
That is why we have things like vm.stockForm.setDepotDistribution()
and related functions. As a side effect, we can also test this much easier - if we call that function, how should the state change? We know that that view should change if AngularJS is working as expected. But what we care about is model/state, since that is where our logic is.
So, my suggestion is this: Create a new method of StockExitFormService
called setDistributionFromShipment()
. The method should take in the shipment UUID.
Within that method, you'll do all of the above code, but you'll also add in a few things:
- You don't need to "find" the distribution type for the
setDistributionFromShipment()
. Hard code it. If the developer called this function, that means they want that UUID to be a shipment and if it is not, throw an error. - Make sure you increment and decrement the
this._queriesInProgress
variable (https://github.com/IMA-WorldHealth/bhima/blob/master/client/src/modules/stock/StockExitForm.service.js#L62) That will ensure that the loading behaves as you expect. You might want to wait until theStockExitFormService.isLoading()
is done to even start looking up the shipment to avoid a race condition. - Internally, once everything is finished loading, you can call the
this.validate()
method to ensure that all your errors are shown to the client.
That is how I would approach it.
What is probably currently happening is a race condition between the loading of the shipment and the loading of the depot stock. But without a good way to reproduce this, it is just a guess.
Also, I highly encourage you to never pass more than a UUID between states, unless they are some sort of edit/update modal on the same page. I suppose you could think of it as a child/parent relationship. But if $state.params.shipment
is an object, you are already in risky territory. Don't do it. Pass just the uuid, then reload the shipment information from the database yourself in the service/controller.
This makes sure that, if the shipment is already exited via another computer's interface, the stock exit will error rather than do a duplicate exit. It's not fool proof, but it reduces your chance of having stale data.
Thanks for the suggestions!
@jmcameron what is the state of this PR?
@jmcameron do you need another review?
@jmcameron do you need another review?
Yes, this is ready for another test. If it works, I still need to clean up some things before releasing it.
Temporarily disable