bhima icon indicating copy to clipboard operation
bhima copied to clipboard

Fix stock exit via shipment action menu

Open jmcameron opened this issue 2 years ago • 6 comments

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

jmcameron avatar Sep 07 '22 01:09 jmcameron

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:

  1. 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.
  2. 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 the StockExitFormService.isLoading() is done to even start looking up the shipment to avoid a race condition.
  3. 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.

jniles avatar Sep 12 '22 14:09 jniles

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.

jniles avatar Sep 12 '22 14:09 jniles

Thanks for the suggestions!

jmcameron avatar Sep 12 '22 14:09 jmcameron

@jmcameron what is the state of this PR?

mbayopanda avatar Sep 14 '22 10:09 mbayopanda

@jmcameron do you need another review?

mbayopanda avatar Sep 19 '22 10:09 mbayopanda

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

jmcameron avatar Sep 19 '22 14:09 jmcameron

Temporarily disable

jmcameron avatar Aug 23 '23 12:08 jmcameron