From 5b0b8199d77bd781087d0bebc9fa25490d8a65e7 Mon Sep 17 00:00:00 2001 From: Alexander Lee Date: Tue, 17 Jan 2023 17:29:25 +0800 Subject: [PATCH] Feat/generic notification creation (#523) * Feat: move notifications router to authenticated subrouter * Feat: modify create notification arguments * Feat: add notification middleware to handle edit notifications * Fix: allow next on routeHandler * Feat: add creatnotification middleware to authenticatesSitesRouter * Chore: add notification types * Feat: add notification changes when reviews are modified * Fix: swap order of subrouters * Fix: review request router dependencies * Fix: time period for updating notification * Fix: updating createdAt * Fix: tests * Chore: swap to promise.all * fix: rebase errors * Fix: add jsdoc * Feat: add link to notificationHandler * Fix: swap to promise.all and add links * fix(review): fixed triggering event for request approved notif * chore(notifications.spec): added correct ports * Fix: update message on update notification * Fix: notification integration tests * Feat: add generic notification handler tests (#572) * Feat: add generic notification handler tests * Fix: router initialisation and cleanup * fix: update github actions to use runInBand * Fix: update tests * Fix: reset database tables before integration tests * Chore: modify imports * Nit: test comment spelling Co-authored-by: Kishore <42832651+kishore03109@users.noreply.github.com> * Nit: comments and change var names Co-authored-by: Kishore <42832651+kishore03109@users.noreply.github.com> Co-authored-by: seaerchin Co-authored-by: Kishore <42832651+kishore03109@users.noreply.github.com> --- .github/workflows/ci.yml | 2 +- .../NotificationOnEditHandler.spec.ts | 239 ++++++++++++++++++ src/integration/Notifications.spec.ts | 118 +++++++-- src/integration/Reviews.spec.ts | 11 +- src/middleware/notificationOnEditHandler.ts | 75 ++++++ src/middleware/routeHandler.js | 4 +- .../__tests__/Notifications.spec.ts | 8 +- .../v2/authenticated/__tests__/review.spec.ts | 50 +++- src/routes/v2/authenticated/index.js | 11 + .../notifications.ts | 13 +- src/routes/v2/authenticated/review.ts | 55 +++- .../v2/authenticatedSites/collectionPages.js | 16 +- .../v2/authenticatedSites/collections.js | 25 +- src/routes/v2/authenticatedSites/contactUs.js | 5 +- src/routes/v2/authenticatedSites/homepage.js | 5 +- src/routes/v2/authenticatedSites/index.js | 9 +- .../v2/authenticatedSites/mediaCategories.js | 20 +- .../v2/authenticatedSites/mediaFiles.js | 15 +- .../v2/authenticatedSites/navigation.js | 5 +- .../authenticatedSites/resourceCategories.js | 20 +- .../v2/authenticatedSites/resourcePages.js | 15 +- .../v2/authenticatedSites/resourceRoom.js | 10 +- src/routes/v2/authenticatedSites/settings.js | 5 +- .../v2/authenticatedSites/unlinkedPages.js | 20 +- src/server.js | 14 +- src/services/identity/NotificationsService.ts | 59 ++--- .../__tests__/NotificationsService.spec.ts | 13 +- src/utils/notification-utils.ts | 20 +- 28 files changed, 702 insertions(+), 160 deletions(-) create mode 100644 src/integration/NotificationOnEditHandler.spec.ts create mode 100644 src/middleware/notificationOnEditHandler.ts rename src/routes/v2/{authenticatedSites => authenticated}/__tests__/Notifications.spec.ts (91%) rename src/routes/v2/{authenticatedSites => authenticated}/notifications.ts (83%) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index af3359d29..f858b4531 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -71,7 +71,7 @@ jobs: key: ${{ runner.OS }}-node-${{ hashFiles('**/package-lock.json') }} - run: npm ci - run: npm run dev:services - - run: . .env.test && npx jest + - run: . .env.test && npx jest --runInBand - run: docker compose down gatekeep: diff --git a/src/integration/NotificationOnEditHandler.spec.ts b/src/integration/NotificationOnEditHandler.spec.ts new file mode 100644 index 000000000..24d208925 --- /dev/null +++ b/src/integration/NotificationOnEditHandler.spec.ts @@ -0,0 +1,239 @@ +import express from "express" +import request from "supertest" + +import { NotificationOnEditHandler } from "@middleware/notificationOnEditHandler" + +import UserSessionData from "@classes/UserSessionData" + +import { + Notification, + Repo, + Reviewer, + ReviewMeta, + ReviewRequest, + ReviewRequestView, + Site, + SiteMember, + User, + Whitelist, +} from "@database/models" +import { generateRouterForUserWithSite } from "@fixtures/app" +import { + mockEmail, + mockIsomerUserId, + mockSiteName, +} from "@fixtures/sessionData" +import { GitHubService } from "@services/db/GitHubService" +import * as ReviewApi from "@services/db/review" +import { ConfigYmlService } from "@services/fileServices/YmlFileServices/ConfigYmlService" +import { getUsersService, notificationsService } from "@services/identity" +import CollaboratorsService from "@services/identity/CollaboratorsService" +import IsomerAdminsService from "@services/identity/IsomerAdminsService" +import SitesService from "@services/identity/SitesService" +import ReviewRequestService from "@services/review/ReviewRequestService" +import { sequelize } from "@tests/database" + +const mockSiteId = "1" +const mockSiteMemberId = "1" + +const mockGithubService = { + getPullRequest: jest.fn(), + getComments: jest.fn(), +} +const usersService = getUsersService(sequelize) +const reviewRequestService = new ReviewRequestService( + (mockGithubService as unknown) as typeof ReviewApi, + User, + ReviewRequest, + Reviewer, + ReviewMeta, + ReviewRequestView +) +const sitesService = new SitesService({ + siteRepository: Site, + gitHubService: (mockGithubService as unknown) as GitHubService, + configYmlService: (jest.fn() as unknown) as ConfigYmlService, + usersService, + isomerAdminsService: (jest.fn() as unknown) as IsomerAdminsService, + reviewRequestService, +}) +const collaboratorsService = new CollaboratorsService({ + siteRepository: Site, + siteMemberRepository: SiteMember, + sitesService, + usersService, + whitelist: Whitelist, +}) + +const notificationsHandler = new NotificationOnEditHandler({ + reviewRequestService, + collaboratorsService, + sitesService, + notificationsService, +}) + +// Set up express with defaults and use the router under test +const router = express() +const dummySubrouter = express() +dummySubrouter.get("/:siteName/test", async (req, res, next) => + // Dummy subrouter + next() +) +router.use(dummySubrouter) + +// We handle the test slightly differently - jest interprets the end of the test as when the response is sent, +// but we normally create a notification after this response, due to the position of the middleware +// the solution to get tests working is to send a response only after the notification middleware +router.use(async (req, res, next) => { + // Inserts notification handler after all other subrouters + // Needs to be awaited so jest doesn't exit prematurely upon receiving response status + await notificationsHandler.createNotification(req as any, res as any, next) + res.status(200).send(200) +}) +const userSessionData = new UserSessionData({ + isomerUserId: mockIsomerUserId, + email: mockEmail, +}) +const app = generateRouterForUserWithSite(router, userSessionData, mockSiteName) + +describe("Notifications Router", () => { + const mockAdditionalUserId = "2" + const mockAdditionalSiteId = "2" + const mockAdditionalSiteMemberId = "2" + const mockAnotherSiteMemberId = "3" + + beforeAll(async () => { + // Mock github service return + mockGithubService.getPullRequest.mockResolvedValue({ + title: "title", + body: "body", + changed_files: [], + created_at: new Date(), + }) + + // We need to force the relevant tables to start from a clean slate + // Otherwise, some tests may fail due to the auto-incrementing IDs + // not starting from 1 + await User.sync({ force: true }) + await Site.sync({ force: true }) + await Repo.sync({ force: true }) + await SiteMember.sync({ force: true }) + await Notification.sync({ force: true }) + await ReviewMeta.sync({ force: true }) + await ReviewRequest.sync({ force: true }) + + // Set up User and Site table entries + await User.create({ + id: mockIsomerUserId, + }) + await User.create({ + id: mockAdditionalUserId, + }) + await Site.create({ + id: mockSiteId, + name: mockSiteName, + apiTokenName: "token", + jobStatus: "READY", + siteStatus: "LAUNCHED", + creatorId: mockIsomerUserId, + }) + await SiteMember.create({ + userId: mockIsomerUserId, + siteId: mockSiteId, + role: "ADMIN", + id: mockSiteMemberId, + }) + await Repo.create({ + name: mockSiteName, + url: "url", + siteId: mockSiteId, + }) + await SiteMember.create({ + userId: mockAdditionalUserId, + siteId: mockSiteId, + role: "ADMIN", + id: mockAdditionalSiteMemberId, + }) + await Site.create({ + id: mockAdditionalSiteId, + name: "mockSite2", + apiTokenName: "token", + jobStatus: "READY", + siteStatus: "LAUNCHED", + creatorId: mockIsomerUserId, + }) + await SiteMember.create({ + userId: mockIsomerUserId, + siteId: mockAdditionalSiteId, + role: "ADMIN", + id: mockAnotherSiteMemberId, + }) + await Repo.create({ + name: `${mockSiteName}2`, + url: "url", + siteId: mockAdditionalSiteId, + }) + }) + + afterAll(async () => { + await SiteMember.sync({ force: true }) + await Site.sync({ force: true }) + await User.sync({ force: true }) + await Repo.sync({ force: true }) + }) + + describe("createNotification handler", () => { + afterEach(async () => { + // Clean up so that different tests using + // the same notifications don't interfere with each other + await Notification.sync({ force: true }) + await ReviewMeta.sync({ force: true }) + await ReviewRequest.sync({ force: true }) + }) + it("should create a new notification when called", async () => { + // Arrange + await ReviewRequest.create({ + id: 1, + requestorId: mockIsomerUserId, + siteId: mockSiteId, + reviewStatus: "OPEN", + }) + await ReviewMeta.create({ + reviewId: 1, + pullRequestNumber: 1, + reviewLink: "test", + }) + mockGithubService.getComments.mockResolvedValueOnce([]) + + // Act + await request(app).get(`/${mockSiteName}/test`) + + // Assert + // Notification should be sent to all site members other than the creator + expect( + ( + await Notification.findAll({ + where: { + userId: mockAdditionalUserId, + siteId: mockSiteId, + siteMemberId: mockAdditionalSiteMemberId, + firstReadTime: null, + }, + }) + ).length + ).toEqual(1) + expect( + ( + await Notification.findAll({ + where: { + userId: mockIsomerUserId, + siteId: mockSiteId, + siteMemberId: mockSiteMemberId, + firstReadTime: null, + }, + }) + ).length + ).toEqual(0) + }) + }) +}) diff --git a/src/integration/Notifications.spec.ts b/src/integration/Notifications.spec.ts index ac2ee5901..79b822be5 100644 --- a/src/integration/Notifications.spec.ts +++ b/src/integration/Notifications.spec.ts @@ -1,11 +1,21 @@ import express from "express" import request from "supertest" -import { NotificationsRouter as _NotificationsRouter } from "@routes/v2/authenticatedSites/notifications" - -import { Notification, Repo, Site, SiteMember, User } from "@database/models" -import { generateRouter } from "@fixtures/app" +import { + Notification, + Repo, + Reviewer, + ReviewMeta, + ReviewRequest, + ReviewRequestView, + Site, + SiteMember, + User, + Whitelist, +} from "@database/models" +import { generateRouter, generateRouterForUserWithSite } from "@fixtures/app" import UserSessionData from "@root/classes/UserSessionData" +import UserWithSiteSessionData from "@root/classes/UserWithSiteSessionData" import { formatNotification, highPriorityOldReadNotification, @@ -20,31 +30,78 @@ import { mockIsomerUserId, mockSiteName, } from "@root/fixtures/sessionData" +import { getAuthorizationMiddleware } from "@root/middleware" +import { NotificationsRouter as _NotificationsRouter } from "@root/routes/v2/authenticated/notifications" import { SitesRouter as _SitesRouter } from "@root/routes/v2/authenticated/sites" -import { notificationsService } from "@services/identity" +import { genericGitHubAxiosInstance } from "@root/services/api/AxiosInstance" +import { GitHubService } from "@root/services/db/GitHubService" +import { ConfigYmlService } from "@root/services/fileServices/YmlFileServices/ConfigYmlService" +import CollaboratorsService from "@root/services/identity/CollaboratorsService" +import SitesService from "@root/services/identity/SitesService" +import ReviewRequestService from "@root/services/review/ReviewRequestService" +import * as ReviewApi from "@services/db/review" +import { + getIdentityAuthService, + getUsersService, + isomerAdminsService, + notificationsService, +} from "@services/identity" +import { sequelize } from "@tests/database" const MOCK_SITE = "mockSite" const MOCK_SITE_ID = "1" const MOCK_SITE_MEMBER_ID = "1" -const notificationsRouter = new _NotificationsRouter({ notificationsService }) +const gitHubService = new GitHubService({ + axiosInstance: genericGitHubAxiosInstance, +}) +const identityAuthService = getIdentityAuthService(gitHubService) +const usersService = getUsersService(sequelize) +const configYmlService = new ConfigYmlService({ gitHubService }) +const reviewRequestService = new ReviewRequestService( + (gitHubService as unknown) as typeof ReviewApi, + User, + ReviewRequest, + Reviewer, + ReviewMeta, + ReviewRequestView +) +const sitesService = new SitesService({ + siteRepository: Site, + gitHubService, + configYmlService, + usersService, + isomerAdminsService, + reviewRequestService, +}) +const collaboratorsService = new CollaboratorsService({ + siteRepository: Site, + siteMemberRepository: SiteMember, + sitesService, + usersService, + whitelist: Whitelist, +}) +const authorizationMiddleware = getAuthorizationMiddleware({ + identityAuthService, + usersService, + isomerAdminsService, + collaboratorsService, +}) +const notificationsRouter = new _NotificationsRouter({ + notificationsService, + authorizationMiddleware, +}) const notificationsSubrouter = notificationsRouter.getRouter() // Set up express with defaults and use the router under test const subrouter = express() -// As we set certain properties on res.locals when the user signs in using github -// In order to do integration testing, we must expose a middleware -// that allows us to set this properties also -subrouter.use((req, res, next) => { - const userSessionData = new UserSessionData({ - isomerUserId: mockIsomerUserId, - email: mockEmail, - }) - res.locals.userSessionData = userSessionData - next() + +subrouter.use("/:siteName", notificationsSubrouter) +const userSessionData = new UserSessionData({ + isomerUserId: mockIsomerUserId, + email: mockEmail, }) -subrouter.use(notificationsSubrouter) -const app = generateRouter(subrouter) +const app = generateRouterForUserWithSite(subrouter, userSessionData, MOCK_SITE) describe("Notifications Router", () => { const MOCK_ADDITIONAL_USER_ID = "2" @@ -53,6 +110,15 @@ describe("Notifications Router", () => { const MOCK_ANOTHER_SITE_MEMBER_ID = "3" beforeAll(async () => { + // We need to force the relevant tables to start from a clean slate + // Otherwise, some tests may fail due to the auto-incrementing IDs + // not starting from 1 + await User.sync({ force: true }) + await Site.sync({ force: true }) + await Repo.sync({ force: true }) + await SiteMember.sync({ force: true }) + await Notification.sync({ force: true }) + // Set up User and Site table entries await User.create({ id: mockIsomerUserId, @@ -62,7 +128,7 @@ describe("Notifications Router", () => { }) await Site.create({ id: MOCK_SITE_ID, - name: MOCK_SITE, + name: mockSiteName, apiTokenName: "token", jobStatus: "READY", siteStatus: "LAUNCHED", @@ -75,7 +141,7 @@ describe("Notifications Router", () => { id: MOCK_SITE_MEMBER_ID, }) await Repo.create({ - name: mockSiteName, + name: MOCK_SITE, url: "url", siteId: MOCK_SITE_ID, }) @@ -87,7 +153,7 @@ describe("Notifications Router", () => { }) await Site.create({ id: MOCK_ADDITIONAL_SITE_ID, - name: MOCK_SITE, + name: `${mockSiteName}2`, apiTokenName: "token", jobStatus: "READY", siteStatus: "LAUNCHED", @@ -100,7 +166,7 @@ describe("Notifications Router", () => { id: MOCK_ANOTHER_SITE_MEMBER_ID, }) await Repo.create({ - name: `${mockSiteName}2`, + name: `${MOCK_SITE}2`, url: "url", siteId: MOCK_ADDITIONAL_SITE_ID, }) @@ -187,7 +253,7 @@ describe("Notifications Router", () => { ].map((notification) => formatNotification(notification)) // Act - const actual = await request(app).get("/") + const actual = await request(app).get(`/${MOCK_SITE}`) // Assert expect(actual.body).toMatchObject(expected) @@ -277,7 +343,7 @@ describe("Notifications Router", () => { ].map((notification) => formatNotification(notification)) // Act - const actual = await request(app).get("/") + const actual = await request(app).get(`/${MOCK_SITE}`) // Assert expect(actual.body).toMatchObject(expected) @@ -358,7 +424,7 @@ describe("Notifications Router", () => { ].map((notification) => formatNotification(notification)) // Act - const actual = await request(app).get("/allNotifications") + const actual = await request(app).get(`/${MOCK_SITE}/allNotifications`) // Assert expect(actual.body).toMatchObject(expected) @@ -424,7 +490,7 @@ describe("Notifications Router", () => { const expected = 200 // Act - const actual = await request(app).post("/").send({}) + const actual = await request(app).post(`/${MOCK_SITE}`).send({}) // Assert expect(actual.statusCode).toBe(expected) diff --git a/src/integration/Reviews.spec.ts b/src/integration/Reviews.spec.ts index 8876b6e7e..7179bafef 100644 --- a/src/integration/Reviews.spec.ts +++ b/src/integration/Reviews.spec.ts @@ -7,6 +7,7 @@ import { SitesRouter as _SitesRouter } from "@routes/v2/authenticated/sites" import { IsomerAdmin, + Notification, Repo, Reviewer, ReviewMeta, @@ -70,8 +71,9 @@ import { import { ReviewRequestStatus } from "@root/constants" import { ReviewRequestDto } from "@root/types/dto/review" import { GitHubService } from "@services/db/GitHubService" +import * as ReviewApi from "@services/db/review" import { ConfigYmlService } from "@services/fileServices/YmlFileServices/ConfigYmlService" -import { getUsersService } from "@services/identity" +import { getUsersService, notificationsService } from "@services/identity" import CollaboratorsService from "@services/identity/CollaboratorsService" import IsomerAdminsService from "@services/identity/IsomerAdminsService" import SitesService from "@services/identity/SitesService" @@ -83,7 +85,7 @@ const configYmlService = new ConfigYmlService({ gitHubService }) const usersService = getUsersService(sequelize) const isomerAdminsService = new IsomerAdminsService({ repository: IsomerAdmin }) const reviewRequestService = new ReviewRequestService( - gitHubService, + (gitHubService as unknown) as typeof ReviewApi, User, ReviewRequest, Reviewer, @@ -110,7 +112,8 @@ const ReviewsRouter = new _ReviewsRouter( reviewRequestService, usersService, sitesService, - collaboratorsService + collaboratorsService, + notificationsService ) const reviewsSubrouter = ReviewsRouter.getRouter() const subrouter = express() @@ -136,6 +139,8 @@ describe("Review Requests Integration Tests", () => { await Site.sync({ force: true }) await Repo.sync({ force: true }) await SiteMember.sync({ force: true }) + await Notification.sync({ force: true }) + await ReviewMeta.sync({ force: true }) await User.create(MOCK_USER_DBENTRY_ONE) await User.create(MOCK_USER_DBENTRY_TWO) diff --git a/src/middleware/notificationOnEditHandler.ts b/src/middleware/notificationOnEditHandler.ts new file mode 100644 index 000000000..1a8a36f99 --- /dev/null +++ b/src/middleware/notificationOnEditHandler.ts @@ -0,0 +1,75 @@ +import autoBind from "auto-bind" + +import UserWithSiteSessionData from "@root/classes/UserWithSiteSessionData" +import { SiteMember, User } from "@root/database/models" +import CollaboratorsService from "@root/services/identity/CollaboratorsService" +import NotificationsService from "@root/services/identity/NotificationsService" +import SitesService from "@root/services/identity/SitesService" +import ReviewRequestService from "@root/services/review/ReviewRequestService" +import { RequestHandler } from "@root/types" + +export class NotificationOnEditHandler { + private readonly reviewRequestService: ReviewRequestService + + private readonly sitesService: SitesService + + private readonly collaboratorsService: CollaboratorsService + + private readonly notificationsService: NotificationsService + + constructor({ + reviewRequestService, + sitesService, + collaboratorsService, + notificationsService, + }: { + reviewRequestService: ReviewRequestService + sitesService: SitesService + collaboratorsService: CollaboratorsService + notificationsService: NotificationsService + }) { + this.reviewRequestService = reviewRequestService + this.sitesService = sitesService + this.collaboratorsService = collaboratorsService + this.notificationsService = notificationsService + // We need to bind all methods because we don't invoke them from the class directly + autoBind(this) + } + + /** + * Creates a notification. Requires attachSiteHandler as a precondition + */ + createNotification: RequestHandler< + never, + unknown, + unknown, + never, + { userWithSiteSessionData: UserWithSiteSessionData } + > = async (req, res, next) => { + const { userWithSiteSessionData } = res.locals + const { siteName, isomerUserId: userId, email } = userWithSiteSessionData + const site = await this.sitesService.getBySiteName(siteName) + const users = await this.collaboratorsService.list(siteName, userId) + if (!site) throw new Error("Site should always exist") + const reviewRequests = await this.reviewRequestService.listReviewRequest( + userWithSiteSessionData, + site + ) + if (reviewRequests.length === 0) return + // For now, we only have 1 active review request + const reviewRequest = reviewRequests[0] + + await Promise.all( + users.map(async (user: User & { SiteMember: SiteMember }) => { + if (user.id.toString() === userId) return // Don't create notification for the source user + const { SiteMember: siteMember } = user + await this.notificationsService.create({ + siteMember, + link: `/sites/${siteName}/review/${reviewRequest.id}`, + notificationType: "updated_request", + notificationSourceUsername: email, + }) + }) + ) + } +} diff --git a/src/middleware/routeHandler.js b/src/middleware/routeHandler.js index db2221df6..2e582b890 100644 --- a/src/middleware/routeHandler.js +++ b/src/middleware/routeHandler.js @@ -24,7 +24,7 @@ const attachWriteRouteHandlerWrapper = (routeHandler) => async ( ) => { const { siteName } = req.params await lock(siteName) - routeHandler(req, res).catch(async (err) => { + routeHandler(req, res, next).catch(async (err) => { await unlock(siteName) next(err) }) @@ -61,7 +61,7 @@ const attachRollbackRouteHandlerWrapper = (routeHandler) => async ( await unlock(siteName) next(err) } - routeHandler(req, res).catch(async (err) => { + routeHandler(req, res, next).catch(async (err) => { try { await backOff(() => revertCommit(originalCommitSha, siteName, accessToken) diff --git a/src/routes/v2/authenticatedSites/__tests__/Notifications.spec.ts b/src/routes/v2/authenticated/__tests__/Notifications.spec.ts similarity index 91% rename from src/routes/v2/authenticatedSites/__tests__/Notifications.spec.ts rename to src/routes/v2/authenticated/__tests__/Notifications.spec.ts index 060d9cbf5..0eca69a4d 100644 --- a/src/routes/v2/authenticatedSites/__tests__/Notifications.spec.ts +++ b/src/routes/v2/authenticated/__tests__/Notifications.spec.ts @@ -3,10 +3,10 @@ import request from "supertest" import { attachReadRouteHandlerWrapper } from "@middleware/routeHandler" -import { NotificationsRouter as _NotificationsRouter } from "@routes/v2/authenticatedSites/notifications" - import { generateRouter } from "@fixtures/app" import { mockSiteName, mockIsomerUserId } from "@fixtures/sessionData" +import { AuthorizationMiddleware } from "@root/middleware/authorization" +import { NotificationsRouter as _NotificationsRouter } from "@root/routes/v2/authenticated/notifications" import NotificationsService from "@services/identity/NotificationsService" describe("Notifications Router", () => { @@ -15,9 +15,13 @@ describe("Notifications Router", () => { listAll: jest.fn(), markNotificationsAsRead: jest.fn(), } + const mockAuthorizationMiddleware = { + verifySiteMember: jest.fn(), + } const NotificationsRouter = new _NotificationsRouter({ notificationsService: (mockNotificationsService as unknown) as NotificationsService, + authorizationMiddleware: (mockAuthorizationMiddleware as unknown) as AuthorizationMiddleware, }) const subrouter = express() diff --git a/src/routes/v2/authenticated/__tests__/review.spec.ts b/src/routes/v2/authenticated/__tests__/review.spec.ts index c860e44a7..602fc9f59 100644 --- a/src/routes/v2/authenticated/__tests__/review.spec.ts +++ b/src/routes/v2/authenticated/__tests__/review.spec.ts @@ -1,15 +1,18 @@ import express from "express" import request from "supertest" +import RequestNotFoundError from "@errors/RequestNotFoundError" + import { attachReadRouteHandlerWrapper } from "@middleware/routeHandler" import { ReviewsRouter as _ReviewsRouter } from "@routes/v2/authenticated/review" import { generateRouterForDefaultUserWithSite } from "@fixtures/app" +import { mockUserId } from "@fixtures/identity" +import { MOCK_USER_EMAIL_ONE, MOCK_USER_EMAIL_TWO } from "@fixtures/users" import { CollaboratorRoles } from "@root/constants" -import RequestNotFoundError from "@root/errors/RequestNotFoundError" -import { MOCK_USER_EMAIL_ONE, MOCK_USER_EMAIL_TWO } from "@root/fixtures/users" import CollaboratorsService from "@services/identity/CollaboratorsService" +import NotificationsService from "@services/identity/NotificationsService" import SitesService from "@services/identity/SitesService" import UsersService from "@services/identity/UsersService" import ReviewRequestService from "@services/review/ReviewRequestService" @@ -48,11 +51,16 @@ describe("Review Requests Router", () => { list: jest.fn(), } + const mockNotificationsService = { + create: jest.fn(), + } + const ReviewsRouter = new _ReviewsRouter( (mockReviewRequestService as unknown) as ReviewRequestService, (mockIdentityUsersService as unknown) as UsersService, (mockSitesService as unknown) as SitesService, - (mockCollaboratorsService as unknown) as CollaboratorsService + (mockCollaboratorsService as unknown) as CollaboratorsService, + (mockNotificationsService as unknown) as NotificationsService ) const subrouter = express() @@ -169,11 +177,13 @@ describe("Review Requests Router", () => { SiteMember: { role: CollaboratorRoles.Admin, }, + id: mockUserId, }, ]) mockReviewRequestService.createReviewRequest.mockResolvedValueOnce( mockPullRequestNumber ) + mockNotificationsService.create.mockResolvedValueOnce([]) // Act const response = await request(app) @@ -196,6 +206,7 @@ describe("Review Requests Router", () => { expect( mockReviewRequestService.createReviewRequest ).toHaveBeenCalledTimes(1) + expect(mockNotificationsService.create).toHaveBeenCalledTimes(1) }) it("should return 404 if the site does not exist", async () => { @@ -216,6 +227,7 @@ describe("Review Requests Router", () => { expect( mockReviewRequestService.createReviewRequest ).not.toHaveBeenCalled() + expect(mockNotificationsService.create).not.toHaveBeenCalled() }) it("should return 404 if user is not a site collaborator", async () => { @@ -237,6 +249,7 @@ describe("Review Requests Router", () => { expect( mockReviewRequestService.createReviewRequest ).not.toHaveBeenCalled() + expect(mockNotificationsService.create).not.toHaveBeenCalled() }) it("should return 400 if no reviewers are provided", async () => { @@ -263,6 +276,7 @@ describe("Review Requests Router", () => { expect( mockReviewRequestService.createReviewRequest ).not.toHaveBeenCalled() + expect(mockNotificationsService.create).not.toHaveBeenCalled() }) it("should return 400 if provided reviewer is not an admin", async () => { @@ -291,6 +305,7 @@ describe("Review Requests Router", () => { expect( mockReviewRequestService.createReviewRequest ).not.toHaveBeenCalled() + expect(mockNotificationsService.create).not.toHaveBeenCalled() }) }) @@ -570,6 +585,7 @@ describe("Review Requests Router", () => { SiteMember: { role: CollaboratorRoles.Admin, }, + id: mockUserId, }, ]) mockReviewRequestService.updateReviewRequest.mockResolvedValueOnce( @@ -773,6 +789,7 @@ describe("Review Requests Router", () => { it("should return 200 with the review request successfully marked as approved", async () => { // Arrange const mockReviewRequest = { reviewers: [{ email: MOCK_USER_EMAIL_ONE }] } + const mockReviewer = "reviewer@test.gov.sg" mockSitesService.getBySiteName.mockResolvedValueOnce("site") mockReviewRequestService.getReviewRequest.mockResolvedValueOnce( mockReviewRequest @@ -780,6 +797,15 @@ describe("Review Requests Router", () => { mockReviewRequestService.approveReviewRequest.mockResolvedValueOnce( undefined ) + mockCollaboratorsService.list.mockResolvedValueOnce([ + { + email: mockReviewer, + SiteMember: { + role: CollaboratorRoles.Admin, + }, + id: mockUserId, + }, + ]) // Act const response = await request(app).post(`/mockSite/review/12345/approve`) @@ -791,6 +817,7 @@ describe("Review Requests Router", () => { expect( mockReviewRequestService.approveReviewRequest ).toHaveBeenCalledTimes(1) + expect(mockNotificationsService.create).toHaveBeenCalledTimes(1) }) it("should return 404 if the site does not exist", async () => { @@ -807,6 +834,7 @@ describe("Review Requests Router", () => { expect( mockReviewRequestService.approveReviewRequest ).not.toHaveBeenCalled() + expect(mockNotificationsService.create).not.toHaveBeenCalled() }) it("should return 404 if the review request does not exist", async () => { @@ -826,6 +854,7 @@ describe("Review Requests Router", () => { expect( mockReviewRequestService.approveReviewRequest ).not.toHaveBeenCalled() + expect(mockNotificationsService.create).not.toHaveBeenCalled() }) it("should return 403 if the user is not a reviewer", async () => { @@ -846,6 +875,7 @@ describe("Review Requests Router", () => { expect( mockReviewRequestService.approveReviewRequest ).not.toHaveBeenCalled() + expect(mockNotificationsService.create).not.toHaveBeenCalled() }) }) @@ -1097,6 +1127,7 @@ describe("Review Requests Router", () => { it("should return 200 with the review request closed successfully", async () => { // Arrange const mockReviewRequest = { requestor: { email: MOCK_USER_EMAIL_ONE } } + const mockReviewer = "reviewer@test.gov.sg" mockSitesService.getBySiteName.mockResolvedValueOnce("site") mockReviewRequestService.getReviewRequest.mockResolvedValueOnce( mockReviewRequest @@ -1107,6 +1138,15 @@ describe("Review Requests Router", () => { mockReviewRequestService.deleteAllReviewRequestViews.mockResolvedValueOnce( undefined ) + mockCollaboratorsService.list.mockResolvedValueOnce([ + { + email: mockReviewer, + SiteMember: { + role: CollaboratorRoles.Admin, + }, + id: mockUserId, + }, + ]) // Act const response = await request(app).delete(`/mockSite/review/12345`) @@ -1121,6 +1161,7 @@ describe("Review Requests Router", () => { expect( mockReviewRequestService.deleteAllReviewRequestViews ).toHaveBeenCalledTimes(1) + expect(mockNotificationsService.create).toHaveBeenCalledTimes(1) }) it("should return 404 if the site does not exist", async () => { @@ -1138,6 +1179,7 @@ describe("Review Requests Router", () => { expect( mockReviewRequestService.deleteAllReviewRequestViews ).not.toHaveBeenCalled() + expect(mockNotificationsService.create).not.toHaveBeenCalled() }) it("should return 404 if the review request does not exist", async () => { @@ -1158,6 +1200,7 @@ describe("Review Requests Router", () => { expect( mockReviewRequestService.deleteAllReviewRequestViews ).not.toHaveBeenCalled() + expect(mockNotificationsService.create).not.toHaveBeenCalled() }) it("should return 404 if the user is not the requestor of the review request", async () => { @@ -1179,6 +1222,7 @@ describe("Review Requests Router", () => { expect( mockReviewRequestService.deleteAllReviewRequestViews ).not.toHaveBeenCalled() + expect(mockNotificationsService.create).not.toHaveBeenCalled() }) }) diff --git a/src/routes/v2/authenticated/index.js b/src/routes/v2/authenticated/index.js index d3015f411..435fc03d6 100644 --- a/src/routes/v2/authenticated/index.js +++ b/src/routes/v2/authenticated/index.js @@ -1,5 +1,7 @@ import { attachSiteHandler } from "@root/middleware" +import { NotificationsRouter } from "./notifications" + const express = require("express") const { @@ -20,6 +22,7 @@ const getAuthenticatedSubrouter = ({ collaboratorsService, authorizationMiddleware, reviewRouter, + notificationsService, }) => { const netlifyTomlService = new NetlifyTomlService() @@ -33,6 +36,10 @@ const getAuthenticatedSubrouter = ({ }) const usersRouter = new UsersRouter({ usersService }) const netlifyTomlV2Router = new NetlifyTomlRouter({ netlifyTomlService }) + const notificationsRouter = new NotificationsRouter({ + authorizationMiddleware, + notificationsService, + }) const authenticatedSubrouter = express.Router({ mergeParams: true }) @@ -52,6 +59,10 @@ const getAuthenticatedSubrouter = ({ reviewRouter.getRouter() ) authenticatedSubrouter.use("/sites", sitesRouterWithReviewRequest) + authenticatedSubrouter.use( + "/sites/:siteName/notifications", + notificationsRouter.getRouter() + ) authenticatedSubrouter.use("/user", usersRouter.getRouter()) authenticatedSubrouter.use("/netlify-toml", netlifyTomlV2Router.getRouter()) diff --git a/src/routes/v2/authenticatedSites/notifications.ts b/src/routes/v2/authenticated/notifications.ts similarity index 83% rename from src/routes/v2/authenticatedSites/notifications.ts rename to src/routes/v2/authenticated/notifications.ts index 6f20772eb..832d54d4c 100644 --- a/src/routes/v2/authenticatedSites/notifications.ts +++ b/src/routes/v2/authenticated/notifications.ts @@ -7,6 +7,8 @@ import { } from "@middleware/routeHandler" import UserWithSiteSessionData from "@root/classes/UserWithSiteSessionData" +import { attachSiteHandler } from "@root/middleware" +import { AuthorizationMiddleware } from "@root/middleware/authorization" import { RequestHandler } from "@root/types" import NotificationsService, { NotificationResponse, @@ -14,14 +16,21 @@ import NotificationsService, { interface NotificationsRouterProps { notificationsService: NotificationsService + authorizationMiddleware: AuthorizationMiddleware } // eslint-disable-next-line import/prefer-default-export export class NotificationsRouter { private readonly notificationsService - constructor({ notificationsService }: NotificationsRouterProps) { + private readonly authorizationMiddleware + + constructor({ + notificationsService, + authorizationMiddleware, + }: NotificationsRouterProps) { this.notificationsService = notificationsService + this.authorizationMiddleware = authorizationMiddleware autoBind(this) } @@ -78,6 +87,8 @@ export class NotificationsRouter { getRouter() { const router = express.Router({ mergeParams: true }) + router.use(attachSiteHandler) + router.use(this.authorizationMiddleware.verifySiteMember) router.get("/", attachReadRouteHandlerWrapper(this.getRecentNotifications)) router.get( diff --git a/src/routes/v2/authenticated/review.ts b/src/routes/v2/authenticated/review.ts index be1e51652..2191ed03d 100644 --- a/src/routes/v2/authenticated/review.ts +++ b/src/routes/v2/authenticated/review.ts @@ -13,7 +13,9 @@ import UserSessionData from "@classes/UserSessionData" import UserWithSiteSessionData from "@classes/UserWithSiteSessionData" import { CollaboratorRoles } from "@root/constants" +import { SiteMember, User } from "@root/database/models" import CollaboratorsService from "@root/services/identity/CollaboratorsService" +import NotificationsService from "@root/services/identity/NotificationsService" import SitesService from "@root/services/identity/SitesService" import UsersService from "@root/services/identity/UsersService" import { isIsomerError, RequestHandler } from "@root/types" @@ -37,16 +39,20 @@ export class ReviewsRouter { private readonly collaboratorsService + private readonly notificationsService + constructor( reviewRequestService: ReviewRequestService, identityUsersService: UsersService, sitesService: SitesService, - collaboratorsService: CollaboratorsService + collaboratorsService: CollaboratorsService, + notificationsService: NotificationsService ) { this.reviewRequestService = reviewRequestService this.identityUsersService = identityUsersService this.sitesService = sitesService this.collaboratorsService = collaboratorsService + this.notificationsService = notificationsService autoBind(this) } @@ -187,6 +193,23 @@ export class ReviewsRouter { description ) + // Step 5: Create notifications + await Promise.all( + collaborators.map(async (user: User & { SiteMember: SiteMember }) => { + // Don't send notification to self + if (user.id.toString() === userWithSiteSessionData.isomerUserId) return + const notificationType = reviewersMap[user.email || ""] + ? "sent_request" + : "request_created" + await this.notificationsService.create({ + siteMember: user.SiteMember, + link: `/sites/${siteName}/review/${pullRequestNumber}`, + notificationType, + notificationSourceUsername: userWithSiteSessionData.email, + }) + }) + ) + return res.status(200).send({ pullRequestNumber, }) @@ -697,6 +720,22 @@ export class ReviewsRouter { // as the underlying Github API returns 404 if // the requested review could not be found. await this.reviewRequestService.approveReviewRequest(possibleReviewRequest) + + // Step 6: Create notifications + const collaborators = await this.collaboratorsService.list(siteName) + await Promise.all( + collaborators.map(async (user: User & { SiteMember: SiteMember }) => { + // Don't send notification to self + if (user.id.toString() === userWithSiteSessionData.isomerUserId) return + await this.notificationsService.create({ + siteMember: user.SiteMember, + link: `/sites/${siteName}/review/${requestId}`, + notificationType: "request_approved", + notificationSourceUsername: userWithSiteSessionData.email, + }) + }) + ) + return res.status(200).send() } @@ -998,6 +1037,20 @@ export class ReviewsRouter { // The error is discarded as we are guaranteed to have a review request await this.reviewRequestService.deleteAllReviewRequestViews(site, requestId) + // Step 7: Create notifications + const collaborators = await this.collaboratorsService.list(siteName) + await Promise.all( + collaborators.map(async (user: User & { SiteMember: SiteMember }) => { + // Don't send notification to self + if (user.id.toString() === userWithSiteSessionData.isomerUserId) return + await this.notificationsService.create({ + siteMember: user.SiteMember, + link: `/sites/${siteName}/review/${requestId}`, + notificationType: "request_cancelled", + notificationSourceUsername: userWithSiteSessionData.email, + }) + }) + ) return res.status(200).send() } diff --git a/src/routes/v2/authenticatedSites/collectionPages.js b/src/routes/v2/authenticatedSites/collectionPages.js index 7226ad9c8..7487b6e0f 100644 --- a/src/routes/v2/authenticatedSites/collectionPages.js +++ b/src/routes/v2/authenticatedSites/collectionPages.js @@ -24,7 +24,7 @@ class CollectionPagesRouter { } // Create new page in collection - async createCollectionPage(req, res) { + async createCollectionPage(req, res, next) { const { userWithSiteSessionData } = res.locals const { collectionName, subcollectionName } = req.params @@ -58,7 +58,8 @@ class CollectionPagesRouter { ) } - return res.status(200).json(createResp) + res.status(200).json(createResp) + return next() } // Read page in collection @@ -90,7 +91,7 @@ class CollectionPagesRouter { } // Update page in collection - async updateCollectionPage(req, res) { + async updateCollectionPage(req, res, next) { const { userWithSiteSessionData } = res.locals const { pageName, collectionName, subcollectionName } = req.params @@ -156,12 +157,12 @@ class CollectionPagesRouter { ) } } - /* eslint-enable no-lonely-if */ - return res.status(200).json(updateResp) + res.status(200).json(updateResp) + return next() } // Delete page in collection - async deleteCollectionPage(req, res) { + async deleteCollectionPage(req, res, next) { const { userWithSiteSessionData } = res.locals const { pageName, collectionName, subcollectionName } = req.params @@ -183,7 +184,8 @@ class CollectionPagesRouter { }) } - return res.status(200).send("OK") + res.status(200).send("OK") + return next() } getRouter() { diff --git a/src/routes/v2/authenticatedSites/collections.js b/src/routes/v2/authenticatedSites/collections.js index 502d161e8..e9a84f5c6 100644 --- a/src/routes/v2/authenticatedSites/collections.js +++ b/src/routes/v2/authenticatedSites/collections.js @@ -58,7 +58,7 @@ class CollectionsRouter { } // Create new collection/subcollection - async createCollectionDirectory(req, res) { + async createCollectionDirectory(req, res, next) { const { userWithSiteSessionData } = res.locals const { collectionName } = req.params @@ -87,11 +87,12 @@ class CollectionsRouter { ) } - return res.status(200).json(createResp) + res.status(200).json(createResp) + return next() } // Rename collection/subcollection - async renameCollectionDirectory(req, res) { + async renameCollectionDirectory(req, res, next) { const { userWithSiteSessionData, githubSessionData } = res.locals const { collectionName, subcollectionName } = req.params @@ -119,11 +120,12 @@ class CollectionsRouter { ) } - return res.status(200).send("OK") + res.status(200).send("OK") + return next() } // Delete collection/subcollection - async deleteCollectionDirectory(req, res) { + async deleteCollectionDirectory(req, res, next) { const { userWithSiteSessionData, githubSessionData } = res.locals const { collectionName, subcollectionName } = req.params @@ -145,11 +147,12 @@ class CollectionsRouter { } ) } - return res.status(200).send("OK") + res.status(200).send("OK") + return next() } // Reorder collection/subcollection - async reorderCollectionDirectory(req, res) { + async reorderCollectionDirectory(req, res, next) { const { userWithSiteSessionData } = res.locals const { collectionName, subcollectionName } = req.params @@ -175,11 +178,12 @@ class CollectionsRouter { } ) } - return res.status(200).json(reorderResp) + res.status(200).json(reorderResp) + return next() } // Move collection/subcollection pages - async moveCollectionDirectoryPages(req, res) { + async moveCollectionDirectoryPages(req, res, next) { const { userWithSiteSessionData } = res.locals const { collectionName, subcollectionName } = req.params @@ -211,7 +215,8 @@ class CollectionsRouter { objArray: items, }) } - return res.status(200).send("OK") + res.status(200).send("OK") + return next() } getRouter() { diff --git a/src/routes/v2/authenticatedSites/contactUs.js b/src/routes/v2/authenticatedSites/contactUs.js index e85ddab80..b146aa1f9 100644 --- a/src/routes/v2/authenticatedSites/contactUs.js +++ b/src/routes/v2/authenticatedSites/contactUs.js @@ -30,7 +30,7 @@ class ContactUsRouter { } // Update contactUs index file - async updateContactUs(req, res) { + async updateContactUs(req, res, next) { const { userWithSiteSessionData } = res.locals const { error } = UpdateContactUsSchema.validate(req.body) @@ -45,7 +45,8 @@ class ContactUsRouter { { content: pageBody, frontMatter, sha } ) - return res.status(200).json(updatedContactUsPage) + res.status(200).json(updatedContactUsPage) + return next() } getRouter() { diff --git a/src/routes/v2/authenticatedSites/homepage.js b/src/routes/v2/authenticatedSites/homepage.js index 0d41851fe..003c6bca3 100644 --- a/src/routes/v2/authenticatedSites/homepage.js +++ b/src/routes/v2/authenticatedSites/homepage.js @@ -30,7 +30,7 @@ class HomepageRouter { } // Update homepage index file - async updateHomepage(req, res) { + async updateHomepage(req, res, next) { const { userWithSiteSessionData } = res.locals const { error } = UpdateHomepageSchema.validate(req.body, { @@ -51,7 +51,8 @@ class HomepageRouter { } ) - return res.status(200).json(updatedHomepage) + res.status(200).json(updatedHomepage) + return next() } getRouter() { diff --git a/src/routes/v2/authenticatedSites/index.js b/src/routes/v2/authenticatedSites/index.js index 267322504..539ae3a15 100644 --- a/src/routes/v2/authenticatedSites/index.js +++ b/src/routes/v2/authenticatedSites/index.js @@ -1,7 +1,5 @@ import { attachSiteHandler } from "@root/middleware" -import { NotificationsRouter } from "./notifications" - const express = require("express") const { @@ -92,6 +90,7 @@ const getAuthenticatedSitesSubrouter = ({ configYmlService, apiLogger, notificationsService, + notificationOnEditHandler, }) => { const collectionYmlService = new CollectionYmlService({ gitHubService }) const homepagePageService = new HomepagePageService({ gitHubService }) @@ -188,7 +187,6 @@ const getAuthenticatedSitesSubrouter = ({ const navigationV2Router = new NavigationRouter({ navigationYmlService: navYmlService, }) - const notificationsRouter = new NotificationsRouter({ notificationsService }) const authenticatedSitesSubrouter = express.Router({ mergeParams: true }) @@ -229,10 +227,7 @@ const getAuthenticatedSitesSubrouter = ({ authenticatedSitesSubrouter.use("/contactUs", contactUsV2Router.getRouter()) authenticatedSitesSubrouter.use("/homepage", homepageV2Router.getRouter()) authenticatedSitesSubrouter.use("/settings", settingsV2Router.getRouter()) - authenticatedSitesSubrouter.use( - "/notifications", - notificationsRouter.getRouter() - ) + authenticatedSitesSubrouter.use(notificationOnEditHandler.createNotification) return authenticatedSitesSubrouter } diff --git a/src/routes/v2/authenticatedSites/mediaCategories.js b/src/routes/v2/authenticatedSites/mediaCategories.js index 2d3002686..a64dcb86c 100644 --- a/src/routes/v2/authenticatedSites/mediaCategories.js +++ b/src/routes/v2/authenticatedSites/mediaCategories.js @@ -37,7 +37,7 @@ class MediaCategoriesRouter { } // Create new media directory - async createMediaDirectory(req, res) { + async createMediaDirectory(req, res, next) { const { userWithSiteSessionData, githubSessionData } = res.locals const { error } = CreateMediaDirectoryRequestSchema.validate(req.body) @@ -52,11 +52,12 @@ class MediaCategoriesRouter { } ) - return res.status(200).json(createResp) + res.status(200).json(createResp) + return next() } // Rename resource category - async renameMediaDirectory(req, res) { + async renameMediaDirectory(req, res, next) { const { userWithSiteSessionData, githubSessionData } = res.locals const { directoryName } = req.params @@ -72,11 +73,12 @@ class MediaCategoriesRouter { } ) - return res.status(200).send("OK") + res.status(200).send("OK") + return next() } // Delete resource category - async deleteMediaDirectory(req, res) { + async deleteMediaDirectory(req, res, next) { const { userWithSiteSessionData, githubSessionData } = res.locals const { directoryName } = req.params @@ -87,11 +89,12 @@ class MediaCategoriesRouter { directoryName, } ) - return res.status(200).send("OK") + res.status(200).send("OK") + return next() } // Move resource category - async moveMediaFiles(req, res) { + async moveMediaFiles(req, res, next) { const { userWithSiteSessionData, githubSessionData } = res.locals const { directoryName } = req.params @@ -110,7 +113,8 @@ class MediaCategoriesRouter { objArray: items, } ) - return res.status(200).send("OK") + res.status(200).send("OK") + return next() } getRouter() { diff --git a/src/routes/v2/authenticatedSites/mediaFiles.js b/src/routes/v2/authenticatedSites/mediaFiles.js index c444eaa1e..6443930d2 100644 --- a/src/routes/v2/authenticatedSites/mediaFiles.js +++ b/src/routes/v2/authenticatedSites/mediaFiles.js @@ -23,7 +23,7 @@ class MediaFilesRouter { } // Create new page in collection - async createMediaFile(req, res) { + async createMediaFile(req, res, next) { const { userWithSiteSessionData } = res.locals const { directoryName } = req.params @@ -39,7 +39,8 @@ class MediaFilesRouter { } ) - return res.status(200).json(createResp) + res.status(200).json(createResp) + return next() } // Read page in collection @@ -56,7 +57,7 @@ class MediaFilesRouter { } // Update page in collection - async updateMediaFile(req, res) { + async updateMediaFile(req, res, next) { const { userWithSiteSessionData, githubSessionData } = res.locals const { fileName, directoryName } = req.params @@ -84,11 +85,12 @@ class MediaFilesRouter { sha, }) } - return res.status(200).json(updateResp) + res.status(200).json(updateResp) + return next() } // Delete page in collection - async deleteMediaFile(req, res) { + async deleteMediaFile(req, res, next) { const { userWithSiteSessionData } = res.locals const { fileName, directoryName } = req.params @@ -101,7 +103,8 @@ class MediaFilesRouter { sha, }) - return res.status(200).send("OK") + res.status(200).send("OK") + return next() } getRouter() { diff --git a/src/routes/v2/authenticatedSites/navigation.js b/src/routes/v2/authenticatedSites/navigation.js index ba81fdd5f..291942a04 100644 --- a/src/routes/v2/authenticatedSites/navigation.js +++ b/src/routes/v2/authenticatedSites/navigation.js @@ -30,7 +30,7 @@ class NavigationRouter { } // Update navigation index file - async updateNavigation(req, res) { + async updateNavigation(req, res, next) { const { error } = UpdateNavigationRequestSchema.validate(req.body) if (error) throw new BadRequestError(error.message) @@ -44,7 +44,8 @@ class NavigationRouter { { fileContent, sha } ) - return res.status(200).json(updatedNavigationPage) + res.status(200).json(updatedNavigationPage) + return next() } getRouter() { diff --git a/src/routes/v2/authenticatedSites/resourceCategories.js b/src/routes/v2/authenticatedSites/resourceCategories.js index 9160fde25..19dce7207 100644 --- a/src/routes/v2/authenticatedSites/resourceCategories.js +++ b/src/routes/v2/authenticatedSites/resourceCategories.js @@ -35,7 +35,7 @@ class ResourceCategoriesRouter { } // Create new resource category - async createResourceDirectory(req, res) { + async createResourceDirectory(req, res, next) { const { userWithSiteSessionData } = res.locals const { resourceRoomName } = req.params @@ -50,11 +50,12 @@ class ResourceCategoriesRouter { } ) - return res.status(200).json(createResp) + res.status(200).json(createResp) + return next() } // Rename resource category - async renameResourceDirectory(req, res) { + async renameResourceDirectory(req, res, next) { const { userWithSiteSessionData, githubSessionData } = res.locals const { resourceRoomName, resourceCategoryName } = req.params @@ -71,11 +72,12 @@ class ResourceCategoriesRouter { } ) - return res.status(200).send("OK") + res.status(200).send("OK") + return next() } // Delete resource category - async deleteResourceDirectory(req, res) { + async deleteResourceDirectory(req, res, next) { const { userWithSiteSessionData, githubSessionData } = res.locals const { resourceRoomName, resourceCategoryName } = req.params @@ -87,11 +89,12 @@ class ResourceCategoriesRouter { resourceCategoryName, } ) - return res.status(200).send("OK") + res.status(200).send("OK") + return next() } // Move resource category - async moveResourceDirectoryPages(req, res) { + async moveResourceDirectoryPages(req, res, next) { const { userWithSiteSessionData, githubSessionData } = res.locals const { resourceRoomName, resourceCategoryName } = req.params @@ -111,7 +114,8 @@ class ResourceCategoriesRouter { objArray: items, } ) - return res.status(200).send("OK") + res.status(200).send("OK") + return next() } getRouter() { diff --git a/src/routes/v2/authenticatedSites/resourcePages.js b/src/routes/v2/authenticatedSites/resourcePages.js index 22819d188..c4111e1bf 100644 --- a/src/routes/v2/authenticatedSites/resourcePages.js +++ b/src/routes/v2/authenticatedSites/resourcePages.js @@ -23,7 +23,7 @@ class ResourcePagesRouter { } // Create new page in resource category - async createResourcePage(req, res) { + async createResourcePage(req, res, next) { const { userWithSiteSessionData } = res.locals const { resourceRoomName, resourceCategoryName } = req.params @@ -44,7 +44,8 @@ class ResourcePagesRouter { } ) - return res.status(200).json(createResp) + res.status(200).json(createResp) + return next() } // Read page in resource category @@ -66,7 +67,7 @@ class ResourcePagesRouter { } // Update page in resource category - async updateResourcePage(req, res) { + async updateResourcePage(req, res, next) { const { userWithSiteSessionData } = res.locals const { resourceRoomName, resourceCategoryName, pageName } = req.params @@ -104,11 +105,12 @@ class ResourcePagesRouter { } ) } - return res.status(200).json(updateResp) + res.status(200).json(updateResp) + return next() } // Delete page in resource category - async deleteResourcePage(req, res) { + async deleteResourcePage(req, res, next) { const { userWithSiteSessionData } = res.locals const { resourceRoomName, resourceCategoryName, pageName } = req.params @@ -122,7 +124,8 @@ class ResourcePagesRouter { sha, }) - return res.status(200).send("OK") + res.status(200).send("OK") + return next() } getRouter() { diff --git a/src/routes/v2/authenticatedSites/resourceRoom.js b/src/routes/v2/authenticatedSites/resourceRoom.js index c66c017ed..f50d06f34 100644 --- a/src/routes/v2/authenticatedSites/resourceRoom.js +++ b/src/routes/v2/authenticatedSites/resourceRoom.js @@ -48,7 +48,7 @@ class ResourceRoomRouter { } // Create new resource room - async createResourceRoomDirectory(req, res) { + async createResourceRoomDirectory(req, res, next) { const { userWithSiteSessionData } = res.locals const { error } = CreateResourceDirectoryRequestSchema.validate(req.body) @@ -61,11 +61,12 @@ class ResourceRoomRouter { } ) - return res.status(200).json(createResp) + res.status(200).json(createResp) + return next() } // Rename resource room - async renameResourceRoomDirectory(req, res) { + async renameResourceRoomDirectory(req, res, next) { const { userWithSiteSessionData } = res.locals const { resourceRoomName } = req.params @@ -80,7 +81,8 @@ class ResourceRoomRouter { } ) - return res.status(200).send("OK") + res.status(200).send("OK") + return next() } // Delete resource room diff --git a/src/routes/v2/authenticatedSites/settings.js b/src/routes/v2/authenticatedSites/settings.js index 7f1ed377e..8d5b87850 100644 --- a/src/routes/v2/authenticatedSites/settings.js +++ b/src/routes/v2/authenticatedSites/settings.js @@ -39,7 +39,7 @@ class SettingsRouter { }) } - async updateSettingsPage(req, res) { + async updateSettingsPage(req, res, next) { const { body } = req const { userWithSiteSessionData } = res.locals @@ -73,7 +73,8 @@ class SettingsRouter { updatedFooterContent, updatedNavigationContent, }) - return res.status(200).send("OK") + res.status(200).send("OK") + return next() } getRouter() { diff --git a/src/routes/v2/authenticatedSites/unlinkedPages.js b/src/routes/v2/authenticatedSites/unlinkedPages.js index f1c734d50..0b82a7ca6 100644 --- a/src/routes/v2/authenticatedSites/unlinkedPages.js +++ b/src/routes/v2/authenticatedSites/unlinkedPages.js @@ -34,7 +34,7 @@ class UnlinkedPagesRouter { return res.status(200).json(listResp) } - async createUnlinkedPage(req, res) { + async createUnlinkedPage(req, res, next) { const { userWithSiteSessionData } = res.locals const { error } = CreatePageRequestSchema.validate(req.body) @@ -52,7 +52,8 @@ class UnlinkedPagesRouter { } ) - return res.status(200).json(createResp) + res.status(200).json(createResp) + return next() } async readUnlinkedPage(req, res) { @@ -69,7 +70,7 @@ class UnlinkedPagesRouter { return res.status(200).json({ pageName, sha, content }) } - async updateUnlinkedPage(req, res) { + async updateUnlinkedPage(req, res, next) { const { userWithSiteSessionData } = res.locals const { pageName } = req.params @@ -105,10 +106,11 @@ class UnlinkedPagesRouter { ) } - return res.status(200).json(updateResp) + res.status(200).json(updateResp) + return next() } - async deleteUnlinkedPage(req, res) { + async deleteUnlinkedPage(req, res, next) { const { userWithSiteSessionData } = res.locals const { pageName } = req.params @@ -120,10 +122,11 @@ class UnlinkedPagesRouter { sha, }) - return res.status(200).send("OK") + res.status(200).send("OK") + return next() } - async moveUnlinkedPages(req, res) { + async moveUnlinkedPages(req, res, next) { const { userWithSiteSessionData } = res.locals const { error } = MoveDirectoryPagesRequestSchema.validate(req.body) @@ -143,7 +146,8 @@ class UnlinkedPagesRouter { objArray: items, } ) - return res.status(200).send("OK") + res.status(200).send("OK") + return next() } getRouter() { diff --git a/src/server.js b/src/server.js index f755f1dee..4d01c7446 100644 --- a/src/server.js +++ b/src/server.js @@ -42,6 +42,7 @@ import ReviewRequestService from "@services/review/ReviewRequestService" import { apiLogger } from "./middleware/apiLogger" import { AuthorizationMiddleware } from "./middleware/authorization" +import { NotificationOnEditHandler } from "./middleware/notificationOnEditHandler" import getAuthenticatedSubrouterV1 from "./routes/v1/authenticated" import getAuthenticatedSitesSubrouterV1 from "./routes/v1/authenticatedSites" import getAuthenticatedSubrouter from "./routes/v2/authenticated" @@ -156,12 +157,19 @@ const authorizationMiddleware = getAuthorizationMiddleware({ isomerAdminsService, collaboratorsService, }) +const notificationOnEditHandler = new NotificationOnEditHandler({ + reviewRequestService, + sitesService, + collaboratorsService, + notificationsService, +}) const reviewRouter = new ReviewsRouter( reviewRequestService, usersService, sitesService, - collaboratorsService + collaboratorsService, + notificationsService ) const authenticatedSubrouterV1 = getAuthenticatedSubrouterV1({ authenticationMiddleware, @@ -185,6 +193,7 @@ const authenticatedSubrouterV2 = getAuthenticatedSubrouter({ collaboratorsService, authorizationMiddleware, reviewRouter, + notificationsService, }) const authenticatedSitesSubrouterV2 = getAuthenticatedSitesSubrouter({ @@ -194,6 +203,7 @@ const authenticatedSitesSubrouterV2 = getAuthenticatedSitesSubrouter({ configYmlService, apiLogger, notificationsService, + notificationOnEditHandler, }) const authV2Router = new AuthRouter({ authenticationMiddleware, authService, apiLogger }) const formsgRouter = new FormsgRouter({ usersService, infraService }) @@ -230,7 +240,7 @@ app.use("/v1", authenticatedSubrouterV1) app.use("/v2/auth", authV2Router.getRouter()) // Endpoints which have require login, but not site access token app.use("/v2", authenticatedSubrouterV2) -// Endpoints which have siteName, used to inject site access token +// Endpoints which modify the github repo, used to inject site access token app.use("/v2/sites/:siteName", authenticatedSitesSubrouterV2) // FormSG Backend handler routes diff --git a/src/services/identity/NotificationsService.ts b/src/services/identity/NotificationsService.ts index 149c4cebb..fcbde7221 100644 --- a/src/services/identity/NotificationsService.ts +++ b/src/services/identity/NotificationsService.ts @@ -171,41 +171,19 @@ class NotificationsService { } async create({ - siteName, - userId, + siteMember, link, notificationType, notificationSourceUsername, }: { - siteName: string - userId: string + siteMember: SiteMember 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, + user_id: siteMember.userId, type: notificationType, created_at: { [Op.gte]: getNotificationExpiryDate(notificationType), @@ -218,31 +196,34 @@ class NotificationsService { model: Site, as: "site", required: true, - include: [ - { - model: Repo, - required: true, - where: { - name: siteName, - }, - }, - ], + where: { + id: siteMember.siteId, + }, }, ], }) if (recentTargetNotification) { // Update existing notification - await recentTargetNotification.update({ - firstReadTime: null, - createdAt: new Date(), - }) + // createdAt is a special column which must be flagged as changed + recentTargetNotification.changed("createdAt", true) + await recentTargetNotification.update( + { + firstReadTime: null, + createdAt: new Date(), + message: getNotificationMessage( + notificationType, + notificationSourceUsername + ), + }, + { raw: true } + ) } else { // Create new notification await this.repository.create({ siteMemberId: siteMember?.id, siteId: siteMember?.siteId, - userId, + userId: siteMember.userId, message: getNotificationMessage( notificationType, notificationSourceUsername diff --git a/src/services/identity/__tests__/NotificationsService.spec.ts b/src/services/identity/__tests__/NotificationsService.spec.ts index 611cd57ca..eb5c296a6 100644 --- a/src/services/identity/__tests__/NotificationsService.spec.ts +++ b/src/services/identity/__tests__/NotificationsService.spec.ts @@ -162,6 +162,10 @@ describe("Notification Service", () => { }) describe("create", () => { + const mockSiteMember = ({ + userId: mockUserId, + siteId: 1, + } as unknown) as SiteMember it("should create a new notification if no similar one exists", async () => { // Arrange MockSiteMember.findOne.mockResolvedValueOnce({ id: mockUserId }) @@ -169,8 +173,7 @@ describe("Notification Service", () => { // Act const actual = NotificationsService.create({ - userId: mockUserId, - siteName: mockSiteName, + siteMember: mockSiteMember, link: "link", notificationType: "sent_request", notificationSourceUsername: "user", @@ -178,7 +181,6 @@ describe("Notification Service", () => { // Assert await expect(actual).resolves.not.toThrow() - expect(MockSiteMember.findOne).toHaveBeenCalledTimes(1) expect(MockRepository.findOne).toHaveBeenCalledTimes(1) expect(MockRepository.create).toHaveBeenCalledTimes(1) }) @@ -189,12 +191,12 @@ describe("Notification Service", () => { MockSiteMember.findOne.mockResolvedValueOnce({ id: mockUserId }) MockRepository.findOne.mockResolvedValueOnce({ update: notificationUpdate, + changed: jest.fn(), }) // Act const actual = NotificationsService.create({ - userId: mockUserId, - siteName: mockSiteName, + siteMember: mockSiteMember, link: "link", notificationType: "sent_request", notificationSourceUsername: "user", @@ -202,7 +204,6 @@ describe("Notification Service", () => { // Assert await expect(actual).resolves.not.toThrow() - expect(MockSiteMember.findOne).toHaveBeenCalledTimes(1) expect(MockRepository.findOne).toHaveBeenCalledTimes(1) expect(notificationUpdate).toHaveBeenCalledTimes(1) }) diff --git a/src/utils/notification-utils.ts b/src/utils/notification-utils.ts index 9b0e0158c..43924b6d2 100644 --- a/src/utils/notification-utils.ts +++ b/src/utils/notification-utils.ts @@ -1,13 +1,23 @@ import moment from "moment" -export type NotificationType = "sent_request" | "updated_request" +export type NotificationType = + | "sent_request" + | "updated_request" + | "request_created" + | "request_approved" + | "request_cancelled" export const getNotificationExpiryDate = ( notificationType: NotificationType ) => { switch (notificationType) { + case "request_created": + case "request_approved": + case "request_cancelled": + // Always notify for review request information + return moment() default: - return moment().subtract(3, "months") + return moment().subtract(3, "hours") } } @@ -17,7 +27,13 @@ export const getNotificationMessage = ( ) => { switch (notificationType) { case "sent_request": + return `${sourceUsername} has sent you a review request.` + case "request_created": return `${sourceUsername} created a review request.` + case "request_approved": + return `${sourceUsername} has approved a review request.` + case "request_cancelled": + return `${sourceUsername} has cancelled a review request.` case "updated_request": return `${sourceUsername} made changes to a review request.` default: