feat: add new notification columns to User
#2263
Conversation
omBratteng
requested review from
rebelchris,
ilasw,
sshanzel,
nensidosari and
AmarTrebinjac
omBratteng
changed the title
User
| name = 'User1726691862710' | ||
|
|
||
| public async up(queryRunner: QueryRunner): Promise<void> { | ||
| await queryRunner.query(`ALTER TABLE "user" ADD "followingEmail" boolean NOT NULL DEFAULT true`); |
There was a problem hiding this comment.
Just thinking is there any case where we want to consider false now?
I do think when someone turned off all emails for instance or all push this should be false... Maybe something to consider in the migration (should be a easy query)
From FE it should work the same.
Turning the main email off should turn off all sub emails immediatly.
CC: @capJavert fyi
There was a problem hiding this comment.
I agree, we can do once of UPDATE query or script but we need to check with product because they usually don't like emails off 🙈
There was a problem hiding this comment.
@omBratteng can you test/prepare the query on your end and we can run it once before we go live? (product confirmed it should act like Chris mentioned)
There was a problem hiding this comment.
@capJavert do we only want to check for the Activity and Community updates, or also personalized digest?
Because when email notifications are "Off", all of them are off. But if you enable digest, it technically back on.
There was a problem hiding this comment.
I think the first one is fine for now
There was a problem hiding this comment.
For push notifications it's either on or it's not. So there we could just keep it on as default
There was a problem hiding this comment.
We should probably run it manually in batches. Once when this is merged, and once just as we go live to catch any changes that might've been done while UI wasn't in place
There was a problem hiding this comment.
👍 Batches yes, user table can block a lot of things.
There was a problem hiding this comment.
Anything else missing from the PR?
There was a problem hiding this comment.
Easier to run a query to turn it off for 120k users, then a query to enable it for the other 870k that has either enabled
|
Some more tests need to be updated I think. |
omBratteng
force-pushed
the
AS-584-new-notifications
branch
from
075d82d
to
76b7380
Compare
Extends user table to add two new columns,
followingEmailandfollowNotificationsAS-584