From a78d7e4c78f60b72ab9a46c2e1890be2b7c46f0e Mon Sep 17 00:00:00 2001 From: Alexander Lee Date: Fri, 21 Oct 2022 12:06:45 +0800 Subject: [PATCH] Feat/notifications (#508) * Feat: add notification database model and update related models * Chore: add migrations Also adds id to site_members table for easier reference * Feat: add notificationService * Feat: add notificationUtils * Feat: add notifications router * Chore: initialise Notifications table and services * Fix: remove unused imports * Fix: change behaviour of quick retrieval Always returns only new notifications now, unless there are none, in which case it returns most recent 6 * Chore: remove unused imports * Refactor: findAll method * Chore: add notificationResponse type * Feat: add created_at sorting criteria * Fix: notification sorting order * Chore: add documentation for sort criteria * Fix: rebase errors * Fix: rebase errors for tests --- ...6081632-change-primary-key-site-members.js | 80 ++++++ .../20220926081632-create-notifications.js | 80 ++++++ src/database/models/Notification.ts | 88 ++++++ src/database/models/SiteMember.ts | 20 ++ src/database/models/index.ts | 1 + src/routes/v2/authenticatedSites/index.js | 8 + .../v2/authenticatedSites/notifications.ts | 94 +++++++ src/server.js | 11 +- src/services/identity/NotificationsService.ts | 260 ++++++++++++++++++ src/services/identity/index.ts | 14 +- src/tests/database.ts | 8 + src/utils/notification-utils.ts | 33 +++ 12 files changed, 695 insertions(+), 2 deletions(-) create mode 100644 src/database/migrations/20220926081632-change-primary-key-site-members.js create mode 100644 src/database/migrations/20220926081632-create-notifications.js create mode 100644 src/database/models/Notification.ts create mode 100644 src/routes/v2/authenticatedSites/notifications.ts create mode 100644 src/services/identity/NotificationsService.ts create mode 100644 src/utils/notification-utils.ts diff --git a/src/database/migrations/20220926081632-change-primary-key-site-members.js b/src/database/migrations/20220926081632-change-primary-key-site-members.js new file mode 100644 index 000000000..ebb4222a8 --- /dev/null +++ b/src/database/migrations/20220926081632-change-primary-key-site-members.js @@ -0,0 +1,80 @@ +module.exports = { + async up(queryInterface, Sequelize) { + await queryInterface.sequelize.transaction(async (transaction) => { + Promise.all([ + queryInterface.changeColumn("site_members", "user_id", { + allowNull: false, + primaryKey: false, + type: Sequelize.BIGINT, + references: { + model: "users", + key: "id", + }, + onUpdate: "CASCADE", + onDelete: "CASCADE", + transaction, + }), + queryInterface.changeColumn("site_members", "site_id", { + type: Sequelize.BIGINT, + allowNull: false, + primaryKey: false, + references: { + model: "sites", + key: "id", + }, + onUpdate: "CASCADE", + onDelete: "CASCADE", + transaction, + }), + queryInterface.addColumn( + "site_members", // name of Source model + "id", // name of column we're adding + { + unique: true, + allowNull: false, + autoIncrement: true, + primaryKey: true, + type: Sequelize.BIGINT, + transaction, + } + ), + ]) + }) + }, + + async down(queryInterface, Sequelize) { + await queryInterface.sequelize.transaction(async (transaction) => { + Promise.all([ + queryInterface.removeColumn( + "site_members", // name of Source Model + "id", // name of column we want to remove + { transaction } + ), + queryInterface.changeColumn("site_members", "user_id", { + allowNull: false, + primaryKey: true, + type: Sequelize.BIGINT, + references: { + model: "users", + key: "id", + }, + onUpdate: "CASCADE", + onDelete: "CASCADE", + transaction, + }), + queryInterface.changeColumn("site_members", "site_id", { + type: Sequelize.BIGINT, + allowNull: false, + primaryKey: true, + references: { + model: "sites", + key: "id", + }, + onUpdate: "CASCADE", + onDelete: "CASCADE", + transaction, + }), + ]) + }) + }, +} diff --git a/src/database/migrations/20220926081632-create-notifications.js b/src/database/migrations/20220926081632-create-notifications.js new file mode 100644 index 000000000..7ce3b876e --- /dev/null +++ b/src/database/migrations/20220926081632-create-notifications.js @@ -0,0 +1,80 @@ +module.exports = { + up: async (queryInterface, Sequelize) => { + await queryInterface.createTable("notifications", { + id: { + allowNull: false, + autoIncrement: true, + primaryKey: true, + type: Sequelize.BIGINT, + }, + site_member_id: { + allowNull: false, + type: Sequelize.BIGINT, + references: { + model: "site_members", + key: "id", + }, + onUpdate: "CASCADE", + onDelete: "CASCADE", + }, + site_id: { + allowNull: false, + type: Sequelize.BIGINT, + references: { + model: "sites", + key: "id", + }, + onUpdate: "CASCADE", + onDelete: "CASCADE", + }, + user_id: { + allowNull: false, + type: Sequelize.BIGINT, + references: { + model: "users", + key: "id", + }, + onUpdate: "CASCADE", + onDelete: "CASCADE", + }, + message: { + allowNull: true, + type: Sequelize.STRING, + }, + link: { + allowNull: true, + type: Sequelize.STRING, + }, + source_username: { + allowNull: false, + type: Sequelize.STRING, + }, + type: { + allowNull: false, + type: Sequelize.STRING, + }, + first_read_time: { + allowNull: true, + type: Sequelize.DATE, + }, + priority: { + allowNull: false, + type: Sequelize.BIGINT, + }, + created_at: { + type: Sequelize.DATE, + allowNull: false, + defaultValue: Sequelize.fn("NOW"), + }, + updated_at: { + type: Sequelize.DATE, + allowNull: false, + defaultValue: Sequelize.fn("NOW"), + }, + }) + }, + + down: async (queryInterface) => { + await queryInterface.dropTable("notifications") + }, +} diff --git a/src/database/models/Notification.ts b/src/database/models/Notification.ts new file mode 100644 index 000000000..500ad404d --- /dev/null +++ b/src/database/models/Notification.ts @@ -0,0 +1,88 @@ +import { + DataType, + Column, + Model, + Table, + CreatedAt, + UpdatedAt, + DeletedAt, + BelongsToMany, + HasOne, + BelongsTo, + ForeignKey, +} from "sequelize-typescript" + +import { Site } from "@database/models/Site" +import { SiteMember } from "@database/models/SiteMember" +import { User } from "@database/models/User" + +@Table({ tableName: "notifications" }) +export class Notification extends Model { + @Column({ + autoIncrement: true, + primaryKey: true, + allowNull: false, + type: DataType.BIGINT, + }) + id!: number + + @ForeignKey(() => SiteMember) + siteMemberId!: number + + @BelongsTo(() => SiteMember) + siteMember!: SiteMember + + @ForeignKey(() => Site) + siteId!: number + + @BelongsTo(() => Site) + site!: Site + + @ForeignKey(() => User) + userId!: number + + @BelongsTo(() => User) + user!: Site + + @Column({ + allowNull: true, + type: DataType.TEXT, + }) + message!: string + + @Column({ + allowNull: true, + type: DataType.TEXT, + }) + link!: string + + @Column({ + allowNull: true, + type: DataType.TEXT, + }) + sourceUsername!: string + + @Column({ + allowNull: false, + type: DataType.TEXT, + }) + type!: string + + @Column({ + allowNull: true, + type: DataType.DATE, + }) + firstReadTime!: Date | null + + @Column({ + allowNull: false, + type: DataType.BIGINT, + }) + priority!: number + + @CreatedAt + createdAt!: Date + + @UpdatedAt + updatedAt!: Date +} diff --git a/src/database/models/SiteMember.ts b/src/database/models/SiteMember.ts index d5c466691..0f54bee02 100644 --- a/src/database/models/SiteMember.ts +++ b/src/database/models/SiteMember.ts @@ -1,8 +1,10 @@ import { + BelongsTo, Column, CreatedAt, DataType, ForeignKey, + HasMany, Model, Table, UpdatedAt, @@ -10,11 +12,20 @@ import { import { CollaboratorRoles } from "@constants/index" +import { Notification } from "@database/models/Notification" import { Site } from "@database/models/Site" import { User } from "@database/models/User" @Table({ tableName: "site_members" }) export class SiteMember extends Model { + @Column({ + autoIncrement: true, + primaryKey: true, + allowNull: false, + type: DataType.BIGINT, + }) + id!: number + @ForeignKey(() => User) @Column userId!: number @@ -34,4 +45,13 @@ export class SiteMember extends Model { @UpdatedAt updatedAt!: Date + + @BelongsTo(() => Site) + site!: Site + + @BelongsTo(() => User) + user!: User + + @HasMany(() => Notification) + notifications?: Notification[] } diff --git a/src/database/models/index.ts b/src/database/models/index.ts index a8bacbc35..1d6ceaedb 100644 --- a/src/database/models/index.ts +++ b/src/database/models/index.ts @@ -9,3 +9,4 @@ export * from "@database/models/IsomerAdmin" export * from "@database/models/ReviewMeta" export * from "@database/models/ReviewRequest" export * from "@database/models/Reviewers" +export * from "@database/models/Notification" diff --git a/src/routes/v2/authenticatedSites/index.js b/src/routes/v2/authenticatedSites/index.js index c97d7dfe6..43f9c8200 100644 --- a/src/routes/v2/authenticatedSites/index.js +++ b/src/routes/v2/authenticatedSites/index.js @@ -1,5 +1,7 @@ import { attachSiteHandler } from "@root/middleware" +import { NotificationsRouter } from "./notifications" + const express = require("express") const { @@ -88,6 +90,7 @@ const getAuthenticatedSitesSubrouter = ({ authorizationMiddleware, gitHubService, configYmlService, + notificationsService, }) => { const collectionYmlService = new CollectionYmlService({ gitHubService }) const homepagePageService = new HomepagePageService({ gitHubService }) @@ -184,6 +187,7 @@ const getAuthenticatedSitesSubrouter = ({ const navigationV2Router = new NavigationRouter({ navigationYmlService: navYmlService, }) + const notificationsRouter = new NotificationsRouter({ notificationsService }) const authenticatedSitesSubrouter = express.Router({ mergeParams: true }) @@ -221,6 +225,10 @@ const getAuthenticatedSitesSubrouter = ({ authenticatedSitesSubrouter.use("/contactUs", contactUsV2Router.getRouter()) authenticatedSitesSubrouter.use("/homepage", homepageV2Router.getRouter()) authenticatedSitesSubrouter.use("/settings", settingsV2Router.getRouter()) + authenticatedSitesSubrouter.use( + "/notifications", + notificationsRouter.getRouter() + ) return authenticatedSitesSubrouter } diff --git a/src/routes/v2/authenticatedSites/notifications.ts b/src/routes/v2/authenticatedSites/notifications.ts new file mode 100644 index 000000000..6f20772eb --- /dev/null +++ b/src/routes/v2/authenticatedSites/notifications.ts @@ -0,0 +1,94 @@ +import autoBind from "auto-bind" +import express from "express" + +import { + attachReadRouteHandlerWrapper, + attachWriteRouteHandlerWrapper, +} from "@middleware/routeHandler" + +import UserWithSiteSessionData from "@root/classes/UserWithSiteSessionData" +import { RequestHandler } from "@root/types" +import NotificationsService, { + NotificationResponse, +} from "@services/identity/NotificationsService" + +interface NotificationsRouterProps { + notificationsService: NotificationsService +} + +// eslint-disable-next-line import/prefer-default-export +export class NotificationsRouter { + private readonly notificationsService + + constructor({ notificationsService }: NotificationsRouterProps) { + this.notificationsService = notificationsService + autoBind(this) + } + + getRecentNotifications: RequestHandler< + never, + NotificationResponse[], + unknown, + unknown, + { userWithSiteSessionData: UserWithSiteSessionData } + > = async (req, res) => { + const { userWithSiteSessionData } = res.locals + const { siteName, isomerUserId: userId } = userWithSiteSessionData + + const notifications = await this.notificationsService.listRecent({ + siteName, + userId, + }) + return res.status(200).json(notifications) + } + + getAllNotifications: RequestHandler< + never, + NotificationResponse[], + unknown, + unknown, + { userWithSiteSessionData: UserWithSiteSessionData } + > = async (req, res) => { + const { userWithSiteSessionData } = res.locals + const { siteName, isomerUserId: userId } = userWithSiteSessionData + + const notifications = await this.notificationsService.listAll({ + siteName, + userId, + }) + return res.status(200).json(notifications) + } + + markNotificationsAsRead: RequestHandler< + never, + unknown, + unknown, + unknown, + { userWithSiteSessionData: UserWithSiteSessionData } + > = async (req, res) => { + const { userWithSiteSessionData } = res.locals + const { siteName, isomerUserId: userId } = userWithSiteSessionData + + await this.notificationsService.markNotificationsAsRead({ + siteName, + userId, + }) + return res.status(200).send("OK") + } + + getRouter() { + const router = express.Router({ mergeParams: true }) + + router.get("/", attachReadRouteHandlerWrapper(this.getRecentNotifications)) + router.get( + "/allNotifications", + attachReadRouteHandlerWrapper(this.getAllNotifications) + ) + router.post( + "/", + attachWriteRouteHandlerWrapper(this.markNotificationsAsRead) + ) + + return router + } +} diff --git a/src/server.js b/src/server.js index bede11a80..a682e6764 100644 --- a/src/server.js +++ b/src/server.js @@ -13,6 +13,10 @@ import { Repo, Deployment, IsomerAdmin, + Notification, + ReviewRequest, + ReviewMeta, + Reviewer, } from "@database/models" import bootstrap from "@root/bootstrap" import { @@ -24,13 +28,13 @@ import { getIdentityAuthService, getUsersService, isomerAdminsService, + notificationsService, } from "@services/identity" import DeploymentsService from "@services/identity/DeploymentsService" import ReposService from "@services/identity/ReposService" import SitesService from "@services/identity/SitesService" import InfraService from "@services/infra/InfraService" -import { AuthorizationMiddleware } from "./middleware/authorization" import getAuthenticatedSubrouterV1 from "./routes/v1/authenticated" import getAuthenticatedSitesSubrouterV1 from "./routes/v1/authenticatedSites" import getAuthenticatedSubrouter from "./routes/v2/authenticated" @@ -48,6 +52,10 @@ const sequelize = initSequelize([ Repo, Deployment, IsomerAdmin, + Notification, + ReviewRequest, + ReviewMeta, + Reviewer, ]) const usersService = getUsersService(sequelize) @@ -131,6 +139,7 @@ const authenticatedSitesSubrouterV2 = getAuthenticatedSitesSubrouter({ authenticationMiddleware, gitHubService, configYmlService, + notificationsService, }) const authV2Router = new AuthRouter({ authenticationMiddleware, authService }) const formsgRouter = new FormsgRouter({ usersService, infraService }) diff --git a/src/services/identity/NotificationsService.ts b/src/services/identity/NotificationsService.ts new file mode 100644 index 000000000..149c4cebb --- /dev/null +++ b/src/services/identity/NotificationsService.ts @@ -0,0 +1,260 @@ +import { FindOptions, ModelStatic, Op, Sequelize } from "sequelize" + +import { Notification, Site, Repo, SiteMember } from "@database/models" +import { + NotificationType, + getNotificationExpiryDate, + getNotificationMessage, + getNotificationPriority, +} from "@root/utils/notification-utils" + +const NUM_RECENT_NOTIFICATIONS = 6 + +interface NotificationsServiceProps { + repository: ModelStatic + siteMember: ModelStatic +} + +export interface NotificationResponse { + message: string + createdAt: Date + link: string + isRead: boolean + sourceUsername: string + type: string +} + +class NotificationsService { + // NOTE: Explicitly specifying using keyed properties to ensure + // that the types are synced. + private readonly repository: NotificationsServiceProps["repository"] + + private readonly siteMember: NotificationsServiceProps["siteMember"] + + constructor({ repository, siteMember }: NotificationsServiceProps) { + this.repository = repository + this.siteMember = siteMember + } + + formatNotifications(notifications: Notification[]) { + return notifications.map((notification) => ({ + message: notification.message, + createdAt: notification.createdAt, + link: notification.link, + isRead: !!notification.firstReadTime, + sourceUsername: notification.sourceUsername, + type: notification.type, + })) + } + + async findAll({ + siteName, + userId, + findOptions, + }: { + siteName: string + userId: string + findOptions?: FindOptions + }) { + // We want separate sorting for unread notifications and read notifications - for unread, high priority notifications should go first + // while for read, newer notifications should be displayed first, regardless of priority + // The second sort criteria only affects unread notifications and is used to allow high priority notifications to be sorted first (priority > created_at) + // Read notifications are unaffected by the second sort criteria and will continue to be sorted in the remaining order (first_read_time > created_at > priority) + return this.repository.findAll({ + where: { + user_id: userId, + }, + order: [ + ["first_read_time", "DESC NULLS FIRST"], + [ + Sequelize.literal( + "CASE WHEN first_read_time IS NULL THEN priority ELSE 999 END" + ), + "ASC", + ], + ["created_at", "DESC"], + ["priority", "ASC"], // Low numbers indicate a higher priority + ], + include: [ + { + model: Site, + as: "site", + required: true, + include: [ + { + model: Repo, + required: true, + where: { + name: siteName, + }, + }, + ], + }, + ], + ...findOptions, + }) + } + + async listRecent({ siteName, userId }: { siteName: string; userId: string }) { + const newNotifications = await this.findAll({ + siteName, + userId, + findOptions: { + where: { + userId, + firstReadTime: { + [Op.eq]: null, + }, + }, + }, + }) + + if (newNotifications.length > 0) + return this.formatNotifications(newNotifications) + + const mostRecentNotifications = await this.findAll({ + siteName, + userId, + findOptions: { + limit: NUM_RECENT_NOTIFICATIONS, + }, + }) + + return this.formatNotifications(mostRecentNotifications) + } + + async listAll({ siteName, userId }: { siteName: string; userId: string }) { + const notifications = await this.findAll({ + siteName, + userId, + }) + return this.formatNotifications(notifications) + } + + async markNotificationsAsRead({ + siteName, + userId, + }: { + siteName: string + userId: string + }) { + const siteMember = await this.siteMember.findOne({ + where: { user_id: userId }, + include: [ + { + model: Site, + required: true, + include: [ + { + model: Repo, + required: true, + where: { + name: siteName, + }, + }, + ], + }, + ], + }) + const readAtDate = new Date() + await this.repository.update( + { + firstReadTime: readAtDate, + }, + { + where: { + site_member_id: siteMember?.id, + first_read_time: null, + }, + } + ) + } + + async create({ + siteName, + userId, + link, + notificationType, + notificationSourceUsername, + }: { + siteName: string + userId: string + link: string + notificationType: NotificationType + notificationSourceUsername: string + }) { + const siteMember = await this.siteMember.findOne({ + where: { user_id: userId }, + include: [ + { + model: Site, + required: true, + include: [ + { + model: Repo, + required: true, + where: { + name: siteName, + }, + }, + ], + }, + ], + }) + + // Look for a recent notification to decide whether to create a new notification or update the old one + const recentTargetNotification = await this.repository.findOne({ + where: { + user_id: userId, + type: notificationType, + created_at: { + [Op.gte]: getNotificationExpiryDate(notificationType), + }, + link, + source_username: notificationSourceUsername, + }, + include: [ + { + model: Site, + as: "site", + required: true, + include: [ + { + model: Repo, + required: true, + where: { + name: siteName, + }, + }, + ], + }, + ], + }) + + if (recentTargetNotification) { + // Update existing notification + await recentTargetNotification.update({ + firstReadTime: null, + createdAt: new Date(), + }) + } else { + // Create new notification + await this.repository.create({ + siteMemberId: siteMember?.id, + siteId: siteMember?.siteId, + userId, + message: getNotificationMessage( + notificationType, + notificationSourceUsername + ), // helper method here + link, + sourceUsername: notificationSourceUsername, + type: notificationType, + firstReadTime: null, + priority: getNotificationPriority(notificationType), // get priority + }) + } + } +} + +export default NotificationsService diff --git a/src/services/identity/index.ts b/src/services/identity/index.ts index 332d14cbf..91791e8d2 100644 --- a/src/services/identity/index.ts +++ b/src/services/identity/index.ts @@ -2,7 +2,13 @@ import { Sequelize } from "sequelize-typescript" import logger from "@logger/logger" -import { User, Whitelist, IsomerAdmin } from "@database/models" +import { + User, + Whitelist, + IsomerAdmin, + Notification, + SiteMember, +} from "@database/models" import { GitHubService } from "@services/db/GitHubService" import SmsClient from "@services/identity/SmsClient" import TotpGenerator from "@services/identity/TotpGenerator" @@ -10,6 +16,7 @@ import { mailer } from "@services/utilServices/MailClient" import AuthService from "./AuthService" import IsomerAdminsService from "./IsomerAdminsService" +import NotificationsService from "./NotificationsService" import UsersService from "./UsersService" const { OTP_EXPIRY, OTP_SECRET, NODE_ENV } = process.env @@ -54,3 +61,8 @@ export const getIdentityAuthService = (gitHubService: GitHubService) => export const isomerAdminsService = new IsomerAdminsService({ repository: IsomerAdmin, }) + +export const notificationsService = new NotificationsService({ + repository: Notification, + siteMember: SiteMember, +}) diff --git a/src/tests/database.ts b/src/tests/database.ts index 56a00bf05..6628fd24f 100644 --- a/src/tests/database.ts +++ b/src/tests/database.ts @@ -10,6 +10,10 @@ import { Repo, Deployment, IsomerAdmin, + Notification, + ReviewRequest, + ReviewMeta, + Reviewer, } from "@database/models" const sequelize = new Sequelize({ @@ -25,6 +29,10 @@ sequelize.addModels([ Repo, Deployment, IsomerAdmin, + Notification, + ReviewRequest, + ReviewMeta, + Reviewer, ]) // eslint-disable-next-line import/prefer-default-export diff --git a/src/utils/notification-utils.ts b/src/utils/notification-utils.ts new file mode 100644 index 000000000..9b0e0158c --- /dev/null +++ b/src/utils/notification-utils.ts @@ -0,0 +1,33 @@ +import moment from "moment" + +export type NotificationType = "sent_request" | "updated_request" + +export const getNotificationExpiryDate = ( + notificationType: NotificationType +) => { + switch (notificationType) { + default: + return moment().subtract(3, "months") + } +} + +export const getNotificationMessage = ( + notificationType: NotificationType, + sourceUsername: string +) => { + switch (notificationType) { + case "sent_request": + return `${sourceUsername} created a review request.` + case "updated_request": + return `${sourceUsername} made changes to a review request.` + default: + return "Default notification" + } +} + +export const getNotificationPriority = (notificationType: NotificationType) => { + switch (notificationType) { + default: + return 2 + } +}