-
Notifications
You must be signed in to change notification settings - Fork 105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rumer report #5599
Rumer report #5599
Conversation
- Implement the rumer report Note: This PR requires the execution of updates in the migrate file closes Third-Culture-Software#5395
- Use table stock_movement_status instead stock_movement
…ader of Rumer Report
@lomamech, can you rebase this to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lomamech just a few comments here. Overall looks really good!
I think my inline comments should be clear enough. Let me know if you have any questions.
{{#each configurationData}} | ||
<tr> | ||
<td> {{ inventoryText }} </td> | ||
<td><strong> {{ quantityOpening }} </strong></td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be class="text-right"
as well.
</td> | ||
{{/each}} | ||
<td><strong> {{ quantityTotalExit }} </strong></td> | ||
<td><strong> {{ quantityEnding }} </strong></td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both quantityTotalExit
and quantityEnding
should be text-right
as well.
|
||
const sqlDailyConsumption = ` | ||
SELECT BUID(sms.inventory_uuid) AS uuid, inv.code, inv.text AS inventory_text, | ||
sms.out_quantity_consumption AS quantity, sms.date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I talked to AG Neron, and apparently this needs to have both the consumption and the exits due to loss. So, this should be changed to
SELECT (sms.out_quantity_consumption + sms.out_quantity_exit) AS quantity
sms.out_quantity_consumption AS quantity, sms.date | ||
FROM stock_movement_status AS sms | ||
JOIN inventory AS inv ON inv.uuid = sms.inventory_uuid | ||
WHERE sms.depot_uuid = ? AND DATE(sms.date) >= DATE(?) AND DATE(sms.date) <= DATE(?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the sms.date
column is a DATE
and not a DATETIME
or TIMESTAMP
, you don't need to wrap it in DATE()
. By wrapping it, you lose a performance gain from the INDEX.
You can change this code to:
WHERE sms.depot_uuid = ? AND sms.date >= DATE(?) AND sms.date <= DATE(?)
SUM(sms.out_quantity_consumption) AS quantity, sms.date | ||
FROM stock_movement_status AS sms | ||
JOIN inventory AS inv ON inv.uuid = sms.inventory_uuid | ||
WHERE sms.depot_uuid = ? AND DATE(sms.date) >= DATE(?) AND DATE(sms.date) <= DATE(?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about DATE
here and the exit
+ consumption
calculation.
const sqlStockEntryMonth = ` | ||
SELECT BUID(inv.uuid) AS uuid, inv.code, inv.text AS inventory_text, SUM(sm.quantity) AS quantity, | ||
sm.date, sm.is_exit | ||
FROM stock_movement AS sm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to get this from stock_movement_status
from the column in_quantity
.
core.getInventoryQuantityAndConsumption(parameterEndingStock), | ||
]); | ||
|
||
inventoriesEnding.forEach(inventory => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be helpful to add an option to remove the inventories with 0
movements. Could you add that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small precision, do you allude to the products whose quantity at the beginning of the month is null and have had no movement during the month
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically allow the user to remove products where the beginning balance is 0, the movements are 0, and the ending balance are 0.
{{#each header as | key |}} | ||
<th style="min-width:20px;" class="text-center">{{key}}</th> | ||
{{/each}} | ||
<th style="width: 7.5%;" class="text-center">{{translate 'FORM.LABELS.TOTAL'}} </th> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inventory.dailyConsumption = dailyConsumption; | ||
}); | ||
|
||
if (inventoriesOpening.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are doing lots of these loops that look very similar.
Can you combine them into a single loop? Instead of checking if inventoriesOpening.length
, then doing a configurationData.forEach()
, just add the inventoriesOpening.forEach()
into the configurationData.forEach()
loop. Like this:
configurationData.forEach(row => {
// first, add the opening balances:
inventoriesOpening.forEach(inv => {
if (row.inventoryUuid === inv.inventory_uuid) { row.quantityOpening = inv.quantity; }
});
// next process the entries
inventoriesEntry.forEach(inv => {
if (row.inventoryUuid === inv.uuid) { row.quantityTotalEntry = inv.quantity; }
});
// finally process the exit quantities
monthlyConsumption.forEach(inv => {
if (row.inventoryUuid === inv.uuid) { row.quantityTotalExit = inv.quantity; }
});
});
- Remove inventories out of stock and 0 movement
bors try |
tryBuild succeeded: |
Avoids repeating too many SQL queries and just uses the same one for all the RUMER fields. Also adds red text when the ending stock is negative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything here looks good to me. I think I see an easy performance improvement, so I'll make that change in a commit myself. After that, this is ready to roll. Great job @lomamech!
|
||
// default values | ||
vm.reportDetails = { | ||
includePurchaseEntry : 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can probably remove this. I don't think it is ever used.
GROUP BY inv.uuid; | ||
`; | ||
|
||
const sqlStockEntryMonth = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like all these use the same query. I think we can probably gain a significant performance boost by combining them. I'll go ahead and make that change in a commit.
bors r+ |
Build succeeded: |
Note: This PR requires the execution of updates in the migrate file
closes #5395