-
Notifications
You must be signed in to change notification settings - Fork 119
feat: notification categories #3958
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| import type { MigrationInterface, QueryRunner } from 'typeorm'; | ||
|
|
||
| /** | ||
| * Denormalizes NotificationV2.type onto user_notification so the notifications | ||
| * feed can filter by type (`WHERE un."type" = ANY(...)`) using an index instead | ||
| * of scanning a heavy user's rows and discarding non-matching types post-join. | ||
| * | ||
| * The column is nullable with no default, so adding it is an instant metadata | ||
| * change (no table rewrite). New rows are populated by storeNotificationBundleV2; | ||
| * historical rows are backfilled separately in batches (see | ||
| * src/migration/notes/backfill-user-notification-type.sql) — a full UPDATE here | ||
| * would run inside the migration transaction and lock the largest table. | ||
| * | ||
| * The index is created non-CONCURRENTLY (migrations run in a transaction). In | ||
| * production build it live with CREATE INDEX CONCURRENTLY first so this becomes | ||
| * an IF NOT EXISTS no-op; smaller environments create it inline. | ||
| */ | ||
| export class AddUserNotificationType1782500000000 | ||
| implements MigrationInterface | ||
| { | ||
| name = 'AddUserNotificationType1782500000000'; | ||
|
|
||
| public async up(queryRunner: QueryRunner): Promise<void> { | ||
| await queryRunner.query(/* sql */ ` | ||
| ALTER TABLE "user_notification" ADD COLUMN IF NOT EXISTS "type" text; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need type here, would join not work? |
||
| `); | ||
| await queryRunner.query(/* sql */ ` | ||
| CREATE INDEX IF NOT EXISTS "IDX_user_notification_userId_type_date" | ||
| ON "user_notification" ("userId", "type", (COALESCE("showAt", "createdAt")) DESC); | ||
| `); | ||
| } | ||
|
|
||
| public async down(queryRunner: QueryRunner): Promise<void> { | ||
| await queryRunner.query(/* sql */ ` | ||
| DROP INDEX IF EXISTS "IDX_user_notification_userId_type_date"; | ||
| `); | ||
| await queryRunner.query(/* sql */ ` | ||
| ALTER TABLE "user_notification" DROP COLUMN IF EXISTS "type"; | ||
| `); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| -- Backfill user_notification.type from notification_v2 for rows created before | ||
| -- the AddUserNotificationType migration. Run OUTSIDE a migration transaction | ||
| -- (e.g. psql against the primary) so each batch commits independently and locks | ||
| -- stay short. Safe to re-run; it only touches rows whose type is still NULL. | ||
| -- | ||
| -- user_notification is one of the largest tables, so this is intentionally a | ||
| -- bounded loop rather than a single UPDATE. | ||
|
|
||
| DO $$ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure on the consequences, but i'd do the limit per user and not 10k globally |
||
| DECLARE | ||
| rows_updated integer; | ||
| BEGIN | ||
| LOOP | ||
| WITH batch AS ( | ||
| SELECT un."notificationId", un."userId" | ||
| FROM "user_notification" un | ||
| WHERE un."type" IS NULL | ||
| LIMIT 10000 | ||
| FOR UPDATE SKIP LOCKED | ||
| ) | ||
| UPDATE "user_notification" un | ||
| SET "type" = nv2."type" | ||
| FROM batch | ||
| JOIN "notification_v2" nv2 ON nv2."id" = batch."notificationId" | ||
| WHERE un."notificationId" = batch."notificationId" | ||
| AND un."userId" = batch."userId"; | ||
|
|
||
| GET DIAGNOSTICS rows_updated = ROW_COUNT; | ||
| RAISE NOTICE 'backfilled % rows', rows_updated; | ||
| EXIT WHEN rows_updated = 0; | ||
|
|
||
| COMMIT; | ||
| PERFORM pg_sleep(0.1); | ||
| END LOOP; | ||
| END $$; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -337,6 +337,94 @@ export const postNewCommentNotificationTypes = [ | |
| NotificationType.SquadNewComment, | ||
| ]; | ||
|
|
||
| // Human-friendly buckets the notifications feed filters by. The client sends a | ||
| // category and the API owns which notification types belong to it, so buckets | ||
| // can be retuned (or new types slotted in) without a frontend release. | ||
| export enum NotificationFilterCategory { | ||
| Upvotes = 'upvotes', | ||
| Mentions = 'mentions', | ||
| Comments = 'comments', | ||
| Followers = 'followers', | ||
| Squads = 'squads', | ||
| Updates = 'updates', | ||
| } | ||
|
|
||
| // Explicit buckets. `Updates` is intentionally absent: it's the complement of | ||
| // everything below, so a brand-new notification type lands in `Updates` (and | ||
| // in "All activity") automatically without touching this map. | ||
| const notificationCategoryToTypes: Partial< | ||
| Record<NotificationFilterCategory, NotificationType[]> | ||
| > = { | ||
| [NotificationFilterCategory.Upvotes]: [ | ||
| NotificationType.ArticleUpvoteMilestone, | ||
| NotificationType.CommentUpvoteMilestone, | ||
| ], | ||
|
Comment on lines
+355
to
+361
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't have this kind of mapping somewhere else, eg. per CIO topics for example? |
||
| [NotificationFilterCategory.Mentions]: [ | ||
| NotificationType.PostMention, | ||
| NotificationType.CommentMention, | ||
| ], | ||
| [NotificationFilterCategory.Comments]: [ | ||
| NotificationType.ArticleNewComment, | ||
| NotificationType.SquadNewComment, | ||
| NotificationType.CommentReply, | ||
| NotificationType.SquadReply, | ||
| ], | ||
| [NotificationFilterCategory.Followers]: [NotificationType.UserFollow], | ||
| [NotificationFilterCategory.Squads]: [ | ||
| NotificationType.SquadPostAdded, | ||
| NotificationType.SquadMemberJoined, | ||
| NotificationType.SquadBlocked, | ||
| NotificationType.PromotedToAdmin, | ||
| NotificationType.PromotedToModerator, | ||
| NotificationType.DemotedToMember, | ||
| NotificationType.SquadPublicApproved, | ||
| NotificationType.SquadFeatured, | ||
| NotificationType.SquadSubscribeToNotification, | ||
| NotificationType.SourcePostSubmitted, | ||
| NotificationType.SourcePostApproved, | ||
| NotificationType.SourcePostRejected, | ||
| NotificationType.ArticlePicked, | ||
| ], | ||
| }; | ||
|
|
||
| const categorizedNotificationTypes = Object.values( | ||
| notificationCategoryToTypes, | ||
| ).flat(); | ||
|
|
||
| // Resolves a category to a `un."type"` filter. Returns `include` for the | ||
| // explicit buckets and `exclude` for `Updates` (the complement), or null when | ||
| // the category is unknown so the caller can reject it. | ||
| export const getNotificationCategoryFilter = ( | ||
| category: string, | ||
| ): { include: NotificationType[] } | { exclude: NotificationType[] } | null => { | ||
| if (category === NotificationFilterCategory.Updates) { | ||
| return { exclude: categorizedNotificationTypes }; | ||
| } | ||
|
|
||
| const include = | ||
| notificationCategoryToTypes[category as NotificationFilterCategory]; | ||
| return include ? { include } : null; | ||
| }; | ||
|
|
||
| const notificationTypeToCategory = Object.entries( | ||
| notificationCategoryToTypes, | ||
| ).reduce( | ||
| (acc, [category, types]) => { | ||
| types.forEach((type) => { | ||
| acc[type] = category as NotificationFilterCategory; | ||
| }); | ||
| return acc; | ||
| }, | ||
| {} as Partial<Record<NotificationType, NotificationFilterCategory>>, | ||
| ); | ||
|
|
||
| // The bucket a single notification belongs to (inverse of the category map). | ||
| // Anything uncategorized falls into `Updates`, matching the filter complement. | ||
| export const getNotificationCategory = ( | ||
| type: NotificationType, | ||
| ): NotificationFilterCategory => | ||
| notificationTypeToCategory[type] ?? NotificationFilterCategory.Updates; | ||
|
|
||
| type NotificationPreferenceUnion = NotificationPreferenceComment & | ||
| NotificationPreferencePost & | ||
| NotificationPreferenceSource; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ import { | |
| notificationPreferenceMap, | ||
| getUnreadNotificationsCount, | ||
| commentReplyNotificationTypes, | ||
| getNotificationCategoryFilter, | ||
| } from '../notifications/common'; | ||
| import { ValidationError } from 'apollo-server-errors'; | ||
| import { mapCloudinaryUrl } from '../common'; | ||
|
|
@@ -110,6 +111,11 @@ export const typeDefs = /* GraphQL */ ` | |
| """ | ||
| type: String! | ||
| """ | ||
| Filter bucket the notification belongs to (upvotes, mentions, comments, | ||
| followers, squads, updates), derived from its type | ||
| """ | ||
| category: String! | ||
| """ | ||
| Referenced entity's id of the notification | ||
| """ | ||
| referenceId: String! | ||
|
|
@@ -267,6 +273,12 @@ export const typeDefs = /* GraphQL */ ` | |
| Paginate first | ||
| """ | ||
| first: Int | ||
|
|
||
| """ | ||
| Only return notifications in this category (upvotes, mentions, comments, | ||
| followers, squads, updates). Omit for all activity. | ||
| """ | ||
| category: String | ||
| ): NotificationConnection! @auth | ||
|
|
||
| notificationPreferences( | ||
|
|
@@ -359,11 +371,18 @@ export const resolvers: IResolvers<unknown, BaseContext> = { | |
| .getOne(), | ||
| notifications: async ( | ||
| source, | ||
| args: ConnectionArguments, | ||
| args: ConnectionArguments & { category?: string }, | ||
| ctx: AuthContext, | ||
| info, | ||
| ): Promise<ConnectionRelay<NotificationV2>> => { | ||
| const page = notificationsPageGenerator.connArgsToPage(args); | ||
| const { category } = args; | ||
| const categoryFilter = category | ||
| ? getNotificationCategoryFilter(category) | ||
| : null; | ||
| if (category && !categoryFilter) { | ||
| throw new ValidationError(`unknown notification category: ${category}`); | ||
| } | ||
| return graphorm.queryPaginated( | ||
| ctx, | ||
| info, | ||
|
|
@@ -379,6 +398,18 @@ export const resolvers: IResolvers<unknown, BaseContext> = { | |
| .andWhere(`(un."showAt" IS NULL OR un."showAt" <= NOW())`) | ||
| .addOrderBy(`COALESCE(un."showAt", un."createdAt")`, 'DESC'); | ||
|
|
||
| // Filter on the denormalized un."type" (not notification_v2) so the | ||
| // (userId, type, date) index can drive the query for heavy users. | ||
| if (categoryFilter && 'include' in categoryFilter) { | ||
| builder.queryBuilder.andWhere(`un."type" = ANY(:types)`, { | ||
| types: categoryFilter.include, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need to check |
||
| }); | ||
| } else if (categoryFilter) { | ||
| builder.queryBuilder.andWhere(`un."type" <> ALL(:types)`, { | ||
| types: categoryFilter.exclude, | ||
| }); | ||
| } | ||
|
|
||
| builder.queryBuilder.limit(page.limit); | ||
| if (page.timestamp) { | ||
| builder.queryBuilder = builder.queryBuilder.andWhere( | ||
|
|
||
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.
Do we need index?