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

Fix distribution filtering by category bug and refactor for performance (#4590) #4806

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

Conversation

coalest
Copy link
Collaborator

@coalest coalest commented Nov 28, 2024

Resolves #4590

Description

Type of change

How Has This Been Tested?

Added request specs

Screenshots

Below is out of date. That behavior will be moved to a new PR if it is desired
Note changes in table header for value and value amounts are now just of those items. (Many items from seeds have no value which is why the number is either very low or blank.)

Screenshot from 2024-11-28 15-03-44

Screenshot from 2024-11-28 15-03-29

Because we are no longer loading all the line_items and items for every distribution into memory, this /distributions page is now faster. Object allocation dropped significantly and it went from ~480ms to ~170ms in my development environment (seed data + laptop). But not sure what production env/data looks like.

Before:
Screenshot from 2024-11-28 14-29-52
After:
Screenshot from 2024-11-28 14-29-59

@coalest
Copy link
Collaborator Author

coalest commented Nov 28, 2024

@cielf This PR changes the way the values are calculated when filtering distributions by items. This change wasn't requested, but I think it makes sense given that's how the total quantities act currently. Let me know if this is unwanted, and I can take that behavior out of this PR.

@cielf
Copy link
Collaborator

cielf commented Nov 29, 2024

@coalest We'd want to take that column change to the business -- we have a Stakeholder Circle next Wednesday. I can definitely see a benefit to it, but they might have reasons to want to keep the other.

If it's trivial to split that out into a different PR, please do so -- otherwise, we'll hold off on this PR until next week.

@coalest
Copy link
Collaborator Author

coalest commented Nov 30, 2024

@cielf That makes sense. I can split it into a different PR. I will move this PR back to draft until I have removed that bit of logic.

@coalest coalest changed the title Improve distribution filtering and refactor for performance (#4590) WIP: Improve distribution filtering and refactor for performance (#4590) Nov 30, 2024
@coalest coalest requested review from dorner and cielf December 2, 2024 13:23
@coalest coalest changed the title WIP: Improve distribution filtering and refactor for performance (#4590) Improve distribution filtering and refactor for performance (#4590) Dec 2, 2024
@coalest
Copy link
Collaborator Author

coalest commented Dec 2, 2024

I removed the change regarding the value calculations. Ready for review now

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.

From light testing against prod data, this is noticeably faster (not surprised).

@coalest coalest changed the title Improve distribution filtering and refactor for performance (#4590) Fix distribution filtering by category bug and refactor for performance (#4590) Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants