Store type of movements in stock_movement_status #5360
Labels
Comments
This was referenced Feb 17, 2021
bors bot
added a commit
that referenced
this issue
Feb 22, 2021
5372: Rework stock_movement_status calculation r=jmcameron a=jniles This is a full rewrite of the `stock_movement_status` table and associated stored procedures. The goal of this rewrite is to improve clarity, remove user-defined variables that may potentially collide across MySQL connections, and improve the performance of the `GetCMM()` calculation. Closes #5073. Closes #5144. Closes #5332. Closes #5360. ----    This PR contains many far reaching changes. Here is a lot of the notable ones: 1. I've removed the `avg_consumption` column from the inventory table. It was never used, as far as I can tell, to do any reporting. It was used in the stock import feature, but we don't use the `avg_consumption` to report on, so I just removed that code. 2. I've removed the `stock_consumption` table and associated procedures. It is used in several reports and they are now broken. However, I would argue they were always broken since the `stock_consumption` did not record accurate information about stock consumption. 3. I've entirely changed the structure behind the `stock_movement_status` table for the following reasons: 1. The [previous structure](https://github.com/IMA-WorldHealth/bhima/blob/e455a8011db69ad48839afc35977d3dd041d3241/server/models/schema.sql#L2016) was very hard to audit. For example, it had `start_date`, `end_date`, and `quantity`. Is that the quantity at the start? The quantity at the end? If I wanted to find the value on date X, I needed to do a BETWEEN query. 2. The table did not distinguish between consumption information and exit information. This made the `GetCMM()` calculation have to look up additional information from the `stock_movement` table. A user would need to as well. 3. In theory, we'll be calling `GetCMM()` more than we will be moving stock. It makes sense to make the performance tradeoff to increase the speed of `GetCMM()` and decrease the speed of inserting stock. 5. I've renamed the `GetCMM()` call to `GetAMC()` for clarity. It also takes in a single date and uses the depot to find the correct value of the date range. This prevents us from having inconsistent results because a developer didn't properly compute the date range. If we need to support a custom date range, we'll just write another SP. 6. I've slightly changed the API of `GetAMC()`: 1. It returns `head_days` and `tail_days` instead of `days_before_stock_consumption`. That is because `days_before_stock_consumption` is the real number of days before the first consumption. Instead, it the days between the start of the window and the next record. Similarly, `tail_days` gives you the number of days between the last consumption record and the end of the window. 2. It returns the `quantity_in_stock` so that we can perform stock out date calculations. 3. `first_inventory_movement_date` -> `min_date` 4. `last_inventory_movement_date` -> `max_date` ------ It isn't all good. In fact, initializing this is terrible. On my machine (4GB of RAM): ```mysql mysql> use vanga; mysql> CALL zRecomputeStockMovementStatus(); Query OK, 0 rows affected (22 min 4.81 sec) mysql> use imck; mysql> CALL zRecomputeStockMovementStatus(); Query OK, 0 rows affected (4 min 20.31 sec) ``` This is _horrible_. I did get a lot faster speeds when I was just testing on MySQL 8, but the syntax didn't work on MySQL 5.7 (#5364). When I fixed it on MySQL5.7, I ran into issues on MySQL 8. Basically, we got the worst performance of both worlds by trying to get it to work correctly on both worlds. However, I still think it is worth merging because the performance of any individual transaction isn't bad. It is only when rebuilding the database that things go horribly long. ---- ### How to test Download this PR and run the tests! (`yarn test:integration` and `yarn test:integration:stock`). If that works, try with a production database. Note, as above, you'll need to `CALL zRecomputeStockMovementStatus()` to build the table. Co-authored-by: Jonathan Cameron <jmcameron@gmail.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> Co-authored-by: mbayopanda <mbayopanda@gmail.com> Co-authored-by: Jonathan Niles <jonathanwniles@gmail.com>
bors bot
added a commit
that referenced
this issue
Mar 4, 2021
5341: Refactor: Stock Management r=jniles a=jniles This is the off-main-branch work that @jmcameron and @jniles are working on to refactor stock management. It shouldn't be merged until we have finished all the major stock management upgrades and can test it as a whole. The goal of this branch is to do a major upgrade to our stock management modules that may have far reaching implications and are integrated too tightly to be able to do progressively on `master`. Basically, we'll relax our requirements to allow ourselves to "break things" temporarily on this branch, then have a single large merge into the main branch. ### Issues Closed by the PR Closes #4304. Closes #5073. Closes #5144. Closes #5183. Closes #5185. Closes #5186. Closes #5291. Closes #5311. Closes #5319. Closes #5322. Closes #5325. Closes #5326. Closes #5327. Closes #5328. Closes #5332. Closes #5333. Closes #5349. Closes #5354. Closes #5360. Closes #5416. Closes #5422. Closes #5430. Closes #5431. Closes #5444. Closes #5447. ### Things to Test Report/Module | Assigned to | Issues Found | Works on master? (Y/N) | Fix Here? (Y/N) | Fixed? (Y/N) -- | -- | -- | -- | -- | -- Rapports > Articles en Stock | @mbayopanda | Status, CMM, etc broken | N | Y | Rapports > Comparaison des articles factures sur les recus | @jniles | #5414 | Y | N | Rapports > Consommations Menseulles | @jmcameron | SQL Error | Y | Y | Y (#5436) Rapports > Entrees des stocks | @mbayopanda | | Y | N | Rapports > Fiche de stock | @jniles | #5415 | Y | Y | Y (#5415) Rapports > Rapport d'expiration | @jmcameron | | Y | Y | Y (#5438) Rapports > Rapport graphique des mouvements | @mbayopanda | #5416 | Y | N | Rapports > Sorties des stocks | @jniles | #5419 | Y | N | Rapports > Valeur de stock | @jmcameron | OK | Y | N | Rapports > Rapport graphique de la consommation de stock | @mbayopanda | #5418 | Y | N | Rapports > Rapport sur les variations de stock | @jniles | OK | Y | N | Stock > Ajustement | @jmcameron | OK | Y | N | Stock > Ajustement des stocks | @mbayopanda | #5428 | Y | N | N | Stock > Articles en stock | @jniles | #5422 | Y | Y | Y( #5432) Stock > Assignation de stock | @jmcameron | Y | Y | N | Stock > Entrée de stock | @mbayopanda | #5429 is related to purchase analysis | Y | N | Stock > Gestion de depots | @jniles | #5423 | Y | N | Stock > Importation de stock | @jmcameron | N (wrong num args) | N #5426 | Y | Y (#5427) Stock > Lots en stock | @mbayopanda | #5430, #5431 | Y, Y | N, Y | Stock > Movements de stock | @jniles | #5424 | Y | Y | Y(#5459) Stock > Parametres de stock | @jmcameron | Seems OK | Y | N | Stock > Requisition | @mbayopanda | Seems OK | Y | N | Stock > Sortie de stock | @jniles | #5425 | N | N | Stock > Tableau de bord | @jmcameron | #5400 (comment) | Y | N | I've left @lomamech off this list because I believe he is working on a separate PR. If he gets done with that, we'll move some of these over to him. @mbayopanda can you review the titles of these modules? I think the French may be wrong for some of them. For example, I don't understand why we have "Ajustement des stocks" and "value de stock". Shouldn't we use "des stocks" or "de stock" in every one? Co-authored-by: Jonathan Niles <jonathanwniles@gmail.com> Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com> Co-authored-by: Jonathan Cameron <jmcameron@gmail.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently, our
stock_movement_statustable looks like this:One issue is that we have to do extra work to figure out what kind of movement these movements represent. Not all exists are consumption events, and they depend on the type of movement and the depot settings.
To ease our calculations, let's encode these directly into the
stock_movement_statustable. Let's add the following columns:out_quantity_consumption- the quantity exiting the depot via a consumption event.out_quantity_exit- the quantity exiting the depot via a normal exit.These would replace the
out_quantityamount, since you could recover that simply by summing.It might also be useful to have the
start_quantityandend_quantityjust to be explicit. Right now, it is unclear if thequantityrefers to be or after the movement.The text was updated successfully, but these errors were encountered: