From d820b961b0145d8380eacbc1297d2174b436ef55 Mon Sep 17 00:00:00 2001 From: Aileen Nowak Date: Thu, 23 Feb 2023 16:01:13 +0200 Subject: [PATCH] Avoid sending milestone email when actual value is too far from milestone no issue - Switches to used newly added config values throughout the services - Updated the `shouldSendEmail` fn to check if actual value is too far from achieved milestone as determined by the percentage setting (e. g. 998 members should not accidentally receive an email for achieving 100 members) --- .../services/milestones/MilestoneQueries.js | 8 ++-- .../server/services/milestones/service.js | 7 +++- .../e2e-server/services/milestones.test.js | 5 ++- ghost/milestones/lib/MilestonesService.js | 38 ++++++++++++++----- .../milestones/test/MilestonesService.test.js | 34 ++++++++++++++++- .../lib/SlackNotifications.js | 5 ++- 6 files changed, 77 insertions(+), 20 deletions(-) diff --git a/ghost/core/core/server/services/milestones/MilestoneQueries.js b/ghost/core/core/server/services/milestones/MilestoneQueries.js index db75a0722b..d5eb197bf0 100644 --- a/ghost/core/core/server/services/milestones/MilestoneQueries.js +++ b/ghost/core/core/server/services/milestones/MilestoneQueries.js @@ -1,10 +1,12 @@ -const MIN_DAYS_SINCE_IMPORTED = 7; - module.exports = class MilestoneQueries { #db; + /** @type {number} */ + #minDaysSinceImported; + constructor(deps) { this.#db = deps.db; + this.#minDaysSinceImported = deps.minDaysSinceImported; } /** @@ -32,7 +34,7 @@ module.exports = class MilestoneQueries { */ async hasImportedMembersInPeriod() { const importedThreshold = new Date(); - importedThreshold.setDate(importedThreshold.getDate() - MIN_DAYS_SINCE_IMPORTED); + importedThreshold.setDate(importedThreshold.getDate() - this.#minDaysSinceImported); const [hasImportedMembers] = await this.#db.knex('members_subscribe_events') .count('id as count') diff --git a/ghost/core/core/server/services/milestones/service.js b/ghost/core/core/server/services/milestones/service.js index bfe676ddcc..b1d606fa7a 100644 --- a/ghost/core/core/server/services/milestones/service.js +++ b/ghost/core/core/server/services/milestones/service.js @@ -42,11 +42,14 @@ module.exports = { MilestoneModel: models.Milestone }); - const queries = new MilestoneQueries({db}); + const queries = new MilestoneQueries({ + db, + minDaysSinceImported: milestonesConfig?.minDaysSinceImported || 7 + }); this.api = new MilestonesService({ repository, - milestonesConfig, // avoid using getters and pass as JSON + milestonesConfig, queries }); } diff --git a/ghost/core/test/e2e-server/services/milestones.test.js b/ghost/core/test/e2e-server/services/milestones.test.js index b6c2358b7e..8caca01343 100644 --- a/ghost/core/test/e2e-server/services/milestones.test.js +++ b/ghost/core/test/e2e-server/services/milestones.test.js @@ -145,7 +145,10 @@ describe('Milestones Service', function () { const milestonesConfig = { arr: [{currency: 'usd', values: [100, 150]}], - members: [10, 20, 30] + members: [10, 20, 30], + minDaysSinceImported: 7, + minDaysSinceLastEmail: 14, + maxPercentageFromMilestone: 0.35 }; before(async function () { diff --git a/ghost/milestones/lib/MilestonesService.js b/ghost/milestones/lib/MilestonesService.js index f08cbc1e4c..62870828aa 100644 --- a/ghost/milestones/lib/MilestonesService.js +++ b/ghost/milestones/lib/MilestonesService.js @@ -23,6 +23,8 @@ const Milestone = require('./Milestone'); * @prop {string} milestonesConfig.arr.currency * @prop {number[]} milestonesConfig.arr.values * @prop {number[]} milestonesConfig.members + * @prop {number} milestonesConfig.maxPercentageFromMilestone + * @prop {number} milestonesConfig.minDaysSinceLastEmail */ module.exports = class MilestonesService { @@ -133,7 +135,7 @@ module.exports = class MilestonesService { * @returns {Promise} */ async #saveMileStoneAndSendEmail(milestone) { - const {shouldSendEmail, reason} = await this.#shouldSendEmail(); + const {shouldSendEmail, reason} = await this.#shouldSendEmail(milestone); if (shouldSendEmail) { milestone.emailSentAt = new Date(); @@ -147,31 +149,47 @@ module.exports = class MilestonesService { } /** + * @param {object} milestone + * @param {number} milestone.value + * @param {'arr'|'members'} milestone.type + * @param {object} milestone.meta + * @param {string|null} [milestone.currency] + * @param {Date|null} [milestone.emailSentAt] * * @returns {Promise<{shouldSendEmail: boolean, reason: string}>} */ - async #shouldSendEmail() { - let canHaveEmail; + async #shouldSendEmail(milestone) { + let emailTooSoon = false; + let emailTooClose = false; let reason = null; - // Two cases in which we don't want to send an email + // Three cases in which we don't want to send an email // 1. There has been an import of members within the last week // 2. The last email has been sent less than two weeks ago + // 3. The current members or ARR value is x% above the achieved milestone + // as defined in default shared config for `maxPercentageFromMilestone` const lastMilestoneSent = await this.#repository.getLastEmailSent(); - if (!lastMilestoneSent) { - canHaveEmail = true; - } else { + if (lastMilestoneSent) { const differenceInTime = new Date().getTime() - new Date(lastMilestoneSent.emailSentAt).getTime(); const differenceInDays = differenceInTime / (1000 * 3600 * 24); - canHaveEmail = differenceInDays >= 14; + emailTooSoon = differenceInDays <= this.#milestonesConfig.minDaysSinceLastEmail; + } + + if (milestone?.meta) { + // Check how much the value currently differs from the milestone + const difference = (milestone?.meta?.currentMembers || milestone?.meta?.currentARR) - milestone.value; + const differenceInPercentage = difference / milestone.value; + + emailTooClose = differenceInPercentage >= this.#milestonesConfig.maxPercentageFromMilestone; } const hasMembersImported = await this.#queries.hasImportedMembersInPeriod(); - const shouldSendEmail = canHaveEmail && !hasMembersImported; + const shouldSendEmail = !emailTooSoon && !hasMembersImported && !emailTooClose; if (!shouldSendEmail) { - reason = hasMembersImported ? 'import' : 'email'; + reason = hasMembersImported ? 'import' : + emailTooSoon ? 'email' : 'tooFar'; } return {shouldSendEmail, reason}; diff --git a/ghost/milestones/test/MilestonesService.test.js b/ghost/milestones/test/MilestonesService.test.js index 928302f9fe..6bb884bad6 100644 --- a/ghost/milestones/test/MilestonesService.test.js +++ b/ghost/milestones/test/MilestonesService.test.js @@ -38,8 +38,10 @@ describe('MilestonesService', function () { values: [1000, 10000, 50000, 100000, 250000, 500000, 1000000] } ], - members: [100, 1000, 10000, 50000, 100000, 250000, 500000, 1000000] - + members: [100, 1000, 10000, 50000, 100000, 250000, 500000, 1000000], + minDaysSinceImported: 7, + minDaysSinceLastEmail: 14, + maxPercentageFromMilestone: 0.35 }; describe('ARR Milestones', function () { @@ -263,6 +265,34 @@ describe('MilestonesService', function () { const domainEventSpyResult = domainEventSpy.getCall(1).args[0]; assert(domainEventSpyResult.data.meta.reason === 'email'); }); + + it('Adds members milestone but does not send email when difference to milestone is above threshold', async function () { + repository = new InMemoryMilestoneRepository({DomainEvents}); + + const milestoneEmailService = new MilestonesService({ + repository, + milestonesConfig, + queries: { + async getMembersCount() { + return 784; + }, + async hasImportedMembersInPeriod() { + return false; + }, + async getDefaultCurrency() { + return 'nzd'; + } + } + }); + + const membersResult = await milestoneEmailService.checkMilestones('members'); + assert(membersResult.type === 'members'); + assert(membersResult.value === 100); + assert(membersResult.emailSentAt === null); + assert(domainEventSpy.callCount === 1); + const domainEventSpyResult = domainEventSpy.getCall(0).args[0]; + assert(domainEventSpyResult.data.meta.reason === 'tooFar'); + }); }); describe('Members Milestones', function () { diff --git a/ghost/slack-notifications/lib/SlackNotifications.js b/ghost/slack-notifications/lib/SlackNotifications.js index 049b22a729..7aedd832cc 100644 --- a/ghost/slack-notifications/lib/SlackNotifications.js +++ b/ghost/slack-notifications/lib/SlackNotifications.js @@ -49,7 +49,7 @@ class SlackNotifications { * @param {object} eventData * @param {import('@tryghost/milestones/lib/InMemoryMilestoneRepository').Milestone} eventData.milestone * @param {object} [eventData.meta] - * @param {'import'|'email'} [eventData.meta.reason] + * @param {'import'|'email'|'tooFar'} [eventData.meta.reason] * @param {number} [eventData.meta.currentARR] * @param {number} [eventData.meta.currentMembers] * @@ -58,7 +58,8 @@ class SlackNotifications { async notifyMilestoneReceived({milestone, meta}) { const hasImportedMembers = meta?.reason === 'import' ? 'has imported members' : null; const lastEmailTooSoon = meta?.reason === 'email' ? 'last email too recent' : null; - const emailNotSentReason = hasImportedMembers || lastEmailTooSoon; + const tooFarFromMilestone = meta?.reason === 'tooFar' ? 'too far from milestone' : null; + const emailNotSentReason = hasImportedMembers || lastEmailTooSoon || tooFarFromMilestone; const milestoneTypePretty = milestone.type === 'arr' ? 'ARR' : 'Members'; const valueFormatted = this.#getFormattedAmount({amount: milestone.value, currency: milestone?.currency}); const emailSentText = milestone?.emailSentAt ? this.#getFormattedDate(milestone?.emailSentAt) : `no / ${emailNotSentReason}`;