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

Fix: changing distribution type should show accurate percentage #4201

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

davecreaser
Copy link
Contributor

Description

  • Change the way the total percentage is being calculated to not do any rounding until the final step, increasing accuracy.
  • This also fixes a bug where switching from distribution type equal (which will always be 100%) to unequal suddenly makes the percentage not 100%.

Testing

  • Step 1. Make a split payment using equal as the distribution type. Have an awkward number of recipients compared to the total amount so that the percentages have a number of decimals.
Screenshot 2025-01-28 at 17 08 48
  • Step 2. Change the distribution type to unequal. The total percentage should still say 100%, and you should be able to submit the form.
Screenshot 2025-01-28 at 17 09 08
  • Step 3. Play around with different amounts / recipients with an uneven distribution type. The percentage should always be correct.
Screenshot 2025-01-28 at 17 09 49 Screenshot 2025-01-28 at 17 10 03

Diffs

Changes 🏗

  • Change the way the total percentage is calculated for split payments

Resolves #3856

@davecreaser davecreaser requested a review from a team as a code owner January 28, 2025 17:10
@davecreaser davecreaser self-assigned this Jan 28, 2025
Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

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

Nice job, this stuff was pretty pesky, nicely ironed out 💯

  1. Created the classic "let's make .33333333333 decimals" payments
    image
  2. Changing to unequal leaves everything intact
    image

A bug I've found (which probably makes no sense fixing here), but if we add values beyond the 4th decimal, it will allow creating the action
image
but since we round to 4 decimals, the end result is correct
image

kinda confusing 🤔

anyhow, this doesn't affect this PR so I'll gladly smash approve 🚢

Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Really nice work on this one @davecreaser 💯

Tested it and seems the values do add up to 100% when switching from equal to unequal distribution type
Screenshot 2025-01-30 at 14 00 08
Screenshot 2025-01-30 at 14 00 53
Screenshot 2025-01-30 at 14 01 54

Screen.Recording.2025-01-30.at.14.02.50.mov

Nice!

Comment on lines +67 to +83
export const getUnevenSplitPaymentTotalPercentage = (
amount: number,
recipients: SplitPaymentRecipientsFieldModel[],
) => {
if (!amount) {
return 0;
}

const total =
recipients?.reduce((acc, recipient) => {
return acc + Number(recipient.amount);
}, 0) || 0;

const percentage = (100 * total) / amount;

return Number(percentage.toFixed(4));
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

Very nice! Tested both equal and unequal and changed various percentages.

Everything is acting as expected.

Very nice fix! 🎉

Screencast.from.2025-01-30.18-49-04.mp4

@davecreaser davecreaser merged commit c937529 into master Jan 30, 2025
2 checks passed
@davecreaser davecreaser deleted the fix/#3856-fix-split-payment-percentages branch January 30, 2025 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants