🐛 Fixed removing comped subscriptions for members with active subs#15332
Conversation
…subscriptions refs 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 - Allow the addition of comped products to a member, as long as that member doesn't have other active subscriptions - Includes tests for these edge cases
Codecov Report
@@ Coverage Diff @@
## main #15332 +/- ##
==========================================
+ Coverage 52.75% 58.51% +5.76%
==========================================
Files 1399 767 -632
Lines 89896 64101 -25795
Branches 10216 5642 -4574
==========================================
- Hits 47422 37509 -9913
+ Misses 42423 26541 -15882
Partials 51 51 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
…active subscriptions
…active subscriptions
allouis
left a comment
allouis
left a comment
There was a problem hiding this comment.
Couple comments but otherwise 👍
| it('Cannot add complimentary subscriptions to a member with an active subscription', async function () { | ||
| if (!memberWithPaidSubscription) { | ||
| // Previous test failed | ||
| this.skip(); |
There was a problem hiding this comment.
This is an interesting pattern, have you seen this used elsewhere in the tests?
I'm not against it, as it is frustrating when a single test failing causes a cascade of later ones failing.
Ultimately that's because we have tests depending on some global state outside of the test, which IMO is not good.
We could fix this by making sure each test does the setup that it needs, but that adds overhead.
Using this pattern at least makes the coupling explicit 🤔 Though what would happen if we removed the previous test, it would render this test useless right?
I'd be interested in your thoughts either way on this, and if you think it's a good idea - let's talk with Hannah about sharing/using this pattern going forwards
There was a problem hiding this comment.
Yeah, I introduced this pattern a couple of months ago in the same file. I didn't really think a lot about it when writing these additional tests. I don't think this pattern is ideal, but it was an effort to prevent having to repeat all the Stripe mocking in every test. But moving that repeated part to the top might be a better solution... 🤔 Or we could build the models manually without mocking Stripe...
There was a problem hiding this comment.
Gonna move this to a tech debt issue and clean up the whole test file in one go 🧹
There was a problem hiding this comment.
@SimonBackx what was the outcome of the cleanup? If the memberWithPaidSubscription is used in a group of test, like it seems to be done here, probably best would be grouping such tests into a "describe" and having a memberWithPaidSubscription be set up through fixtures? 🤔
…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
3 participants
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.