From 5b76b08a99d177f82bf0bcf19f753b69c0047326 Mon Sep 17 00:00:00 2001 From: Alexandre Bourdin Date: Tue, 30 May 2023 12:30:46 +0300 Subject: [PATCH] [web] Split test slack notifications (#13186) * first test of notification * add notification in all methods * Format variants, store modification author * Move webhook URLs to saas settings * Add split test admin URL in notification payload * Display modifications author in split test admin * Extract modals from split test edit page * Confirmation modal for reverting a test, add/show comments, show version badge * Update integration tests and populate authors on save * Show version history button even with 1 version * Fix linting * Set slack webhook URLs for staging and prod * Update conditions to display split test admin modals * Extract the split test creation modal into a separate component * Extract split test slack notification management into a separate module --------- Co-authored-by: Lucie Germain GitOrigin-RevId: 8b69b4b2318b87312fbdd4c02e13c1a6f920a8e9 --- package-lock.json | 40 ++++++++++++ .../SplitTests/SlackNotificationManager.js | 61 +++++++++++++++++++ .../Features/SplitTests/SplitTestManager.js | 54 ++++++++++++---- services/web/app/src/models/SplitTest.js | 7 +++ services/web/package.json | 1 + 5 files changed, 150 insertions(+), 13 deletions(-) create mode 100644 services/web/app/src/Features/SplitTests/SlackNotificationManager.js diff --git a/package-lock.json b/package-lock.json index 6f899e6366..edd49e9ae4 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8651,6 +8651,29 @@ "integrity": "sha512-+iTbntw2IZPb/anVDbypzfQa+ay64MW0Zo8aJ8gZPWMMK6/OubMVb6lUPMagqjOPnmtauXnFCACVl3O7ogjeqQ==", "dev": true }, + "node_modules/@slack/types": { + "version": "1.10.0", + "resolved": "https://registry.npmjs.org/@slack/types/-/types-1.10.0.tgz", + "integrity": "sha512-tA7GG7Tj479vojfV3AoxbckalA48aK6giGjNtgH6ihpLwTyHE3fIgRrvt8TWfLwW8X8dyu7vgmAsGLRG7hWWOg==", + "engines": { + "node": ">= 8.9.0", + "npm": ">= 5.5.1" + } + }, + "node_modules/@slack/webhook": { + "version": "6.1.0", + "resolved": "https://registry.npmjs.org/@slack/webhook/-/webhook-6.1.0.tgz", + "integrity": "sha512-7AYNISyAjn/lA/VDwZ307K5ft5DojXgBd3DRrGoFN8XxIwIyRALdFhxBiMgAqeJH8eWoktvNwLK24R9hREEqpA==", + "dependencies": { + "@slack/types": "^1.2.1", + "@types/node": ">=12.0.0", + "axios": "^0.21.4" + }, + "engines": { + "node": ">= 12.13.0", + "npm": ">= 6.12.0" + } + }, "node_modules/@socket.io/component-emitter": { "version": "3.1.0", "resolved": "https://registry.npmjs.org/@socket.io/component-emitter/-/component-emitter-3.1.0.tgz", @@ -37395,6 +37418,7 @@ "@replit/codemirror-indentation-markers": "overleaf/codemirror-indentation-markers#1b1f93c0bcd04293aea6986aa2275185b2c56803", "@replit/codemirror-vim": "overleaf/codemirror-vim#07f1b50f4b2e703792da75a29e9e1e479b6b7067", "@sentry/browser": "^7.8.1", + "@slack/webhook": "^6.1.0", "@uppy/core": "^1.15.0", "@uppy/dashboard": "^1.11.0", "@uppy/react": "^1.11.0", @@ -45821,6 +45845,7 @@ "@replit/codemirror-indentation-markers": "overleaf/codemirror-indentation-markers#1b1f93c0bcd04293aea6986aa2275185b2c56803", "@replit/codemirror-vim": "overleaf/codemirror-vim#07f1b50f4b2e703792da75a29e9e1e479b6b7067", "@sentry/browser": "^7.8.1", + "@slack/webhook": "^6.1.0", "@testing-library/cypress": "^9.0.0", "@testing-library/dom": "^9.3.0", "@testing-library/react": "^12.1.5", @@ -47396,6 +47421,21 @@ "integrity": "sha512-+iTbntw2IZPb/anVDbypzfQa+ay64MW0Zo8aJ8gZPWMMK6/OubMVb6lUPMagqjOPnmtauXnFCACVl3O7ogjeqQ==", "dev": true }, + "@slack/types": { + "version": "1.10.0", + "resolved": "https://registry.npmjs.org/@slack/types/-/types-1.10.0.tgz", + "integrity": "sha512-tA7GG7Tj479vojfV3AoxbckalA48aK6giGjNtgH6ihpLwTyHE3fIgRrvt8TWfLwW8X8dyu7vgmAsGLRG7hWWOg==" + }, + "@slack/webhook": { + "version": "6.1.0", + "resolved": "https://registry.npmjs.org/@slack/webhook/-/webhook-6.1.0.tgz", + "integrity": "sha512-7AYNISyAjn/lA/VDwZ307K5ft5DojXgBd3DRrGoFN8XxIwIyRALdFhxBiMgAqeJH8eWoktvNwLK24R9hREEqpA==", + "requires": { + "@slack/types": "^1.2.1", + "@types/node": ">=12.0.0", + "axios": "^0.21.4" + } + }, "@socket.io/component-emitter": { "version": "3.1.0", "resolved": "https://registry.npmjs.org/@socket.io/component-emitter/-/component-emitter-3.1.0.tgz", diff --git a/services/web/app/src/Features/SplitTests/SlackNotificationManager.js b/services/web/app/src/Features/SplitTests/SlackNotificationManager.js new file mode 100644 index 0000000000..811e62a931 --- /dev/null +++ b/services/web/app/src/Features/SplitTests/SlackNotificationManager.js @@ -0,0 +1,61 @@ +const logger = require('@overleaf/logger') +const Settings = require('@overleaf/settings') +const { IncomingWebhook } = require('@slack/webhook') +const moment = require('moment') +const SplitTestUtils = require('./SplitTestUtils') + +async function sendNotification(splitTest, action, user) { + const lastVersion = SplitTestUtils.getCurrentVersion(splitTest) + const url = lastVersion.analyticsEnabled + ? Settings.splitTest.notification.splitTestSlackWebhookUrl + : Settings.splitTest.notification.gradualRolloutSlackWebhookUrl + if (!url) { + logger.info('Skipping slack notification as webhook URL is not configured') + return + } + + const webhook = new IncomingWebhook(url) + + const defaultRolloutPercent = + 100 - + lastVersion.variants.reduce( + (total, variant) => total + variant.rolloutPercent, + 0 + ) + const variantsConfig = [ + `- default: ${defaultRolloutPercent}%`, + ...lastVersion.variants.map( + variant => `- ${variant.name}: ${variant.rolloutPercent}%` + ), + ].join('\n') + + const payload = { + name: splitTest.name, + action, + phase: lastVersion.phase, + description: splitTest.description, + ticketURL: splitTest.ticketUrl, + variantsConfig, + active: lastVersion.active.toString(), + author: user.email, + date: + moment(lastVersion.createdAt).utc().format('Do MMM YYYY, h:mm a') + + ' UTC', + comment: lastVersion.comment ? `with comment: ${lastVersion.comment}` : '', + versionNumber: `${lastVersion.versionNumber}`, + url: `${Settings.siteUrl}/admin/split-test/edit/${splitTest.name}`, + } + try { + const { send: sendPayload } = webhook // workaround for the lint_flag_res_send_usage rule false-positive + await sendPayload.call(webhook, payload) + } catch (err) { + logger.error( + { err }, + 'Failed to notify split test notifications Slack webhook' + ) + } +} + +module.exports = { + sendNotification, +} diff --git a/services/web/app/src/Features/SplitTests/SplitTestManager.js b/services/web/app/src/Features/SplitTests/SplitTestManager.js index 6e92d059d5..9d67b3cb32 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestManager.js +++ b/services/web/app/src/Features/SplitTests/SplitTestManager.js @@ -47,7 +47,11 @@ async function getSplitTests({ name, phase, type, active, archived }) { filters.archived = { $ne: true } } try { - return await SplitTest.find(filters).limit(100).exec() + return await SplitTest.find(filters) + .populate('archivedBy', ['email', 'first_name', 'last_name']) + .populate('versions.author', ['email', 'first_name', 'last_name']) + .limit(100) + .exec() } catch (error) { throw OError.tag(error, 'Failed to get split tests list') } @@ -55,18 +59,19 @@ async function getSplitTests({ name, phase, type, active, archived }) { async function getSplitTest(query) { try { - return await SplitTest.findOne(query).exec() + return await SplitTest.findOne(query) + .populate('archivedBy', ['email', 'first_name', 'last_name']) + .populate('versions.author', ['email', 'first_name', 'last_name']) + .exec() } catch (error) { throw OError.tag(error, 'Failed to get split test', { query }) } } -async function createSplitTest({ - name, - configuration, - badgeInfo = {}, - info = {}, -}) { +async function createSplitTest( + { name, configuration, badgeInfo = {}, info = {} }, + userId +) { const stripedVariants = [] let stripeStart = 0 _checkNewVariantsConfiguration([], configuration.variants) @@ -102,13 +107,14 @@ async function createSplitTest({ analyticsEnabled: configuration.active && configuration.analyticsEnabled, variants: stripedVariants, + author: userId, }, ], }) return _saveSplitTest(splitTest) } -async function updateSplitTestConfig(name, configuration) { +async function updateSplitTestConfig({ name, configuration, comment }, userId) { const splitTest = await getSplitTest({ name }) if (!splitTest) { throw new OError(`Cannot update split test '${name}': not found`) @@ -134,6 +140,8 @@ async function updateSplitTestConfig(name, configuration) { active: configuration.active, analyticsEnabled: configuration.active && configuration.analyticsEnabled, variants: updatedVariants, + author: userId, + comment, }) return _saveSplitTest(splitTest) } @@ -160,7 +168,7 @@ async function updateSplitTestBadgeInfo(name, badgeInfo) { return _saveSplitTest(splitTest) } -async function switchToNextPhase(name) { +async function switchToNextPhase({ name, comment }, userId) { const splitTest = await getSplitTest({ name }) if (!splitTest) { throw new OError( @@ -192,11 +200,17 @@ async function switchToNextPhase(name) { variant.rolloutPercent = 0 variant.rolloutStripes = [] } + lastVersionCopy.author = userId + lastVersionCopy.comment = comment + lastVersionCopy.createdAt = new Date() splitTest.versions.push(lastVersionCopy) return _saveSplitTest(splitTest) } -async function revertToPreviousVersion(name, versionNumber) { +async function revertToPreviousVersion( + { name, versionNumber, comment }, + userId +) { const splitTest = await getSplitTest({ name }) if (!splitTest) { throw new OError( @@ -232,11 +246,13 @@ async function revertToPreviousVersion(name, versionNumber) { const previousVersionCopy = previousVersion.toObject() previousVersionCopy.versionNumber = lastVersion.versionNumber + 1 previousVersionCopy.createdAt = new Date() + previousVersionCopy.author = userId + previousVersionCopy.comment = comment splitTest.versions.push(previousVersionCopy) return _saveSplitTest(splitTest) } -async function archive(name) { +async function archive(name, userId) { const splitTest = await getSplitTest({ name }) if (!splitTest) { throw new OError(`Cannot archive split test with ID '${name}': not found`) @@ -246,6 +262,7 @@ async function archive(name) { } splitTest.archived = true splitTest.archivedAt = new Date() + splitTest.archivedBy = userId return _saveSplitTest(splitTest) } @@ -320,7 +337,18 @@ function _getTotalRolloutPercentage(variants) { async function _saveSplitTest(splitTest) { try { - return (await splitTest.save()).toObject() + const savedSplitTest = await splitTest.save() + await savedSplitTest.populate('archivedBy', [ + 'email', + 'first_name', + 'last_name', + ]) + await savedSplitTest.populate('versions.author', [ + 'email', + 'first_name', + 'last_name', + ]) + return savedSplitTest.toObject() } catch (error) { throw OError.tag(error, 'Failed to save split test', { splitTest: JSON.stringify(splitTest), diff --git a/services/web/app/src/models/SplitTest.js b/services/web/app/src/models/SplitTest.js index 25b3a86a39..167fce9a41 100644 --- a/services/web/app/src/models/SplitTest.js +++ b/services/web/app/src/models/SplitTest.js @@ -1,5 +1,6 @@ const mongoose = require('../infrastructure/Mongoose') const { Schema } = mongoose +const { ObjectId } = Schema const MIN_NAME_LENGTH = 3 const MAX_NAME_LENGTH = 200 @@ -92,6 +93,11 @@ const VersionSchema = new Schema( type: Date, default: Date.now, }, + author: { type: ObjectId, ref: 'User' }, + comment: { + type: String, + required: false, + }, }, { _id: false } ) @@ -145,6 +151,7 @@ const SplitTestSchema = new Schema( type: Date, required: false, }, + archivedBy: { type: ObjectId, ref: 'User' }, badgeInfo: { type: BadgeInfoSchema, required: false, diff --git a/services/web/package.json b/services/web/package.json index 4ea841cb11..073d5ab7d7 100644 --- a/services/web/package.json +++ b/services/web/package.json @@ -116,6 +116,7 @@ "@replit/codemirror-indentation-markers": "overleaf/codemirror-indentation-markers#1b1f93c0bcd04293aea6986aa2275185b2c56803", "@replit/codemirror-vim": "overleaf/codemirror-vim#07f1b50f4b2e703792da75a29e9e1e479b6b7067", "@sentry/browser": "^7.8.1", + "@slack/webhook": "^6.1.0", "@uppy/core": "^1.15.0", "@uppy/dashboard": "^1.11.0", "@uppy/react": "^1.11.0",