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

Reduce distribution value totals by item category filter on distribution index page #4924

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

coalest
Copy link
Collaborator

@coalest coalest commented Jan 15, 2025

Doesn't resolve any issue. Extracted from #4806

Description

Previously. when filtering distributions by item names or item categories, the column for total quantity of items in each distribution was reduced but the value column was not. (So if you have 5 items in category diapers for a given distribution, the quantity would show 5 but the value would show whatever the full value of the distribution is, not just the value of those diapers.)

This PR changes this behavior so that the value column now behaviors similar to the quantity column. The value column should now show only the value of the items of the filtered item or item category.

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Updated tests to match the new behavior.

Screenshots

Note the changes in the value column in the screenshots below:

Before:

image

After:

image

@coalest coalest requested review from cielf and dorner January 15, 2025 12:58
@coalest coalest changed the title Reduce distribution value totals by item filters on distribution index page Reduce distribution value totals by item categroy filter on distribution index page Jan 16, 2025
@coalest coalest changed the title Reduce distribution value totals by item categroy filter on distribution index page Reduce distribution value totals by item category filter on distribution index page Jan 16, 2025
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

It turns out that we've got some inconsistencies in how we are handling exports prior to this. In that we are treating filtering by items and filtering item by category differently. We need to expand this to include fixing that (much as I don't usually want any scope creep, I don't see an incremental path that makes sense).

So, with that in mind, could you, in the exports:
1/ for total values, make the filter by item work the same way as the filter by item category (i.e. it shows the number of items that match the filter),
2/ for the Total number of items, for the category filter , show the number of items in the category
3/ In both the item and item category cases, make the headers for the number of items and the total value indicate the filter (we do that for number of items for the item filter now).

This should leave us in a situation where the total number of items and the total value in the export will match the values in the index in all cases.

Thank you.

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

The above comment should be requested changes.

@coalest
Copy link
Collaborator Author

coalest commented Jan 17, 2025

I'm a little confused by what you mean. I just pushed a commit as to what I think you meant.

Basically filtering by item or item category reduces both total number and total value of the distributions in both the index page and in the export csv. And the export csv headers include the filters

@cielf
Copy link
Collaborator

cielf commented Jan 18, 2025

Looks to me like you interpreted correctly. Thanks! LGTM.

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.

2 participants