From c90055f716aa484130769b12e7bc87f94a8f63e6 Mon Sep 17 00:00:00 2001 From: Alexander Lee Date: Tue, 17 Jan 2023 13:16:56 +0800 Subject: [PATCH] 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> --- .github/workflows/ci.yml | 2 +- .../NotificationOnEditHandler.spec.ts | 239 ++++++++++++++++++ src/integration/Notifications.spec.ts | 12 +- src/integration/Reviews.spec.ts | 11 +- .../v2/authenticated/__tests__/review.spec.ts | 50 +++- 5 files changed, 306 insertions(+), 8 deletions(-) create mode 100644 src/integration/NotificationOnEditHandler.spec.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b0bba564c..a15d91b15 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 2eabb919c..79b822be5 100644 --- a/src/integration/Notifications.spec.ts +++ b/src/integration/Notifications.spec.ts @@ -39,6 +39,7 @@ import { ConfigYmlService } from "@root/services/fileServices/YmlFileServices/Co 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, @@ -58,7 +59,7 @@ const identityAuthService = getIdentityAuthService(gitHubService) const usersService = getUsersService(sequelize) const configYmlService = new ConfigYmlService({ gitHubService }) const reviewRequestService = new ReviewRequestService( - gitHubService, + (gitHubService as unknown) as typeof ReviewApi, User, ReviewRequest, Reviewer, @@ -109,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, 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/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() }) })