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)

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')

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
             });
         }

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 () {

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<Milestone>}
      */
     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};

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 () {

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}`;