New message email notifs #2162
New message email notifs #2162
Conversation
…o new-message-email-notifs
…o new-message-email-notifs
|
This is such a massive change, do you have a test plan? What should we be looking at? |
|
To be honest I don't know how to test any these changes besides doing it locally. We don't have a way right now to test workers, since the second we deploy a new worker it will start pulling off the queue. The key changes of this PR are in Athena - whenever a user's timeout is completed, I take each thread and do one last combover to remove deleted messages. This means that if a user deletes their message in the ~5min time period since they posted it and notifications go out, people won't get notified. Additionally, I've made it so that the emails handle image messages instead of just showing an imgix url in the email. And the last big change is the subject/preview text for the emails. So before it would say: Now that subject/preview is dynamic based on how many threads and unique people are in the notification. So for example: 1 user : 1 thread in the email: 1 user : N threads in email: N users : 1 thread: N users : X threads: Reviewing those now, maybe the preview text isn't totally perfect. The goal of all this is to have my dynamic email subjects/previews to draw people in. Using people's names, thread titles, and providing context about volume should all help here. And the last thing in the PR is visual updates to bring this email's style in line with our new thread and mention notification emails. Do you have any suggestions for how to further test this ? |
|
I meant locally haha, from all those tiny code changes I wasn't 100% sure what I was looking for! Will test this thoroughly locally once you make the subject lines perfect and this is ready to be shipped 💯 (I guess tomorrow) We should add automated e2e to our workers in the future. |
|
This is good to test locally now - I'll probably just tweak the copy on the preview text to something more compelling than a loose replication of the subject line. Thanks! |
…o new-message-email-notifs
…ctrum/spectrum into new-message-email-notifs
|
Okay updated the preview. Format will now look like: 1 user : 1 thread in the email: 1 user : N threads in email: N users : 1 thread: N users : X threads: SO now we have |
|
@mxstbr I'll leave this open if you were wanting to test tomorrow - but will ship it at the same time as the digest PR tomorrow |
|
@mxstbr did you manage to test this today? if not no worries, I can do another round of testing on my own - would like to ship all the email stuff today if possible |
|
Finally got around to testing everything tonight - afaik this is good to go. I will merge this and ship everything tomorrow. |
|
Kk gonna start working through this merge and deploy this morning! |
|
@mxstbr did you want to take a look at any more code here, or shall i admin merge? |
mxstbr
left a comment
mxstbr
left a comment
There was a problem hiding this comment.
It's kinda hard to review the code here since the changes are so all over the place, didn't see anything that stuck out to me as a bug.
If you tested this locally I say let's ship it!
|
Okay everything is deployed to the workers and api. i'll be keeping an eye on everything today. |
THE BATTLE HAS BEEN WON AGAINST ASYNCHRONOUS CODE
Deploy steps:
Closes #2138
Deployment notes
Requires deployments to iris + hermes
Run database migration?
YES/NO
Release notes
We improved the design of our notification emails for new replies in your conversations to make it easier to quickly preview and catch up on what people are saying while you're away.