-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Optimised getLastSeenEventTimestamp
function
#20134
base: main
Are you sure you want to change the base?
Conversation
ref ENG-832 - Added migrations for latest_event_timestamp column in emails table. - updated schema - updated emails model
ref ENG-832 - added a new column in the email table called `latest_event_timestamp`. - everytime `handleDelivered`, `handleOpened` and `handlePermanentFailed` fires, it will also update the `latest_event_timestmapi` in the email table. - we then conditionally remove 3 email queries in favour of 1 query that will get the latest event timestamp
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
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 👍
|
||
if (maxTimestamp) { | ||
if (!(maxTimestamp instanceof Date)) { | ||
maxTimestamp = new Date(maxTimestamp); |
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.
Maybe worth explaining why we need to convert to a Date here?
// SQLite returns a string instead of a Date
if (maxFailedAt && !(maxFailedAt instanceof Date)) { | ||
// SQLite returns a string instead of a Date | ||
maxFailedAt = new Date(maxFailedAt); | ||
} |
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'm guessing we're keeping the old logic to smoothen the rollout, as existing emails won't have the latest_event_timestamp
field. Shall we make a note to clean this up in a couple of weeks from 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.
You could also return early here, to separate the new logic from the old one, e.g.
if (maxTimestamp) {
if (!(maxTimestamp instanceof Date)) {
maxTimestamp = new Date(maxTimestamp);
}
return maxTimestamp;
}
// Legacy logic
...
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.
Yes correct, I'll add a note in the comments.
ref ENG-832
depends on #20118 for migration column in emails table, will rebase accordingly.
handleDelivered
,handleOpened
andhandlePermanentFailed
fires, it will also update thelatest_event_timestmap
in the email table.