From fe2bcf461ffbd8bd9132927f12b6c8e51f4f75b2 Mon Sep 17 00:00:00 2001 From: Alexandre Bourdin Date: Mon, 24 Jan 2022 17:03:53 +0100 Subject: [PATCH] Assignment by id for recurly webhook (#6465) * Allow split test assignment by ID for recurly webhook * Small refactoring of assignment logic * Add tests for getAssignmentForUser * Cleanup following review comments * Provide default value for sync option in split test handler GitOrigin-RevId: 828cad3a1f3a0f3efd25f427d00a3c530ae2f087 --- .../Features/SplitTests/SplitTestHandler.js | 107 +++++++++++------- .../Subscription/RecurlyEventHandler.js | 2 +- .../Subscription/RecurlyEventHandlerTests.js | 10 +- 3 files changed, 74 insertions(+), 45 deletions(-) diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.js b/services/web/app/src/Features/SplitTests/SplitTestHandler.js index 3bc71ebd6f..9ac264ad4d 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.js +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.js @@ -41,7 +41,7 @@ const DEFAULT_ASSIGNMENT = { * @param options {Object} - for test purposes only, to force the synchronous update of the user's profile * @returns {Promise<{variant: string, analytics: {segmentation: {splitTest: string, variant: string, phase: string, versionNumber: number}|{}}}>} */ -async function getAssignment(req, splitTestName, options) { +async function getAssignment(req, splitTestName, { sync = false } = {}) { const query = req.query || {} if (query[splitTestName]) { return { @@ -54,13 +54,30 @@ async function getAssignment(req, splitTestName, options) { const { userId, analyticsId } = AnalyticsManager.getIdsFromSession( req.session ) - return _getAssignment( + return _getAssignment(splitTestName, { analyticsId, userId, - req.session, - splitTestName, - options - ) + session: req.session, + sync, + }) +} + +/** + * Get the assignment of a user to a split test by their user ID. + * + * Warning: this does not support query parameters override. Wherever possible, `getAssignment` should be used instead. + * + * @param userId the user ID + * @param splitTestName the unique name of the split test + * @param options {Object} - for test purposes only, to force the synchronous update of the user's profile + * @returns {Promise<{variant: string, analytics: {segmentation: {splitTest: string, variant: string, phase: string, versionNumber: number}|{}}}>} + */ +async function getAssignmentForUser( + userId, + splitTestName, + { sync = false } = {} +) { + return _getAssignment(splitTestName, { userId, sync }) } /** @@ -72,8 +89,13 @@ async function getAssignment(req, splitTestName, options) { * @param options {Object} - for test purposes only, to force the synchronous update of the user's profile * @returns {Promise} */ -async function assignInLocalsContext(req, res, splitTestName, options) { - const assignment = await getAssignment(req, splitTestName, options) +async function assignInLocalsContext( + req, + res, + splitTestName, + { sync = false } = {} +) { + const assignment = await getAssignment(req, splitTestName, { sync }) LocalsHelper.setSplitTestVariant( res.locals, splitTestName, @@ -82,48 +104,49 @@ async function assignInLocalsContext(req, res, splitTestName, options) { } async function _getAssignment( - analyticsId, - userId, - session, splitTestName, - options + { analyticsId, userId, session, sync } ) { if (!analyticsId && !userId) { return DEFAULT_ASSIGNMENT } + const splitTest = await splitTestCache.get(splitTestName) - if (splitTest) { - const currentVersion = splitTest.getCurrentVersion() + const currentVersion = splitTest?.getCurrentVersion() + if (!splitTest || !currentVersion?.active) { + return DEFAULT_ASSIGNMENT + } + + if (session) { const cachedVariant = _getCachedVariantFromSession( session, splitTest.name, currentVersion ) - if (currentVersion.active) { - if (cachedVariant) { - return _makeAssignment(splitTest, cachedVariant, currentVersion) - } - const { activeForUser, selectedVariantName, phase, versionNumber } = - await _getAssignmentMetadata(analyticsId, userId, splitTest) - if (activeForUser) { - const assignmentConfig = { - userId, - analyticsId, - session, - splitTestName, - variantName: selectedVariantName, - phase, - versionNumber, - } - if (options && options.sync === true) { - await _updateVariantAssignment(assignmentConfig) - } else { - _updateVariantAssignment(assignmentConfig) - } - return _makeAssignment(splitTest, selectedVariantName, currentVersion) - } + if (cachedVariant) { + return _makeAssignment(splitTest, cachedVariant, currentVersion) } } + const { activeForUser, selectedVariantName, phase, versionNumber } = + await _getAssignmentMetadata(analyticsId, userId, splitTest) + if (activeForUser) { + const assignmentConfig = { + userId, + analyticsId, + session, + splitTestName, + variantName: selectedVariantName, + phase, + versionNumber, + } + if (sync === true) { + await _updateVariantAssignment(assignmentConfig) + } else { + _updateVariantAssignment(assignmentConfig) + } + return _makeAssignment(splitTest, selectedVariantName, currentVersion) + } + return DEFAULT_ASSIGNMENT } @@ -147,7 +170,11 @@ async function _getAssignmentMetadata(analyticsId, userId, splitTest) { } } } - const percentile = _getPercentile(analyticsId, splitTest.name, phase) + const percentile = _getPercentile( + analyticsId || userId, + splitTest.name, + phase + ) const selectedVariantName = _getVariantFromPercentile( currentVersion.variants, percentile @@ -210,7 +237,7 @@ async function _updateVariantAssignment({ }, }) AnalyticsManager.setUserPropertyForAnalyticsId( - analyticsId, + user.analyticsId || analyticsId || userId, `split-test-${splitTestName}-${versionNumber}`, variantName ) @@ -276,9 +303,11 @@ async function _getUser(id) { module.exports = { getAssignment: callbackify(getAssignment), + getAssignmentForUser: callbackify(getAssignmentForUser), assignInLocalsContext: callbackify(assignInLocalsContext), promises: { getAssignment, + getAssignmentForUser, assignInLocalsContext, }, } diff --git a/services/web/app/src/Features/Subscription/RecurlyEventHandler.js b/services/web/app/src/Features/Subscription/RecurlyEventHandler.js index 44418a6c12..7ddfcfb36a 100644 --- a/services/web/app/src/Features/Subscription/RecurlyEventHandler.js +++ b/services/web/app/src/Features/Subscription/RecurlyEventHandler.js @@ -63,7 +63,7 @@ async function sendSubscriptionStartedEvent(eventData) { // send the trial onboarding email if (isTrial) { - const assignment = await SplitTestHandler.promises.getAssignment( + const assignment = await SplitTestHandler.promises.getAssignmentForUser( userId, 'trial-onboarding-email' ) diff --git a/services/web/test/unit/src/Subscription/RecurlyEventHandlerTests.js b/services/web/test/unit/src/Subscription/RecurlyEventHandlerTests.js index 6506e6e02a..b6ff18f001 100644 --- a/services/web/test/unit/src/Subscription/RecurlyEventHandlerTests.js +++ b/services/web/test/unit/src/Subscription/RecurlyEventHandlerTests.js @@ -31,7 +31,7 @@ describe('RecurlyEventHandler', function () { }), '../SplitTests/SplitTestHandler': (this.SplitTestHandler = { promises: { - getAssignment: sinon.stub().resolves({ active: false }), + getAssignmentForUser: sinon.stub().resolves({ variant: 'default' }), }, }), '../Analytics/AnalyticsManager': (this.AnalyticsManager = { @@ -76,16 +76,16 @@ describe('RecurlyEventHandler', function () { true ) sinon.assert.calledWith( - this.SplitTestHandler.promises.getAssignment, + this.SplitTestHandler.promises.getAssignmentForUser, this.userId, 'trial-onboarding-email' ) }) it('sends free trial onboarding email if user in ab group', async function () { - this.SplitTestHandler.promises.getAssignment = sinon + this.SplitTestHandler.promises.getAssignmentForUser = sinon .stub() - .resolves({ active: true, variant: 'send-email' }) + .resolves({ variant: 'send-email' }) this.userId = '123456789trial' this.eventData.account.account_code = this.userId @@ -94,7 +94,7 @@ describe('RecurlyEventHandler', function () { await this.RecurlyEventHandler.sendSubscriptionStartedEvent(this.eventData) sinon.assert.calledWith( - this.SplitTestHandler.promises.getAssignment, + this.SplitTestHandler.promises.getAssignmentForUser, this.userId, 'trial-onboarding-email' )