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

HelpScout 1199960 - Fixing intlFormat.ts error when currency is not defined #987

Merged
merged 4 commits into from
Aug 2, 2024

Conversation

dr-bizz
Copy link
Contributor

@dr-bizz dr-bizz commented Aug 1, 2024

Description

In this PR, I've fixed an error that happens when the currency is not defined. If the currency is not defined, it shows the white screen of death.

I've ensured the currency is defined and that even if it fails, it will return some value to prevent the white screen of death from happening.

HelpScout issue: https://secure.helpscout.net/conversation/2666844539/1199960?folderId=7296147

To Test:

  • Impersonate the user in question.
  • Navigate to contacts and click on the contact with the last name "Dose"
  • Click on the donations tab in the contact drawer.
  • The donations should now show without an error.

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

@dr-bizz dr-bizz requested a review from wrandall22 August 1, 2024 15:06
Copy link
Contributor

github-actions bot commented Aug 1, 2024

Bundle sizes [mpdx-react]

Compared against 1b734b6

No significant changes found

@dr-bizz dr-bizz requested a review from wjames111 August 1, 2024 20:49
@dr-bizz dr-bizz added the Preview Environment Add this label to create an Amplify Preview label Aug 1, 2024
Copy link
Contributor

github-actions bot commented Aug 1, 2024

Preview branch generated at https://fix-intl-format-error.d3dytjb8adxkk5.amplifyapp.com

Copy link
Contributor

@wjames111 wjames111 left a comment

Choose a reason for hiding this comment

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

Looks great, not sure I can add much insight into your PR's but I added a few thoughts.

minimumFractionDigits: decimal ? 2 : 0,
maximumFractionDigits: decimal ? 2 : 0,
}).format(Number.isFinite(amount) ? amount : 0);
if (!currency) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we get the same effect by giving it default argument of USD? I guess in that case we’d have to give locale a default as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is what we had before with currency ?? 'USD', but for some reason it was still causing an error. Adding the if statement it prevent the error from happening.


it('handles an error', () => {
expect(currencyFormat(1234.56, ' ', 'en-GB')).toEqual('1234.56 ');
});
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know what happens if an absurd currency is given?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it is and it causes an error, we will revert to only returning the total amount with the currency in the text after it.
See line 36 on src/lib/intlFormat.ts

@wjames111
Copy link
Contributor

I'm getting a server error from the preview. I'm assuming it's unrelated.

@dr-bizz dr-bizz merged commit b77c3de into main Aug 2, 2024
18 checks passed
@dr-bizz dr-bizz deleted the fix-intl-format-error branch August 2, 2024 15:05
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.

2 participants