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

[MPDX-8227] Show multiple 14 month partner currency tables #1076

Merged
merged 15 commits into from
Sep 19, 2024

Conversation

canac
Copy link
Contributor

@canac canac commented Sep 16, 2024

Description

The 14 month partner currency report was only showing one table even when there were multiple currencies given. This PR fixes that in the UI and in the CSV export. It also does a fair amount of refactoring to move complex logic into independent helper functions. Lastly, it adds tests for those helper functions.

@dr-bizz The print styles you added were causing weird issues with multiple tables, so I had to remove a lot of them. It still seems to print well, but I'd love your feedback on whether I unintentionally removed some crucial print styles.

@wrandall22 This should fix the rest of the inconsistencies I was seeing in CSV exports. There are still differences in rounding (Angular appears to round down, and React rounds to the nearest dollar, which I believe is more correct). I'm investigating one more discrepancy involving totals, but Angular seems clearly wrong in this case because the export includes a number that's no where on the table in the UI.

MPDX-8227

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

@canac canac added the Preview Environment Add this label to create an Amplify Preview label Sep 16, 2024
@canac canac requested a review from dr-bizz September 16, 2024 21:39
Copy link
Contributor

Preview branch generated at https://8227-14-month-currencies.d3dytjb8adxkk5.amplifyapp.com

Copy link
Contributor

github-actions bot commented Sep 16, 2024

Bundle sizes [mpdx-react]

Compared against bc5798e

No significant changes found

@canac canac changed the title [MPDX-8227] Show multiple 14 partner currency tables [MPDX-8227] Show multiple 14 month partner currency tables Sep 16, 2024
@wrandall22
Copy link
Contributor

wrandall22 commented Sep 17, 2024

Some things I still see as discrepancies (looking at exports):

React has (and Angular does not have):
Commitment amount, Commitment currency, Commitment frequency (these are all in Angular, but formatted differently)
Committed monthly equivalent, In hand monthly equivalent, Missing in hand monthly equivalent, In hand special gifts, In hand date range

Angular has (and React does not have):
Pledge (this is a combined field of those mentioned above), Average, Minimum, Maximum

Angular formats the status in a more readable manner. The dates are formatted differently between the two exports. Angular has the currency symbol, React does not.

@wrandall22
Copy link
Contributor

The multiple tables stuff is looking pretty good from my perspective. I do notice that the Avg and Min now get calculated differently between Angular and React on both reports (Angular has 42, React has 84). React is probably more accurate in this case.

@canac
Copy link
Contributor Author

canac commented Sep 17, 2024

React has (and Angular does not have): Commitment amount, Commitment currency, Commitment frequency (these are all in Angular, but formatted differently) Committed monthly equivalent, In hand monthly equivalent, Missing in hand monthly equivalent, In hand special gifts, In hand date range

Angular has (and React does not have): Pledge (this is a combined field of those mentioned above), Average, Minimum, Maximum

This is because the PR to add those fields to Angular is in staging but never merged into production. If you compare an export from this branch's preview environment and an export from Angular staging, they're nearly identical, ignoring rounding differences.

Angular formats the status in a more readable manner. The dates are formatted differently between the two exports. Angular has the currency symbol, React does not.

I changed the date formatting to be more similar (i.e. MM YY for en-US). It's not always identical because we now format dates in a locale-dependent way. I added the currency symbol to React as well.

Angular also gives a different value for the totals of foreign currencies. Here is a condensed version of the output with extraneous totals removed. The totals for GPB and UGS should be 10,000 and 333,333,333, respectively, not 12,719 and 895. I believe that the React is correct in this case.

Angular

Currency GBP £
Partner Status Commitment Amount Commitment Currency Commitment Frequency 24-Sep 24-Aug Total (last month excluded from total)
Duck, Daffy and Daphney partner_financial 30 USD Monthly 0 10000 10000
Totals 0 10000 12719
Currency UGS UGS
Partner Status Commitment Amount Commitment Currency Commitment Frequency 24-Sep 24-Aug Total (last month excluded from total)
Duck, Donald and Daisy 0 USD 333333333 0 333333333
Totals 333333333 0 895
Currency USD $
Partner Status Commitment Amount Commitment Currency Commitment Frequency 24-Sep 24-Aug Total (last month excluded from total)
Duck, Daffy and Daphney partner_financial 30 USD Monthly 84 0 84
Totals 84 0 84

React

Currency USD $
Partner Status Commitment Amount Commitment Currency Commitment Frequency 24-Sep 24-Aug Total (last month excluded from total)
Duck, Daffy and Daphney partner_financial 30 USD Monthly 84 0 84
Totals 84 0 84
Currency UGS UGS
Partner Status Commitment Amount Commitment Currency Commitment Frequency 24-Sep 24-Aug Total (last month excluded from total)
Duck, Donald and Daisy 0 USD 333333333 0 333333333
Totals 333333333 0 333333333
Currency GBP £
Partner Status Commitment Amount Commitment Currency Commitment Frequency 24-Sep 24-Aug Total (last month excluded from total)
Duck, Daffy and Daphney partner_financial 30 USD Monthly 0 10000 10000
Totals 0 10000 10000

@canac canac requested a review from wrandall22 September 18, 2024 21:38
@canac
Copy link
Contributor Author

canac commented Sep 18, 2024

@wrandall22 Do you mind reviewing this since Daniel is going to be out for the rest of the week?

@canac canac force-pushed the 8227-14-month-currencies branch from b351161 to 9edb045 Compare September 19, 2024 18:41
@canac canac enabled auto-merge September 19, 2024 18:41
@canac canac merged commit bbac2f1 into main Sep 19, 2024
18 checks passed
@canac canac deleted the 8227-14-month-currencies branch September 19, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Preview Environment Add this label to create an Amplify Preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants