New Supporter Revenue cookie lifecycle #13277
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
rupertbates
force-pushed
the
user-benefits-cookies
branch
from
513b12d to
effeacc
Compare
|
Size Change: -1.47 kB (-0.16%) Total Size: 899 kB
ℹ️ View Unchanged
|
rupertbates
commented
Feb 11, 2025
| @@ -0,0 +1,17 @@ | |||
| import { getCookie } from '@guardian/libs'; | |||
|
|
|||
| export const USER_BENEFITS_EXPIRY_COOKIE = 'gu_user_features_expiry'; | |||
There was a problem hiding this comment.
I'm going to change the name of this cookie to gu_user_benefits_expiry at some point but I'm waiting for the go ahead from Transparency and Consent before doing that.
There was a problem hiding this comment.
I've got the go ahead from Emily for this change now.
rupertbates
commented
Feb 11, 2025
| if ((await isUserLoggedInOktaRefactor()) && featuresDataIsOld()) { | ||
| return requestNewData(); | ||
| } else if ((await userHasDataAfterSignOut()) && !forcedAdFreeMode) { | ||
| deleteAllCookies(); |
There was a problem hiding this comment.
We don't explicitly delete any user benefits cookies now, we just let the expiry time remove them which will take a maximum of 24 hrs.
There was a problem hiding this comment.
And to check my understanding -
if the user is signed in and the cookie has expired, then the client will refresh the cookie? And so there cannot be a period between expiry and the next refresh where the benefits are missing
There was a problem hiding this comment.
I think there could be a gap, but if there is (if the benefits expiry cookie is not up to date) then we err on the side of giving benefits.
There was a problem hiding this comment.
Yes correct, if the user is signed in and the cookies are missing or expired in the case of the expiry cookie then we grant the benefit. For example with the hide support messaging benefit :
rupertbates
force-pushed
the
user-benefits-cookies
branch
from
1a42fd9 to
e542c62
Compare
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
rupertbates
changed the title
tomrf1
reviewed
Feb 11, 2025
| const expiresValue = timeInDaysFromNow(daysTillExpiry); | ||
| setCookie({ | ||
| name: cookieName, | ||
| value: expiresValue.toString(), |
There was a problem hiding this comment.
Am I right in thinking the value is never used by DCR?
There was a problem hiding this comment.
Yes that's right, we have to give it some value so I thought I'd put the expiry value in there even though we don't really use it. If you have a better suggestion though I'm open to changing it.
tjmw
reviewed
Feb 11, 2025
| return tmpDate.getTime(); | ||
| }; | ||
|
|
||
| export const extendCookieExpiry = ( |
There was a problem hiding this comment.
This function works like an upsert right? I.e. it'll create if it doesn't already exist or update if it does. I wonder if there's a name which reflects that, just to make it really clear.
There was a problem hiding this comment.
I've changed this to createOrRenewCookie
github-actions
bot
removed
the
run_chromatic
…-benefits-cookies
tjmw
approved these changes
Feb 12, 2025
| describe('Refreshing the features data', () => { | ||
| const expectUserBenefitExpiryCookieHasBeenSetCorrectly = () => { | ||
| const expectedNextRefreshDate = new Date( | ||
| timeInDaysFromNow(1), |
There was a problem hiding this comment.
Is there a danger here that the system clock could tick over to the next second between the cookie being set and running this assertion, resulting in flaky tests? Should we be freezing time in the tests?
There was a problem hiding this comment.
This check just looks at the date though not the time, so I think it's ok?
There was a problem hiding this comment.
Ah sorry yep you're right, how did I miss that?!
github-actions
bot
removed
the
run_chromatic
|
Seen on PROD (merged by @rupertbates 9 minutes and 46 seconds ago) Please check your changes! |
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 does this change?
This PR changes the way that DCR handles supporter-revenue cookies to be consistent with this document
Why?
As described in the document, this gives us greater control over the benefits received by users on DCR, particularly in the way they can be set from support.theguardian.com.