Improved email verification required checks (#16060)

fixes https://github.com/TryGhost/Team/issues/2366
refs https://ghost.slack.com/archives/C02G9E68C/p1670232405014209

Probem described in issue.

In the old MEGA flow:
- The `email_verification_required` check is now repeated inside the job

In the new email service flow:
- The `email_verification_required` is now checked (didn't happen
before)
- When generating the email batch recipients, we only include members
that were created before the email was created. That way it is
impossible to avoid limit checks by inserting new members between
creating an email and sending an email.
- We don't need to repeat the check inside the job because of the above
changes

Improved handling of large imports:
- When checking `email_verification_required`, we now also check if the
import threshold is reached (a new method is introduced in
vertificationTrigger specifically for this usage). If it is, we start
the verification progress. This is required for long running imports
that only check the verification threshold at the very end.
- This change increases the concurrency of fastq to 3 (refs
https://ghost.slack.com/archives/C02G9E68C/p1670232405014209). So when
running a long import, it is now possible to send emails without having
to wait for the import. Above change makes sure it is not possible to
get around the verification limits.

Refactoring:
- Removed the need to use `updateVerificationTrigger` by making
thresholds getters instead of fixed variables.
- Improved awaiting of members import job in regression test

ghost/core/core/server/services/email-service/wrapper.js

@@ -101,7 +101,8 @@ class EmailServiceWrapper {
             emailRenderer,
             emailSegmenter,
             limitService,
-            membersRepository
+            membersRepository,
+            verificationTrigger: membersService.verificationTrigger
         });
 
         this.controller = new EmailController(this.service, {

ghost/core/core/server/services/mega/mega.js

@@ -185,7 +185,7 @@ const addEmail = async (postModel, options) => {
         await limitService.errorIfWouldGoOverLimit('emails');
     }
 
-    if (settingsCache.get('email_verification_required') === true) {
+    if (await membersService.verificationTrigger.checkVerificationRequired()) {
         throw new errors.HostLimitError({
             message: tpl(messages.emailSendingDisabled)
         });
@@ -312,6 +312,14 @@ async function sendEmailJob({emailId, options}) {
             await limitService.errorIfWouldGoOverLimit('emails');
         }
 
+        // Check email verification required
+        // We need to check this inside the job again
+        if (await membersService.verificationTrigger.checkVerificationRequired()) {
+            throw new errors.HostLimitError({
+                message: tpl(messages.emailSendingDisabled)
+            });
+        }
+
         // Check if the email is still pending. And set the status to submitting in one transaction.
         let hasSingleAccess = false;
         let emailModel;

ghost/core/core/server/services/members/service.js

@@ -67,11 +67,11 @@ const processImport = async (options) => {
     return await membersImporter.process({...options, verificationTrigger});
 };
 
-const updateVerificationTrigger = () => {
-    verificationTrigger = new VerificationTrigger({
-        apiTriggerThreshold: _.get(config.get('hostSettings'), 'emailVerification.apiThreshold'),
-        adminTriggerThreshold: _.get(config.get('hostSettings'), 'emailVerification.adminThreshold'),
-        importTriggerThreshold: _.get(config.get('hostSettings'), 'emailVerification.importThreshold'),
+const initVerificationTrigger = () => {
+    return new VerificationTrigger({
+        getApiTriggerThreshold: () => _.get(config.get('hostSettings'), 'emailVerification.apiThreshold'),
+        getAdminTriggerThreshold: () => _.get(config.get('hostSettings'), 'emailVerification.adminThreshold'),
+        getImportTriggerThreshold: () => _.get(config.get('hostSettings'), 'emailVerification.importThreshold'),
         isVerified: () => config.get('hostSettings:emailVerification:verified') === true,
         isVerificationRequired: () => settingsCache.get('email_verification_required') === true,
         sendVerificationEmail: async ({subject, message, amountTriggered}) => {
@@ -133,7 +133,8 @@ module.exports = {
             getMembersApi: () => module.exports.api
         });
 
-        updateVerificationTrigger();
+        verificationTrigger = initVerificationTrigger();
+        module.exports.verificationTrigger = verificationTrigger;
 
         if (!env?.startsWith('testing')) {
             const membersMigrationJobName = 'members-migrations';
@@ -163,16 +164,14 @@ module.exports = {
     },
 
     ssr: null,
+    verificationTrigger: null,
 
     stripeConnect: require('./stripe-connect'),
 
     processImport: processImport,
 
     stats: membersStats,
-    export: require('./exporter/query'),
-
-    // Only for tests
-    _updateVerificationTrigger: updateVerificationTrigger
+    export: require('./exporter/query')
 };
 
 module.exports.middleware = require('./middleware');

ghost/core/test/e2e-api/admin/members.test.js

@@ -16,7 +16,6 @@ const membersService = require('../../../core/server/services/members');
 const memberAttributionService = require('../../../core/server/services/member-attribution');
 const urlService = require('../../../core/server/services/url');
 const urlUtils = require('../../../core/shared/url-utils');
-const {_updateVerificationTrigger} = require('../../../core/server/services/members');
 const settingsCache = require('../../../core/shared/settings-cache');
 
 /**
@@ -712,7 +711,6 @@ describe('Members API', function () {
             verified: false,
             escalationAddress: 'test@example.com'
         });
-        _updateVerificationTrigger();
 
         assert.ok(!settingsCache.get('email_verification_required'), 'Email verification should NOT be required');
 
@@ -766,7 +764,6 @@ describe('Members API', function () {
         await agent.delete(`/members/${memberFailVerification.id}`);
 
         configUtils.restore();
-        _updateVerificationTrigger();
     });
 
     it('Can add and send a signup confirmation email', async function () {

ghost/core/test/integration/services/email-service/batch-sending.test.js

@@ -9,8 +9,9 @@ const jobManager = require('../../../../core/server/services/jobs/job-service');
 let agent;
 const _ = require('lodash');
 const {MailgunEmailProvider} = require('@tryghost/email-service');
-
 const mobileDocWithPaywall = '{"version":"0.3.1","markups":[],"atoms":[],"cards":[["paywall",{}]],"sections":[[1,"p",[[0,[],0,"Free content"]]],[10,0],[1,"p",[[0,[],0,"Members content"]]]]}';
+const configUtils = require('../../../utils/configUtils');
+const {settingsCache} = require('../../../../core/server/services/settings-helpers');
 
 function sortBatches(a, b) {
     const aId = a.get('provider_id');
@@ -81,6 +82,7 @@ describe('Batch sending tests', function () {
         mockManager.mockSetting('mailgun_api_key', 'test');
         mockManager.mockSetting('mailgun_domain', 'example.com');
         mockManager.mockSetting('mailgun_base_url', 'test');
+        mockManager.mockMail();
 
         // We need to stub the Mailgun client before starting Ghost
         sinon.stub(MailgunClient.prototype, 'getInstance').returns({
@@ -145,6 +147,57 @@ describe('Batch sending tests', function () {
         assert.equal(memberIds.length, _.uniq(memberIds).length);
     });
 
+    it('Doesn\'t include members created after the email in the batches', async function () {
+        // If we create a new member (e.g. a member that was imported) after the email was created, they should not be included in the email
+
+        // Prepare a post and email model
+        const completedPromise = jobManager.awaitCompletion('batch-sending-service-job');
+        const emailModel = await createPublishedPostEmail();
+
+        // Create a new member that is subscribed
+        const laterMember = await models.Member.add({
+            name: 'Member that is added later',
+            email: 'member-that-is-added-later@example.com',
+            status: 'free',
+            newsletters: [{
+                id: fixtureManager.get('newsletters', 0).id
+            }]
+        });
+
+        // Check the batches are not yet generated
+        const earlyBatches = await models.EmailBatch.findAll({filter: `email_id:${emailModel.id}`});
+        assert.equal(earlyBatches.models.length, 0);
+
+        // Await sending job
+        await completedPromise;
+
+        await emailModel.refresh();
+        assert.equal(emailModel.get('status'), 'submitted');
+        assert.equal(emailModel.get('email_count'), 4);
+
+        // Did we create batches?
+        const batches = await models.EmailBatch.findAll({filter: `email_id:${emailModel.id}`});
+        assert.equal(batches.models.length, 1);
+
+        // Did we create recipients?
+        const emailRecipients = await models.EmailRecipient.findAll({filter: `email_id:${emailModel.id}`});
+        assert.equal(emailRecipients.models.length, 4);
+
+        for (const recipient of emailRecipients.models) {
+            assert.equal(recipient.get('batch_id'), batches.models[0].id);
+            assert.notEqual(recipient.get('member_id'), laterMember.id);
+        }
+
+        // Create a new email and see if it is included now
+        const completedPromise2 = jobManager.awaitCompletion('batch-sending-service-job');
+        const emailModel2 = await createPublishedPostEmail();
+        await completedPromise2;
+        await emailModel2.refresh();
+        assert.equal(emailModel2.get('email_count'), 5);
+        const emailRecipients2 = await models.EmailRecipient.findAll({filter: `email_id:${emailModel2.id}`});
+        assert.equal(emailRecipients2.models.length, emailRecipients.models.length + 1);
+    });
+
     it('Splits recipients in free and paid batch', async function () {
         // Prepare a post and email model
         const completedPromise = jobManager.awaitCompletion('batch-sending-service-job');
@@ -164,7 +217,7 @@ describe('Batch sending tests', function () {
 
         await emailModel.refresh();
         assert(emailModel.get('status'), 'submitted');
-        assert.equal(emailModel.get('email_count'), 4);
+        assert.equal(emailModel.get('email_count'), 5);
 
         // Did we create batches?
         const batches = await models.EmailBatch.findAll({filter: `email_id:${emailModel.id}`});
@@ -189,7 +242,7 @@ describe('Batch sending tests', function () {
 
         // Did we create recipients?
         const emailRecipientsFirstBatch = await models.EmailRecipient.findAll({filter: `email_id:${emailModel.id}+batch_id:${firstBatch.id}`});
-        assert.equal(emailRecipientsFirstBatch.models.length, 2);
+        assert.equal(emailRecipientsFirstBatch.models.length, 3);
 
         const emailRecipientsSecondBatch = await models.EmailRecipient.findAll({filter: `email_id:${emailModel.id}+batch_id:${secondBatch.id}`});
         assert.equal(emailRecipientsSecondBatch.models.length, 2);
@@ -258,11 +311,11 @@ describe('Batch sending tests', function () {
 
         await emailModel.refresh();
         assert.equal(emailModel.get('status'), 'submitted');
-        assert.equal(emailModel.get('email_count'), 4);
+        assert.equal(emailModel.get('email_count'), 5);
 
         // Did we create batches?
         const batches = await models.EmailBatch.findAll({filter: `email_id:${emailModel.id}`});
-        assert.equal(batches.models.length, 4);
+        assert.equal(batches.models.length, 5);
 
         const emailRecipients = [];
 
@@ -318,13 +371,13 @@ describe('Batch sending tests', function () {
 
         await emailModel.refresh();
         assert.equal(emailModel.get('status'), 'failed');
-        assert.equal(emailModel.get('email_count'), 4);
+        assert.equal(emailModel.get('email_count'), 5);
 
         // Did we create batches?
         let batches = await models.EmailBatch.findAll({filter: `email_id:${emailModel.id}`});
-        assert.equal(batches.models.length, 4);
+        assert.equal(batches.models.length, 5);
 
-        // sort batches by provider_id (nullable) because findAll doesn't have order option
+        // sort batches by id because findAll doesn't have order option
         batches.models.sort(sortBatches);
 
         let emailRecipients = [];
@@ -334,7 +387,7 @@ describe('Batch sending tests', function () {
         for (const batch of batches.models) {
             count += 1;
 
-            if (count === 4) {
+            if (count === 5) {
                 assert.equal(batch.get('provider_id'), null);
                 assert.equal(batch.get('status'), 'failed');
                 assert.equal(batch.get('error_status_code'), 500);
@@ -342,8 +395,14 @@ describe('Batch sending tests', function () {
                 const errorData = JSON.parse(batch.get('error_data'));
                 assert.equal(errorData.error.status, 500);
                 assert.deepEqual(errorData.messageData.to.length, 1);
+            } else {
+                if (count === 4) {
+                    // We sorted on provider_id so the count is slightly off
+                    assert.equal(batch.get('provider_id'), 'stubbed-email-id-5');
                 } else {
                     assert.equal(batch.get('provider_id'), 'stubbed-email-id-' + count);
+                }
+
                 assert.equal(batch.get('status'), 'submitted');
                 assert.equal(batch.get('error_status_code'), null);
                 assert.equal(batch.get('error_message'), null);
@@ -374,14 +433,14 @@ describe('Batch sending tests', function () {
         batches.models.sort(sortBatches);
 
         assert.equal(emailModel.get('status'), 'submitted');
-        assert.equal(emailModel.get('email_count'), 4);
+        assert.equal(emailModel.get('email_count'), 5);
 
         // Did we keep the batches?
         batches = await models.EmailBatch.findAll({filter: `email_id:${emailModel.id}`});
 
         // sort batches by provider_id (nullable) because findAll doesn't have order option
         batches.models.sort(sortBatches);
-        assert.equal(batches.models.length, 4);
+        assert.equal(batches.models.length, 5);
 
         emailRecipients = [];
 
@@ -407,6 +466,62 @@ describe('Batch sending tests', function () {
         assert.equal(memberIds.length, _.uniq(memberIds).length);
     });
 
+    it('Cannot send an email if verification is required', async function () {
+        // First enable import thresholds
+        configUtils.set('hostSettings:emailVerification', {
+            apiThreshold: 100,
+            adminThreshold: 100,
+            importThreshold: 100,
+            verified: false,
+            escalationAddress: 'test@example.com'
+        });
+
+        // We stub a lot of imported members to mimic a large import that is in progress but is not yet finished
+        // the current verification required value is off. But when creating an email, we need to update that check to avoid this issue.
+        const members = require('../../../../core/server/services/members');
+        const events = members.api.events;
+        const getSignupEvents = sinon.stub(events, 'getSignupEvents').resolves({
+            meta: {
+                pagination: {
+                    total: 100000
+                }
+            }
+        });
+
+        assert.equal(settingsCache.get('email_verification_required'), false, 'This test requires email verification to be disabled initially');
+
+        const post = {
+            title: 'A random test post',
+            status: 'draft',
+            feature_image_alt: 'Testing sending',
+            feature_image_caption: 'Testing <b>feature image caption</b>',
+            created_at: moment().subtract(2, 'days').toISOString(),
+            updated_at: moment().subtract(2, 'days').toISOString(),
+            created_by: ObjectId().toHexString(),
+            updated_by: ObjectId().toHexString()
+        };
+
+        const res = await agent.post('posts/')
+            .body({posts: [post]})
+            .expectStatus(201);
+
+        const id = res.body.posts[0].id;
+
+        const updatedPost = {
+            status: 'published',
+            updated_at: res.body.posts[0].updated_at
+        };
+
+        const newsletterSlug = fixtureManager.get('newsletters', 0).slug;
+        await agent.put(`posts/${id}/?newsletter=${newsletterSlug}`)
+            .body({posts: [updatedPost]})
+            .expectStatus(403);
+        sinon.assert.calledOnce(getSignupEvents);
+        assert.equal(settingsCache.get('email_verification_required'), true);
+
+        configUtils.restore();
+    });
+
     // TODO: Link tracking
     // TODO: Replacement fallbacks
 });

ghost/core/test/integration/services/email-service/email-event-storage.test.js

@@ -81,7 +81,7 @@ describe('EmailEventStorage', function () {
         assert.deepEqual(result.memberIds, [memberId]);
 
         // Now wait for events processed
-        await sleep(100);
+        await sleep(200);
 
         // Check if status has changed to delivered, with correct timestamp
         const updatedEmailRecipient = await models.EmailRecipient.findOne({
@@ -135,7 +135,7 @@ describe('EmailEventStorage', function () {
         assert.deepEqual(result.memberIds, [memberId]);
 
         // Now wait for events processed
-        await sleep(100);
+        await sleep(200);
 
         // Check if status has changed to delivered, with correct timestamp
         const updatedEmailRecipient = await models.EmailRecipient.findOne({
@@ -186,7 +186,7 @@ describe('EmailEventStorage', function () {
         assert.deepEqual(result.memberIds, [memberId]);
 
         // Now wait for events processed
-        await sleep(100);
+        await sleep(200);
 
         // Check if status has changed to delivered, with correct timestamp
         const updatedEmailRecipient = await models.EmailRecipient.findOne({

ghost/core/test/regression/api/admin/members-importer.test.js

@@ -8,10 +8,10 @@ const config = require('../../../../core/shared/config');
 const configUtils = require('../../../utils/configUtils');
 const settingsCache = require('../../../../core/shared/settings-cache');
 const models = require('../../../../core/server/models');
+const jobManager = require('../../../../core/server/services/jobs/job-service');
 
 const {mockManager, sleep} = require('../../../utils/e2e-framework');
 const assert = require('assert');
-const {_updateVerificationTrigger} = require('../../../../core/server/services/members');
 
 let request;
 
@@ -253,7 +253,6 @@ describe('Members Importer API', function () {
             verified: false,
             escalationAddress: 'test@example.com'
         });
-        _updateVerificationTrigger();
 
         const res = await request
             .post(localUtils.API.getApiQuery(`members/upload/`))
@@ -321,7 +320,8 @@ describe('Members Importer API', function () {
             verified: false,
             escalationAddress: 'test@example.com'
         });
-        _updateVerificationTrigger();
+
+        const awaitCompletion = jobManager.awaitCompletion('members-import');
 
         const res = await request
             .post(localUtils.API.getApiQuery(`members/upload/`))
@@ -338,7 +338,7 @@ describe('Members Importer API', function () {
         should.exist(jsonResponse.meta);
 
         // Wait for the job to finish
-        await sleep(15000);
+        await awaitCompletion;
 
         assert(!!settingsCache.get('email_verification_required'), 'Email verification should now be required');
 

ghost/core/test/unit/server/services/mega/mega.test.js

@@ -8,6 +8,16 @@ const membersService = require('../../../../../core/server/services/members');
 
 describe('MEGA', function () {
     describe('addEmail', function () {
+        before(function () {
+            membersService.verificationTrigger = {
+                checkVerificationRequired: sinon.stub().resolves(false)
+            };
+        });
+
+        after(function () {
+            membersService.verificationTrigger = null;
+        });
+
         afterEach(function () {
             sinon.restore();
         });

ghost/email-service/lib/batch-sending-service.js

@@ -155,7 +155,10 @@ class BatchSendingService {
 
             // Avoiding Bookshelf for performance reasons
             let members;
-            let lastId = null;
+
+            // Start with the id of the email, which is an objectId. We'll only fetch members that are created before the email. This is a special property of ObjectIds.
+            // Note: we use ID and not created_at, because imported members could set a created_at in the future or past and avoid limit checking.
+            let lastId = email.id;
 
             while (!members || lastId) {
                 logging.info(`Fetching members batch for email ${email.id} segment ${segment}, lastId: ${lastId}`);
@@ -165,17 +168,17 @@ class BatchSendingService {
                     .orderByRaw('id DESC')
                     .select('members.id', 'members.uuid', 'members.email', 'members.name').limit(BATCH_SIZE + 1);
 
-                if (members.length > BATCH_SIZE) {
-                    lastId = members[members.length - 2].id;
-                } else {
-                    lastId = null;
-                }
-
                 if (members.length > 0) {
                     totalCount += Math.min(members.length, BATCH_SIZE);
                     const batch = await this.createBatch(email, segment, members.slice(0, BATCH_SIZE));
                     batches.push(batch);
                 }
+
+                if (members.length > BATCH_SIZE) {
+                    lastId = members[members.length - 2].id;
+                } else {
+                    break;
+                }
             }
         }
 
@@ -184,8 +187,8 @@ class BatchSendingService {
         if (email.get('email_count') !== totalCount) {
             logging.error(`Email ${email.id} has wrong recipient count ${totalCount}, expected ${email.get('email_count')}. Updating the model.`);
 
-            // We update the email model because this will probably happen a few times because of the time difference
-            // between creating the email and sending it (or when the email failed initially and is retried a day later)
+            // We update the email model because this might happen in rare cases where the initial member count changed (e.g. deleted members)
+            // between creating the email and sending it
             await email.save({
                 email_count: totalCount
             }, {patch: true, require: false, autoRefresh: false});

ghost/email-service/lib/email-service.js

@@ -4,6 +4,7 @@
  * @typedef {object} Post
  * @typedef {object} Email
  * @typedef {object} LimitService
+ * @typedef {{checkVerificationRequired(): Promise<boolean>}} VerificationTrigger
  */
 
 const BatchSendingService = require('./batch-sending-service');
@@ -15,7 +16,8 @@ const SendingService = require('./sending-service');
 
 const messages = {
     archivedNewsletterError: 'Cannot send email to archived newsletters',
-    missingNewsletterError: 'The post does not have a newsletter relation'
+    missingNewsletterError: 'The post does not have a newsletter relation',
+    emailSendingDisabled: `Email sending is temporarily disabled because your account is currently in review. You should have an email about this from us already, but you can also reach us any time at support@ghost.org`
 };
 
 class EmailService {
@@ -27,6 +29,7 @@ class EmailService {
     #emailSegmenter;
     #limitService;
     #membersRepository;
+    #verificationTrigger;
 
     /**
      *
@@ -40,6 +43,7 @@ class EmailService {
      * @param {EmailSegmenter} dependencies.emailSegmenter
      * @param {LimitService} dependencies.limitService
      * @param {object} dependencies.membersRepository
+     * @param {VerificationTrigger} dependencies.verificationTrigger
      */
     constructor({
         batchSendingService,
@@ -49,7 +53,8 @@ class EmailService {
         emailRenderer,
         emailSegmenter,
         limitService,
-        membersRepository
+        membersRepository,
+        verificationTrigger
     }) {
         this.#batchSendingService = batchSendingService;
         this.#models = models;
@@ -59,12 +64,13 @@ class EmailService {
         this.#limitService = limitService;
         this.#membersRepository = membersRepository;
         this.#sendingService = sendingService;
+        this.#verificationTrigger = verificationTrigger;
     }
 
     /**
      * @private
      */
-    async checkLimits() {
+    async checkLimits(addedCount = 0) {
         // Check host limit for allowed member count and throw error if over limit
         // - do this even if it's a retry so that there's no way around the limit
         if (this.#limitService.isLimited('members')) {
@@ -73,7 +79,14 @@ class EmailService {
 
         // Check host limit for disabled emails or going over emails limit
         if (this.#limitService.isLimited('emails')) {
-            await this.#limitService.errorIfWouldGoOverLimit('emails');
+            await this.#limitService.errorIfWouldGoOverLimit('emails', {addedCount});
+        }
+
+        // Check if email verification is required
+        if (await this.#verificationTrigger.checkVerificationRequired()) {
+            throw new errors.HostLimitError({
+                message: tpl(messages.emailSendingDisabled)
+            });
         }
     }
 
@@ -99,6 +112,8 @@ class EmailService {
         }
 
         const emailRecipientFilter = post.get('email_recipient_filter');
+        const emailCount = await this.#emailSegmenter.getMembersCount(newsletter, emailRecipientFilter);
+        await this.checkLimits(emailCount);
 
         const email = await this.#models.Email.add({
             post_id: post.id,
@@ -112,13 +127,12 @@ class EmailService {
             subject: this.#emailRenderer.getSubject(post),
             from: this.#emailRenderer.getFromAddress(post, newsletter),
             replyTo: this.#emailRenderer.getReplyToAddress(post, newsletter),
-            email_count: await this.#emailSegmenter.getMembersCount(newsletter, emailRecipientFilter),
+            email_count: emailCount,
             source: post.get('lexical') || post.get('mobiledoc'),
             source_type: post.get('lexical') ? 'lexical' : 'mobiledoc'
         });
 
         try {
-            await this.checkLimits();
             this.#batchSendingService.scheduleEmail(email);
         } catch (e) {
             await email.save({

ghost/job-manager/lib/job-manager.js

@@ -39,7 +39,7 @@ class JobManager {
      * @param {Object} [options.domainEvents] - domain events emitter
      */
     constructor({errorHandler, workerMessageHandler, JobModel, domainEvents}) {
-        this.queue = fastq(this, worker, 1);
+        this.queue = fastq(this, worker, 3);
         this._jobMessageHandler = this._jobMessageHandler.bind(this);
         this._jobErrorHandler = this._jobErrorHandler.bind(this);
         this.#domainEvents = domainEvents;

ghost/members-importer/lib/importer.js

@@ -33,7 +33,7 @@ module.exports = class MembersCSVImporter {
      * @param {() => Promise<import('@tryghost/tiers/lib/Tier')>} options.getDefaultTier - async function returning default Member Tier
      * @param {Function} options.sendEmail - function sending an email
      * @param {(string) => boolean} options.isSet - Method checking if specific feature is enabled
-     * @param {({job, offloaded}) => void} options.addJob - Method registering an async job
+     * @param {({job, offloaded, name}) => void} options.addJob - Method registering an async job
      * @param {Object} options.knex - An instance of the Ghost Database connection
      * @param {Function} options.urlFor - function generating urls
      * @param {Object} options.context
@@ -334,7 +334,8 @@ module.exports = class MembersCSVImporter {
                         logging.error(e);
                     }
                 },
-                offloaded: false
+                offloaded: false,
+                name: 'members-import'
             });
 
             return {

ghost/verification-trigger/lib/verification-trigger.js

@@ -14,9 +14,9 @@ class VerificationTrigger {
     /**
      *
      * @param {object} deps
-     * @param {number} deps.apiTriggerThreshold Threshold for triggering API&Import sourced verifications
-     * @param {number} deps.adminTriggerThreshold Threshold for triggering Admin sourced verifications
-     * @param {number} deps.importTriggerThreshold Threshold for triggering Import sourced verifications
+     * @param {() => number} deps.getApiTriggerThreshold Threshold for triggering API&Import sourced verifications
+     * @param {() => number} deps.getAdminTriggerThreshold Threshold for triggering Admin sourced verifications
+     * @param {() => number} deps.getImportTriggerThreshold Threshold for triggering Import sourced verifications
      * @param {() => boolean} deps.isVerified Check Ghost config to see if we are already verified
      * @param {() => boolean} deps.isVerificationRequired Check Ghost settings to see whether verification has been requested
      * @param {(content: {subject: string, message: string, amountTriggered: number}) => Promise<void>} deps.sendVerificationEmail Sends an email to the escalation address to confirm that customer needs to be verified
@@ -25,9 +25,9 @@ class VerificationTrigger {
      * @param {any} deps.eventRepository For querying events
      */
     constructor({
-        apiTriggerThreshold,
-        adminTriggerThreshold,
-        importTriggerThreshold,
+        getApiTriggerThreshold,
+        getAdminTriggerThreshold,
+        getImportTriggerThreshold,
         isVerified,
         isVerificationRequired,
         sendVerificationEmail,
@@ -35,9 +35,9 @@ class VerificationTrigger {
         Settings,
         eventRepository
     }) {
-        this._apiTriggerThreshold = apiTriggerThreshold;
-        this._adminTriggerThreshold = adminTriggerThreshold;
-        this._importTriggerThreshold = importTriggerThreshold;
+        this._getApiTriggerThreshold = getApiTriggerThreshold;
+        this._getAdminTriggerThreshold = getAdminTriggerThreshold;
+        this._getImportTriggerThreshold = getImportTriggerThreshold;
         this._isVerified = isVerified;
         this._isVerificationRequired = isVerificationRequired;
         this._sendVerificationEmail = sendVerificationEmail;
@@ -49,6 +49,18 @@ class VerificationTrigger {
         DomainEvents.subscribe(MemberCreatedEvent, this._handleMemberCreatedEvent);
     }
 
+    get _apiTriggerThreshold() {
+        return this._getApiTriggerThreshold();
+    }
+
+    get _adminTriggerThreshold() {
+        return this._getAdminTriggerThreshold();
+    }
+
+    get _importTriggerThreshold() {
+        return this._getImportTriggerThreshold();
+    }
+
     /**
      *
      * @param {MemberCreatedEvent} event
@@ -93,12 +105,32 @@ class VerificationTrigger {
         }
     }
 
+    /**
+     * Returns false if email verification is required to send an email. It also updates the verification check and might activate email verification.
+     * Use this when sending emails.
+     */
+    async checkVerificationRequired() {
+        // Check if import threshold is reached (could happen that a long import is in progress and we didn't check the threshold yet)
+        await this.testImportThreshold();
+        return this._isVerificationRequired() && !this._isVerified();
+    }
+
     async testImportThreshold() {
         if (!isFinite(this._importTriggerThreshold)) {
             // Infinite threshold, quick path
             return;
         }
 
+        if (this._isVerified()) {
+            // Already verified, no need to check limits
+            return;
+        }
+
+        if (this._isVerificationRequired()) {
+            // Already requested verification, no need to calculate again
+            return;
+        }
+
         const createdAt = new Date();
         createdAt.setDate(createdAt.getDate() - 30);
         const events = await this._eventRepository.getSignupEvents({}, {

ghost/verification-trigger/package.json

@@ -7,7 +7,7 @@
   "main": "index.js",
   "scripts": {
     "dev": "echo \"Implement me!\"",
-    "test:unit": "NODE_ENV=testing c8 --all --check-coverage --reporter text --reporter cobertura mocha './test/**/*.test.js'",
+    "test:unit": "NODE_ENV=testing c8 --all --check-coverage --100 --reporter text --reporter cobertura mocha './test/**/*.test.js'",
     "test": "yarn test:unit",
     "lint": "eslint . --ext .js --cache"
   },

ghost/verification-trigger/test/verification-trigger.test.js

@@ -1,6 +1,7 @@
 // Switch these lines once there are useful utils
 // const testUtils = require('./utils');
 const sinon = require('sinon');
+const assert = require('assert');
 require('./utils');
 const VerificationTrigger = require('../index');
 const DomainEvents = require('@tryghost/domain-events');
@@ -9,7 +10,7 @@ const {MemberCreatedEvent} = require('@tryghost/member-events');
 describe('Import threshold', function () {
     it('Creates a threshold based on config', async function () {
         const trigger = new VerificationTrigger({
-            importTriggerThreshold: 2,
+            getImportTriggerThreshold: () => 2,
             membersStats: {
                 getTotalMembers: async () => 1
             }
@@ -21,7 +22,7 @@ describe('Import threshold', function () {
 
     it('Increases the import threshold to the number of members', async function () {
         const trigger = new VerificationTrigger({
-            importTriggerThreshold: 2,
+            getImportTriggerThreshold: () => 2,
             membersStats: {
                 getTotalMembers: async () => 3
             }
@@ -34,7 +35,7 @@ describe('Import threshold', function () {
     it('Does not check members count when config threshold is infinite', async function () {
         const membersStub = sinon.stub().resolves(null);
         const trigger = new VerificationTrigger({
-            importTriggerThreshold: Infinity,
+            getImportTriggerThreshold: () => Infinity,
             memberStats: {
                 getTotalMembers: membersStub
             }
@@ -167,7 +168,7 @@ describe('Email verification flow', function () {
         });
 
         new VerificationTrigger({
-            apiTriggerThreshold: 2,
+            getApiTriggerThreshold: () => 2,
             Settings: {
                 edit: settingsStub
             },
@@ -204,7 +205,7 @@ describe('Email verification flow', function () {
         });
 
         const trigger = new VerificationTrigger({
-            importTriggerThreshold: 2,
+            getImportTriggerThreshold: () => 2,
             Settings: {
                 edit: settingsStub
             },
@@ -236,6 +237,63 @@ describe('Email verification flow', function () {
         });
     });
 
+    it('checkVerificationRequired also checks import', async function () {
+        const emailStub = sinon.stub().resolves(null);
+        let isVerificationRequired = false;
+        const isVerificationRequiredStub = sinon.stub().callsFake(() => {
+            return isVerificationRequired;
+        });
+        const settingsStub = sinon.stub().callsFake(() => {
+            isVerificationRequired = true;
+            return Promise.resolve();
+        });
+        const eventStub = sinon.stub().resolves({
+            meta: {
+                pagination: {
+                    total: 10
+                }
+            }
+        });
+
+        const trigger = new VerificationTrigger({
+            getImportTriggerThreshold: () => 2,
+            Settings: {
+                edit: settingsStub
+            },
+            membersStats: {
+                getTotalMembers: () => 15
+            },
+            isVerified: () => false,
+            isVerificationRequired: isVerificationRequiredStub,
+            sendVerificationEmail: emailStub,
+            eventRepository: {
+                getSignupEvents: eventStub
+            }
+        });
+
+        assert.equal(await trigger.checkVerificationRequired(), true);
+        sinon.assert.calledOnce(emailStub);
+    });
+
+    it('testImportThreshold does not calculate anything if already verified', async function () {
+        const trigger = new VerificationTrigger({
+            getImportTriggerThreshold: () => 2,
+            isVerified: () => true
+        });
+
+        assert.equal(await trigger.testImportThreshold(), undefined);
+    });
+
+    it('testImportThreshold does not calculate anything if already pending', async function () {
+        const trigger = new VerificationTrigger({
+            getImportTriggerThreshold: () => 2,
+            isVerified: () => false,
+            isVerificationRequired: () => true
+        });
+
+        assert.equal(await trigger.testImportThreshold(), undefined);
+    });
+
     it('Triggers when a number of members are added from Admin', async function () {
         const emailStub = sinon.stub().resolves(null);
         const settingsStub = sinon.stub().resolves(null);
@@ -248,7 +306,7 @@ describe('Email verification flow', function () {
         });
 
         const trigger = new VerificationTrigger({
-            adminTriggerThreshold: 2,
+            getAdminTriggerThreshold: () => 2,
             Settings: {
                 edit: settingsStub
             },
@@ -296,8 +354,8 @@ describe('Email verification flow', function () {
         });
 
         const trigger = new VerificationTrigger({
-            adminTriggerThreshold: 2,
-            apiTriggerThreshold: 2,
+            getAdminTriggerThreshold: () => 2,
+            getApiTriggerThreshold: () => 2,
             Settings: {
                 edit: settingsStub
             },
@@ -345,7 +403,7 @@ describe('Email verification flow', function () {
         });
 
         const trigger = new VerificationTrigger({
-            apiTriggerThreshold: Infinity,
+            getImportTriggerThreshold: () => Infinity,
             Settings: {
                 edit: settingsStub
             },