Merge pull request #6785 from overleaf/em-split-tests-analytics-enabled

Add "analytics enabled" setting to split tests

GitOrigin-RevId: 9ddfda9e246cac7a13361b2d3df6884212583000

services/web/app/src/Features/SplitTests/SplitTestHandler.js

@@ -6,6 +6,7 @@ const crypto = require('crypto')
 const _ = require('lodash')
 const { callbackify } = require('util')
 const SplitTestCache = require('./SplitTestCache')
+const { SplitTest } = require('../../models/SplitTest')
 
 const DEFAULT_VARIANT = 'default'
 const ALPHA_PHASE = 'alpha'
@@ -107,31 +108,37 @@ async function assignInLocalsContext(
  * Get a mapping of the active split test assignments for the given user
  */
 async function getActiveAssignmentsForUser(userId) {
-  const user = await UserGetter.promises.getUser(userId, { splitTests: 1 })
+  const user = await _getUser(userId)
-  if (user == null || user.splitTests == null) {
+  if (user == null) {
     return {}
   }
-  const activeAssignments = {}
-  for (const [splitTestName, assignments] of Object.entries(user.splitTests)) {
-    const splitTest = await SplitTestCache.get(splitTestName)
-    if (splitTest == null) {
-      continue
-    }
-    const currentVersion = splitTest.getCurrentVersion()
-    if (!currentVersion || !currentVersion.active) {
-      continue
-    }
 
-    let assignment
+  const splitTests = await SplitTest.find({
-    if (Array.isArray(assignments)) {
+    $where: 'this.versions[this.versions.length - 1].active',
-      assignment = _.maxBy(assignments, 'versionNumber')
+  }).exec()
-    } else {
+  const assignments = {}
-      // Older format is a single string rather than an array of objects
+  for (const splitTest of splitTests) {
-      assignment = { variantName: assignments }
+    const { activeForUser, selectedVariantName, phase, versionNumber } =
+      await _getAssignmentMetadata(user.analyticsId, user, splitTest)
+    if (activeForUser) {
+      const assignment = {
+        variantName: selectedVariantName,
+        versionNumber,
+        phase,
       }
-    activeAssignments[splitTestName] = assignment
+      const userAssignments = user.splitTests?.[splitTest.name]
+      if (Array.isArray(userAssignments)) {
+        const userAssignment = userAssignments.find(
+          x => x.versionNumber === versionNumber
+        )
+        if (userAssignment) {
+          assignment.assignedAt = userAssignment.assignedAt
         }
-  return activeAssignments
+      }
+      assignments[splitTest.name] = assignment
+    }
+  }
+  return assignments
 }
 
 async function _getAssignment(
@@ -158,8 +165,9 @@ async function _getAssignment(
       return _makeAssignment(splitTest, cachedVariant, currentVersion)
     }
   }
+  const user = userId && (await _getUser(userId))
   const { activeForUser, selectedVariantName, phase, versionNumber } =
-    await _getAssignmentMetadata(analyticsId, userId, splitTest)
+    await _getAssignmentMetadata(analyticsId, user, splitTest)
   if (activeForUser) {
     const assignmentConfig = {
       userId,
@@ -181,26 +189,19 @@ async function _getAssignment(
   return DEFAULT_ASSIGNMENT
 }
 
-async function _getAssignmentMetadata(analyticsId, userId, splitTest) {
+async function _getAssignmentMetadata(analyticsId, user, splitTest) {
   const currentVersion = splitTest.getCurrentVersion()
   const phase = currentVersion.phase
-  if ([ALPHA_PHASE, BETA_PHASE].includes(phase)) {
-    if (userId) {
-      const user = await _getUser(userId)
   if (
-        (phase === ALPHA_PHASE && !(user && user.alphaProgram)) ||
+    !user ||
-        (phase === BETA_PHASE && !(user && user.betaProgram))
+    (phase === ALPHA_PHASE && !user.alphaProgram) ||
+    (phase === BETA_PHASE && !user.betaProgram)
   ) {
     return {
       activeForUser: false,
     }
   }
-    } else {
+  const userId = user?._id.toString()
-      return {
-        activeForUser: false,
-      }
-    }
-  }
   const percentile = _getPercentile(
     analyticsId || userId,
     splitTest.name,

services/web/app/src/Features/SplitTests/SplitTestManager.js

@@ -58,6 +58,8 @@ async function createSplitTest(name, configuration, info = {}) {
         versionNumber: 1,
         phase: configuration.phase,
         active: configuration.active,
+        analyticsEnabled:
+          configuration.active && configuration.analyticsEnabled,
         variants: stripedVariants,
       },
     ],
@@ -84,6 +86,7 @@ async function updateSplitTestConfig(name, configuration) {
       versionNumber: lastVersion.versionNumber + 1,
       phase: configuration.phase,
       active: configuration.active,
+      analyticsEnabled: configuration.active && configuration.analyticsEnabled,
       variants: updatedVariants,
     })
     return _saveSplitTest(splitTest)

services/web/app/src/models/SplitTest.js

@@ -60,6 +60,11 @@ const VersionSchema = new Schema(
       default: true,
       required: true,
     },
+    analyticsEnabled: {
+      type: Boolean,
+      default: true,
+      required: true,
+    },
     variants: [VariantSchema],
     createdAt: {
       type: Date,

services/web/frontend/stylesheets/app/admin-hub.less

@@ -118,6 +118,10 @@
       background: inherit;
       left: 20px;
     }
+    &:disabled + label {
+      opacity: 0.5;
+      cursor: not-allowed;
+    }
   }
 
   label {

services/web/migrations/20220222095146_split_tests_analytics_enabled.js

@@ -0,0 +1,51 @@
+exports.tags = ['saas']
+
+exports.migrate = async client => {
+  const { db } = client
+  await db.splittests.updateMany(
+    {},
+    { $set: { 'versions.$[version].analyticsEnabled': true } },
+    {
+      arrayFilters: [
+        {
+          'version.active': true,
+          'version.analyticsEnabled': { $exists: false },
+        },
+      ],
+    }
+  )
+  await db.splittests.updateMany(
+    {},
+    { $set: { 'versions.$[version].analyticsEnabled': false } },
+    {
+      arrayFilters: [
+        {
+          'version.active': false,
+          'version.analyticsEnabled': { $exists: false },
+        },
+      ],
+    }
+  )
+}
+
+exports.rollback = async client => {
+  const { db } = client
+  await db.splittests.updateMany(
+    {},
+    { $unset: { 'versions.$[version].analyticsEnabled': 1 } },
+    {
+      arrayFilters: [
+        { 'version.active': true, 'version.analyticsEnabled': true },
+      ],
+    }
+  )
+  await db.splittests.updateMany(
+    {},
+    { $unset: { 'versions.$[version].analyticsEnabled': 1 } },
+    {
+      arrayFilters: [
+        { 'version.active': false, 'version.analyticsEnabled': false },
+      ],
+    }
+  )
+}

services/web/test/acceptance/bootstrap.js

@@ -3,6 +3,7 @@ chai.should()
 chai.use(require('chai-as-promised'))
 chai.use(require('chaid'))
 chai.use(require('sinon-chai'))
+chai.use(require('chai-exclude'))
 
 // Do not truncate assertion errors
 chai.config.truncateThreshold = 0

services/web/test/unit/src/SplitTests/SplitTestHandlerTests.js

@@ -11,12 +11,15 @@ const MODULE_PATH = Path.join(
 
 describe('SplitTestHandler', function () {
   beforeEach(function () {
-    this.splitTest = {
+    this.splitTests = [
-      getCurrentVersion: sinon.stub().returns({ active: true }),
+      makeSplitTest('active-test'),
-    }
+      makeSplitTest('legacy-test'),
-    this.inactiveSplitTest = {
+      makeSplitTest('no-analytics-test-1', { analyticsEnabled: false }),
-      getCurrentVersion: sinon.stub().returns({ active: false }),
+      makeSplitTest('no-analytics-test-2', {
-    }
+        analyticsEnabled: false,
+        versionNumber: 2,
+      }),
+    ]
 
     this.UserGetter = {
       promises: {
@@ -24,19 +27,24 @@ describe('SplitTestHandler', function () {
       },
     }
 
+    this.SplitTest = {
+      find: sinon.stub().returns({
+        exec: sinon.stub().resolves(this.splitTests),
+      }),
+    }
+
     this.SplitTestCache = {
       get: sinon.stub().resolves(null),
     }
-    this.SplitTestCache.get.withArgs('legacy-test').resolves(this.splitTest)
+    for (const splitTest of this.splitTests) {
-    this.SplitTestCache.get.withArgs('other-test').resolves(this.splitTest)
+      this.SplitTestCache.get.withArgs(splitTest.name).resolves(splitTest)
-    this.SplitTestCache.get
+    }
-      .withArgs('inactive-test')
-      .resolves(this.inactiveSplitTest)
 
     this.SplitTestHandler = SandboxedModule.require(MODULE_PATH, {
       requires: {
         '../User/UserGetter': this.UserGetter,
         './SplitTestCache': this.SplitTestCache,
+        '../../models/SplitTest': { SplitTest: this.SplitTest },
         '../User/UserUpdater': {},
         '../Analytics/AnalyticsManager': {},
         './LocalsHelper': {},
@@ -49,14 +57,23 @@ describe('SplitTestHandler', function () {
       this.user = {
         _id: ObjectId(),
         splitTests: {
-          'legacy-test': 'legacy-variant',
+          'active-test': [
-          'other-test': [
+            {
-            { variantName: 'default', versionNumber: 1 },
+              variantName: 'default',
-            { variantName: 'latest', versionNumber: 3 },
+              versionNumber: 1,
-            { variantName: 'experiment', versionNumber: 2 },
+              assignedAt: 'active-test-assigned-at',
+            },
           ],
+          'legacy-test': 'legacy-variant',
           'inactive-test': [{ variantName: 'trythis' }],
           'unknown-test': [{ variantName: 'trythis' }],
+          'no-analytics-test-2': [
+            {
+              variantName: 'some-variant',
+              versionNumber: 1,
+              assignedAt: 'no-analytics-assigned-at',
+            },
+          ],
         },
       }
       this.UserGetter.promises.getUser
@@ -69,19 +86,36 @@ describe('SplitTestHandler', function () {
     })
 
     it('handles the legacy assignment format', function () {
-      expect(this.assignments).to.have.property('legacy-test')
+      expect(this.assignments['legacy-test']).to.deep.equal({
-      expect(this.assignments['legacy-test'].variantName).to.equal(
+        variantName: 'variant-1',
-        'legacy-variant'
+        phase: 'release',
-      )
+        versionNumber: 1,
+      })
     })
 
-    it('returns the last assignment for each active test', function () {
+    it('returns the current assignment for each active test', function () {
-      expect(this.assignments).to.have.property('other-test')
+      expect(this.assignments['active-test']).to.deep.equal({
-      expect(this.assignments['other-test'].variantName).to.equal('latest')
+        variantName: 'variant-1',
+        phase: 'release',
+        versionNumber: 1,
+        assignedAt: 'active-test-assigned-at',
+      })
     })
 
-    it('does not return assignments for inactive tests', function () {
+    it('returns the current assignment for tests with analytics disabled', function () {
-      expect(this.assignments).not.to.have.property('inactive-test')
+      expect(this.assignments['no-analytics-test-1']).to.deep.equal({
+        variantName: 'variant-1',
+        phase: 'release',
+        versionNumber: 1,
+      })
+    })
+
+    it('returns the current assignment for tests with analytics disabled that had previous assignments', function () {
+      expect(this.assignments['no-analytics-test-2']).to.deep.equal({
+        variantName: 'variant-1',
+        phase: 'release',
+        versionNumber: 2,
+      })
     })
 
     it('does not return assignments for unknown tests', function () {
@@ -89,7 +123,7 @@ describe('SplitTestHandler', function () {
     })
   })
 
-  describe('with an inexistent user', function () {
+  describe('with an non-existent user', function () {
     beforeEach(async function () {
       const unknownUserId = ObjectId()
       this.assignments =
@@ -115,8 +149,55 @@ describe('SplitTestHandler', function () {
         )
     })
 
-    it('returns empty assignments', function () {
+    it('returns current assignments', function () {
-      expect(this.assignments).to.deep.equal({})
+      expect(this.assignments).to.deep.equal({
+        'active-test': {
+          phase: 'release',
+          variantName: 'variant-1',
+          versionNumber: 1,
+        },
+        'legacy-test': {
+          phase: 'release',
+          variantName: 'variant-1',
+          versionNumber: 1,
+        },
+        'no-analytics-test-1': {
+          phase: 'release',
+          variantName: 'variant-1',
+          versionNumber: 1,
+        },
+        'no-analytics-test-2': {
+          phase: 'release',
+          variantName: 'variant-1',
+          versionNumber: 2,
+        },
+      })
     })
   })
 })
+
+function makeSplitTest(name, opts = {}) {
+  const {
+    active = true,
+    analyticsEnabled = active,
+    phase = 'release',
+    versionNumber = 1,
+  } = opts
+
+  return {
+    name,
+    getCurrentVersion: sinon.stub().returns({
+      active,
+      analyticsEnabled,
+      phase,
+      versionNumber,
+      variants: [
+        {
+          name: 'variant-1',
+          rolloutPercent: 100,
+          rolloutStripes: [{ start: 0, end: 100 }],
+        },
+      ],
+    }),
+  }
+}

services/web/test/unit/src/Subscription/RecurlyClientTests.js

@@ -1,10 +1,5 @@
 const sinon = require('sinon')
-const sinonChai = require('sinon-chai')
+const { expect } = require('chai')
-const chai = require('chai')
-const chaiAsPromised = require('chai-as-promised')
-chai.use(sinonChai)
-chai.use(chaiAsPromised)
-const { expect } = chai
 const recurly = require('recurly')
 const modulePath = '../../../../app/src/Features/Subscription/RecurlyClient'
 const SandboxedModule = require('sandboxed-module')