fix: streak issue - fetching last recover date with timezone #2452
Conversation
sshanzel
requested review from
rebelchris,
omBratteng,
ilasw,
nensidosari and
AmarTrebinjac
This reverts commit 0c418cb.
This comment has been minimized.
This comment has been minimized.
This reverts commit 2944229.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| .select(`us.*`) | ||
| .addSelect( | ||
| `date_trunc('day', us."lastViewAt" at time zone COALESCE(u.timezone, 'utc'))::date`, | ||
| 'lastViewAtTz', | ||
| ) | ||
| .addSelect('u.timezone', 'timezone') |
There was a problem hiding this comment.
Just broke the 3 columns into their separate select statements to make the columns fetched more obvious.
There was a problem hiding this comment.
the date_trunc and other things could become a function so we can reuse, since the same thing is used in some other parts outside of cron. Not needed for this PR though.
There was a problem hiding this comment.
Good point. Do you think of it as a javascript function that concatenates the query or something like a postgresql internal global function we would define?
There was a problem hiding this comment.
| .addSelect('us.currentStreak', 'current') | ||
| .addSelect('u."weekStart"', 'weekStart') | ||
| .addSelect( | ||
| `(date_trunc('day', usa."lastRecoverAt" at time zone COALESCE(u.timezone, 'utc'))::date) - interval '1 day'`, | ||
| `(date_trunc('day', usa."lastRecoverAt"::timestamptz at time zone COALESCE(u.timezone, 'utc'))::date) - interval '1 day'`, |
There was a problem hiding this comment.
Didn't we discuss not doing the timestamptz columns for lots of reasons?
Not sure if we ended up not doing it, but they such a pain...
There was a problem hiding this comment.
Yes, this column is not timestamptz. The problem occurs when you apply the time zone to a column similar to this (just timestamp).
Using at time zone in a non-timezone column messes up the values rather than adjusting to the right timezone. For us to use the at time zone statement, the column must be converted to timestamptz first (on query level only) even when the column is just a pure timestamp. Some nuances working with timezone in postgresql.
There was a problem hiding this comment.
Potentially we can extract date_trunc(...) and everything else in a function maybe so the query is a bit more readable. Does not need to happen in this PR just a note it is starting to be really hard to follow 😆
There was a problem hiding this comment.
Also, we are using AT TIME ZONE in digest and it correctly transforms there from what I see. But there we convert from NOW() so not sure if that is the difference.
There was a problem hiding this comment.
I guess because NOW() includes the timezone offset
There was a problem hiding this comment.
Yes, using now works as intended since it includes the timezone information. Basically a timestamptz type is the value returned from the function.
There was a problem hiding this comment.
Potentially we can extract date_trunc(...) and everything else in a function maybe so the query is a bit more readable. Does not need to happen in this PR just a note it is starting to be really hard to follow 😆
Totally agree, especially have it in many places. Ticker created here: #2452 (comment)
When working with |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🍹 The Update (preview) for dailydotdev/api/prod was successful. Resource Changes Name Type Operation
- vpc-native-api-migration-587473ef kubernetes:batch/v1:Job delete
~ vpc-native-clean-zombie-user-companies-cron kubernetes:batch/v1:CronJob update
~ vpc-native-hourly-notification-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-source-public-threshold-cron kubernetes:batch/v1:CronJob update
~ vpc-native-calculate-top-readers-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-views-cron kubernetes:batch/v1:CronJob update
~ vpc-native-ws-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-update-trending-cron kubernetes:batch/v1:CronJob update
+ vpc-native-api-migration-d9847687 kubernetes:batch/v1:Job create
~ vpc-native-update-highlighted-views-cron kubernetes:batch/v1:CronJob update
~ vpc-native-personalized-digest-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-bg-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-personalized-digest-cron kubernetes:batch/v1:CronJob update
~ vpc-native-sync-subscription-with-cio-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-tags-str-cron kubernetes:batch/v1:CronJob update
~ vpc-native-private-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-clean-zombie-images-cron kubernetes:batch/v1:CronJob update
~ vpc-native-generate-search-invites-cron kubernetes:batch/v1:CronJob update
~ vpc-native-check-analytics-report-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-current-streak-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-tag-recommendations-cron kubernetes:batch/v1:CronJob update
~ vpc-native-daily-digest-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-source-tag-view-cron kubernetes:batch/v1:CronJob update
~ vpc-native-temporal-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-clean-zombie-users-cron kubernetes:batch/v1:CronJob update
~ vpc-native-generic-referral-reminder-cron kubernetes:batch/v1:CronJob update
|
This is a 2-part PR series about the issues surrounding the streak reset.
In this PR, this fixes an issue where the user has just restored their streak, but suddenly it was broken again. Referring to the issue mentioned here: dailydotdev/daily#1538 (comment)
The screenshot below tells the user did not read for two days straight, however, on the next screenshot, it can be seen that the user restored the streak that should allow him to continue the streak even when 2 days had passed.
For context, the user lost his streak and restored it as seen below. At 12:03 the email notification about the streak being reset was sent, and immediately after, the restoration happened.
We have a cron job that runs on an internal, and it takes consideration of the current streak and restores performed by the user. However, there was an issue with how we pulled the data about the date of recovering the streak.
In postgresql, setting a timezone on a
timestampcolumn (without timezone information column), would either push or pull the time depending on the timezone value. In order to set correctly the timezone, we must convert the value itself to atimestamptz, and then set the correct timezone preferred by the user.This explains the reset that happened, the query to pull all users that should get reset was inaccurate by converting inaccurate value from the recovery date.
Note: to show that it actually fixes the issue, I've introduced a test with the user's configuration. I will revert the fix temporarily to see if the test would fail.