Skip to content
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

Merged
merged 7 commits into from
May 6, 2021
Merged

Rumer report #5599

merged 7 commits into from
May 6, 2021

Conversation

lomamech
Copy link
Collaborator

  • Implement the rumer report
    Note: This PR requires the execution of updates in the migrate file

closes #5395

- Implement the rumer report
Note: This PR requires the execution of updates in the migrate file

closes Third-Culture-Software#5395
@lomamech lomamech requested a review from jniles April 30, 2021 10:11
@lomamech
Copy link
Collaborator Author

image

- Use table stock_movement_status instead stock_movement
@lomamech
Copy link
Collaborator Author

lomamech commented May 1, 2021

image

Rumer Report with stock_movement_status data

@jniles
Copy link
Collaborator

jniles commented May 5, 2021

@lomamech, can you rebase this to master?

Copy link
Collaborator

@jniles jniles left a 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>
Copy link
Collaborator

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>
Copy link
Collaborator

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
Copy link
Collaborator

@jniles jniles May 5, 2021

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(?)
Copy link
Collaborator

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(?)
Copy link
Collaborator

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
Copy link
Collaborator

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 => {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is correct.

Copy link
Collaborator

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>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename this column to "Total Exits"? It would match the "Total Entries" at the beginning a lot better.

image

inventory.dailyConsumption = dailyConsumption;
});

if (inventoriesOpening.length) {
Copy link
Collaborator

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; }
  });
});

lomamech added 2 commits May 6, 2021 13:08
- Remove inventories out of stock and 0 movement
@lomamech
Copy link
Collaborator Author

lomamech commented May 6, 2021

image

@jniles
Copy link
Collaborator

jniles commented May 6, 2021

bors try

bors bot added a commit that referenced this pull request May 6, 2021
@bors
Copy link
Contributor

bors bot commented May 6, 2021

try

Build 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.
Copy link
Collaborator

@jniles jniles left a 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,
Copy link
Collaborator

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 = `
Copy link
Collaborator

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.

@jniles
Copy link
Collaborator

jniles commented May 6, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented May 6, 2021

Build succeeded:

@bors bors bot merged commit c1fc6be into Third-Culture-Software:master May 6, 2021
@lomamech lomamech deleted the rumerReport branch June 28, 2021 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the RUMER report
2 participants