From 6af2706f10a8e21917745aa8d27e485bac5435d8 Mon Sep 17 00:00:00 2001 From: Fabien 'egg' O'Carroll Date: Thu, 28 Jan 2021 16:31:02 +0000 Subject: [PATCH] Updated Admin API and Mega to use status flag (#12579) no-issue * Removed support for paid param from v3 & canary API * Updated active subscription checks to use status flag * Updated MEGA to use status filter over paid flag * Removed support for paid option at model level * Installed @tryghost/members-api@1.0.0-rc.0 * Updated members fixtures --- core/frontend/services/themes/middleware.js | 4 +- core/server/api/canary/members.js | 6 +- core/server/api/v3/members.js | 6 +- core/server/models/member.js | 74 +++++-------------- core/server/services/mega/mega.js | 14 ++-- .../server/services/members/content-gating.js | 7 +- core/server/services/members/utils.js | 4 +- package.json | 2 +- test/api-acceptance/admin/members_spec.js | 4 +- test/regression/models/model_members_spec.js | 14 ---- .../output/utils/post-gating_spec.js | 42 +---------- .../output/utils/post-gating_spec.js | 42 +---------- test/utils/fixtures/data-generator.js | 12 ++- yarn.lock | 12 +-- 14 files changed, 57 insertions(+), 186 deletions(-) diff --git a/core/frontend/services/themes/middleware.js b/core/frontend/services/themes/middleware.js index c9ed525a70..7171cc4c3d 100644 --- a/core/frontend/services/themes/middleware.js +++ b/core/frontend/services/themes/middleware.js @@ -133,9 +133,7 @@ function updateLocalTemplateOptions(req, res, next) { firstname: req.member.name && req.member.name.split(' ')[0], avatar_image: req.member.avatar_image, subscriptions: req.member.stripe.subscriptions, - paid: req.member.stripe.subscriptions.filter((subscription) => { - return ['active', 'trialing', 'unpaid', 'past_due'].includes(subscription.status); - }).length !== 0 + paid: req.member.status === 'paid' } : null; hbs.updateLocalTemplateOptions(res.locals, _.merge({}, localTemplateOptions, { diff --git a/core/server/api/canary/members.js b/core/server/api/canary/members.js index ff8f8470ca..3f38aa19b4 100644 --- a/core/server/api/canary/members.js +++ b/core/server/api/canary/members.js @@ -34,8 +34,7 @@ module.exports = { 'order', 'debug', 'page', - 'search', - 'paid' + 'search' ], permissions: true, validation: {}, @@ -304,8 +303,7 @@ module.exports = { options: [ 'limit', 'filter', - 'search', - 'paid' + 'search' ], headers: { disposition: { diff --git a/core/server/api/v3/members.js b/core/server/api/v3/members.js index ff8f8470ca..3f38aa19b4 100644 --- a/core/server/api/v3/members.js +++ b/core/server/api/v3/members.js @@ -34,8 +34,7 @@ module.exports = { 'order', 'debug', 'page', - 'search', - 'paid' + 'search' ], permissions: true, validation: {}, @@ -304,8 +303,7 @@ module.exports = { options: [ 'limit', 'filter', - 'search', - 'paid' + 'search' ], headers: { disposition: { diff --git a/core/server/models/member.js b/core/server/models/member.js index 6b24aef521..475da7af8a 100644 --- a/core/server/models/member.js +++ b/core/server/models/member.js @@ -1,7 +1,6 @@ const ghostBookshelf = require('./base'); const uuid = require('uuid'); const _ = require('lodash'); -const {sequence} = require('@tryghost/promise'); const config = require('../../shared/config'); const crypto = require('crypto'); @@ -108,7 +107,6 @@ const Member = ghostBookshelf.Model.extend({ onSaving: function onSaving(model, attr, options) { let labelsToSave = []; - let ops = []; // CASE: detect lowercase/uppercase label slugs if (!_.isUndefined(this.get('labels')) && !_.isNull(this.get('labels'))) { @@ -129,26 +127,23 @@ const Member = ghostBookshelf.Model.extend({ this.set('labels', labelsToSave); } - // CASE: Detect existing labels with same case-insensitive name and replace - ops.push(function updateLabels() { - return ghostBookshelf.model('Label') - .findAll(Object.assign({ - columns: ['id', 'name'] - }, _.pick(options, 'transacting'))) - .then((labels) => { - labelsToSave.forEach((label) => { - let existingLabel = labels.find((lab) => { - return label.name.toLowerCase() === lab.get('name').toLowerCase(); - }); - label.name = (existingLabel && existingLabel.get('name')) || label.name; - }); - - model.set('labels', labelsToSave); - }); - }); - this.handleAttachedModels(model); - return sequence(ops); + + // CASE: Detect existing labels with same case-insensitive name and replace + return ghostBookshelf.model('Label') + .findAll(Object.assign({ + columns: ['id', 'name'] + }, _.pick(options, 'transacting'))) + .then((labels) => { + labelsToSave.forEach((label) => { + let existingLabel = labels.find((lab) => { + return label.name.toLowerCase() === lab.get('name').toLowerCase(); + }); + label.name = (existingLabel && existingLabel.get('name')) || label.name; + }); + + model.set('labels', labelsToSave); + }); }, handleAttachedModels: function handleAttachedModels(model) { @@ -209,40 +204,6 @@ const Member = ghostBookshelf.Model.extend({ queryBuilder.orWhere('members.email', 'like', `%${query}%`); }, - // TODO: hacky way to filter by members with an active subscription, - // replace with a proper way to do this via filter param. - // NOTE: assumes members will have a single subscription - customQuery: function customQuery(queryBuilder, options) { - if (options.paid === true) { - queryBuilder.innerJoin( - 'members_stripe_customers', - 'members.id', - 'members_stripe_customers.member_id' - ); - queryBuilder.innerJoin( - 'members_stripe_customers_subscriptions', - function () { - this.on( - 'members_stripe_customers.customer_id', - 'members_stripe_customers_subscriptions.customer_id' - ).onIn( - 'members_stripe_customers_subscriptions.status', - ['active', 'trialing', 'past_due', 'unpaid'] - ); - } - ); - } - - if (options.paid === false) { - queryBuilder.leftJoin( - 'members_stripe_customers', - 'members.id', - 'members_stripe_customers.member_id' - ); - queryBuilder.whereNull('members_stripe_customers.member_id'); - } - }, - orderRawQuery(field, direction) { if (field === 'email_open_rate') { return { @@ -276,8 +237,7 @@ const Member = ghostBookshelf.Model.extend({ let options = ghostBookshelf.Model.permittedOptions.call(this, methodName); if (['findPage', 'findAll'].includes(methodName)) { - // TODO: remove 'paid' once it's possible to use in a filter - options = options.concat(['search', 'paid']); + options = options.concat(['search']); } return options; diff --git a/core/server/services/mega/mega.js b/core/server/services/mega/mega.js index ec9c6cd765..ac96da8f13 100644 --- a/core/server/services/mega/mega.js +++ b/core/server/services/mega/mega.js @@ -92,18 +92,19 @@ const sendTestEmail = async (postModel, toEmails) => { const addEmail = async (postModel, options) => { const knexOptions = _.pick(options, ['transacting', 'forUpdate']); - const filterOptions = Object.assign({}, knexOptions, {filter: 'subscribed:true', limit: 1}); + const filterOptions = Object.assign({}, knexOptions, {limit: 1}); const emailRecipientFilter = postModel.get('email_recipient_filter'); switch (emailRecipientFilter) { case 'paid': - filterOptions.paid = true; + filterOptions.filter = 'subscribed:true+status:paid'; break; case 'free': - filterOptions.paid = false; + filterOptions.filter = 'subscribed:true+status:free'; break; case 'all': + filterOptions.filter = 'subscribed:true'; break; case 'none': throw new Error('Cannot sent email to "none" email_recipient_filter'); @@ -288,18 +289,19 @@ async function getEmailMemberRows({emailModel, options}) { const knexOptions = _.pick(options, ['transacting', 'forUpdate']); // TODO: this will clobber a user-assigned filter if/when we allow emails to be sent to filtered member lists - const filterOptions = Object.assign({}, knexOptions, {filter: 'subscribed:true'}); + const filterOptions = Object.assign({}, knexOptions); const recipientFilter = emailModel.get('recipient_filter'); switch (recipientFilter) { case 'paid': - filterOptions.paid = true; + filterOptions.filter = 'subscribed:true+status:paid'; break; case 'free': - filterOptions.paid = false; + filterOptions.filter = 'subscribed:true+status:free'; break; case 'all': + filterOptions.filter = 'subscribed:true'; break; default: throw new Error(`Unknown recipient_filter ${recipientFilter}`); diff --git a/core/server/services/members/content-gating.js b/core/server/services/members/content-gating.js index 33041a968b..631cf12426 100644 --- a/core/server/services/members/content-gating.js +++ b/core/server/services/members/content-gating.js @@ -23,12 +23,7 @@ function checkPostAccess(post, member) { return PERMIT_ACCESS; } - const activeSubscriptions = member.stripe && member.stripe.subscriptions && member.stripe.subscriptions.filter((subscription) => { - return ['active', 'trialing', 'unpaid', 'past_due'].includes(subscription.status); - }); - const memberHasPlan = activeSubscriptions && activeSubscriptions.length; - - if (post.visibility === 'paid' && memberHasPlan) { + if (post.visibility === 'paid' && member.status === 'paid') { return PERMIT_ACCESS; } diff --git a/core/server/services/members/utils.js b/core/server/services/members/utils.js index 5b1aea6d14..ffd2fddac0 100644 --- a/core/server/services/members/utils.js +++ b/core/server/services/members/utils.js @@ -10,8 +10,6 @@ module.exports.formattedMemberResponse = function formattedMemberResponse(member avatar_image: member.avatar_image, subscribed: !!member.subscribed, subscriptions: member.stripe ? member.stripe.subscriptions : [], - paid: member.stripe && member.stripe.subscriptions && member.stripe.subscriptions.filter((subscription) => { - return ['active', 'trialing', 'unpaid', 'past_due'].includes(subscription.status); - }).length !== 0 + paid: member.status === 'paid' }; }; diff --git a/package.json b/package.json index f1fbf1d22a..9f89dd4327 100644 --- a/package.json +++ b/package.json @@ -57,7 +57,7 @@ "@tryghost/kg-mobiledoc-html-renderer": "3.0.1", "@tryghost/magic-link": "0.6.4", "@tryghost/maintenance": "0.1.0", - "@tryghost/members-api": "0.37.7", + "@tryghost/members-api": "1.0.0-rc.0", "@tryghost/members-csv": "0.4.2", "@tryghost/members-ssr": "0.8.8", "@tryghost/mw-session-from-token": "0.1.14", diff --git a/test/api-acceptance/admin/members_spec.js b/test/api-acceptance/admin/members_spec.js index 8e4da9fa06..4097220ea3 100644 --- a/test/api-acceptance/admin/members_spec.js +++ b/test/api-acceptance/admin/members_spec.js @@ -87,9 +87,9 @@ describe('Members API', function () { localUtils.API.checkResponse(jsonResponse.meta.pagination, 'pagination'); }); - it('Can browse with paid', async function () { + it('Can filter by paid status', async function () { const res = await request - .get(localUtils.API.getApiQuery('members/?paid=true')) + .get(localUtils.API.getApiQuery('members/?filter=status:paid')) .set('Origin', config.get('url')) .expect('Content-Type', /json/) .expect('Cache-Control', testUtils.cacheRules.private) diff --git a/test/regression/models/model_members_spec.js b/test/regression/models/model_members_spec.js index c63f36da8d..a1436a9938 100644 --- a/test/regression/models/model_members_spec.js +++ b/test/regression/models/model_members_spec.js @@ -210,20 +210,6 @@ describe('Member Model', function run() { describe('findAll', function () { beforeEach(testUtils.setup('members')); - it('can use custom query', function (done) { - Member.findAll().then(function (allResult) { - allResult.length.should.equal(4); - - return Member.findAll({paid: true}); - }).then(function (queryResult) { - queryResult.length.should.equal(2); - queryResult.models[0].get('email').should.equal('paid@test.com'); - queryResult.models[1].get('email').should.equal('trialing@test.com'); - - done(); - }).catch(done); - }); - it('can use search query', function (done) { Member.findAll({search: 'egg'}).then(function (queryResult) { queryResult.length.should.equal(1); diff --git a/test/unit/api/canary/utils/serializers/output/utils/post-gating_spec.js b/test/unit/api/canary/utils/serializers/output/utils/post-gating_spec.js index 40d7b5851a..c4803d92b2 100644 --- a/test/unit/api/canary/utils/serializers/output/utils/post-gating_spec.js +++ b/test/unit/api/canary/utils/serializers/output/utils/post-gating_spec.js @@ -85,7 +85,7 @@ describe('Unit: canary/utils/serializers/output/utils/post-gating', function () attrs.html.should.eql('

What\'s the matter?

'); }); - it('should hide content attributes when visibility is "paid" and member has no subscription', function () { + it('should hide content attributes when visibility is "paid" and member has status of "free"', function () { const attrs = { visibility: 'paid', plaintext: 'I see dead people', @@ -97,9 +97,7 @@ describe('Unit: canary/utils/serializers/output/utils/post-gating', function () original: { context: { member: { - stripe: { - subscriptions: [] - } + status: 'free' } } } @@ -111,35 +109,7 @@ describe('Unit: canary/utils/serializers/output/utils/post-gating', function () attrs.html.should.eql(''); }); - it('should hide content attributes when visibility is "paid" and member has cancelled subscription', function () { - const attrs = { - visibility: 'paid', - plaintext: 'I see dead people', - html: '

What\'s the matter?

' - }; - - const frame = { - options: {}, - original: { - context: { - member: { - stripe: { - subscriptions: [{ - status: 'canceled' - }] - } - } - } - } - }; - - gating.forPost(attrs, frame); - - attrs.plaintext.should.eql(''); - attrs.html.should.eql(''); - }); - - it('should NOT hide content attributes when visibility is "paid" and member has a subscription', function () { + it('should NOT hide content attributes when visibility is "paid" and member has status of "paid"', function () { const attrs = { visibility: 'paid', plaintext: 'Secret paid content', @@ -151,11 +121,7 @@ describe('Unit: canary/utils/serializers/output/utils/post-gating', function () original: { context: { member: { - stripe: { - subscriptions: [{ - status: 'active' - }] - } + status: 'paid' } } } diff --git a/test/unit/api/v2/utils/serializers/output/utils/post-gating_spec.js b/test/unit/api/v2/utils/serializers/output/utils/post-gating_spec.js index 0cd9b2c6ba..3cebdb7628 100644 --- a/test/unit/api/v2/utils/serializers/output/utils/post-gating_spec.js +++ b/test/unit/api/v2/utils/serializers/output/utils/post-gating_spec.js @@ -82,7 +82,7 @@ describe('Unit: v2/utils/serializers/output/utils/post-gating', function () { attrs.html.should.eql('

What\'s the matter?

'); }); - it('should hide content attributes when visibility is "paid" and member has no subscription', function () { + it('should hide content attributes when visibility is "paid" and member has status of "free"', function () { const attrs = { visibility: 'paid', plaintext: 'I see dead people', @@ -93,9 +93,7 @@ describe('Unit: v2/utils/serializers/output/utils/post-gating', function () { original: { context: { member: { - stripe: { - subscriptions: [] - } + status: 'free' } } } @@ -107,35 +105,7 @@ describe('Unit: v2/utils/serializers/output/utils/post-gating', function () { attrs.html.should.eql(''); }); - it('should hide content attributes when visibility is "paid" and member has cancelled subscription', function () { - const attrs = { - visibility: 'paid', - plaintext: 'I see dead people', - html: '

What\'s the matter?

' - }; - - const frame = { - options: {}, - original: { - context: { - member: { - stripe: { - subscriptions: [{ - status: 'canceled' - }] - } - } - } - } - }; - - gating.forPost(attrs, frame); - - attrs.plaintext.should.eql(''); - attrs.html.should.eql(''); - }); - - it('should NOT hide content attributes when visibility is "paid" and member has a subscription', function () { + it('should NOT hide content attributes when visibility is "paid" and member has status of "paid"', function () { const attrs = { visibility: 'paid', plaintext: 'Secret paid content', @@ -147,11 +117,7 @@ describe('Unit: v2/utils/serializers/output/utils/post-gating', function () { original: { context: { member: { - stripe: { - subscriptions: [{ - status: 'active' - }] - } + status: 'paid' } } } diff --git a/test/utils/fixtures/data-generator.js b/test/utils/fixtures/data-generator.js index 3458d99b59..684a08d382 100644 --- a/test/utils/fixtures/data-generator.js +++ b/test/utils/fixtures/data-generator.js @@ -311,26 +311,30 @@ DataGenerator.Content = { id: ObjectId.generate(), email: 'member1@test.com', name: 'Mr Egg', - uuid: 'f6f91461-d7d8-4a3f-aa5d-8e582c40b340' + uuid: 'f6f91461-d7d8-4a3f-aa5d-8e582c40b340', + status: 'free' }, { id: ObjectId.generate(), email: 'member2@test.com', email_open_rate: 50, - uuid: 'f6f91461-d7d8-4a3f-aa5d-8e582c40b341' + uuid: 'f6f91461-d7d8-4a3f-aa5d-8e582c40b341', + status: 'free' }, { id: ObjectId.generate(), email: 'paid@test.com', name: 'Egon Spengler', email_open_rate: 80, - uuid: 'f6f91461-d7d8-4a3f-aa5d-8e582c40b342' + uuid: 'f6f91461-d7d8-4a3f-aa5d-8e582c40b342', + status: 'paid' }, { id: ObjectId.generate(), email: 'trialing@test.com', name: 'Ray Stantz', - uuid: 'f6f91461-d7d8-4a3f-aa5d-8e582c40b343' + uuid: 'f6f91461-d7d8-4a3f-aa5d-8e582c40b343', + status: 'paid' } ], diff --git a/yarn.lock b/yarn.lock index 07b9e18184..c53813d2df 100644 --- a/yarn.lock +++ b/yarn.lock @@ -553,17 +553,17 @@ dependencies: ghost-ignition "^4.4.3" -"@tryghost/members-api@0.37.7": - version "0.37.7" - resolved "https://registry.yarnpkg.com/@tryghost/members-api/-/members-api-0.37.7.tgz#95102f775a0647898cf26919eb543a5bd0aa7778" - integrity sha512-bE/5MO2OPak3OhJK4piz+U9ocnYRTMM/7zBYIOtTDCvGBmCsEUoChUEHNqHHrcE9Iw/cYcdPPqgtlcGkXbCgug== +"@tryghost/members-api@1.0.0-rc.0": + version "1.0.0-rc.0" + resolved "https://registry.yarnpkg.com/@tryghost/members-api/-/members-api-1.0.0-rc.0.tgz#e7769b8e886ea32c975e507268a1677b1af890a2" + integrity sha512-gHLwgyXl5wXlvNvtRYA6N4Nqod+oWEIa7g62lpZQCUQFeJkSZT1UKwTCCTs6Rd3CPjxo0QVk5dMb3y1vxLc/mg== dependencies: - "@tryghost/magic-link" "^0.6.6" + "@tryghost/magic-link" "^0.6.5" bluebird "^3.5.4" body-parser "^1.19.0" cookies "^0.8.0" express "^4.16.4" - ghost-ignition "4.4.3" + ghost-ignition "4.4.2" got "^9.6.0" jsonwebtoken "^8.5.1" leaky-bucket "2.2.0"