[server] private repo access after cancellation #4811
Conversation
svenefftinge
requested review from
a team,
jankeromnes and
geropl
and removed request for
a team and
jankeromnes
| @@ -101,7 +101,7 @@ export class SubscriptionService { | |||
| async hasActiveAndNotYetCancelledPaidSubscription(userId: string, date: Date): Promise<boolean> { | |||
| const subscriptions = await this.accountingDB.findActiveSubscriptionsForUser(userId, date.toISOString()); | |||
| return subscriptions | |||
| .filter(s => Subscription.isActive(s, date.toISOString()) && !Subscription.isCancelled(s)) | |||
| .filter(s => Subscription.isActive(s, date.toISOString())) | |||
There was a problem hiding this comment.
The method does what it says on the tin (name), and is used in the other places correctly I think.
But this call is odd indeed: I agree that we should call hasActivePaidSubscription with the implementation you suggested.
There was a problem hiding this comment.
@jankeromnes I faintly recall we added made the change here together, right? Do you recall what the motivation was for checking isActive && !isCancelled instead of isActive? IMO this was because of a user that:
- bought a subscription
- quickly cancelled it (and we reimbursed them)
- expected to be back in trial mode immediately (which we promised him when offered the "cancel-and-reimburse")
In general the user-frriendly pattern of "easy-in-easy-out" makes sense here.
svenefftinge
force-pushed
the
private-repos-after-cancellation
branch
from
d8920a8
to
e4ed389
Compare
| @@ -285,24 +285,6 @@ export class EligibilityService { | |||
| return new Date(freeTrialStartDate); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
I couldn't find a user, so good to remove.
| @@ -252,7 +252,7 @@ export class EligibilityService { | |||
| return true; | |||
| } | |||
|
|
|||
| return this.subscriptionService.hasActiveAndNotYetCancelledPaidSubscription(user.id, date); | |||
| return this.subscriptionService.hasActivePaidSubscription(user.id, date); | |||
| @@ -314,7 +296,7 @@ export class EligibilityService { | |||
| if (start === undefined) { | |||
| return undefined; | |||
| } | |||
| if (await this.subscriptionService.hasActiveAndNotYetCancelledPaidSubscription(user.id, date)) { | |||
| if (await this.subscriptionService.hasActivePaidSubscription(user.id, date)) { | |||
There was a problem hiding this comment.
This is an edge case, I'm sure we can handle the impact. We dearly need (more) tests including reasoning for this kind of policy changes. 🙈
If a user cancels their subscription, they should still be able to use it until it ends.