TryGhost/Ghost
1
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2024-12-25 11:55:03 +03:00
Code Issues Projects Releases Wiki Activity

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

Browse Source 
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
This commit is contained in:
Simon Backx 2022-08-30 17:36:52 +02:00 committed by GitHub
parent dbfcc5a733
commit e7786ca482
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 157 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

117
ghost/core/test/e2e-api/admin/members.test.js
View File

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

59
ghost/members-api/lib/repositories/member.js
View File

@ -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
}
}
}