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

Use discount api response for cancellation offer payment date #1384

Merged
merged 2 commits into from
Sep 6, 2024

Conversation

rBangay
Copy link
Contributor

@rBangay rBangay commented Sep 6, 2024

What does this PR change?

Use the discount api response to return the next payment price (after the cancellation offer period ends). This should handle the case where the returning payment price will be different from the price paid before the offer was taken up, a price rise for example 👍

Screenshot 2024-09-06 at 14 19 43

@rBangay rBangay force-pushed the supporter-plus-cancellation-offer-pricing branch from 581ef98 to 74d0a91 Compare September 6, 2024 13:18
@rBangay rBangay marked this pull request as ready for review September 6, 2024 13:21
@rBangay rBangay changed the title using discaount api response to return the next payment price in the … Use discount api response for cancellation offer payment date Sep 6, 2024
Copy link
Member

@tjmw tjmw left a comment

Choose a reason for hiding this comment

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

Looks good! I left some nitpicky comments to take or leave, see what you think (no obligation to make any changes, I think they're all subjective).

Comment on lines 178 to 181
const strikethroughPrice = routerState.nonDiscountedPayments.reduce(
(prev, current) =>
prev && prev.amount > current.amount ? prev : current,
).amount;
Copy link
Member

Choose a reason for hiding this comment

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

Not that I'm not a fan of reduce (I am, and think I probably overuse it) but what about this as an alternative? Maybe it's slightly clearer what the intention is?

Suggested change
const strikethroughPrice = routerState.nonDiscountedPayments.reduce(
(prev, current) =>
prev && prev.amount > current.amount ? prev : current,
).amount;
const strikethroughPrice = Math.max(...routerState.nonDiscountedPayments.map(p => p.amount));

Or splitting it into two:

    const allAmounts = routerState.nonDiscountedPayments.map(p => p.amount);
    const strikeThroughPrice = Math.max(...allAmounts);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I like this 👍

Comment on lines 189 to 192
const nextNonDiscountedPrice = routerState.nonDiscountedPayments.reduce(
(prev, current) =>
prev && prev.amount > current.amount ? prev : current,
).amount;
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth extracting this to a method, since it's the same logic repeated in SupporterPlusOffer.tsx?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, good shout 👍

…various components into a utility function that can be shared
@rBangay rBangay merged commit e65d310 into main Sep 6, 2024
13 checks passed
@rBangay rBangay deleted the supporter-plus-cancellation-offer-pricing branch September 6, 2024 14:59
@prout-bot
Copy link
Collaborator

Seen on PROD (merged by @rBangay 12 minutes and 37 seconds ago) Please check your changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants