🐛 Fixed removing comped subscriptions for members with active subs (#15332)

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

@@ -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',

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}',
@@ -408,25 +409,49 @@ module.exports = class MemberRepository {
             const productsToModify = productsToAdd.concat(productsToRemove);
            
             if (productsToModify.length !== 0) {
-                const exisitingSubscriptions = await initialMember.related('stripeSubscriptions').fetch(sharedOptions);
+                // 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'));
+
+                    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';
+                    }
+                }
+
+                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.existingSubscriptions)});
-                }
+                        throw new errors.BadRequestError({message: tpl(messages.addProductWithActiveSubscription)});
                     }
 
-            // 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) {
                     memberStatusData.status = 'comped';
-                } else {
-                    // CASE: We are not changing any products - leave the status alone
                 }
             }
         }