Infrastructure/ghost
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2025-01-20 22:42:53 -05:00
Code Issues Activity

Allow opt-in for existing members when creating a newsletter (#14567)

Browse source 
- Added changes to opt-in existing members for new newsletters

Co-authored-by: Hannah Wolfe <github.erisds@gmail.com>
This commit is contained in:
Matt Hanley 2022-04-25 21:07:46 +01:00
parent 6e92229a07
commit 5f31f52139
8 changed files with 253 additions and 24 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
core/server/api/canary/newsletters.js
View file

@ -70,7 +70,8 @@ module.exports = {
add: {
statusCode: 201,
options: [
'include'
'include',
'opt_in_existing'
],
validation: {
options: {

14
core/server/models/member.js
View file

@ -370,6 +370,20 @@ const Member = ghostBookshelf.Model.extend({
query.transacting(unfilteredOptions.transacting);
}
return query;
},
fetchAllSubscribed(unfilteredOptions = {}) {
// we use raw queries instead of model relationships because model hydration is expensive
const query = ghostBookshelf.knex('members_newsletters')
.join('newsletters', 'members_newsletters.newsletter_id', '=', 'newsletters.id')
.where('newsletters.status', 'active')
.distinct('member_id as id');
if (unfilteredOptions.transacting) {
query.transacting(unfilteredOptions.transacting);
}
return query;
}
});

33
core/server/models/newsletter.js
View file

@ -1,4 +1,5 @@
const ghostBookshelf = require('./base');
const ObjectID = require('bson-objectid');
const Newsletter = ghostBookshelf.Model.extend({
tableName: 'newsletters',
@ -19,6 +20,19 @@ const Newsletter = ghostBookshelf.Model.extend({
show_header_name: true
},
members() {
return this.belongsToMany('Member', 'members_newsletters', 'newsletter_id', 'member_id')
.query((qb) => {
// avoids bookshelf adding a `DISTINCT` to the query
// we know the result set will already be unique and DISTINCT hurts query performance
qb.columns('members.*');
});
},
posts() {
return this.hasMany('Post');
},
async onSaving(model, _attr, options) {
ghostBookshelf.Model.prototype.onSaving.apply(this, arguments);
@ -37,6 +51,25 @@ const Newsletter = ghostBookshelf.Model.extend({
model.set({slug: cleanSlug});
}
}
},
subscribeMembersById(memberIds, unfilteredOptions = {}) {
let pivotRows = [];
for (const memberId of memberIds) {
pivotRows.push({
id: ObjectID().toHexString(),
member_id: memberId.id,
newsletter_id: this.id
});
}
const query = ghostBookshelf.knex.batchInsert('members_newsletters', pivotRows);
if (unfilteredOptions.transacting) {
query.transacting(unfilteredOptions.transacting);
}
return query;
}
}, {
orderDefaultOptions: function orderDefaultOptions() {

1
core/server/services/newsletters/index.js
View file

@ -8,6 +8,7 @@ const MAGIC_LINK_TOKEN_VALIDITY = 24 * 60 * 60 * 1000;
module.exports = new NewslettersService({
NewsletterModel: models.Newsletter,
MemberModel: models.Member,
mail,
singleUseTokenProvider: new SingleUseTokenProvider(models.SingleUseToken, MAGIC_LINK_TOKEN_VALIDITY),
urlUtils

72
core/server/services/newsletters/service.js
View file

@ -2,18 +2,21 @@ const _ = require('lodash');
const MagicLink = require('@tryghost/magic-link');
const logging = require('@tryghost/logging');
const verifyEmailTemplate = require('./emails/verify-email');
const debug = require('@tryghost/debug')('services:newsletters');
class NewslettersService {
/**
*
* @param {Object} options
* @param {Object} options.NewsletterModel
* @param {Object} options.MemberModel
* @param {Object} options.mail
* @param {Object} options.singleUseTokenProvider
* @param {Object} options.urlUtils
*/
constructor({NewsletterModel, mail, singleUseTokenProvider, urlUtils}) {
constructor({NewsletterModel, MemberModel, mail, singleUseTokenProvider, urlUtils}) {
this.NewsletterModel = NewsletterModel;
this.MemberModel = MemberModel;
this.urlUtils = urlUtils;
/* email verification setup */
@ -68,17 +71,23 @@ class NewslettersService {
}
/**
*
* @param {Object} options browse options
* @returns
* @public
* @param {Object} [options] options
* @returns {Promise<object>} JSONified Newsletter models
*/
async browse(options) {
async browse(options = {}) {
let newsletters = await this.NewsletterModel.findAll(options);
return newsletters.toJSON();
}
async add(attrs, options) {
/**
* @public
* @param {object} attrs model properties
* @param {Object} [options] options
* @param {Object} [options] options.transacting
* @returns {Promise<{object}>} Newsetter Model with verification metadata
*/
async add(attrs, options = {}) {
// remove any email properties that are not allowed to be set without verification
const {cleanedAttrs, emailsToVerify} = await this.prepAttrsForEmailVerification(attrs);
@ -89,11 +98,34 @@ class NewslettersService {
// add the model now because we need the ID for sending verification emails
const newsletter = await this.NewsletterModel.add(cleanedAttrs, options);
// subscribe existing members if opt_in_existing=true
if (options.opt_in_existing) {
debug(`Subscribing members to newsletter '${newsletter.get('name')}'`);
// subscribe members that have an existing subscription to an active newsletter
const memberIds = await this.MemberModel.fetchAllSubscribed(_.pick(options, 'transacting'));
newsletter.meta = newsletter.meta || {};
newsletter.meta.opted_in_member_count = memberIds.length;
if (memberIds.length) {
debug(`Found ${memberIds.length} members to subscribe`);
await newsletter.subscribeMembersById(memberIds, options);
}
}
// send any verification emails and respond with the appropriate meta added
return this.respondWithEmailVerification(newsletter, emailsToVerify);
}
async edit(attrs, options) {
/**
* @public
* @param {object} attrs model properties
* @param {Object} [options] options
* @returns {Promise<{object}>} Newsetter Model with verification metadata
*/
async edit(attrs, options = {}) {
// fetch newsletter first so we can compare changed emails
const originalNewsletter = await this.NewsletterModel.findOne(options, {require: true});
@ -104,6 +136,11 @@ class NewslettersService {
return this.respondWithEmailVerification(updatedNewsletter, emailsToVerify);
}
/**
* @public
* @param {string} token - token that provides details of what to update
* @returns {Promise<{object}>} Newsetter Model
*/
async verifyPropertyUpdate(token) {
const data = await this.magicLinkService.getDataFromToken(token);
const {id, property, value} = data;
@ -114,8 +151,11 @@ class NewslettersService {
return this.NewsletterModel.edit(attrs, {id});
}
/* Email verification (private) */
/* Email verification Internals */
/**
* @private
*/
async prepAttrsForEmailVerification(attrs, newsletter) {
const cleanedAttrs = _.cloneDeep(attrs);
const emailsToVerify = [];
@ -133,6 +173,9 @@ class NewslettersService {
return {cleanedAttrs, emailsToVerify};
}
/**
* @private
*/
async requiresEmailVerification({email, hasChanged}) {
if (!email || !hasChanged) {
return false;
@ -143,20 +186,25 @@ class NewslettersService {
return true;
}
/**
* @private
*/
async respondWithEmailVerification(newsletter, emailsToVerify) {
if (emailsToVerify.length > 0) {
for (const {email, property} of emailsToVerify) {
await this.sendEmailVerificationMagicLink({id: newsletter.get('id'), email, property});
}
newsletter.meta = {
sent_email_verification: emailsToVerify.map(v => v.property)
};
newsletter.meta = newsletter.meta || {};
newsletter.meta.sent_email_verification = emailsToVerify.map(v => v.property);
}
return newsletter;
}
/**
* @private
*/
async sendEmailVerificationMagicLink({id, email, property = 'sender_from'}) {
const [,toDomain] = email.split('@');

48
test/e2e-api/admin/__snapshots__/newsletters.test.js.snap
View file

@ -1,5 +1,53 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP
exports[`Newsletters API Can add a newsletter - and subscribe existing members 1: [body] 1`] = `
Object {
"meta": Object {
"opted_in_member_count": 6,
},
"newsletters": Array [
Object {
"body_font_category": "serif",
"created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/,
"description": null,
"footer_content": null,
"header_image": null,
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"name": "New newsletter with existing members subscribed",
"sender_email": null,
"sender_name": "Test",
"sender_reply_to": "newsletter",
"show_badge": true,
"show_feature_image": true,
"show_header_icon": true,
"show_header_name": true,
"show_header_title": true,
"slug": "new-newsletter-with-existing-members-subscribed",
"sort_order": 5,
"status": "active",
"subscribe_on_signup": true,
"title_alignment": "center",
"title_font_category": "serif",
"updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/,
"visibility": "members",
},
],
}
`;
exports[`Newsletters API Can add a newsletter - and subscribe existing members 2: [headers] 1`] = `
Object {
"access-control-allow-origin": "http://127.0.0.1:2369",
"cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0",
"content-length": "699",
"content-type": "application/json; charset=utf-8",
"etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,
"location": StringMatching /https\\?:\\\\/\\\\/\\.\\*\\?\\\\/newsletters\\\\/\\[a-f0-9\\]\\{24\\}\\\\//,
"vary": "Origin, Accept-Encoding",
"x-powered-by": "Express",
}
`;
exports[`Newsletters API Can add a newsletter - with custom sender_email 1: [body] 1`] = `
Object {
"meta": Object {

53
test/e2e-api/admin/newsletters.test.js
View file

@ -1,6 +1,15 @@
const {agentProvider, mockManager, fixtureManager, matchers} = require('../../utils/e2e-framework');
const {anyEtag, anyObjectId, anyString, anyISODateTime, anyLocationFor} = matchers;
const testUtils = require('../../utils');
const {anyEtag, anyObjectId, anyISODateTime, anyLocationFor} = matchers;
const assert = require('assert');
const models = require('../../../core/server/models');
const assertMemberRelationCount = async (newsletterId, expectedCount) => {
const newsletter = await models.Newsletter.findOne({id: newsletterId}, {withRelated: 'members'});
assert.equal(newsletter.related('members').length, expectedCount);
};
const newsletterSnapshot = {
id: anyObjectId,
@ -40,7 +49,7 @@ describe('Newsletters API', function () {
it('Can read a newsletter', async function () {
await agent
.get(`newsletters/${testUtils.DataGenerator.Content.newsletters[0].id}/`)
.get(`newsletters/${fixtureManager.get('newsletters', 0).id}/`)
.expectStatus(200)
.matchBodySnapshot({
newsletters: [newsletterSnapshot]
@ -65,7 +74,7 @@ describe('Newsletters API', function () {
it('Can include members & posts counts when reading a newsletter', async function () {
await agent
.get(`newsletters/${testUtils.DataGenerator.Content.newsletters[0].id}/?include=count.members,count.posts`)
.get(`newsletters/${fixtureManager.get('newsletters', 0).id}/?include=count.members,count.posts`)
.expectStatus(200)
.matchBodySnapshot({
newsletters: new Array(1).fill(newsletterSnapshot)
@ -96,7 +105,7 @@ describe('Newsletters API', function () {
.body({newsletters: [newsletter]})
.expectStatus(201)
.matchBodySnapshot({
newsletters: new Array(1).fill(newsletterSnapshot)
newsletters: [newsletterSnapshot]
})
.matchHeaderSnapshot({
etag: anyEtag,
@ -125,7 +134,7 @@ describe('Newsletters API', function () {
.body({newsletters: [newsletter]})
.expectStatus(201)
.matchBodySnapshot({
newsletters: new Array(1).fill(newsletterSnapshot),
newsletters: [newsletterSnapshot],
meta: {
sent_email_verification: ['sender_email']
}
@ -141,6 +150,38 @@ describe('Newsletters API', function () {
});
});
it('Can add a newsletter - and subscribe existing members', async function () {
const newsletter = {
name: 'New newsletter with existing members subscribed',
sender_name: 'Test',
sender_email: null,
sender_reply_to: 'newsletter',
status: 'active',
subscribe_on_signup: true,
title_font_category: 'serif',
body_font_category: 'serif',
show_header_icon: true,
show_header_title: true,
show_badge: true,
sort_order: 0
};
const {body} = await agent
.post(`newsletters/?opt_in_existing=true`)
.body({newsletters: [newsletter]})
.expectStatus(201)
.matchBodySnapshot({
newsletters: [newsletterSnapshot]
})
.matchHeaderSnapshot({
etag: anyEtag,
location: anyLocationFor('newsletters')
});
// Assert that the newsletter has 6 related members in the DB
await assertMemberRelationCount(body.newsletters[0].id, 6);
});
it('Can edit newsletters', async function () {
const id = fixtureManager.get('newsletters', 0).id;
await agent.put(`newsletters/${id}`)

53
test/unit/server/services/newsletters/service.test.js
View file

@ -31,6 +31,7 @@ describe('NewslettersService', function () {
newsletterService = new NewslettersService({
NewsletterModel: models.Newsletter,
MemberModel: models.Member,
mail,
singleUseTokenProvider: tokenProvider,
urlUtils: urlUtils.stubUrlUtilsFromConfig()
@ -62,10 +63,14 @@ describe('NewslettersService', function () {
});
describe('add', function () {
let addStub,getNextAvailableSortOrderStub;
let addStub, fetchMembersStub, fakeMemberIds, subscribeStub, getNextAvailableSortOrderStub;
beforeEach(function () {
// Stub add as a function that returns a get
addStub = sinon.stub(models.Newsletter, 'add').returns({get: getStub});
fakeMemberIds = new Array(3).fill({id: 1});
subscribeStub = sinon.stub().returns(fakeMemberIds);
// Stub add as a function that returns get & subscribeMembersById methods
addStub = sinon.stub(models.Newsletter, 'add').returns({get: getStub, subscribeMembersById: subscribeStub});
fetchMembersStub = sinon.stub(models.Member, 'fetchAllSubscribed').returns([]);
getNextAvailableSortOrderStub = sinon.stub(models.Newsletter, 'getNextAvailableSortOrder').returns(1);
});
@ -73,6 +78,7 @@ describe('NewslettersService', function () {
assert.rejects(await newsletterService.add, {name: 'TypeError'});
sinon.assert.notCalled(addStub);
sinon.assert.notCalled(getNextAvailableSortOrderStub);
sinon.assert.notCalled(fetchMembersStub);
});
it('will attempt to add empty object without verification', async function () {
@ -80,7 +86,8 @@ describe('NewslettersService', function () {
assert.equal(result.meta, undefined); // meta property has not been added
sinon.assert.calledOnce(getNextAvailableSortOrderStub);
sinon.assert.calledOnceWithExactly(addStub, {sort_order: 1}, undefined);
sinon.assert.notCalled(fetchMembersStub);
sinon.assert.calledOnceWithExactly(addStub, {sort_order: 1}, {});
});
it('will override sort_order', async function () {
@ -90,6 +97,7 @@ describe('NewslettersService', function () {
await newsletterService.add(data, options);
sinon.assert.calledOnce(getNextAvailableSortOrderStub);
sinon.assert.notCalled(fetchMembersStub);
sinon.assert.calledOnceWithExactly(addStub, {name: 'hello world', sort_order: 1}, options);
});
@ -101,6 +109,7 @@ describe('NewslettersService', function () {
assert.equal(result.meta, undefined); // meta property has not been added
sinon.assert.calledOnceWithExactly(addStub, data, options);
sinon.assert.notCalled(fetchMembersStub);
});
it('will trigger verification when sender_email is provided', async function () {
@ -117,6 +126,40 @@ describe('NewslettersService', function () {
sinon.assert.calledOnceWithExactly(addStub, {name: 'hello world', sort_order: 1}, options);
mockManager.assert.sentEmail({to: 'test@example.com'});
sinon.assert.calledOnceWithExactly(tokenProvider.create, {id: undefined, property: 'sender_email', value: 'test@example.com'});
sinon.assert.notCalled(fetchMembersStub);
});
it('will try to find existing members when opt_in_existing is provided', async function () {
const data = {name: 'hello world'};
const options = {opt_in_existing: true};
const result = await newsletterService.add(data, options);
assert.deepEqual(result.meta, {
opted_in_member_count: 0
});
sinon.assert.calledOnceWithExactly(addStub, {name: 'hello world', sort_order: 1}, options);
mockManager.assert.sentEmailCount(0);
sinon.assert.calledOnce(fetchMembersStub);
});
it('will try to subscribe existing members when opt_in_existing provided + members exist', async function () {
const data = {name: 'hello world'};
const options = {opt_in_existing: true, transacting: 'foo'};
fetchMembersStub.returns(fakeMemberIds);
const result = await newsletterService.add(data, options);
assert.deepEqual(result.meta, {
opted_in_member_count: 3
});
sinon.assert.calledOnceWithExactly(addStub, {name: 'hello world', sort_order: 1}, options);
mockManager.assert.sentEmailCount(0);
sinon.assert.calledOnceWithExactly(fetchMembersStub, {transacting: 'foo'});
sinon.assert.calledOnceWithExactly(subscribeStub, fakeMemberIds, options);
});
});
@ -137,7 +180,7 @@ describe('NewslettersService', function () {
const result = await newsletterService.edit({});
assert.equal(result.meta, undefined); // meta property has not been added
sinon.assert.calledOnceWithExactly(editStub, {}, undefined);
sinon.assert.calledOnceWithExactly(editStub, {}, {});
});
it('will pass object and options through to model when there are no fields needing verification', async function () {