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

Resolve #4746: Add total of FMVs to Purchase Index; Fix pagination of Amount spent #4847

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

Conversation

Hungle2911
Copy link

Resolves #4746

Description

Add totals for the Fair Market Value to the current purchase index page. Fix pagination of Amount spent and FMV.

Type of change

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

How Has This Been Tested?

  • I tested by adding new purchases and calculating new total manually.
  • I added new unit tests

Screenshots

Screenshot 2024-12-11 at 4 39 33 PM

cielf
cielf previously requested changes Dec 12, 2024
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.

Hi @Hungle2911 -- I do a functional review before the technical - I noticed that the "this page" value for the fair market value was wrong, so I dug a little to figure out why.

I also noticed that the body of the page isn't paging properly - but that's also not working on main and is out of the scope of this PR.

app/controllers/purchases_controller.rb Outdated Show resolved Hide resolved
@cielf
Copy link
Collaborator

cielf commented Dec 14, 2024

The out-of-scope problem mentioned above is addressed in PR #4854

@cielf cielf dismissed their stale review December 14, 2024 15:14

Addressed

@cielf
Copy link
Collaborator

cielf commented Dec 14, 2024

The numbers LGTM -- @dorner -- we should get this and 4854 in on the same release.

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 to me - thanks!

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.

Ah dangit - @Hungle2911 there are conflicts with main now.

@cielf
Copy link
Collaborator

cielf commented Dec 15, 2024

Entirely expected. I'll take a look, but I expect them to be easy.

@cielf cielf requested a review from dorner December 15, 2024 00:59
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.

Add total of FMVs to Purchase Index; Fix pagination of Amount spent
3 participants