diff --git a/ghost/core/core/server/services/email-service/wrapper.js b/ghost/core/core/server/services/email-service/wrapper.js
index 8dd48e8997..9b2cfcaf62 100644
--- a/ghost/core/core/server/services/email-service/wrapper.js
+++ b/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, {
diff --git a/ghost/core/core/server/services/mega/mega.js b/ghost/core/core/server/services/mega/mega.js
index d61fa4b548..3347273d68 100644
--- a/ghost/core/core/server/services/mega/mega.js
+++ b/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;
diff --git a/ghost/core/core/server/services/members/service.js b/ghost/core/core/server/services/members/service.js
index be86e64622..ab519c02a9 100644
--- a/ghost/core/core/server/services/members/service.js
+++ b/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');
diff --git a/ghost/core/test/e2e-api/admin/members.test.js b/ghost/core/test/e2e-api/admin/members.test.js
index 0213573460..214a2618f0 100644
--- a/ghost/core/test/e2e-api/admin/members.test.js
+++ b/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 () {
diff --git a/ghost/core/test/integration/services/email-service/batch-sending.test.js b/ghost/core/test/integration/services/email-service/batch-sending.test.js
index c633a454ac..1f5d3206a1 100644
--- a/ghost/core/test/integration/services/email-service/batch-sending.test.js
+++ b/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);
@@ -343,7 +396,13 @@ describe('Batch sending tests', function () {
assert.equal(errorData.error.status, 500);
assert.deepEqual(errorData.messageData.to.length, 1);
} else {
- assert.equal(batch.get('provider_id'), 'stubbed-email-id-' + count);
+ 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 feature image caption',
+ 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
});
diff --git a/ghost/core/test/integration/services/email-service/email-event-storage.test.js b/ghost/core/test/integration/services/email-service/email-event-storage.test.js
index 54fb73c5df..466add5e8a 100644
--- a/ghost/core/test/integration/services/email-service/email-event-storage.test.js
+++ b/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({
diff --git a/ghost/core/test/regression/api/admin/members-importer.test.js b/ghost/core/test/regression/api/admin/members-importer.test.js
index 3541e17704..3ce2b82f5a 100644
--- a/ghost/core/test/regression/api/admin/members-importer.test.js
+++ b/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');
diff --git a/ghost/core/test/unit/server/services/mega/mega.test.js b/ghost/core/test/unit/server/services/mega/mega.test.js
index d6c88697d6..d8ad86e022 100644
--- a/ghost/core/test/unit/server/services/mega/mega.test.js
+++ b/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();
});
diff --git a/ghost/email-service/lib/batch-sending-service.js b/ghost/email-service/lib/batch-sending-service.js
index 5ef06afce9..e5d3a3c8b4 100644
--- a/ghost/email-service/lib/batch-sending-service.js
+++ b/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});
diff --git a/ghost/email-service/lib/email-service.js b/ghost/email-service/lib/email-service.js
index cc708a536e..cb80a142fd 100644
--- a/ghost/email-service/lib/email-service.js
+++ b/ghost/email-service/lib/email-service.js
@@ -4,6 +4,7 @@
* @typedef {object} Post
* @typedef {object} Email
* @typedef {object} LimitService
+ * @typedef {{checkVerificationRequired(): Promise}} 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({
diff --git a/ghost/job-manager/lib/job-manager.js b/ghost/job-manager/lib/job-manager.js
index 029f8cabf9..2f98cf892c 100644
--- a/ghost/job-manager/lib/job-manager.js
+++ b/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;
diff --git a/ghost/members-importer/lib/importer.js b/ghost/members-importer/lib/importer.js
index fec8849702..a8e49b7a78 100644
--- a/ghost/members-importer/lib/importer.js
+++ b/ghost/members-importer/lib/importer.js
@@ -11,7 +11,7 @@ const messages = {
filenameCollision: 'Filename already exists, please try again.'
};
-// The key should correspond to a member model field (unless it's a special purpose field like 'complimentary_plan')
+// The key should correspond to a member model field (unless it's a special purpose field like 'complimentary_plan')
// the value should represent an allowed field name coming from user input
const DEFAULT_CSV_HEADER_MAPPING = {
email: 'email',
@@ -33,7 +33,7 @@ module.exports = class MembersCSVImporter {
* @param {() => Promise} 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 {
diff --git a/ghost/verification-trigger/lib/verification-trigger.js b/ghost/verification-trigger/lib/verification-trigger.js
index 0ec025e0b8..98ad8e17c0 100644
--- a/ghost/verification-trigger/lib/verification-trigger.js
+++ b/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} 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({}, {
diff --git a/ghost/verification-trigger/package.json b/ghost/verification-trigger/package.json
index ea0ca8bc46..fccae29a85 100644
--- a/ghost/verification-trigger/package.json
+++ b/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"
},
diff --git a/ghost/verification-trigger/test/verification-trigger.test.js b/ghost/verification-trigger/test/verification-trigger.test.js
index 9085fc8c63..95d535a3d3 100644
--- a/ghost/verification-trigger/test/verification-trigger.test.js
+++ b/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
},
@@ -283,7 +341,7 @@ describe('Email verification flow', function () {
amountTriggered: 10
});
});
-
+
it('Triggers when a number of members are added from API', async function () {
const emailStub = sinon.stub().resolves(null);
const settingsStub = sinon.stub().resolves(null);
@@ -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
},