-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GLVSC-593: Check expiration of paid accounts offline #3522
base: main
Are you sure you want to change the base?
GLVSC-593: Check expiration of paid accounts offline #3522
Conversation
613b2b9
to
beda0e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial things I noticed. Will test e2e next week (will take a bit to set up/mimic an account with a paid expired license).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I can only really simulate having an expired license in testing, but it showed the right messaging for me on Pro features and account view. Only suggestion would be to apply this state to our default pro50 promo as well.
f1a7185
to
9c29606
Compare
9c29606
to
302c5da
Compare
Heads-up: Just noticed after talking with @eamodio that we're currently filtering out expired paid licenses during the check-in computations: https://github.com/gitkraken/vscode-gitlens/blob/main/src/plus/gk/checkin.ts#L69-L71 This might mean that those paid licenses with expired date never make it into someone's license data, which means they would never enter the new state being set up here. We might need to update those computations:
|
That's right, they get filtered out when you log in. My changes only apply if you were already logged in and while offline your account expires.
Gotcha, so instead of filtering out now we will take it into account and only replace the effective license if the actual license isn't a paid expired. Good catch |
d439669
to
3e6a3c4
Compare
3e6a3c4
to
713ad28
Compare
src/plus/gk/checkin.ts
Outdated
(getSubscriptionPlanPriority(actual.id) >= getSubscriptionPlanPriority(effective.id) && !isActualLicenseExpired) | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other issue I am seeing with computeSubscriptionState
that could happen if we let expired subscriptions through:
- You have a paid, but expired, Pro license.
- You are on a (reactivated) Pro trial.
Note that this conditional is not triggered so effective = { ...actual }
does not happen, but...
effective
is still set in 171 to a trial license with effective.id === SubscriptionPlanId.Pro
. And actual
is an expired license with actual.id === SubscriptionPlanId.Pro
.
So when we hit computeSubscriptionState
, it will match the actual.id ===
effective.idcheck (which previously wouldn't happen unless we hit this
effective = { ...actual }` line, and that would only happen if it was an active paid license since we were filtering out paid licenses that were expired.
Ultimately, this would cause your license to show as a paid Pro license rather than an active Pro trial.
To avoid that regression, I suggest we just attach an isTrial
property to these and we can add that to the condition in computeSubscriptionState
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with all feedback except this one
677526d
to
ff61804
Compare
src/plus/gk/checkin.ts
Outdated
@@ -176,6 +177,7 @@ export function getSubscriptionFromCheckIn( | |||
new Date(license.latestEndDate), | |||
license.latestStatus === 'cancelled', | |||
license.nextOptInDate ?? data.nextOptInDate, | |||
license.latestStatus === 'trial' || license.latestStatus === 'in_trial', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably fine, but I'm not 100% sure if we can rely on latestStatus
coming through as 'trial' for trial licenses the way we are hoping for it to here. Since effective licenses from the api check-in response were always intended to be for trials, and paid licenses were always intended to be non-trial, we could probably just get away with using true
in effective licenses and false
for paid ones when setting them up from the check-in response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm, okay, I just uploaded a commit with this
4a63648
to
d2518d4
Compare
2310bc7
to
f42ce2b
Compare
f42ce2b
to
69e4ba6
Compare
Going to give this one more review/test pass today and we can merge if it looks good. |
Let's pause on this PR until after the 16.0 release, so we can spend more time and focus on the priority 16.0 items. Going to bump this back to draft in the meantime. |
Description
Paid licenses can expire while offline, so with this PR we introduce some checks and a new
Paid expired
state to users that own an expired license, so we can customize messages and promotions for them if we want to.Adding @justinrobots as CC to this PR because I'd like him to review the messages I set in case we want to change them or if I made a typo. 😄
Here's a screenshot of how it would look like for users that have a pro/enterprise account that has expired:
Checklist
Fixes $XXX -
orCloses #XXX -
prefix to auto-close the issue that your PR addresses