GLVSC-593: Check expiration of paid accounts offline #3522

sergiolms · 2024-09-06T11:03:13Z

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

I have followed the guidelines in the Contributing document
My changes follow the coding style of this project
My changes build without any errors or warnings
My changes have been formatted and linted
My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

axosoft-ramint

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).

src/webviews/apps/shared/components/feature-badge.ts

package.json

axosoft-ramint

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.

package.json

src/plus/gk/account/promos.ts

axosoft-ramint · 2024-09-17T19:01:49Z

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:

If the best license available is an expired paid license, we should probably let it through so that we can recognize this "expired paid" state rather than defaulting to "free", which may offer options like "start trial" that the user isn't eligible for yet.
But we should be careful not to have the paid, expired license data replace the trial data, in case someone does reactivate trial with an expired license: https://github.com/gitkraken/vscode-gitlens/blob/main/src/plus/gk/checkin.ts#L179-L181

sergiolms · 2024-09-18T08:25:19Z

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.

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.

We might need to update those computations:

If the best license available is an expired paid license, we should probably let it through so that we can recognize this "expired paid" state rather than defaulting to "free", which may offer options like "start trial" that the user isn't eligible for yet.

But we should be careful not to have the paid, expired license data replace the trial data, in case someone does reactivate trial with an expired license: https://github.com/gitkraken/vscode-gitlens/blob/main/src/plus/gk/checkin.ts#L179-L181

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

src/plus/gk/checkin.ts

axosoft-ramint · 2024-09-19T21:47:15Z

src/plus/gk/checkin.ts

+		(getSubscriptionPlanPriority(actual.id) >= getSubscriptionPlanPriority(effective.id) && !isActualLicenseExpired)
+	) {


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 thiseffective = { ...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.

Updated with all feedback except this one

axosoft-ramint · 2024-09-25T18:12:47Z

src/plus/gk/checkin.ts

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


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.

hmmm, okay, I just uploaded a commit with this

axosoft-ramint · 2024-10-15T14:56:27Z

Going to give this one more review/test pass today and we can merge if it looks good.

src/plus/gk/checkin.ts

axosoft-ramint · 2024-10-16T21:16:21Z

When I am in the "expired paid" state, the account view is confusing:

There is no indication here that my license is expired. Furthermore, it provides links to sign in/sign up, when it should be asking me to renew my license.

Any chance you can take a pass at the content here for this state?

d13 · 2024-10-16T23:36:27Z

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.

sergiolms requested review from justinrobots, axosoft-ramint and sergeibbb September 6, 2024 11:03

sergiolms self-assigned this Sep 6, 2024

sergiolms force-pushed the GLVSC-593_check_expiration_of_paid_accounts_offline branch from 613b2b9 to beda0e0 Compare September 6, 2024 11:36

axosoft-ramint reviewed Sep 6, 2024

View reviewed changes

src/webviews/apps/shared/components/feature-badge.ts Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

sergiolms requested a review from axosoft-ramint September 9, 2024 12:24

axosoft-ramint reviewed Sep 10, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

src/plus/gk/account/promos.ts Show resolved Hide resolved

d13 linked an issue Sep 12, 2024 that may be closed by this pull request

Check expiration of paid accounts even when user cannot check in (i.e. offline) #3569

Open

sergiolms force-pushed the GLVSC-593_check_expiration_of_paid_accounts_offline branch from f1a7185 to 9c29606 Compare September 13, 2024 09:28

sergiolms requested a review from axosoft-ramint September 13, 2024 09:28

sergiolms force-pushed the GLVSC-593_check_expiration_of_paid_accounts_offline branch from 9c29606 to 302c5da Compare September 17, 2024 07:46

axosoft-ramint approved these changes Sep 17, 2024

View reviewed changes

sergiolms force-pushed the GLVSC-593_check_expiration_of_paid_accounts_offline branch 2 times, most recently from d439669 to 3e6a3c4 Compare September 19, 2024 14:10

sergiolms requested a review from axosoft-ramint September 19, 2024 14:10

sergiolms force-pushed the GLVSC-593_check_expiration_of_paid_accounts_offline branch from 3e6a3c4 to 713ad28 Compare September 19, 2024 14:11

axosoft-ramint self-assigned this Sep 19, 2024

axosoft-ramint reviewed Sep 19, 2024

View reviewed changes

sergiolms force-pushed the GLVSC-593_check_expiration_of_paid_accounts_offline branch from 677526d to ff61804 Compare September 23, 2024 08:06

axosoft-ramint reviewed Sep 25, 2024

View reviewed changes

sergiolms force-pushed the GLVSC-593_check_expiration_of_paid_accounts_offline branch from 4a63648 to d2518d4 Compare September 26, 2024 15:13

sergiolms force-pushed the GLVSC-593_check_expiration_of_paid_accounts_offline branch 2 times, most recently from 2310bc7 to f42ce2b Compare October 4, 2024 11:01

sergiolms added 4 commits October 15, 2024 16:20

set allowed as false if subscription has expired

99373a7

Add new Paid Expired state to detect when a license has expired

dc4ace3

feedback changes

71249b0

take into account expired paid licenses in getSubscriptionFromCheckIn

79b418c

sergiolms added 3 commits October 15, 2024 16:28

remove expired licenses filter & check actual license expiration

0eaee8e

add isTrial check for pro trial accounts

f7feac4

don't rely only on latestStatus to set license as trial

69e4ba6

sergiolms force-pushed the GLVSC-593_check_expiration_of_paid_accounts_offline branch from f42ce2b to 69e4ba6 Compare October 15, 2024 14:28

axosoft-ramint self-requested a review October 15, 2024 14:56

axosoft-ramint requested changes Oct 15, 2024

View reviewed changes

src/plus/gk/checkin.ts Outdated Show resolved Hide resolved

check expiration time directly

1bbe2b2

sergiolms requested a review from axosoft-ramint October 16, 2024 13:31

d13 marked this pull request as draft October 16, 2024 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GLVSC-593: Check expiration of paid accounts offline #3522

GLVSC-593: Check expiration of paid accounts offline #3522

sergiolms commented Sep 6, 2024

axosoft-ramint left a comment

axosoft-ramint left a comment

axosoft-ramint commented Sep 17, 2024

sergiolms commented Sep 18, 2024

axosoft-ramint Sep 19, 2024

sergiolms Sep 20, 2024

axosoft-ramint Sep 25, 2024

sergiolms Sep 26, 2024

axosoft-ramint commented Oct 15, 2024

axosoft-ramint commented Oct 16, 2024

d13 commented Oct 16, 2024

		(getSubscriptionPlanPriority(actual.id) >= getSubscriptionPlanPriority(effective.id) && !isActualLicenseExpired)
		) {

GLVSC-593: Check expiration of paid accounts offline #3522

Are you sure you want to change the base?

GLVSC-593: Check expiration of paid accounts offline #3522

Conversation

sergiolms commented Sep 6, 2024

Description

Checklist

axosoft-ramint left a comment

Choose a reason for hiding this comment

axosoft-ramint left a comment

Choose a reason for hiding this comment

axosoft-ramint commented Sep 17, 2024

sergiolms commented Sep 18, 2024

axosoft-ramint Sep 19, 2024

Choose a reason for hiding this comment

sergiolms Sep 20, 2024

Choose a reason for hiding this comment

axosoft-ramint Sep 25, 2024

Choose a reason for hiding this comment

sergiolms Sep 26, 2024

Choose a reason for hiding this comment

axosoft-ramint commented Oct 15, 2024

axosoft-ramint commented Oct 16, 2024

d13 commented Oct 16, 2024