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-8192] Do not allow fractional admin percentages #1058

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

canac
Copy link
Contributor

@canac canac commented Sep 11, 2024

Description

Require the admin percentage to be a whole number.

MPDX-8192

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 11, 2024
@canac canac requested a review from dr-bizz September 11, 2024 20:57
Copy link
Contributor

Preview branch generated at https://8192-validate-percentage.d3dytjb8adxkk5.amplifyapp.com

Copy link
Contributor

github-actions bot commented Sep 11, 2024

Bundle sizes [mpdx-react]

Compared against 1531998

No significant changes found

Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

The Admin Percentage looks great. Can you add the rule of whole numbers to initial goal and letter cost?

I know I didn't specify that on the Task.
Screenshot 2024-09-12 at 9 28 59 AM

@@ -177,7 +177,7 @@ const appealFormSchema = yup.object({
.test(
i18n.t('Is positive?'),
i18n.t('Must use a positive number for Initial Goal'),
(value) => parseFloat(value as unknown as string) >= 0,
(value) => typeof value === 'number' && value >= 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

When testing on https://8192-validate-percentage.d3dytjb8adxkk5.amplifyapp.com/
I was still able to enter a number with a decimal point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying the admin percentage validation is good but there isn't validation yet on the initial goal?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes that is correct

@canac
Copy link
Contributor Author

canac commented Sep 12, 2024

Can you add the rule of whole numbers to initial goal and letter cost?

@dr-bizz It kind of seems weird to me that we're trying to keep users from entering fractional amounts, but the goal still probably won't probably be a whole number (e.g. with the numbers below). It feels like we should be rounding the goal amount too if we want everything to be a whole number. Let me know what you think.

Screenshot 2024-09-12 at 10 21 31 AM

Also, the goal amount is editable in Angular but not in React. That needs fixed too.

@dr-bizz
Copy link
Contributor

dr-bizz commented Sep 13, 2024

@dr-bizz It kind of seems weird to me that we're trying to keep users from entering fractional amounts, but the goal still probably won't probably be a whole number (e.g. with the numbers below). It feels like we should be rounding the goal amount too if we want everything to be a whole number. Let me know what you think.

This is how the old MPDX was, not saying it's correct. But you can bring this up with Scott and see if he wants to make the change. I'm okay with it if he is.

@dr-bizz
Copy link
Contributor

dr-bizz commented Sep 13, 2024

I've created a task for the Goal input - MPDX-8208

@canac
Copy link
Contributor Author

canac commented Sep 13, 2024

If we're duplicating Angular, the Angular version allows you to enter fractional amounts for all four fields, including the admin percentage.

Screenshot 2024-09-13 at 9 04 26 AM

@dr-bizz
Copy link
Contributor

dr-bizz commented Sep 13, 2024

If we're duplicating Angular, the Angular version allows you to enter fractional amounts for all four fields, including the admin percentage.

Screenshot 2024-09-13 at 9 04 26 AM

Have you tried clicking submit?

@canac
Copy link
Contributor Author

canac commented Sep 13, 2024

@dr-bizz My bad, you're right 🤦‍♂️. I'll add the whole number validation to the three fields.

@dr-bizz
Copy link
Contributor

dr-bizz commented Sep 13, 2024

All good. That got me the first time as well.

@canac canac requested a review from dr-bizz September 13, 2024 14:38
Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

Awesome work! 🔥💯

@canac canac merged commit 1b15d8a into main Sep 13, 2024
18 checks passed
@canac canac deleted the 8192-validate-percentage branch September 13, 2024 15:04
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