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

Enforced estimated price #928

Merged
merged 8 commits into from
Sep 25, 2023
Merged

Enforced estimated price #928

merged 8 commits into from
Sep 25, 2023

Conversation

shtukas
Copy link
Contributor

@shtukas shtukas commented Sep 16, 2023

In a previous recent PR ( #923 ) we introduced a check that detect when the final price of a subscription after amendment is higher than the estimated price. This is not a condition that would happen naturally in the engine, but it's not a strict impossibility either and it was reported that some Guardian weekly Quarterly (and at least one Annual) subscription had suffered from that problem.

In this PR

  1. We get rid of priceCorrectionFactorForPriceCap which is no longer used. It was introduced as a mechanism for computing the right (possibly capped) prices during the making of charge overrides, but we no longer to that (see 3. below)

  2. Replace PriceCap.cappedPrice by PriceCap's apply function. This makes the code slightly more readable.

  3. Update the signature of updateOfRatePlansToCurrent to no longer passing a price correction factor, but the price we are enforcing. This is a very simple way to solve the mis-pricing problem. Indeed, instead of hoping that the price after amendment will be the one we found during estimation, we simply enforce it. The logic is much more simple to understand and although we keep the price check (just in case), we should no longer observe that kind of mis-pricing.

  4. Note that we are still, at least in the case of legacy migrations, applying the price cap. The three places we are calling PriceCap are the notification handler, while making the communication to the user, the Salesforce price rise creation handler, to send the correct (capped) price to Salesforce, and then during the amendment handler. Using the exact same logic during those three handlers, instead of (before this change) using PriceCap in the two first but then a correction factor during the last one, was complicating one's mental model of what is actually happening.

@shtukas shtukas force-pushed the ph-20230916-2156-price-corrections branch from bfa74e4 to 60fa9b6 Compare September 16, 2023 23:43
@shtukas shtukas changed the title test Enforced estimated price Sep 16, 2023
@shtukas shtukas marked this pull request as ready for review September 17, 2023 00:12
@shtukas
Copy link
Contributor Author

shtukas commented Sep 17, 2023

Subscription 439444, which was the first to alarm after we introduced the final price check: #923 is now behaving correctly (meaning relatively to the estimated price, which also happens to be the price cap relatively to its old price)

Before this change:

01 439444 in Zuora with old code

After this change:

03 439444 in Zuora with new code

Copy link
Member

@kelvin-chappell kelvin-chappell left a comment

Choose a reason for hiding this comment

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

🚀

@shtukas
Copy link
Contributor Author

shtukas commented Sep 25, 2023

Failure case, obtained by momentarily allowing check for supporter+ and manually editing the cohort item:

Screenshot 2023-09-25 at 19 28 26

@shtukas shtukas merged commit b89c764 into main Sep 25, 2023
1 check passed
@shtukas shtukas deleted the ph-20230916-2156-price-corrections branch September 25, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants