-
Notifications
You must be signed in to change notification settings - Fork 124
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
NEOS-1579: add free trial timer to header #2871
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2871 +/- ##
==========================================
+ Coverage 29.58% 39.04% +9.45%
==========================================
Files 303 303
Lines 35407 35434 +27
==========================================
+ Hits 10476 13836 +3360
+ Misses 23765 19924 -3841
- Partials 1166 1674 +508 ☔ View full report in Codecov by Sentry. |
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 pretty cool, left some comments regarding the handling of the logic.
@@ -246,6 +246,8 @@ message IsAccountStatusValidResponse { | |||
optional uint64 allowed_record_count = 5; | |||
// The current status of the account. Default is valid. | |||
AccountStatus account_status = 6; | |||
// The timestamp of when the account |
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.
// The timestamp of when the account | |
// The timestamp of when the account was created. |
accountId, err := s.verifyUserInAccount(ctx, req.Msg.GetAccountId()) | ||
if err != nil { | ||
return nil, err | ||
} |
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 isn't really necessary here because the GetAccountStatus
handles it.
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
|
||
const TRIAL_DURATION_DAYS = 14; | ||
|
||
// TODO: created_at is coming back as blank |
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.
Is this still valid?
// TODO: created_at is coming back as blank | ||
|
||
function TrialCountdown(props: TrialCountdownProps) { | ||
const { createdDate, isAccountStatusValidResp } = props; |
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.
As a heads up, we don't currently store our created at with a timestamp, so it comes back on the server as the date in UTC (which is fine because we handle all of the billing in UTC).
but you'll want to make sure all of the date handling here is done in UTC format.
isAccountStatusValidResp: IsAccountStatusValidResponse | undefined; | ||
} | ||
|
||
const TRIAL_DURATION_DAYS = 14; |
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.
Ideally we could do most or all of this on the server so we don't have this copied in two places.
const now = Date.now(); | ||
const trialEndDate = new Date( | ||
createdDate + TRIAL_DURATION_DAYS * 24 * 60 * 60 * 1000 | ||
); | ||
const daysRemaining = Math.max( | ||
0, | ||
Math.ceil((trialEndDate.getTime() - now) / (1000 * 60 * 60 * 24)) | ||
); |
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.
Would be cleaner to use date-fns to do most of this for you so that it properly handles time changes like daylight savings, etc.
An example of something more functional:
import { differenceInDays, addDays, isAfter } from 'date-fns';
interface TrialStatus {
isTrialExpired: boolean;
isNearingEnd: boolean;
daysRemaining: number;
trialEndDate: Date;
}
export function checkTrialStatus(createdAt: Date): TrialStatus {
const TRIAL_DURATION_DAYS = 14;
const WARNING_THRESHOLD_DAYS = 3;
const currentDate = new Date();
const trialEndDate = addDays(createdAt, TRIAL_DURATION_DAYS);
const daysRemaining = differenceInDays(trialEndDate, currentDate);
return {
isTrialExpired: isAfter(currentDate, trialEndDate),
isNearingEnd: daysRemaining <= WARNING_THRESHOLD_DAYS && daysRemaining > 0,
daysRemaining: Math.max(0, daysRemaining),
trialEndDate
};
}
And then to use it:
const trialStatus = checkTrialStatus(createdAt);
if (trialStatus.isTrialExpired) {
console.log('Trial has expired');
} else if (trialStatus.isNearingEnd) {
console.log(`Trial ending soon! ${trialStatus.daysRemaining} days remaining`);
} else {
console.log(`${trialStatus.daysRemaining} days remaining 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.
I don't think this handles the last day (hours remaining) but you kind of get the point here.
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.
Improvements. Still some logical issues with the handling of isExpired vs isAlmostExpired.
Would also be great to see the date-fns used here instead of manually calculating the difference.
const daysRemaining = Math.max( | ||
0, | ||
Math.ceil((trialEndDate - now) / (1000 * 60 * 60 * 24)) | ||
); | ||
|
||
const isExpired = | ||
isAccountStatusValidResp?.accountStatus == | ||
AccountStatus.ACCOUNT_TRIAL_EXPIRED; | ||
const isAlmostExpired = daysRemaining <= 3; |
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.
I still think this would be clearer if the date-fns were used here as it's going to handle more edge cases.
const daysRemaining = differenceInDays(trailEndDate, now);
const isAlmostExpired = daysRemaining <= 3
|
||
interface TrialCountdownProps { | ||
trialEndDate: number; | ||
isAccountStatusValidResp: IsAccountStatusValidResponse | undefined; |
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.
might be worth simplifying these props to just pass in an isExpired
and do the calculation outside of this function. make it less smart.
AccountStatus.ACCOUNT_TRIAL_EXPIRED; | ||
const isAlmostExpired = daysRemaining <= 3; | ||
|
||
if (isExpired) |
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.
based on this logic it doesn't look like the isAlmostExpired
will ever actually be used.
If isExpired
is true, then isAlmostExpired
will always be true..
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.
whoops meant to clean that up, that was leftover code
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.
LGTM, minor thing regarding the math.max
data?.trialExpiresAt?.toDate() ?? Date.now() | ||
).getTime(); | ||
|
||
const daysRemaining = differenceInDays(Math.max(trialEndDate), Date.now()); |
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.
const daysRemaining = differenceInDays(Math.max(trialEndDate), Date.now()); | |
const daysRemaining = Math.max(0, differenceInDays(trialEndDate, Date.now())); |
This feature adds a free trial countdown to the top nav menu to give users an idea of how many days they have left in their trial.