Skip to content

Conversation

@GiovannyCordeiro
Copy link
Contributor

Resolves #5004

Description

Bug fix (non-breaking change which fixes an issue)

Type of change

  • Changes to the “Itemized donation” and “Itemized distributions” pages by removing “Unknown” to zero
  • Tests have been carried out to guarantee this change

How Has This Been Tested?

I ran a test by creating a fictitious donation of five units, followed by a simulated distribution of those same units. In this way, the visualization in the "reports" tab of both windows showed zero, as expected.

@GiovannyCordeiro GiovannyCordeiro force-pushed the 5004-refact-unknow-for-zero branch 2 times, most recently from ae07152 to 761a5f8 Compare June 11, 2025 20:19
@cielf cielf requested a review from dorner June 12, 2025 19:07
@cielf
Copy link
Collaborator

cielf commented Jun 12, 2025

Functionality looks good. Asking @dorner for a review.

@GiovannyCordeiro GiovannyCordeiro force-pushed the 5004-refact-unknow-for-zero branch from 761a5f8 to 74c3bc3 Compare June 16, 2025 22:03
@cielf
Copy link
Collaborator

cielf commented Jun 17, 2025

Hey @GiovannyCordeiro A process note here -- please avoid force pushes once we've started reviews.

@GiovannyCordeiro
Copy link
Contributor Author

Oh... Sorry @cielf, I just wanted the branch to be up to date with the main branch for revision.

@cielf
Copy link
Collaborator

cielf commented Jun 18, 2025

Rechecked the functional - still looks good.

@cielf
Copy link
Collaborator

cielf commented Jun 18, 2025

Hrmm @GiovannyCordeiro It looks like there is a test failing that is related to your work.

@GiovannyCordeiro
Copy link
Contributor Author

The error was happening because I didn't set the filter to enter the range of the donation creation I made. Sorry.

But I've fixed it!

@cielf
Copy link
Collaborator

cielf commented Jun 18, 2025

Very good! The next step is @dorner's review -- to set expectations, that may take up to 2 weeks due to him being extra busy atm.

@@ -0,0 +1,99 @@
RSpec.describe "Reports Distributions", type: :system, js: true do
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're not really testing interactivity here - if we know the parameters we can just send them in and inspect the output. System tests are a lot slower and flakier - can we move this to a request test instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK! I'll do it!

@cielf
Copy link
Collaborator

cielf commented Jul 13, 2025

@GiovannyCordeiro is this ready for re-review?

@GiovannyCordeiro
Copy link
Contributor Author

Not yet. Unfortunately, I haven't been able to dedicate time to it in the last two weeks. I still need to identify why the test logic doesn't work properly when a code change occurs. I hope to take a look tomorrow. Sorry, @cielf

@cielf
Copy link
Collaborator

cielf commented Jul 20, 2025

I believe @GiovannyCordeiro said they have to go quiet for awhile on another issue.

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.

Total on Hand is Unknown for inactive Items on Itemized Reports -- should be 0.

3 participants