Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 88 additions & 0 deletions __tests__/notifications.ts
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,94 @@ describe('query notifications', () => {
new Date(notificationV2Fixture.createdAt).getTime(),
);
});

const CATEGORY_QUERY = `
query Notifications($category: String) {
notifications(category: $category) {
edges { node { type title createdAt } }
}
}`;

const seedTypedNotifications = async (types: NotificationType[]) => {
const notifs = await con.getRepository(NotificationV2).save(
types.map((type, index) => ({
...notificationV2Fixture,
uniqueKey: `cat-${index}`,
type,
})),
);
await con.getRepository(UserNotification).insert(
notifs.map((n) => ({
userId: '1',
notificationId: n.id,
createdAt: notificationV2Fixture.createdAt,
type: n.type,
})),
);
};

it('should filter notifications by category to its mapped types', async () => {
loggedUser = '1';
await seedTypedNotifications([
NotificationType.ArticleUpvoteMilestone,
NotificationType.CommentUpvoteMilestone,
NotificationType.UserFollow,
NotificationType.CommentMention,
]);

const res = await client.query(CATEGORY_QUERY, {
variables: { category: 'upvotes' },
});
const returned = res.data.notifications.edges
.map(({ node }) => node.type)
.sort();
expect(returned).toEqual(
[
NotificationType.ArticleUpvoteMilestone,
NotificationType.CommentUpvoteMilestone,
].sort(),
);
});

it('should treat updates as the complement of the explicit categories', async () => {
loggedUser = '1';
await seedTypedNotifications([
NotificationType.ArticleUpvoteMilestone, // upvotes -> excluded
NotificationType.UserFollow, // followers -> excluded
NotificationType.BriefingReady, // uncategorized -> updates
NotificationType.DigestReady, // uncategorized -> updates
]);

const res = await client.query(CATEGORY_QUERY, {
variables: { category: 'updates' },
});
const returned = res.data.notifications.edges
.map(({ node }) => node.type)
.sort();
expect(returned).toEqual(
[NotificationType.BriefingReady, NotificationType.DigestReady].sort(),
);
});

it('should return all notifications when category is omitted', async () => {
loggedUser = '1';
await seedTypedNotifications([
NotificationType.UserFollow,
NotificationType.CommentMention,
]);

const res = await client.query(CATEGORY_QUERY);
expect(res.data.notifications.edges).toHaveLength(2);
});

it('should reject an unknown category', () => {
loggedUser = '1';
return testQueryErrorCode(
client,
{ query: CATEGORY_QUERY, variables: { category: 'bogus' } },
'GRAPHQL_VALIDATION_FAILED',
);
});
});

const prepareNotificationPreferences = async ({
Expand Down
7 changes: 7 additions & 0 deletions src/entity/notifications/UserNotification.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Column, Entity, Index, ManyToOne, PrimaryColumn } from 'typeorm';
import type { User } from '../user';
import { NotificationV2 } from './NotificationV2';
import type { NotificationType } from '../../notifications/common';

@Entity()
@Index(
Expand Down Expand Up @@ -47,4 +48,10 @@ export class UserNotification {

@Column({ type: 'timestamp', nullable: true, default: null })
showAt: Date | null;

// Denormalized from NotificationV2.type so the notifications feed can filter
// by type using a (userId, type, date) index without scanning a heavy user's
// rows and discarding non-matching types post-join.
@Column({ type: 'text', nullable: true })
type?: NotificationType;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need index?

}
10 changes: 10 additions & 0 deletions src/graphorm/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { getPermissionsForMember } from '../schema/sources';
import {
getNotificationCategory,
NotificationType,
} from '../notifications/common';
import { GraphORM, GraphORMField, QueryBuilder } from './graphorm';
import {
Bookmark,
Expand Down Expand Up @@ -1603,6 +1607,12 @@ const obj = new GraphORM({
rawSelect: true,
select: () => `COALESCE(un."showAt", un."createdAt")`,
},
// Derived server-side from `type` so the client never re-implements the
// category map (the notifications page reads this for the type badge).
category: {
select: 'type',
transform: (value: NotificationType) => getNotificationCategory(value),
},
avatars: {
relation: {
isMany: true,
Expand Down
41 changes: 41 additions & 0 deletions src/migration/1782500000000-AddUserNotificationType.ts
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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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";
`);
}
}
35 changes: 35 additions & 0 deletions src/migration/notes/backfill-user-notification-type.sql
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 $$

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 $$;
88 changes: 88 additions & 0 deletions src/notifications/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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;
Expand Down
3 changes: 2 additions & 1 deletion src/notifications/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ export async function storeNotificationBundleV2(
.addSelect(':createdAt', 'createdAt')
.addSelect(':uniqueKey', 'uniqueKey')
.addSelect(':showAt', 'showAt')
.addSelect(':notificationType', 'type')
.from(User, 'u')
.where('u.id IN (:...userIds)', { userIds: userChunk })
.setParameters({
Expand Down Expand Up @@ -196,7 +197,7 @@ export async function storeNotificationBundleV2(
const [query, params] = selectQuery.getQueryAndParameters();

await entityManager.query(
`INSERT INTO "user_notification" ("userId", "notificationId", "createdAt", "uniqueKey", "showAt", "public")
`INSERT INTO "user_notification" ("userId", "notificationId", "createdAt", "uniqueKey", "showAt", "type", "public")
${query}
ON CONFLICT ("userId", "uniqueKey") WHERE "uniqueKey" IS NOT NULL DO NOTHING`,
params,
Expand Down
33 changes: 32 additions & 1 deletion src/schema/notifications.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
notificationPreferenceMap,
getUnreadNotificationsCount,
commentReplyNotificationTypes,
getNotificationCategoryFilter,
} from '../notifications/common';
import { ValidationError } from 'apollo-server-errors';
import { mapCloudinaryUrl } from '../common';
Expand Down Expand Up @@ -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!
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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,
Expand All @@ -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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to check categoryFilter.include is non empty, if it is it will break SQL syntax i think, same below

});
} 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(
Expand Down
Loading