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

feat: add FMV column to purchases #4328

Merged

Conversation

sean-dickinson
Copy link
Collaborator

Resolves #4314

Description

  • Adds the FMV Column to the purchases index page
  • Updates the purchases index controller action to eager load the item of each line_item to avoid n+1 issues.
    • Note I also added vendor here to the includes clause as well as I noticed the n+1 in the logs. This is an unrelated change so if you'd like me to revert that I'd be happy to.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Added a new spec to the purchases system spec
  • Tested manually by viewing the page (with seeded data)

Screenshots

Before
Screenshot 2024-05-03 at 19-28-41 Purchases - Pawnee Diaper Bank

After
Screenshot 2024-05-03 at 19-26-16 Purchases - Pawnee Diaper Bank

Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

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

Overall looks good, but had one request.

@@ -47,6 +47,15 @@
expect(page).to have_text(dollar_value(purchases.sum(&:amount_spent_in_cents)))
expect(page).to have_text(dollar_value(3579))
end

it "user sees FMV column" do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be a request spec instead of a system spec? It's a lighter load.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure thing. I was following the pattern of the test above. Do you want me to move that one too? Or maybe just add a to-do comment to keep this PR scoped to this change?

@sean-dickinson sean-dickinson force-pushed the 4314-add-fmv-column-to-purchases-index branch from 7b5cc2b to 338f791 Compare May 6, 2024 18:05
@sean-dickinson sean-dickinson requested a review from dorner May 6, 2024 18:05
Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

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

Looks good! @cielf did you want to take a look?

@cielf
Copy link
Collaborator

cielf commented May 7, 2024

I think we can put this forward, and that there should be a couple of follow-on things that I'll write up and get into our backlog --
1/ our header alignments could use some work, which will make a series of Good First Issues, and
2/ FMV is a total-able column, so we should give the totals as well. @sean-dickinson: If you're interested in working on putting the totals in, let me know, and I'll prioritize getting the issue groomed.

@cielf cielf merged commit efd5f80 into rubyforgood:main May 7, 2024
19 checks passed
@sean-dickinson
Copy link
Collaborator Author

I think we can put this forward, and that there should be a couple of follow-on things that I'll write up and get into our backlog -- 1/ our header alignments could use some work, which will make a series of Good First Issues, and 2/ FMV is a total-able column, so we should give the totals as well. @sean-dickinson: If you're interested in working on putting the totals in, let me know, and I'll prioritize getting the issue groomed.

I'd be happy to add the totals

@sean-dickinson sean-dickinson deleted the 4314-add-fmv-column-to-purchases-index branch May 7, 2024 19:21
Copy link
Contributor

@sean-dickinson: Your PR feat: add FMV column to purchases is part of today's Human Essentials production release: 2024.05.12.
Thank you very much for your contribution!

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.

[Feature] Add FMV to purchases index
3 participants