Remove the inventory.avg_consumption column #5144
Assignees
Labels
Comments
|
Note, I've worked on removing this value from queries in #5143 |
jniles
added
the
priority:urgent
jniles
added a commit
to jniles/bhima
that referenced
this issue
Feb 4, 2021
This commit removes the avg_consumption column in the inventory table since it does not play a role anymore. Additionally, it unifies the inventory databases to using the same test/data/inventories.sql file. Closes Third-Culture-Software#5144.
jniles
added a commit
to jniles/bhima
that referenced
this issue
Feb 9, 2021
This commit removes the avg_consumption column in the inventory table since it does not play a role anymore. Additionally, it unifies the inventory databases to using the same test/data/inventories.sql file. Closes Third-Culture-Software#5144.
jniles
added a commit
to jniles/bhima
that referenced
this issue
Feb 15, 2021
This commit removes the avg_consumption column in the inventory table since it does not play a role anymore. Additionally, it unifies the inventory databases to using the same test/data/inventories.sql file. Closes Third-Culture-Software#5144.
jniles
added a commit
to jniles/bhima
that referenced
this issue
Feb 16, 2021
This commit removes the avg_consumption column in the inventory table since it does not play a role anymore. Additionally, it unifies the inventory databases to using the same test/data/inventories.sql file. Closes Third-Culture-Software#5144.
jniles
added a commit
to jniles/bhima
that referenced
this issue
Feb 17, 2021
This commit removes the avg_consumption column in the inventory table since it does not play a role anymore. Additionally, it unifies the inventory databases to using the same test/data/inventories.sql file. Closes Third-Culture-Software#5144.
jniles
added a commit
to jniles/bhima
that referenced
this issue
Feb 18, 2021
This commit removes the avg_consumption column in the inventory table since it does not play a role anymore. Additionally, it unifies the inventory databases to using the same test/data/inventories.sql file. Closes Third-Culture-Software#5144.
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>
mbayopanda
pushed a commit
to mbayopanda/bhima
that referenced
this issue
Mar 2, 2021
This commit removes the avg_consumption column in the inventory table since it does not play a role anymore. Additionally, it unifies the inventory databases to using the same test/data/inventories.sql file. Closes Third-Culture-Software#5144.
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
We don't actually need the
inventory.avg_consumptioncolumn anymore since we don't store the average consumption in there anymore. Instead we calculate it. I think we should remove it, but maybe I'm missing a good reason to keep it around.@mbayopanda @lomamech what do you think?
The text was updated successfully, but these errors were encountered: