Prevent Guardian Ad-Lite purchase by users who have either of adFree or allowRejectAll benefits already
#6675
+170
−4
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
|
Size Change: +402 B (+0.02%) Total Size: 2.37 MB ℹ️ View Unchanged
|
tjmw
commented
Jan 7, 2025
| for { | ||
| benefits <- userBenefitsApiService | ||
| .getUserBenefits(userDetails.userDetails.identityId) | ||
| .leftMap(_ => ServerError("Something went wrong calling the user benefits API")) |
There was a problem hiding this comment.
If there's an issue with calling the user benefits API the purchase will be blocked and we'll show a generic error message to the user.
tjmw
commented
Jan 7, 2025
| userDetails: UserDetails, | ||
| isSignedIn: Boolean, | ||
| ) | ||
|
|
||
| private def createSubscription(implicit | ||
| settings: AllSettings, | ||
| request: CreateRequest, | ||
| ): EitherT[Future, CreateSubscriptionError, CreateSubscriptionResponse] = { | ||
|
|
||
| val maybeLoggedInUserDetails = |
There was a problem hiding this comment.
Should this now be "maybeUserDetails" since the signed in status can vary?
There was a problem hiding this comment.
I had this same thought and decided to leave it as is because in the case of this val it's still either:
Noneor- The details of a user who is signed in.
I.e. maybeLoggedInUserDetails won't ever contain information about a user who isn't signed in. Though like you say it's true that the Option type UserDetailsWithSignedInStatus can represent this state. What do you reckon?
There was a problem hiding this comment.
ah, right. That's quite subtle but seems fine to leave as is
|
Does this work for test users? We're going to need to query the code version of user-benefits api I think. |
tjmw
changed the title
adFree benefit alreadyadFree or rejectTracking benefits already
tjmw
force-pushed
the
tw/prevent-guardian-light-purchase
branch
from
2bd1b98 to
1f5716c
Compare
The previous approach wasn't working correctly.
This required moving to a service provider approach, where the service returned depends on whether the current user is a test user.
tjmw
force-pushed
the
tw/prevent-guardian-light-purchase
branch
from
1f5716c to
0be1361
Compare
|
|
andrewHEguardian
approved these changes
Jan 21, 2025
rupertbates
reviewed
Jan 22, 2025
rupertbates
reviewed
Jan 22, 2025
This was left over after the migration to touchpoint envs so we could support test users.
tjmw
changed the title
adFree or rejectTracking benefits alreadyadFree or allowRejectAll benefits already
|
Seen on PROD (merged by @tjmw 10 minutes and 58 seconds ago)
Sentry Release: support-client-side, support |
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.
What are you doing in this PR?
If a non signed-in user already has the
adFreeorallowRejectAllbenefit, they shouldn't be able to purchase Guardian Ad-Lite. We're assuming that a signed-in user withadFreeorallowRejectAllwouldn't make it to the checkout as there's no user journey for this.Trello Card
Why are you doing this?
It doesn't make sense to buy Guardian Ad-Lite if you get ad free reading or can already reject tracking.
How to test
I've tested locally and in CODE. When a signed out user already has the ad-free or reject-tracking benefit, an error is shown:
A user without these benefits is able to purchase successfully.
How can we measure success?
Have we considered potential risks?
Accessibility test checklist
Screenshots