Skip to content
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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sergiolms
Copy link
Contributor

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:
Captura de pantalla 2024-09-06 a las 11 12 01

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

@sergiolms sergiolms self-assigned this Sep 6, 2024
@sergiolms sergiolms force-pushed the GLVSC-593_check_expiration_of_paid_accounts_offline branch from 613b2b9 to beda0e0 Compare September 6, 2024 11:36
Copy link
Contributor

@axosoft-ramint axosoft-ramint left a 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).

src/webviews/apps/shared/components/feature-badge.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@axosoft-ramint axosoft-ramint left a 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.

package.json Outdated Show resolved Hide resolved
src/plus/gk/account/promos.ts Show resolved Hide resolved
@sergiolms sergiolms force-pushed the GLVSC-593_check_expiration_of_paid_accounts_offline branch from f1a7185 to 9c29606 Compare September 13, 2024 09:28
@sergiolms 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
Copy link
Contributor

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:

  1. 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.
  2. 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
Copy link
Contributor Author

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:

  1. 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.
  2. 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

@sergiolms 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 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 axosoft-ramint self-assigned this Sep 19, 2024
src/plus/gk/checkin.ts Outdated Show resolved Hide resolved
src/plus/gk/checkin.ts Outdated Show resolved Hide resolved
Comment on lines 193 to 189
(getSubscriptionPlanPriority(actual.id) >= getSubscriptionPlanPriority(effective.id) && !isActualLicenseExpired)
) {
Copy link
Contributor

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:

  1. You have a paid, but expired, Pro license.
  2. 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.

Copy link
Contributor Author

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

@sergiolms sergiolms force-pushed the GLVSC-593_check_expiration_of_paid_accounts_offline branch from 677526d to ff61804 Compare September 23, 2024 08:06
@@ -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',
Copy link
Contributor

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.

Copy link
Contributor Author

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

@sergiolms sergiolms force-pushed the GLVSC-593_check_expiration_of_paid_accounts_offline branch from 4a63648 to d2518d4 Compare September 26, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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