From 342013dee2c7d88202e8f0585492551f4c6ae645 Mon Sep 17 00:00:00 2001 From: Eugene Long Date: Thu, 8 Apr 2021 13:27:43 +0800 Subject: [PATCH 1/6] refactor(settings-api): shard admin routes into separate files --- ...routes.spec.ts => admin-forms.settings.routes.spec.ts} | 6 +++--- ...min-forms.routes.ts => admin-forms.settings.routes.ts} | 6 +++--- src/app/routes/api/v3/admin/forms/index.ts | 8 +++++++- 3 files changed, 13 insertions(+), 7 deletions(-) rename src/app/routes/api/v3/admin/forms/__tests__/{admin-forms.routes.spec.ts => admin-forms.settings.routes.spec.ts} (97%) rename src/app/routes/api/v3/admin/forms/{admin-forms.routes.ts => admin-forms.settings.routes.ts} (92%) diff --git a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.routes.spec.ts b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts similarity index 97% rename from src/app/routes/api/v3/admin/forms/__tests__/admin-forms.routes.spec.ts rename to src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts index 54450827ee..9724297455 100644 --- a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.routes.spec.ts +++ b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts @@ -11,15 +11,15 @@ import { setupApp } from 'tests/integration/helpers/express-setup' import { buildCelebrateError } from 'tests/unit/backend/helpers/celebrate' import dbHandler from 'tests/unit/backend/helpers/jest-db' -import { AdminFormsRouter } from '../admin-forms.routes' +import { AdminFormsSettingsRouter } from '../admin-forms.settings.routes' const UserModel = getUserModel(mongoose) -const app = setupApp('/admin/forms', AdminFormsRouter, { +const app = setupApp('/admin/forms', AdminFormsSettingsRouter, { setupWithAuth: true, }) -describe('admin-form.routes', () => { +describe('admin-form.settings.routes', () => { let request: Session const USER_ID = new ObjectId() diff --git a/src/app/routes/api/v3/admin/forms/admin-forms.routes.ts b/src/app/routes/api/v3/admin/forms/admin-forms.settings.routes.ts similarity index 92% rename from src/app/routes/api/v3/admin/forms/admin-forms.routes.ts rename to src/app/routes/api/v3/admin/forms/admin-forms.settings.routes.ts index f4683a8d77..478db349b7 100644 --- a/src/app/routes/api/v3/admin/forms/admin-forms.routes.ts +++ b/src/app/routes/api/v3/admin/forms/admin-forms.settings.routes.ts @@ -6,10 +6,10 @@ import { SettingsUpdateDto } from '../../../../../../types/api' import { withUserAuthentication } from '../../../../../modules/auth/auth.middlewares' import { handleUpdateSettings } from '../../../../../modules/form/admin-form/admin-form.controller' -export const AdminFormsRouter = Router() +export const AdminFormsSettingsRouter = Router() // All routes in this handler should be protected by authentication. -AdminFormsRouter.use(withUserAuthentication) +AdminFormsSettingsRouter.use(withUserAuthentication) /** * Joi validator for PATCH /forms/:formId/settings route. @@ -52,7 +52,7 @@ const updateSettingsValidator = celebrate({ * @returns 422 when user in session cannot be retrieved from the database * @returns 500 when database error occurs */ -AdminFormsRouter.route('/:formId([a-fA-F0-9]{24})/settings').patch( +AdminFormsSettingsRouter.route('/:formId([a-fA-F0-9]{24})/settings').patch( updateSettingsValidator, handleUpdateSettings, ) diff --git a/src/app/routes/api/v3/admin/forms/index.ts b/src/app/routes/api/v3/admin/forms/index.ts index 7e91afcd16..e5223aecee 100644 --- a/src/app/routes/api/v3/admin/forms/index.ts +++ b/src/app/routes/api/v3/admin/forms/index.ts @@ -1 +1,7 @@ -export { AdminFormsRouter } from './admin-forms.routes' +import { Router } from 'express' + +import { AdminFormsSettingsRouter } from './admin-forms.settings.routes' + +export const AdminFormsRouter = Router() + +AdminFormsRouter.use(AdminFormsSettingsRouter) From 60ced34de3c61eabeacf0f42eb8f522fa8781b67 Mon Sep 17 00:00:00 2001 From: Eugene Long Date: Thu, 8 Apr 2021 14:14:19 +0800 Subject: [PATCH 2/6] refactor(feedback-api): duplicate adminform feedback endpoints to new /api/v3 router - duplicate adminform feedback endpoint functionalities and update them to use the new api v3 routes - update v3 router to use new endpoints - update frontend api calls to use new endpoints --- .../admin-forms.feedback.routes.spec.ts | 508 ++++++++++++++++++ .../forms/admin-forms.feedback.routes.ts | 63 +++ src/app/routes/api/v3/admin/forms/index.ts | 2 + .../services/form-feedback.client.factory.js | 2 +- 4 files changed, 574 insertions(+), 1 deletion(-) create mode 100644 src/app/routes/api/v3/admin/forms/__tests__/admin-forms.feedback.routes.spec.ts create mode 100644 src/app/routes/api/v3/admin/forms/admin-forms.feedback.routes.ts diff --git a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.feedback.routes.spec.ts b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.feedback.routes.spec.ts new file mode 100644 index 0000000000..8d1a44aa02 --- /dev/null +++ b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.feedback.routes.spec.ts @@ -0,0 +1,508 @@ +/* eslint-disable @typescript-eslint/ban-ts-comment */ +import { ObjectId } from 'bson-ext' +import mongoose from 'mongoose' +import supertest, { Session } from 'supertest-session' + +import { getEncryptedFormModel } from 'src/app/models/form.server.model' +import getFormFeedbackModel from 'src/app/models/form_feedback.server.model' +import getUserModel from 'src/app/models/user.server.model' +import { IFormDocument, IUserSchema, ResponseMode, Status } from 'src/types' + +import { + createAuthedSession, + logoutSession, +} from 'tests/integration/helpers/express-auth' +import { setupApp } from 'tests/integration/helpers/express-setup' +import dbHandler from 'tests/unit/backend/helpers/jest-db' +import { jsonParseStringify } from 'tests/unit/backend/helpers/serialize-data' + +import { insertFormFeedback } from '../../../../../../modules/form/public-form/public-form.service' +import { AdminFormsFeedbackRouter } from '../admin-forms.feedback.routes' + +// Prevent rate limiting. +jest.mock('src/app/utils/limit-rate') + +const UserModel = getUserModel(mongoose) +const EncryptFormModel = getEncryptedFormModel(mongoose) +const FormFeedbackModel = getFormFeedbackModel(mongoose) + +const app = setupApp(undefined, AdminFormsFeedbackRouter, { + setupWithAuth: true, +}) + +describe('admin-form.feedback.routes', () => { + let request: Session + let defaultUser: IUserSchema + + beforeAll(async () => await dbHandler.connect()) + beforeEach(async () => { + request = supertest(app) + const { user } = await dbHandler.insertFormCollectionReqs() + // Default all requests to come from authenticated user. + request = await createAuthedSession(user.email, request) + defaultUser = user + }) + afterEach(async () => { + await dbHandler.clearDatabase() + jest.restoreAllMocks() + }) + afterAll(async () => await dbHandler.closeDatabase()) + + describe('GET /:formId/feedback', () => { + let formForFeedback: IFormDocument + beforeEach(async () => { + formForFeedback = (await EncryptFormModel.create({ + title: 'form to view feedback', + admin: defaultUser._id, + publicKey: 'does not matter', + })) as IFormDocument + }) + + it('should return 200 with empty feedback meta when no feedback exists', async () => { + // Act + const response = await request.get(`/${formForFeedback._id}/feedback`) + + // Assert + expect(response.status).toEqual(200) + expect(response.body).toEqual({ + count: 0, + feedback: [], + }) + }) + + it('should return 200 with form feedback meta when feedback exists', async () => { + // Arrange + const formFeedbacks = [ + { formId: formForFeedback._id, rating: 5, comment: 'nice' }, + { formId: formForFeedback._id, rating: 2, comment: 'not nice' }, + ] + await insertFormFeedback(formFeedbacks[0]) + await insertFormFeedback(formFeedbacks[1]) + + // Act + const response = await request.get(`/${formForFeedback._id}/feedback`) + + // Assert + const expected = { + average: ( + formFeedbacks.reduce((a, b) => a + b.rating, 0) / formFeedbacks.length + ).toFixed(2), + count: formFeedbacks.length, + feedback: [ + expect.objectContaining({ + comment: formFeedbacks[0].comment, + rating: formFeedbacks[0].rating, + date: expect.any(String), + dateShort: expect.any(String), + timestamp: expect.any(Number), + index: 1, + }), + expect.objectContaining({ + comment: formFeedbacks[1].comment, + rating: formFeedbacks[1].rating, + date: expect.any(String), + dateShort: expect.any(String), + timestamp: expect.any(Number), + index: 2, + }), + ], + } + expect(response.status).toEqual(200) + expect(response.body).toEqual(expected) + }) + + it('should return 401 when user is not logged in', async () => { + // Arrange + await logoutSession(request) + + // Act + const response = await request.get(`/${formForFeedback._id}/feedback`) + + // Assert + expect(response.status).toEqual(401) + expect(response.body).toEqual({ message: 'User is unauthorized.' }) + }) + + it('should return 403 when user does not have read permissions for form', async () => { + // Arrange + const anotherUser = ( + await dbHandler.insertFormCollectionReqs({ + userId: new ObjectId(), + mailName: 'some-user', + shortName: 'someUser', + }) + ).user + // Form that defaultUser has no access to. + const inaccessibleForm = await EncryptFormModel.create({ + title: 'Collab form', + publicKey: 'some public key', + admin: anotherUser._id, + permissionList: [], + }) + + // Act + const response = await request.get(`/${inaccessibleForm._id}/feedback`) + + // Assert + expect(response.status).toEqual(403) + expect(response.body).toEqual({ + message: expect.stringContaining( + 'not authorized to perform read operation', + ), + }) + }) + + it('should return 404 when form cannot be found', async () => { + // Act + const response = await request.get(`/${new ObjectId()}/feedback`) + + // Assert + expect(response.status).toEqual(404) + expect(response.body).toEqual({ message: 'Form not found' }) + }) + + it('should return 410 when form is already archived', async () => { + // Arrange + const archivedForm = await EncryptFormModel.create({ + title: 'archived form', + status: Status.Archived, + responseMode: ResponseMode.Encrypt, + publicKey: 'does not matter', + admin: defaultUser._id, + }) + + // Act + const response = await request.get(`/${archivedForm._id}/feedback`) + + // Assert + expect(response.status).toEqual(410) + expect(response.body).toEqual({ message: 'Form has been archived' }) + }) + + it('should return 422 when user in session cannot be retrieved from the database', async () => { + // Arrange + // Delete user after login. + await dbHandler.clearCollection(UserModel.collection.name) + + // Act + const response = await request.get(`/${formForFeedback._id}/feedback`) + + // Assert + expect(response.status).toEqual(422) + expect(response.body).toEqual({ message: 'User not found' }) + }) + + it('should return 500 when database error occurs whilst retrieving form feedback', async () => { + // Arrange + // Mock database error + // @ts-ignore + jest.spyOn(FormFeedbackModel, 'find').mockImplementationOnce(() => ({ + sort: jest.fn().mockReturnThis(), + exec: jest + .fn() + .mockRejectedValueOnce(new Error('something went wrong')), + })) + + // Act + const response = await request.get(`/${formForFeedback._id}/feedback`) + + // Assert + expect(response.status).toEqual(500) + expect(response.body).toEqual({ + message: + 'Error: [something went wrong]. Please refresh and try again. If you still need help, email us at form@open.gov.sg.', + }) + }) + }) + + describe('GET /:formId/feedback/count', () => { + let formForFeedback: IFormDocument + beforeEach(async () => { + formForFeedback = (await EncryptFormModel.create({ + title: 'form to view feedback', + admin: defaultUser._id, + publicKey: 'does not matter', + })) as IFormDocument + }) + + it('should return 200 with 0 count when no feedback exists', async () => { + // Act + const response = await request.get( + `/${formForFeedback._id}/feedback/count`, + ) + + // Assert + expect(response.status).toEqual(200) + expect(response.body).toEqual(0) + }) + + it('should return 200 with feedback count when feedback exists', async () => { + // Arrange + const formFeedbacks = [ + { formId: formForFeedback._id, rating: 5, comment: 'nice' }, + { formId: formForFeedback._id, rating: 2, comment: 'not nice' }, + ] + await insertFormFeedback(formFeedbacks[0]) + await insertFormFeedback(formFeedbacks[1]) + + // Act + const response = await request.get( + `/${formForFeedback._id}/feedback/count`, + ) + + // Assert + expect(response.status).toEqual(200) + expect(response.body).toEqual(formFeedbacks.length) + }) + + it('should return 401 when user is not logged in', async () => { + // Arrange + await logoutSession(request) + + // Act + const response = await request.get( + `/${formForFeedback._id}/feedback/count`, + ) + + // Assert + expect(response.status).toEqual(401) + expect(response.body).toEqual({ message: 'User is unauthorized.' }) + }) + + it('should return 403 when user does not have read permissions for form', async () => { + // Arrange + const anotherUser = ( + await dbHandler.insertFormCollectionReqs({ + userId: new ObjectId(), + mailName: 'some-user', + shortName: 'someUser', + }) + ).user + // Form that defaultUser has no access to. + const inaccessibleForm = await EncryptFormModel.create({ + title: 'Collab form', + publicKey: 'some public key', + admin: anotherUser._id, + permissionList: [], + }) + + // Act + const response = await request.get( + `/${inaccessibleForm._id}/feedback/count`, + ) + + // Assert + expect(response.status).toEqual(403) + expect(response.body).toEqual({ + message: expect.stringContaining( + 'not authorized to perform read operation', + ), + }) + }) + + it('should return 404 when form cannot be found', async () => { + // Act + const response = await request.get(`/${new ObjectId()}/feedback/count`) + + // Assert + expect(response.status).toEqual(404) + expect(response.body).toEqual({ message: 'Form not found' }) + }) + + it('should return 410 when form is already archived', async () => { + // Arrange + const archivedForm = await EncryptFormModel.create({ + title: 'archived form', + status: Status.Archived, + responseMode: ResponseMode.Encrypt, + publicKey: 'does not matter', + admin: defaultUser._id, + }) + + // Act + const response = await request.get(`/${archivedForm._id}/feedback/count`) + + // Assert + expect(response.status).toEqual(410) + expect(response.body).toEqual({ message: 'Form has been archived' }) + }) + + it('should return 422 when user in session cannot be retrieved from the database', async () => { + // Arrange + // Delete user after login. + await dbHandler.clearCollection(UserModel.collection.name) + + // Act + const response = await request.get( + `/${formForFeedback._id}/feedback/count`, + ) + + // Assert + expect(response.status).toEqual(422) + expect(response.body).toEqual({ message: 'User not found' }) + }) + + it('should return 500 when database error occurs whilst retrieving form feedback', async () => { + // Arrange + // Mock database error + // @ts-ignore + jest.spyOn(FormFeedbackModel, 'countDocuments').mockReturnValueOnce({ + exec: jest + .fn() + .mockRejectedValueOnce(new Error('something went wrong')), + }) + + // Act + const response = await request.get( + `/${formForFeedback._id}/feedback/count`, + ) + + // Assert + expect(response.status).toEqual(500) + expect(response.body).toEqual({ + message: + 'Error: [something went wrong]. Please refresh and try again. If you still need help, email us at form@open.gov.sg.', + }) + }) + }) + + describe('GET /:formId/feedback/download', () => { + let formForFeedback: IFormDocument + beforeEach(async () => { + formForFeedback = (await EncryptFormModel.create({ + title: 'form to view feedback', + admin: defaultUser._id, + publicKey: 'does not matter', + })) as IFormDocument + }) + + it('should return 200 with feedback stream when feedbacks exist', async () => { + // Arrange + const formFeedbacks = [ + { formId: formForFeedback._id, rating: 5, comment: 'nice' }, + { formId: formForFeedback._id, rating: 2, comment: 'not nice' }, + ] + await insertFormFeedback(formFeedbacks[0]) + await insertFormFeedback(formFeedbacks[1]) + + // Act + const response = await request + .get(`/${formForFeedback._id}/feedback/download`) + .buffer() + .parse((res, cb) => { + let buffer = '' + res.on('data', (chunk) => { + buffer += chunk + }) + res.on('end', () => { + cb(null, JSON.parse(buffer)) + }) + }) + + // Assert + const expected = await FormFeedbackModel.find({ + formId: formForFeedback._id, + }).exec() + expect(response.status).toEqual(200) + expect(response.body).toEqual(jsonParseStringify(expected)) + }) + + it('should return 200 with empty stream when feedbacks do not exist', async () => { + // Act + const response = await request + .get(`/${formForFeedback._id}/feedback/download`) + .buffer() + + // Assert + expect(response.status).toEqual(200) + expect(response.body).toEqual([]) + }) + + it('should return 401 when user is not logged in', async () => { + // Arrange + await logoutSession(request) + + // Act + const response = await request.get( + `/${formForFeedback._id}/feedback/download`, + ) + + // Assert + expect(response.status).toEqual(401) + expect(response.body).toEqual({ message: 'User is unauthorized.' }) + }) + + it('should return 403 when user does not have read permissions for form', async () => { + // Arrange + const anotherUser = ( + await dbHandler.insertFormCollectionReqs({ + userId: new ObjectId(), + mailName: 'some-user', + shortName: 'someUser', + }) + ).user + // Form that defaultUser has no access to. + const inaccessibleForm = await EncryptFormModel.create({ + title: 'Collab form', + publicKey: 'some public key', + admin: anotherUser._id, + permissionList: [], + }) + + // Act + const response = await request.get( + `/${inaccessibleForm._id}/feedback/download`, + ) + + // Assert + expect(response.status).toEqual(403) + expect(response.body).toEqual({ + message: expect.stringContaining( + 'not authorized to perform read operation', + ), + }) + }) + + it('should return 404 when form cannot be found', async () => { + // Act + const response = await request.get(`/${new ObjectId()}/feedback/download`) + + // Assert + expect(response.status).toEqual(404) + expect(response.body).toEqual({ message: 'Form not found' }) + }) + + it('should return 410 when form is already archived', async () => { + // Arrange + const archivedForm = await EncryptFormModel.create({ + title: 'archived form', + status: Status.Archived, + responseMode: ResponseMode.Encrypt, + publicKey: 'does not matter', + admin: defaultUser._id, + }) + + // Act + const response = await request.get( + `/${archivedForm._id}/feedback/download`, + ) + + // Assert + expect(response.status).toEqual(410) + expect(response.body).toEqual({ message: 'Form has been archived' }) + }) + + it('should return 422 when user in session cannot be retrieved from the database', async () => { + // Arrange + // Delete user after login. + await dbHandler.clearCollection(UserModel.collection.name) + + // Act + const response = await request.get(`/${formForFeedback._id}/feedback`) + + // Assert + expect(response.status).toEqual(422) + expect(response.body).toEqual({ message: 'User not found' }) + }) + }) +}) diff --git a/src/app/routes/api/v3/admin/forms/admin-forms.feedback.routes.ts b/src/app/routes/api/v3/admin/forms/admin-forms.feedback.routes.ts new file mode 100644 index 0000000000..9efc692d04 --- /dev/null +++ b/src/app/routes/api/v3/admin/forms/admin-forms.feedback.routes.ts @@ -0,0 +1,63 @@ +import { Router } from 'express' + +import { withUserAuthentication } from '../../../../../modules/auth/auth.middlewares' +import * as AdminFormController from '../../../../../modules/form/admin-form/admin-form.controller' + +export const AdminFormsFeedbackRouter = Router() + +/** + * Retrieve feedback for a public form + * @route GET /api/v3/admim/forms/:formId/feedback + * @security session + * + * @returns 200 with feedback response + * @returns 401 when user does not exist in session + * @returns 403 when user does not have permissions to access form + * @returns 404 when form cannot be found + * @returns 410 when form is archived + * @returns 422 when user in session cannot be retrieved from the database + * @returns 500 when database error occurs + */ +AdminFormsFeedbackRouter.get( + '/:formId([a-fA-F0-9]{24})/feedback', + withUserAuthentication, + AdminFormController.handleGetFormFeedbacks, +) + +/** + * Count the number of feedback for a form + * @route GET /api/v3/admim/forms/{formId}/feedback/count + * @security session + * + * @returns 200 with feedback counts of given form + * @returns 401 when user does not exist in session + * @returns 403 when user does not have permissions to access form + * @returns 404 when form cannot be found + * @returns 410 when form is archived + * @returns 422 when user in session cannot be retrieved from the database + * @returns 500 when database error occurs + */ +AdminFormsFeedbackRouter.get( + '/:formId([a-fA-F0-9]{24})/feedback/count', + withUserAuthentication, + AdminFormController.handleCountFormFeedback, +) + +/** + * Stream download all feedback for a form + * @route GET /api/v3/admim/forms/{formId}/feedback/download + * @security session + * + * @returns 200 with feedback stream + * @returns 401 when user does not exist in session + * @returns 403 when user does not have permissions to access form + * @returns 404 when form cannot be found + * @returns 410 when form is archived + * @returns 422 when user in session cannot be retrieved from the database + * @returns 500 when database or stream error occurs + */ +AdminFormsFeedbackRouter.get( + '/:formId([a-fA-F0-9]{24})/feedback/download', + withUserAuthentication, + AdminFormController.handleStreamFormFeedback, +) diff --git a/src/app/routes/api/v3/admin/forms/index.ts b/src/app/routes/api/v3/admin/forms/index.ts index e5223aecee..7cf19e6ff1 100644 --- a/src/app/routes/api/v3/admin/forms/index.ts +++ b/src/app/routes/api/v3/admin/forms/index.ts @@ -1,7 +1,9 @@ import { Router } from 'express' +import { AdminFormsFeedbackRouter } from './admin-forms.feedback.routes' import { AdminFormsSettingsRouter } from './admin-forms.settings.routes' export const AdminFormsRouter = Router() AdminFormsRouter.use(AdminFormsSettingsRouter) +AdminFormsRouter.use(AdminFormsFeedbackRouter) diff --git a/src/public/modules/forms/services/form-feedback.client.factory.js b/src/public/modules/forms/services/form-feedback.client.factory.js index c2118f1243..2ba6b61760 100644 --- a/src/public/modules/forms/services/form-feedback.client.factory.js +++ b/src/public/modules/forms/services/form-feedback.client.factory.js @@ -30,7 +30,7 @@ angular.module('forms').factory('FormFeedback', ['$q', '$http', FormFeedback]) function FormFeedback($q, $http) { let resourceUrl = '/:formId/feedback' - const feedbackAdminUrl = '/:formId/adminform/feedback' + const feedbackAdminUrl = '/api/v3/admin/forms/:formId/feedback' let feedbackService = { postFeedback: function (params, body) { let deferred = $q.defer() From 8d893e35a02cec5ba98d31957f147fb8e7c7727d Mon Sep 17 00:00:00 2001 From: Eugene Long Date: Fri, 9 Apr 2021 11:28:53 +0800 Subject: [PATCH 3/6] fix(feedback-api): add auth middleware back to main admin router --- src/app/routes/api/v3/admin/admin.routes.ts | 5 +++++ .../routes/api/v3/admin/forms/admin-forms.settings.routes.ts | 4 ---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/app/routes/api/v3/admin/admin.routes.ts b/src/app/routes/api/v3/admin/admin.routes.ts index 352e965683..8e9767ad21 100644 --- a/src/app/routes/api/v3/admin/admin.routes.ts +++ b/src/app/routes/api/v3/admin/admin.routes.ts @@ -1,7 +1,12 @@ import { Router } from 'express' +import { withUserAuthentication } from '../../../../modules/auth/auth.middlewares' + import { AdminFormsRouter } from './forms' export const AdminRouter = Router() +// All routes in this handler should be protected by authentication. +AdminRouter.use(withUserAuthentication) + AdminRouter.use('/forms', AdminFormsRouter) diff --git a/src/app/routes/api/v3/admin/forms/admin-forms.settings.routes.ts b/src/app/routes/api/v3/admin/forms/admin-forms.settings.routes.ts index 478db349b7..54e2677cf9 100644 --- a/src/app/routes/api/v3/admin/forms/admin-forms.settings.routes.ts +++ b/src/app/routes/api/v3/admin/forms/admin-forms.settings.routes.ts @@ -3,14 +3,10 @@ import { Router } from 'express' import { AuthType, Status } from '../../../../../../types' import { SettingsUpdateDto } from '../../../../../../types/api' -import { withUserAuthentication } from '../../../../../modules/auth/auth.middlewares' import { handleUpdateSettings } from '../../../../../modules/form/admin-form/admin-form.controller' export const AdminFormsSettingsRouter = Router() -// All routes in this handler should be protected by authentication. -AdminFormsSettingsRouter.use(withUserAuthentication) - /** * Joi validator for PATCH /forms/:formId/settings route. */ From 8b5b63e173a64a122c5ce373bf271396c8fafdc0 Mon Sep 17 00:00:00 2001 From: Eugene Long Date: Fri, 9 Apr 2021 11:30:12 +0800 Subject: [PATCH 4/6] test(feedback-api): update test router to use real root route --- .../admin-forms.feedback.routes.spec.ts | 78 ++++++++++++------- 1 file changed, 51 insertions(+), 27 deletions(-) diff --git a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.feedback.routes.spec.ts b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.feedback.routes.spec.ts index 8d1a44aa02..ecc0374c24 100644 --- a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.feedback.routes.spec.ts +++ b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.feedback.routes.spec.ts @@ -26,7 +26,7 @@ const UserModel = getUserModel(mongoose) const EncryptFormModel = getEncryptedFormModel(mongoose) const FormFeedbackModel = getFormFeedbackModel(mongoose) -const app = setupApp(undefined, AdminFormsFeedbackRouter, { +const app = setupApp('/admin/forms', AdminFormsFeedbackRouter, { setupWithAuth: true, }) @@ -48,7 +48,7 @@ describe('admin-form.feedback.routes', () => { }) afterAll(async () => await dbHandler.closeDatabase()) - describe('GET /:formId/feedback', () => { + describe('GET /admin/forms/:formId/feedback', () => { let formForFeedback: IFormDocument beforeEach(async () => { formForFeedback = (await EncryptFormModel.create({ @@ -60,7 +60,9 @@ describe('admin-form.feedback.routes', () => { it('should return 200 with empty feedback meta when no feedback exists', async () => { // Act - const response = await request.get(`/${formForFeedback._id}/feedback`) + const response = await request.get( + `/admin/forms/${formForFeedback._id}/feedback`, + ) // Assert expect(response.status).toEqual(200) @@ -80,7 +82,9 @@ describe('admin-form.feedback.routes', () => { await insertFormFeedback(formFeedbacks[1]) // Act - const response = await request.get(`/${formForFeedback._id}/feedback`) + const response = await request.get( + `/admin/forms/${formForFeedback._id}/feedback`, + ) // Assert const expected = { @@ -116,7 +120,9 @@ describe('admin-form.feedback.routes', () => { await logoutSession(request) // Act - const response = await request.get(`/${formForFeedback._id}/feedback`) + const response = await request.get( + `/admin/forms/${formForFeedback._id}/feedback`, + ) // Assert expect(response.status).toEqual(401) @@ -141,7 +147,9 @@ describe('admin-form.feedback.routes', () => { }) // Act - const response = await request.get(`/${inaccessibleForm._id}/feedback`) + const response = await request.get( + `/admin/forms/${inaccessibleForm._id}/feedback`, + ) // Assert expect(response.status).toEqual(403) @@ -154,7 +162,9 @@ describe('admin-form.feedback.routes', () => { it('should return 404 when form cannot be found', async () => { // Act - const response = await request.get(`/${new ObjectId()}/feedback`) + const response = await request.get( + `/admin/forms/${new ObjectId()}/feedback`, + ) // Assert expect(response.status).toEqual(404) @@ -172,7 +182,9 @@ describe('admin-form.feedback.routes', () => { }) // Act - const response = await request.get(`/${archivedForm._id}/feedback`) + const response = await request.get( + `/admin/forms/${archivedForm._id}/feedback`, + ) // Assert expect(response.status).toEqual(410) @@ -185,7 +197,9 @@ describe('admin-form.feedback.routes', () => { await dbHandler.clearCollection(UserModel.collection.name) // Act - const response = await request.get(`/${formForFeedback._id}/feedback`) + const response = await request.get( + `/admin/forms/${formForFeedback._id}/feedback`, + ) // Assert expect(response.status).toEqual(422) @@ -204,7 +218,9 @@ describe('admin-form.feedback.routes', () => { })) // Act - const response = await request.get(`/${formForFeedback._id}/feedback`) + const response = await request.get( + `/admin/forms/${formForFeedback._id}/feedback`, + ) // Assert expect(response.status).toEqual(500) @@ -215,7 +231,7 @@ describe('admin-form.feedback.routes', () => { }) }) - describe('GET /:formId/feedback/count', () => { + describe('GET /admin/forms/:formId/feedback/count', () => { let formForFeedback: IFormDocument beforeEach(async () => { formForFeedback = (await EncryptFormModel.create({ @@ -228,7 +244,7 @@ describe('admin-form.feedback.routes', () => { it('should return 200 with 0 count when no feedback exists', async () => { // Act const response = await request.get( - `/${formForFeedback._id}/feedback/count`, + `/admin/forms/${formForFeedback._id}/feedback/count`, ) // Assert @@ -247,7 +263,7 @@ describe('admin-form.feedback.routes', () => { // Act const response = await request.get( - `/${formForFeedback._id}/feedback/count`, + `/admin/forms/${formForFeedback._id}/feedback/count`, ) // Assert @@ -261,7 +277,7 @@ describe('admin-form.feedback.routes', () => { // Act const response = await request.get( - `/${formForFeedback._id}/feedback/count`, + `/admin/forms/${formForFeedback._id}/feedback/count`, ) // Assert @@ -288,7 +304,7 @@ describe('admin-form.feedback.routes', () => { // Act const response = await request.get( - `/${inaccessibleForm._id}/feedback/count`, + `/admin/forms/${inaccessibleForm._id}/feedback/count`, ) // Assert @@ -302,7 +318,9 @@ describe('admin-form.feedback.routes', () => { it('should return 404 when form cannot be found', async () => { // Act - const response = await request.get(`/${new ObjectId()}/feedback/count`) + const response = await request.get( + `/admin/forms/${new ObjectId()}/feedback/count`, + ) // Assert expect(response.status).toEqual(404) @@ -320,7 +338,9 @@ describe('admin-form.feedback.routes', () => { }) // Act - const response = await request.get(`/${archivedForm._id}/feedback/count`) + const response = await request.get( + `/admin/forms/${archivedForm._id}/feedback/count`, + ) // Assert expect(response.status).toEqual(410) @@ -334,7 +354,7 @@ describe('admin-form.feedback.routes', () => { // Act const response = await request.get( - `/${formForFeedback._id}/feedback/count`, + `/admin/forms/${formForFeedback._id}/feedback/count`, ) // Assert @@ -354,7 +374,7 @@ describe('admin-form.feedback.routes', () => { // Act const response = await request.get( - `/${formForFeedback._id}/feedback/count`, + `/admin/forms/${formForFeedback._id}/feedback/count`, ) // Assert @@ -366,7 +386,7 @@ describe('admin-form.feedback.routes', () => { }) }) - describe('GET /:formId/feedback/download', () => { + describe('GET /admin/forms/:formId/feedback/download', () => { let formForFeedback: IFormDocument beforeEach(async () => { formForFeedback = (await EncryptFormModel.create({ @@ -387,7 +407,7 @@ describe('admin-form.feedback.routes', () => { // Act const response = await request - .get(`/${formForFeedback._id}/feedback/download`) + .get(`/admin/forms/${formForFeedback._id}/feedback/download`) .buffer() .parse((res, cb) => { let buffer = '' @@ -410,7 +430,7 @@ describe('admin-form.feedback.routes', () => { it('should return 200 with empty stream when feedbacks do not exist', async () => { // Act const response = await request - .get(`/${formForFeedback._id}/feedback/download`) + .get(`/admin/forms/${formForFeedback._id}/feedback/download`) .buffer() // Assert @@ -424,7 +444,7 @@ describe('admin-form.feedback.routes', () => { // Act const response = await request.get( - `/${formForFeedback._id}/feedback/download`, + `/admin/forms/${formForFeedback._id}/feedback/download`, ) // Assert @@ -451,7 +471,7 @@ describe('admin-form.feedback.routes', () => { // Act const response = await request.get( - `/${inaccessibleForm._id}/feedback/download`, + `/admin/forms/${inaccessibleForm._id}/feedback/download`, ) // Assert @@ -465,7 +485,9 @@ describe('admin-form.feedback.routes', () => { it('should return 404 when form cannot be found', async () => { // Act - const response = await request.get(`/${new ObjectId()}/feedback/download`) + const response = await request.get( + `/admin/forms/${new ObjectId()}/feedback/download`, + ) // Assert expect(response.status).toEqual(404) @@ -484,7 +506,7 @@ describe('admin-form.feedback.routes', () => { // Act const response = await request.get( - `/${archivedForm._id}/feedback/download`, + `/admin/forms/${archivedForm._id}/feedback/download`, ) // Assert @@ -498,7 +520,9 @@ describe('admin-form.feedback.routes', () => { await dbHandler.clearCollection(UserModel.collection.name) // Act - const response = await request.get(`/${formForFeedback._id}/feedback`) + const response = await request.get( + `/admin/forms/${formForFeedback._id}/feedback`, + ) // Assert expect(response.status).toEqual(422) From 5e0836ef939e9a65eacd7a646fdb18e3a0e170a0 Mon Sep 17 00:00:00 2001 From: Eugene Long Date: Fri, 9 Apr 2021 15:23:02 +0800 Subject: [PATCH 5/6] refactor(forms-routes): fix tests and clean up endpoints - remove duplicated auth middleware from feedback endpoints - import index level router for form-route tests to account for shifted auth middleware - shard index into admin-forms.routes and index to follow convention --- src/app/routes/api/v3/admin/admin.routes.ts | 5 ----- .../__tests__/admin-forms.feedback.routes.spec.ts | 4 ++-- .../__tests__/admin-forms.settings.routes.spec.ts | 4 ++-- .../v3/admin/forms/admin-forms.feedback.routes.ts | 4 ---- .../api/v3/admin/forms/admin-forms.routes.ts | 14 ++++++++++++++ src/app/routes/api/v3/admin/forms/index.ts | 10 +--------- 6 files changed, 19 insertions(+), 22 deletions(-) create mode 100644 src/app/routes/api/v3/admin/forms/admin-forms.routes.ts diff --git a/src/app/routes/api/v3/admin/admin.routes.ts b/src/app/routes/api/v3/admin/admin.routes.ts index 8e9767ad21..352e965683 100644 --- a/src/app/routes/api/v3/admin/admin.routes.ts +++ b/src/app/routes/api/v3/admin/admin.routes.ts @@ -1,12 +1,7 @@ import { Router } from 'express' -import { withUserAuthentication } from '../../../../modules/auth/auth.middlewares' - import { AdminFormsRouter } from './forms' export const AdminRouter = Router() -// All routes in this handler should be protected by authentication. -AdminRouter.use(withUserAuthentication) - AdminRouter.use('/forms', AdminFormsRouter) diff --git a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.feedback.routes.spec.ts b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.feedback.routes.spec.ts index ecc0374c24..36d3522c8f 100644 --- a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.feedback.routes.spec.ts +++ b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.feedback.routes.spec.ts @@ -17,7 +17,7 @@ import dbHandler from 'tests/unit/backend/helpers/jest-db' import { jsonParseStringify } from 'tests/unit/backend/helpers/serialize-data' import { insertFormFeedback } from '../../../../../../modules/form/public-form/public-form.service' -import { AdminFormsFeedbackRouter } from '../admin-forms.feedback.routes' +import { AdminFormsRouter } from '../admin-forms.routes' // Prevent rate limiting. jest.mock('src/app/utils/limit-rate') @@ -26,7 +26,7 @@ const UserModel = getUserModel(mongoose) const EncryptFormModel = getEncryptedFormModel(mongoose) const FormFeedbackModel = getFormFeedbackModel(mongoose) -const app = setupApp('/admin/forms', AdminFormsFeedbackRouter, { +const app = setupApp('/admin/forms', AdminFormsRouter, { setupWithAuth: true, }) diff --git a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts index 9724297455..c6efd1968c 100644 --- a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts +++ b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts @@ -11,11 +11,11 @@ import { setupApp } from 'tests/integration/helpers/express-setup' import { buildCelebrateError } from 'tests/unit/backend/helpers/celebrate' import dbHandler from 'tests/unit/backend/helpers/jest-db' -import { AdminFormsSettingsRouter } from '../admin-forms.settings.routes' +import { AdminFormsRouter } from '../admin-forms.routes' const UserModel = getUserModel(mongoose) -const app = setupApp('/admin/forms', AdminFormsSettingsRouter, { +const app = setupApp('/admin/forms', AdminFormsRouter, { setupWithAuth: true, }) diff --git a/src/app/routes/api/v3/admin/forms/admin-forms.feedback.routes.ts b/src/app/routes/api/v3/admin/forms/admin-forms.feedback.routes.ts index 9efc692d04..f984fb5888 100644 --- a/src/app/routes/api/v3/admin/forms/admin-forms.feedback.routes.ts +++ b/src/app/routes/api/v3/admin/forms/admin-forms.feedback.routes.ts @@ -1,6 +1,5 @@ import { Router } from 'express' -import { withUserAuthentication } from '../../../../../modules/auth/auth.middlewares' import * as AdminFormController from '../../../../../modules/form/admin-form/admin-form.controller' export const AdminFormsFeedbackRouter = Router() @@ -20,7 +19,6 @@ export const AdminFormsFeedbackRouter = Router() */ AdminFormsFeedbackRouter.get( '/:formId([a-fA-F0-9]{24})/feedback', - withUserAuthentication, AdminFormController.handleGetFormFeedbacks, ) @@ -39,7 +37,6 @@ AdminFormsFeedbackRouter.get( */ AdminFormsFeedbackRouter.get( '/:formId([a-fA-F0-9]{24})/feedback/count', - withUserAuthentication, AdminFormController.handleCountFormFeedback, ) @@ -58,6 +55,5 @@ AdminFormsFeedbackRouter.get( */ AdminFormsFeedbackRouter.get( '/:formId([a-fA-F0-9]{24})/feedback/download', - withUserAuthentication, AdminFormController.handleStreamFormFeedback, ) diff --git a/src/app/routes/api/v3/admin/forms/admin-forms.routes.ts b/src/app/routes/api/v3/admin/forms/admin-forms.routes.ts new file mode 100644 index 0000000000..bb466328f1 --- /dev/null +++ b/src/app/routes/api/v3/admin/forms/admin-forms.routes.ts @@ -0,0 +1,14 @@ +import { Router } from 'express' + +import { withUserAuthentication } from '../../../../../modules/auth/auth.middlewares' + +import { AdminFormsFeedbackRouter } from './admin-forms.feedback.routes' +import { AdminFormsSettingsRouter } from './admin-forms.settings.routes' + +export const AdminFormsRouter = Router() + +// All routes in this handler should be protected by authentication. +AdminFormsRouter.use(withUserAuthentication) + +AdminFormsRouter.use(AdminFormsSettingsRouter) +AdminFormsRouter.use(AdminFormsFeedbackRouter) diff --git a/src/app/routes/api/v3/admin/forms/index.ts b/src/app/routes/api/v3/admin/forms/index.ts index 7cf19e6ff1..7e91afcd16 100644 --- a/src/app/routes/api/v3/admin/forms/index.ts +++ b/src/app/routes/api/v3/admin/forms/index.ts @@ -1,9 +1 @@ -import { Router } from 'express' - -import { AdminFormsFeedbackRouter } from './admin-forms.feedback.routes' -import { AdminFormsSettingsRouter } from './admin-forms.settings.routes' - -export const AdminFormsRouter = Router() - -AdminFormsRouter.use(AdminFormsSettingsRouter) -AdminFormsRouter.use(AdminFormsFeedbackRouter) +export { AdminFormsRouter } from './admin-forms.routes' From 1373753ed55f5b253ad8412c40adce1de96cb061 Mon Sep 17 00:00:00 2001 From: Eugene Long Date: Tue, 13 Apr 2021 13:06:19 +0800 Subject: [PATCH 6/6] ref(feedback-api): rename handle-get-form-feedbacks to feedback --- .../__tests__/admin-form.controller.spec.ts | 18 +++++++++--------- .../form/admin-form/admin-form.controller.ts | 4 ++-- .../form/admin-form/admin-form.routes.ts | 2 +- .../admin/forms/admin-forms.feedback.routes.ts | 2 +- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts index 0808e5140a..f16bc12b7c 100644 --- a/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts +++ b/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts @@ -2324,7 +2324,7 @@ describe('admin-form.controller', () => { }) }) - describe('handleGetFormFeedbacks', () => { + describe('handleGetFormFeedback', () => { const MOCK_USER_ID = new ObjectId().toHexString() const MOCK_FORM_ID = new ObjectId().toHexString() const MOCK_USER = { @@ -2377,7 +2377,7 @@ describe('admin-form.controller', () => { ) // Act - await AdminFormController.handleGetFormFeedbacks( + await AdminFormController.handleGetFormFeedback( MOCK_REQ, mockRes, jest.fn(), @@ -2412,7 +2412,7 @@ describe('admin-form.controller', () => { ) // Act - await AdminFormController.handleGetFormFeedbacks( + await AdminFormController.handleGetFormFeedback( MOCK_REQ, mockRes, jest.fn(), @@ -2446,7 +2446,7 @@ describe('admin-form.controller', () => { ) // Act - await AdminFormController.handleGetFormFeedbacks( + await AdminFormController.handleGetFormFeedback( MOCK_REQ, mockRes, jest.fn(), @@ -2480,7 +2480,7 @@ describe('admin-form.controller', () => { ) // Act - await AdminFormController.handleGetFormFeedbacks( + await AdminFormController.handleGetFormFeedback( MOCK_REQ, mockRes, jest.fn(), @@ -2511,7 +2511,7 @@ describe('admin-form.controller', () => { ) // Act - await AdminFormController.handleGetFormFeedbacks( + await AdminFormController.handleGetFormFeedback( MOCK_REQ, mockRes, jest.fn(), @@ -2538,7 +2538,7 @@ describe('admin-form.controller', () => { ) // Act - await AdminFormController.handleGetFormFeedbacks( + await AdminFormController.handleGetFormFeedback( MOCK_REQ, mockRes, jest.fn(), @@ -2568,7 +2568,7 @@ describe('admin-form.controller', () => { ) // Act - await AdminFormController.handleGetFormFeedbacks( + await AdminFormController.handleGetFormFeedback( MOCK_REQ, mockRes, jest.fn(), @@ -2606,7 +2606,7 @@ describe('admin-form.controller', () => { ) // Act - await AdminFormController.handleGetFormFeedbacks( + await AdminFormController.handleGetFormFeedback( MOCK_REQ, mockRes, jest.fn(), diff --git a/src/app/modules/form/admin-form/admin-form.controller.ts b/src/app/modules/form/admin-form/admin-form.controller.ts index 319204363d..67b287b24d 100644 --- a/src/app/modules/form/admin-form/admin-form.controller.ts +++ b/src/app/modules/form/admin-form/admin-form.controller.ts @@ -531,7 +531,7 @@ export const handleStreamFormFeedback: RequestHandler<{ * @returns 422 when user in session cannot be retrieved from the database * @returns 500 when database error occurs */ -export const handleGetFormFeedbacks: RequestHandler<{ +export const handleGetFormFeedback: RequestHandler<{ formId: string }> = (req, res) => { const { formId } = req.params @@ -551,7 +551,7 @@ export const handleGetFormFeedbacks: RequestHandler<{ logger.error({ message: 'Error retrieving form feedbacks', meta: { - action: 'handleGetFormFeedbacks', + action: 'handleGetFormFeedback', ...createReqMeta(req), userId: sessionUserId, formId, diff --git a/src/app/modules/form/admin-form/admin-form.routes.ts b/src/app/modules/form/admin-form/admin-form.routes.ts index 934d559a9f..a8b4ff6292 100644 --- a/src/app/modules/form/admin-form/admin-form.routes.ts +++ b/src/app/modules/form/admin-form/admin-form.routes.ts @@ -291,7 +291,7 @@ AdminFormsRouter.get( AdminFormsRouter.get( '/:formId([a-fA-F0-9]{24})/adminform/feedback', withUserAuthentication, - AdminFormController.handleGetFormFeedbacks, + AdminFormController.handleGetFormFeedback, ) /** diff --git a/src/app/routes/api/v3/admin/forms/admin-forms.feedback.routes.ts b/src/app/routes/api/v3/admin/forms/admin-forms.feedback.routes.ts index f984fb5888..f7db87c640 100644 --- a/src/app/routes/api/v3/admin/forms/admin-forms.feedback.routes.ts +++ b/src/app/routes/api/v3/admin/forms/admin-forms.feedback.routes.ts @@ -19,7 +19,7 @@ export const AdminFormsFeedbackRouter = Router() */ AdminFormsFeedbackRouter.get( '/:formId([a-fA-F0-9]{24})/feedback', - AdminFormController.handleGetFormFeedbacks, + AdminFormController.handleGetFormFeedback, ) /**