-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shtukas
force-pushed
the
ph-20230916-2156-price-corrections
branch
from
September 16, 2023 23:43
bfa74e4
to
60fa9b6
Compare
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: After this change: |
kelvin-chappell
approved these changes
Sep 19, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
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)Replace
PriceCap.cappedPrice
byPriceCap
's apply function. This makes the code slightly more readable.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.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) usingPriceCap
in the two first but then a correction factor during the last one, was complicating one's mental model of what is actually happening.