From e7786ca4823e0cd5477e402df4b0775ff2eab20a Mon Sep 17 00:00:00 2001 From: Simon Backx Date: Tue, 30 Aug 2022 17:36:52 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20removing=20comped=20subs?= =?UTF-8?q?criptions=20for=20members=20with=20active=20subs=20(#15332)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fixes https://github.com/TryGhost/Team/issues/1859 **Problem:** When for some reason a member has an active subscription (or legacy comped subscription) for product A, and a comped subscription for product B. You cannot remove comped subscription B. **Fixed by:** Updating the API to allow more flexible product changes on members. - Allow the removal of (comped) products on a member, as long as that product doesn't have a related subscription - (still) allow the addition of comped products to a member, as long as that member doesn't have other active subscriptions. This matches the existing behaviour, but now this is only checked for added products. - Includes tests for these edge cases --- ghost/core/test/e2e-api/admin/members.test.js | 117 +++++++++++++++++- ghost/members-api/lib/repositories/member.js | 59 ++++++--- 2 files changed, 157 insertions(+), 19 deletions(-) diff --git a/ghost/core/test/e2e-api/admin/members.test.js b/ghost/core/test/e2e-api/admin/members.test.js index 27c096575b..f46a34af04 100644 --- a/ghost/core/test/e2e-api/admin/members.test.js +++ b/ghost/core/test/e2e-api/admin/members.test.js @@ -38,6 +38,10 @@ async function getPaidProduct() { return await models.Product.findOne({type: 'paid'}); } +async function getOtherPaidProduct() { + return (await models.Product.findAll({type: 'paid'})).models[0]; +} + async function getNewsletters() { return (await models.Newsletter.findAll({filter: 'status:active'})).models; } @@ -840,7 +844,7 @@ describe('Members API', function () { const newMember = body.members[0]; - await agent + const updatedMember = await agent .put(`/members/${newMember.id}/`) .body({members: [compedPayload]}) .expectStatus(200) @@ -1347,7 +1351,7 @@ describe('Members API', function () { mrr: 100 }); - // Save this member for the next test + // Save this member for the next tests memberWithPaidSubscription = newMember; }); @@ -1384,6 +1388,115 @@ describe('Members API', function () { should.deepEqual(memberWithPaidSubscription, readMember, 'Editing a member returns a different format than reading a member'); }); + it('Cannot add complimentary subscriptions to a member with an active subscription', async function () { + if (!memberWithPaidSubscription) { + // Previous test failed + this.skip(); + } + const product = await getOtherPaidProduct(); + + const compedPayload = { + id: memberWithPaidSubscription.id, + tiers: [ + ...memberWithPaidSubscription.tiers, + { + id: product.id + } + ] + }; + + await agent + .put(`/members/${memberWithPaidSubscription.id}/`) + .body({members: [compedPayload]}) + .expectStatus(400); + }); + + it('Cannot remove non complimentary subscriptions directly from a member', async function () { + if (!memberWithPaidSubscription) { + // Previous test failed + this.skip(); + } + + const compedPayload = { + id: memberWithPaidSubscription.id, + // Remove all paid subscriptions (= not allowed atm) + tiers: [] + }; + + await agent + .put(`/members/${memberWithPaidSubscription.id}/`) + .body({members: [compedPayload]}) + .expectStatus(400); + }); + + it('Can remove a complimentary subscription directly from a member with other active subscriptions', async function () { + // This tests for an edge case that shouldn't be possible, but the API should support this to resolve issues + // refs https://github.com/TryGhost/Team/issues/1859 + + if (!memberWithPaidSubscription) { + // Previous test failed + this.skip(); + } + + // Check that the product that we are going to add is not the same as the existing one + const product = await getOtherPaidProduct(); + should(memberWithPaidSubscription.tiers).have.length(1); + should(memberWithPaidSubscription.tiers[0].id).not.eql(product.id); + + // Add it manually + const member = await models.Member.edit({ + products: [ + ...memberWithPaidSubscription.tiers, + { + id: product.id + } + ] + }, {id: memberWithPaidSubscription.id}); + + // Check status + const {body: body2} = await agent + .get(`/members/${memberWithPaidSubscription.id}/`) + .expectStatus(200); + + const beforeMember = body2.members[0]; + assert.equal(beforeMember.tiers.length, 2, 'The member should have two products now'); + + // Now try to remove only the complimentary one + const compedPayload = { + id: memberWithPaidSubscription.id, + // Remove all complimentary subscriptions + tiers: memberWithPaidSubscription.tiers + }; + + const {body} = await agent + .put(`/members/${memberWithPaidSubscription.id}/`) + .body({members: [compedPayload]}) + .expectStatus(200); + + const updatedMember = body.members[0]; + assert.equal(updatedMember.status, 'paid', 'Member should still have the paid status'); + assert.equal(updatedMember.tiers.length, 1, 'The member should have one product now'); + assert.equal(updatedMember.tiers[0].id, memberWithPaidSubscription.tiers[0].id, 'The member should have the paid product'); + }); + + it('Can keep tiers unchanged when modifying a paid member', async function () { + if (!memberWithPaidSubscription) { + // Previous test failed + this.skip(); + } + + const compedPayload = { + id: memberWithPaidSubscription.id, + // Not changed tiers + tiers: [...memberWithPaidSubscription.tiers] + }; + + await agent + .put(`/members/${memberWithPaidSubscription.id}/`) + .body({members: [compedPayload]}) + .expectStatus(200); + }); + it('Can edit by id', async function () { const memberToChange = { name: 'change me', diff --git a/ghost/members-api/lib/repositories/member.js b/ghost/members-api/lib/repositories/member.js index 7f6fa48b00..eeeed52406 100644 --- a/ghost/members-api/lib/repositories/member.js +++ b/ghost/members-api/lib/repositories/member.js @@ -10,7 +10,8 @@ const {NotFoundError} = require('@tryghost/errors'); const messages = { noStripeConnection: 'Cannot {action} without a Stripe Connection', moreThanOneProduct: 'A member cannot have more than one Product', - existingSubscriptions: 'Cannot modify Products for a Member with active Subscriptions', + addProductWithActiveSubscription: 'Cannot add comped Products to a Member with active Subscriptions', + deleteProductWithActiveSubscription: 'Cannot delete a non-comped Product from a Member, because it has an active Subscription for the same product', memberNotFound: 'Could not find Member {id}', subscriptionNotFound: 'Could not find Subscription {id}', productNotFound: 'Could not find Product {id}', @@ -406,27 +407,51 @@ module.exports = class MemberRepository { productsToAdd = _.differenceWith(incomingProductIds, existingProductIds); productsToRemove = _.differenceWith(existingProductIds, incomingProductIds); const productsToModify = productsToAdd.concat(productsToRemove); - + if (productsToModify.length !== 0) { - const exisitingSubscriptions = await initialMember.related('stripeSubscriptions').fetch(sharedOptions); - const existingActiveSubscriptions = exisitingSubscriptions.filter((subscription) => { - return this.isActiveSubscriptionStatus(subscription.get('status')); - }); + // Load active subscriptions information + await initialMember.load( + [ + 'stripeSubscriptions', + 'stripeSubscriptions.stripePrice', + 'stripeSubscriptions.stripePrice.stripeProduct', + 'stripeSubscriptions.stripePrice.stripeProduct.product' + ], sharedOptions); + + const exisitingSubscriptions = initialMember.related('stripeSubscriptions')?.models ?? []; + + if (productsToRemove.length > 0) { + // Only allow to delete comped products without a subscription attached to them + // Other products should be removed by canceling them via the related stripe subscription + const dontAllowToRemoveProductsIds = exisitingSubscriptions + .filter(sub => this.isActiveSubscriptionStatus(sub.get('status'))) + .map(sub => sub.related('stripePrice')?.related('stripeProduct')?.get('product_id')); - if (existingActiveSubscriptions.length) { - throw new errors.BadRequestError({message: tpl(messages.existingSubscriptions)}); + for (const deleteId of productsToRemove) { + if (dontAllowToRemoveProductsIds.includes(deleteId)) { + throw new errors.BadRequestError({message: tpl(messages.deleteProductWithActiveSubscription)}); + } + } + + if (incomingProductIds.length === 0) { + // CASE: We are removing all (comped) products from a member & there were no active subscriptions - the member is "free" + memberStatusData.status = 'free'; + } } - } - // CASE: We are removing all products from a member & there were no active subscriptions - the member is "free" - if (incomingProductIds.length === 0) { - memberStatusData.status = 'free'; - } else { - // CASE: We are changing products & there were not active stripe subscriptions - the member is "comped" - if (productsToModify.length !== 0) { + if (productsToAdd.length > 0) { + // Don't allow to add complimentary subscriptions (= creating a new product) when the member already has an active + // subscription + const existingActiveSubscriptions = exisitingSubscriptions.filter((subscription) => { + return this.isActiveSubscriptionStatus(subscription.get('status')); + }); + + if (existingActiveSubscriptions.length) { + throw new errors.BadRequestError({message: tpl(messages.addProductWithActiveSubscription)}); + } + + // CASE: We are changing products & there were not active stripe subscriptions - the member is "comped" memberStatusData.status = 'comped'; - } else { - // CASE: We are not changing any products - leave the status alone } } }