From 6a64b504b65f3204f8d3cfa2eb54683918a224f0 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Tue, 24 Nov 2020 17:46:46 +0800 Subject: [PATCH 01/12] feat(FeedbackModel): add static method getFeedbackCursorByFormId --- src/app/models/form_feedback.server.model.ts | 14 ++++++++++++++ src/types/form_feedback.ts | 11 +++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/app/models/form_feedback.server.model.ts b/src/app/models/form_feedback.server.model.ts index 97055d281d..3f42609a83 100644 --- a/src/app/models/form_feedback.server.model.ts +++ b/src/app/models/form_feedback.server.model.ts @@ -34,6 +34,20 @@ const FormFeedbackSchema = new Schema( }, ) +/** + * Returns a cursor for all feedback for the form with formId. + * @param formId the form id to return the submissions cursor for + * @returns a cursor to the feedback retrieved + */ +const getFeedbackCursorByFormId: IFormFeedbackModel['getFeedbackCursorByFormId'] = function ( + this: IFormFeedbackModel, + formId, +) { + return this.find({ formId }).batchSize(2000).read('secondary').lean().cursor() +} + +FormFeedbackSchema.statics.getFeedbackCursorByFormId = getFeedbackCursorByFormId + /** * Form Feedback Schema * @param db Active DB Connection diff --git a/src/types/form_feedback.ts b/src/types/form_feedback.ts index f9cae5133f..2bb1bde5e3 100644 --- a/src/types/form_feedback.ts +++ b/src/types/form_feedback.ts @@ -1,4 +1,4 @@ -import { Document, Model } from 'mongoose' +import { Document, Model, QueryCursor } from 'mongoose' import { IFormSchema } from './form' @@ -24,4 +24,11 @@ export interface IFormFeedbackDoc extends IFormFeedbackSchema { created: Date } -export type IFormFeedbackModel = Model +export interface IFormFeedbackModel extends Model { + /** + * Returns a cursor for all feedback for the form with formId. + * @param formId the form id to return the submissions cursor for + * @returns a cursor to the feedback retrieved + */ + getFeedbackCursorByFormId(formId: string): QueryCursor +} From b1d9a375baa75c90f9ff76eace679b4a653787b6 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Wed, 25 Nov 2020 11:58:11 +0800 Subject: [PATCH 02/12] feat(FeedbackSvc): add getFormFeedbackStream fn --- .../__tests__/feedback.service.spec.ts | 18 ++++++++++++++++++ src/app/modules/feedback/feedback.service.ts | 12 ++++++++++++ 2 files changed, 30 insertions(+) diff --git a/src/app/modules/feedback/__tests__/feedback.service.spec.ts b/src/app/modules/feedback/__tests__/feedback.service.spec.ts index 3aa0b8f9fe..4dd4730873 100644 --- a/src/app/modules/feedback/__tests__/feedback.service.spec.ts +++ b/src/app/modules/feedback/__tests__/feedback.service.spec.ts @@ -85,4 +85,22 @@ describe('feedback.service', () => { expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(DatabaseError) }) }) + + describe('getFormFeedbackStream', () => { + it('should return stream successfully', async () => { + // Arrange + const mockFormId = 'some form id' + const mockCursor = ('some cursor' as unknown) as mongoose.QueryCursor + const streamSpy = jest + .spyOn(FormFeedback, 'getFeedbackCursorByFormId') + .mockReturnValue(mockCursor) + + // Act + const actual = FeedbackService.getFormFeedbackStream(mockFormId) + + // Assert + expect(actual).toEqual(mockCursor) + expect(streamSpy).toHaveBeenCalledWith(mockFormId) + }) + }) }) diff --git a/src/app/modules/feedback/feedback.service.ts b/src/app/modules/feedback/feedback.service.ts index 56ba093edc..6bada6b8aa 100644 --- a/src/app/modules/feedback/feedback.service.ts +++ b/src/app/modules/feedback/feedback.service.ts @@ -2,6 +2,7 @@ import mongoose from 'mongoose' import { ResultAsync } from 'neverthrow' import { createLoggerWithLabel } from '../../../config/logger' +import { IFormFeedbackSchema } from '../../../types' import getFormFeedbackModel from '../../models/form_feedback.server.model' import { getMongoErrorMessage } from '../../utils/handle-mongo-error' import { DatabaseError } from '../core/core.errors' @@ -35,3 +36,14 @@ export const getFormFeedbackCount = ( }, ) } + +/** + * Retrieves form feedback stream. + * @param formId the formId of form to retrieve feedback for + * @returns form feedback stream + */ +export const getFormFeedbackStream = ( + formId: string, +): mongoose.QueryCursor => { + return FormFeedbackModel.getFeedbackCursorByFormId(formId) +} From a967986b6a4254c2fd64fb6b23421d675ab8b57b Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Wed, 25 Nov 2020 13:13:02 +0800 Subject: [PATCH 03/12] feat(AdminFormCtl): add handleStreamFormFeedback fn --- .../form/admin-form/admin-form.controller.ts | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) 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 0603b728e8..b86a83dd8b 100644 --- a/src/app/modules/form/admin-form/admin-form.controller.ts +++ b/src/app/modules/form/admin-form/admin-form.controller.ts @@ -1,5 +1,7 @@ import { RequestHandler } from 'express' import { ParamsDictionary } from 'express-serve-static-core' +import { StatusCodes } from 'http-status-codes' +import JSONStream from 'JSONStream' import { createLoggerWithLabel } from '../../../../config/logger' import { AuthType, WithForm } from '../../../../types' @@ -304,3 +306,88 @@ export const handleCountFormFeedback: RequestHandler<{ }) ) } + +export const handleStreamFormFeedback: RequestHandler<{ + formId: string +}> = async (req, res) => { + const { formId } = req.params + const sessionUserId = (req.session as Express.AuthedSession).user._id + + // Step 1: Retrieve currently logged in user. + const hasReadPermissionResult = await UserService.getPopulatedUserById( + sessionUserId, + ).andThen((user) => + // Step 2: Retrieve full form. + FormService.retrieveFullFormById(formId).andThen((fullForm) => + // Step 3: Check whether form is active. + assertFormAvailable(fullForm).andThen(() => + // Step 4: Check whether current user has read permissions to form. + assertHasReadPermissions(user, fullForm), + ), + ), + ) + + const logMeta = { + action: 'handleStreamFormFeedback', + ...createReqMeta(req), + userId: sessionUserId, + formId, + } + + if (hasReadPermissionResult.isErr()) { + logger.error({ + message: 'Error occurred whilst verifying user permissions', + meta: logMeta, + error: hasReadPermissionResult.error, + }) + const { errorMessage, statusCode } = mapRouteError( + hasReadPermissionResult.error, + ) + return res.status(statusCode).json({ message: errorMessage }) + } + + // No errors, start stream. + const cursor = FeedbackService.getFormFeedbackStream(formId) + + cursor + .on('error', (error) => { + logger.error({ + message: 'Error streaming feedback from MongoDB', + meta: logMeta, + error, + }) + return res.status(StatusCodes.INTERNAL_SERVER_ERROR).json({ + message: 'Error retrieving from database.', + }) + }) + .pipe(JSONStream.stringify()) + .on('error', (error) => { + logger.error({ + message: 'Error converting feedback to JSON', + meta: logMeta, + error, + }) + return res.status(StatusCodes.INTERNAL_SERVER_ERROR).json({ + message: 'Error converting feedback to JSON', + }) + }) + .pipe(res.type('json')) + .on('error', (error) => { + logger.error({ + message: 'Error writing feedback to HTTP stream', + meta: logMeta, + error, + }) + return res.status(StatusCodes.INTERNAL_SERVER_ERROR).json({ + message: 'Error writing feedback to HTTP stream', + }) + }) + .on('close', () => { + logger.info({ + message: 'Stream feedback closed', + meta: logMeta, + }) + + return res.end() + }) +} From 3860e347daab1defc986dc90eadff674fe54170e Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Wed, 25 Nov 2020 13:45:26 +0800 Subject: [PATCH 04/12] feat(utils): add exhaustive switch case typeguard --- src/app/modules/form/form.service.ts | 15 ++++++--------- .../__tests__/submission/submission.utils.spec.ts | 6 ++++-- src/app/modules/submission/submission.utils.ts | 7 +++++-- src/app/utils/assert-unreachable.ts | 10 ++++++++++ 4 files changed, 25 insertions(+), 13 deletions(-) create mode 100644 src/app/utils/assert-unreachable.ts diff --git a/src/app/modules/form/form.service.ts b/src/app/modules/form/form.service.ts index 4e390a53fe..136e2bf4a3 100644 --- a/src/app/modules/form/form.service.ts +++ b/src/app/modules/form/form.service.ts @@ -4,6 +4,7 @@ import { err, errAsync, ok, okAsync, Result, ResultAsync } from 'neverthrow' import { createLoggerWithLabel } from '../../../config/logger' import { IFormSchema, IPopulatedForm, Status } from '../../../types' import getFormModel from '../../models/form.server.model' +import { assertUnreachable } from '../../utils/assert-unreachable' import { ApplicationError, DatabaseError } from '../core/core.errors' import { @@ -67,6 +68,10 @@ export const retrieveFullFormById = ( export const isFormPublic = ( form: IPopulatedForm, ): Result => { + if (!form.status) { + return err(new ApplicationError()) + } + switch (form.status) { case Status.Public: return ok(true) @@ -75,14 +80,6 @@ export const isFormPublic = ( case Status.Private: return err(new PrivateFormError(form.inactiveMessage)) default: - logger.error({ - message: 'Encountered invalid form status', - meta: { - action: 'isFormPublic', - formStatus: form.status, - form, - }, - }) - return err(new ApplicationError()) + return assertUnreachable(form.status) } } diff --git a/src/app/modules/submission/__tests__/submission/submission.utils.spec.ts b/src/app/modules/submission/__tests__/submission/submission.utils.spec.ts index 0e7e9ed8b0..fe448f4340 100644 --- a/src/app/modules/submission/__tests__/submission/submission.utils.spec.ts +++ b/src/app/modules/submission/__tests__/submission/submission.utils.spec.ts @@ -46,9 +46,11 @@ describe('submission.utils', () => { }) it('should throw error if called with invalid responseMode', async () => { + // Arrange + const invalidResponseMode = 'something' as ResponseMode // Act + Assert - expect(() => getModeFilter(undefined!)).toThrowError( - 'getResponsesForEachField: Invalid response mode parameter', + expect(() => getModeFilter(invalidResponseMode)).toThrowError( + `This should never be reached in TypeScript: "${invalidResponseMode}"`, ) }) }) diff --git a/src/app/modules/submission/submission.utils.ts b/src/app/modules/submission/submission.utils.ts index 2dce7da9ac..39aceea9cf 100644 --- a/src/app/modules/submission/submission.utils.ts +++ b/src/app/modules/submission/submission.utils.ts @@ -1,18 +1,21 @@ import { BasicField, ResponseMode } from '../../../types' +import { assertUnreachable } from '../../utils/assert-unreachable' import { FIELDS_TO_REJECT } from '../../utils/field-validation/config' type ModeFilterParam = { fieldType: BasicField } -export const getModeFilter = (responseMode: ResponseMode) => { +export const getModeFilter = ( + responseMode: ResponseMode, +): ((responses: T[]) => T[]) => { switch (responseMode) { case ResponseMode.Email: return emailModeFilter case ResponseMode.Encrypt: return encryptModeFilter default: - throw Error('getResponsesForEachField: Invalid response mode parameter') + return assertUnreachable(responseMode) } } diff --git a/src/app/utils/assert-unreachable.ts b/src/app/utils/assert-unreachable.ts new file mode 100644 index 0000000000..bb4fca757f --- /dev/null +++ b/src/app/utils/assert-unreachable.ts @@ -0,0 +1,10 @@ +/** + * Typescript type guard that asserts that all switch cases are exhaustive. + * Use to get compile-time safety for making sure all the cases are handled. + * + * See: + * https://stackoverflow.com/questions/39419170/how-do-i-check-that-a-switch-block-is-exhaustive-in-typescript + */ +export const assertUnreachable = (switchCase: never): never => { + throw new Error(`This should never be reached in TypeScript: "${switchCase}"`) +} From d183d28e36679c7ff96c8214d13c29bf2141fb1e Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Wed, 25 Nov 2020 14:02:06 +0800 Subject: [PATCH 05/12] ref(AdminFormsRoutes): use new handleStreamFormFeedback handler --- src/app/routes/admin-forms.server.routes.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/routes/admin-forms.server.routes.js b/src/app/routes/admin-forms.server.routes.js index c8ae2c7183..2f776f85e8 100644 --- a/src/app/routes/admin-forms.server.routes.js +++ b/src/app/routes/admin-forms.server.routes.js @@ -298,7 +298,7 @@ module.exports = function (app) { */ app .route('/:formId([a-fA-F0-9]{24})/adminform/feedback/download') - .get(authActiveForm(PermissionLevel.Read), adminForms.streamFeedback) + .get(withUserAuthentication, AdminFormController.handleStreamFormFeedback) /** * Transfer form ownership to another user From 25ba13342888f2d0af2ed00cf443a881f65b1334 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Wed, 25 Nov 2020 14:19:34 +0800 Subject: [PATCH 06/12] test(FormFeedbackModel): add tests for model schema and statics --- src/types/form_feedback.ts | 2 +- .../models/form_feedback.server.model.spec.ts | 106 ++++++++++++++++++ 2 files changed, 107 insertions(+), 1 deletion(-) create mode 100644 tests/unit/backend/models/form_feedback.server.model.spec.ts diff --git a/src/types/form_feedback.ts b/src/types/form_feedback.ts index 2bb1bde5e3..8ab08cac7d 100644 --- a/src/types/form_feedback.ts +++ b/src/types/form_feedback.ts @@ -5,7 +5,7 @@ import { IFormSchema } from './form' export interface IFormFeedback { formId: IFormSchema['_id'] rating: number - comment: string + comment?: string _id?: Document['_id'] } diff --git a/tests/unit/backend/models/form_feedback.server.model.spec.ts b/tests/unit/backend/models/form_feedback.server.model.spec.ts new file mode 100644 index 0000000000..b794c0e2a7 --- /dev/null +++ b/tests/unit/backend/models/form_feedback.server.model.spec.ts @@ -0,0 +1,106 @@ +import { ObjectId } from 'bson-ext' +import { omit } from 'lodash' +import mongoose from 'mongoose' + +import getFormFeedbackModel from 'src/app/models/form_feedback.server.model' +import { IFormFeedback } from 'src/types' + +import dbHandler from '../helpers/jest-db' + +const FeedbackModel = getFormFeedbackModel(mongoose) + +describe('form_feedback.server.model', () => { + beforeAll(async () => await dbHandler.connect()) + beforeEach(async () => await dbHandler.clearDatabase()) + afterAll(async () => await dbHandler.closeDatabase()) + + describe('Schema', () => { + const DEFAULT_PARAMS: IFormFeedback = { + formId: new ObjectId(), + rating: 5, + comment: 'feedback comment', + } + + it('should create and save successfully', async () => { + // Act + const actual = await FeedbackModel.create(DEFAULT_PARAMS) + + // Assert + expect(actual).toEqual( + expect.objectContaining({ + ...DEFAULT_PARAMS, + created: expect.any(Date), + lastModified: expect.any(Date), + }), + ) + }) + + it('should save successfully even when comment param is missing', async () => { + // Arrange + const paramsWithoutComment = omit(DEFAULT_PARAMS, 'comment') + // Act + const actual = await FeedbackModel.create(paramsWithoutComment) + + // Assert + expect(actual).toEqual( + expect.objectContaining({ + ...paramsWithoutComment, + created: expect.any(Date), + lastModified: expect.any(Date), + }), + ) + }) + + it('should throw validation error when formId param is missing', async () => { + // Act + const actualPromise = new FeedbackModel( + omit(DEFAULT_PARAMS, 'formId'), + ).save() + + // Assert + await expect(actualPromise).rejects.toThrowError( + mongoose.Error.ValidationError, + ) + }) + + it('should throw validation error when rating param is missing', async () => { + // Act + const actualPromise = new FeedbackModel( + omit(DEFAULT_PARAMS, 'rating'), + ).save() + + // Assert + await expect(actualPromise).rejects.toThrowError( + mongoose.Error.ValidationError, + ) + }) + }) + + describe('Statics', () => { + describe('getFeedbackCursorByFormId', () => { + it('should return cursor to feedback', async () => { + // Arrange + // Create document + const mockFormId = new ObjectId() + const mockFeedbackDoc = await FeedbackModel.create({ + formId: mockFormId, + rating: 5, + comment: 'feedback comment', + }) + + // Act + const cursor = FeedbackModel.getFeedbackCursorByFormId( + mockFormId.toHexString(), + ) + const actualFeedback = [] + for await (const fb of cursor) { + actualFeedback.push(fb) + } + + // Assert + // Cursor should return a lean object instead of a document. + expect(actualFeedback).toEqual([mockFeedbackDoc.toObject()]) + }) + }) + }) +}) From 74c8e660e500976bb7c0775bb13267c0e2bbdc7e Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Wed, 25 Nov 2020 17:35:18 +0800 Subject: [PATCH 07/12] feat(AuthService): add getFormAfterPermissionChecks helper function --- src/app/modules/auth/auth.service.ts | 53 ++++++++++++++++++- .../form/admin-form/admin-form.types.ts | 6 ++- .../form/admin-form/admin-form.utils.ts | 33 +++++++----- .../form/public-form/public-form.service.ts | 2 +- 4 files changed, 76 insertions(+), 18 deletions(-) diff --git a/src/app/modules/auth/auth.service.ts b/src/app/modules/auth/auth.service.ts index 2fc6001e6e..5d8d0295d3 100644 --- a/src/app/modules/auth/auth.service.ts +++ b/src/app/modules/auth/auth.service.ts @@ -2,16 +2,31 @@ import mongoose from 'mongoose' import { errAsync, okAsync, ResultAsync } from 'neverthrow' import validator from 'validator' -import { IAgencySchema, ITokenSchema } from 'src/types' - import config from '../../../config/config' import { createLoggerWithLabel } from '../../../config/logger' import { LINKS } from '../../../shared/constants' +import { + IAgencySchema, + IPopulatedForm, + ITokenSchema, + IUserSchema, +} from '../../../types' import getAgencyModel from '../../models/agency.server.model' import getTokenModel from '../../models/token.server.model' import { compareHash, hashData } from '../../utils/hash' import { generateOtp } from '../../utils/otp' import { ApplicationError, DatabaseError } from '../core/core.errors' +import { PermissionLevel } from '../form/admin-form/admin-form.types' +import { + assertFormAvailable, + getAssertPermissionFn, +} from '../form/admin-form/admin-form.utils' +import { + ForbiddenFormError, + FormDeletedError, + FormNotFoundError, +} from '../form/form.errors' +import { retrieveFullFormById } from '../form/form.service' import { InvalidDomainError, InvalidOtpError } from './auth.errors' @@ -249,3 +264,37 @@ const removeTokenOnSuccess = (email: string) => { }, ) } + +/** + * Retrieves the form of given formId provided that the given user has the + * required permissions. + * + * @returns ok(form) if the user has the required permissions + * @returns err(FormNotFoundError) if form does not exist in the database + * @returns err(FormDeleteError) if form is already archived + * @returns err(ForbiddenFormError if user does not have permission + * @returns err(DatabaseError) if any database error occurs + */ +export const getFormAfterPermissionChecks = ({ + user, + formId, + level, +}: { + user: IUserSchema + formId: string + level: PermissionLevel +}): ResultAsync< + IPopulatedForm, + FormNotFoundError | FormDeletedError | DatabaseError | ForbiddenFormError +> => { + // Step 1: Retrieve full form. + return retrieveFullFormById(formId).andThen((fullForm) => + // Step 2: Check whether form is available to be retrieved. + assertFormAvailable(fullForm).andThen(() => + // Step 3: Check required permission levels. + getAssertPermissionFn(level)(user, fullForm) + // Step 4: If success, return retrieved form. + .map(() => fullForm), + ), + ) +} diff --git a/src/app/modules/form/admin-form/admin-form.types.ts b/src/app/modules/form/admin-form/admin-form.types.ts index aa5f924b91..9431fbd372 100644 --- a/src/app/modules/form/admin-form/admin-form.types.ts +++ b/src/app/modules/form/admin-form/admin-form.types.ts @@ -1,5 +1,6 @@ import { Result } from 'neverthrow' +import { IPopulatedForm, IUserSchema } from '../../../../types' import { ForbiddenFormError } from '../form.errors' export enum PermissionLevel { @@ -8,4 +9,7 @@ export enum PermissionLevel { Delete = 'delete', } -export type FormPermissionResult = Result +export type AssertFormFn = ( + user: IUserSchema, + form: IPopulatedForm, +) => Result diff --git a/src/app/modules/form/admin-form/admin-form.utils.ts b/src/app/modules/form/admin-form/admin-form.utils.ts index 19f3196d69..0824b375d1 100644 --- a/src/app/modules/form/admin-form/admin-form.utils.ts +++ b/src/app/modules/form/admin-form/admin-form.utils.ts @@ -2,7 +2,8 @@ import { StatusCodes } from 'http-status-codes' import { err, ok, Result } from 'neverthrow' import { createLoggerWithLabel } from '../../../../config/logger' -import { IPopulatedForm, IUserSchema, Status } from '../../../../types' +import { IPopulatedForm, Status } from '../../../../types' +import { assertUnreachable } from '../../../utils/assert-unreachable' import { ApplicationError, DatabaseError, @@ -20,7 +21,7 @@ import { CreatePresignedUrlError, InvalidFileTypeError, } from './admin-form.errors' -import { FormPermissionResult } from './admin-form.types' +import { AssertFormFn, PermissionLevel } from './admin-form.types' const logger = createLoggerWithLabel(module) @@ -106,10 +107,7 @@ export const assertFormAvailable = ( * @returns ok(true) if given user has read permissions * @returns err(ForbiddenFormError) if user does not have read permissions */ -export const assertHasReadPermissions = ( - user: IUserSchema, - form: IPopulatedForm, -): FormPermissionResult => { +export const assertHasReadPermissions: AssertFormFn = (user, form) => { // Is form admin. Automatically has permissions. if (String(user._id) === String(form.admin._id)) { return ok(true) @@ -134,10 +132,7 @@ export const assertHasReadPermissions = ( * @returns ok(true) if given user has delete permissions * @returns err(ForbiddenFormError) if user does not have delete permissions */ -export const assertHasDeletePermissions = ( - user: IUserSchema, - form: IPopulatedForm, -): FormPermissionResult => { +export const assertHasDeletePermissions: AssertFormFn = (user, form) => { const isFormAdmin = String(user._id) === String(form.admin._id) // If form admin return isFormAdmin @@ -154,10 +149,7 @@ export const assertHasDeletePermissions = ( * @returns ok(true) if given user has write permissions * @returns err(ForbiddenFormError) if user does not have write permissions */ -export const assertHasWritePermissions = ( - user: IUserSchema, - form: IPopulatedForm, -): FormPermissionResult => { +export const assertHasWritePermissions: AssertFormFn = (user, form) => { // Is form admin. Automatically has permissions. if (String(user._id) === String(form.admin._id)) { return ok(true) @@ -177,3 +169,16 @@ export const assertHasWritePermissions = ( ), ) } + +export const getAssertPermissionFn = (level: PermissionLevel): AssertFormFn => { + switch (level) { + case PermissionLevel.Read: + return assertHasReadPermissions + case PermissionLevel.Write: + return assertHasWritePermissions + case PermissionLevel.Delete: + return assertHasDeletePermissions + default: + return assertUnreachable(level) + } +} diff --git a/src/app/modules/form/public-form/public-form.service.ts b/src/app/modules/form/public-form/public-form.service.ts index 0aa0929d5a..6d924cea1d 100644 --- a/src/app/modules/form/public-form/public-form.service.ts +++ b/src/app/modules/form/public-form/public-form.service.ts @@ -29,7 +29,7 @@ export const insertFormFeedback = ({ }: { formId: string rating: number - comment: string + comment?: string }): ResultAsync => { if (!mongoose.Types.ObjectId.isValid(formId)) { return errAsync(new FormNotFoundError()) From 3438cba0c3a72c2d3cc78517f935bbd77162c5a6 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Wed, 25 Nov 2020 18:04:38 +0800 Subject: [PATCH 08/12] test(AuthService): add test cases for getFormAfterPermissionChecks --- .../auth/__tests__/auth.service.spec.ts | 148 +++++++++++++++++- src/app/modules/auth/auth.service.ts | 4 +- 2 files changed, 145 insertions(+), 7 deletions(-) diff --git a/src/app/modules/auth/__tests__/auth.service.spec.ts b/src/app/modules/auth/__tests__/auth.service.spec.ts index 32488245de..67e80a4252 100644 --- a/src/app/modules/auth/__tests__/auth.service.spec.ts +++ b/src/app/modules/auth/__tests__/auth.service.spec.ts @@ -1,15 +1,32 @@ +import { ObjectId } from 'bson-ext' import mongoose from 'mongoose' +import { err, errAsync, ok, okAsync } from 'neverthrow' +import { mocked } from 'ts-jest/utils' import { ImportMock } from 'ts-mock-imports' import getTokenModel from 'src/app/models/token.server.model' import * as OtpUtils from 'src/app/utils/otp' -import { IAgencySchema } from 'src/types' +import { IAgencySchema, IPopulatedForm, IPopulatedUser } from 'src/types' import dbHandler from 'tests/unit/backend/helpers/jest-db' +import { DatabaseError } from '../../core/core.errors' +import { PermissionLevel } from '../../form/admin-form/admin-form.types' +import * as AdminFormUtils from '../../form/admin-form/admin-form.utils' +import { + ForbiddenFormError, + FormDeletedError, + FormNotFoundError, +} from '../../form/form.errors' +import * as FormService from '../../form/form.service' import { InvalidDomainError, InvalidOtpError } from '../auth.errors' import * as AuthService from '../auth.service' +jest.mock('../../form/form.service') +const MockFormService = mocked(FormService) +jest.mock('../../form/admin-form/admin-form.utils') +const MockAdminFormUtils = mocked(AdminFormUtils) + const TokenModel = getTokenModel(mongoose) const VALID_EMAIL_DOMAIN = 'test.gov.sg' @@ -30,10 +47,10 @@ describe('auth.service', () => { }) // Only need to clear Token collection, and ignore other collections. - beforeEach( - async () => - await dbHandler.clearCollection(TokenModel.collection.collectionName), - ) + beforeEach(async () => { + await dbHandler.clearCollection(TokenModel.collection.collectionName) + jest.resetAllMocks() + }) afterAll(async () => await dbHandler.closeDatabase()) @@ -187,4 +204,125 @@ describe('auth.service', () => { expect(actualResult._unsafeUnwrapErr()).toEqual(expectedError) }) }) + + describe('getFormAfterPermissionChecks', () => { + const MOCK_USER = { + _id: new ObjectId(), + } as IPopulatedUser + it('should return form when user has permissions', async () => { + // Arrange + const mockFormId = new ObjectId().toHexString() + const expectedForm = { + title: 'mock form', + _id: mockFormId, + } as IPopulatedForm + MockFormService.retrieveFullFormById.mockReturnValueOnce( + okAsync(expectedForm), + ) + const assertSpy = jest.fn().mockImplementation(() => () => ok(true)) + MockAdminFormUtils.assertFormAvailable.mockReturnValueOnce(ok(true)) + MockAdminFormUtils.getAssertPermissionFn.mockImplementationOnce(assertSpy) + + // Act + const actualResult = await AuthService.getFormAfterPermissionChecks({ + user: MOCK_USER, + formId: mockFormId, + level: PermissionLevel.Write, + }) + + // Assert + expect(actualResult.isOk()).toEqual(true) + expect(actualResult._unsafeUnwrap()).toEqual(expectedForm) + expect(assertSpy).toHaveBeenCalledWith(PermissionLevel.Write) + }) + + it('should return FormNotFoundError when form does not exist in the database', async () => { + // Arrange + const expectedError = new FormNotFoundError('not found') + MockFormService.retrieveFullFormById.mockReturnValueOnce( + errAsync(expectedError), + ) + + // Act + const actualResult = await AuthService.getFormAfterPermissionChecks({ + user: MOCK_USER, + formId: new ObjectId().toHexString(), + level: PermissionLevel.Read, + }) + + // Assert + expect(actualResult.isErr()).toEqual(true) + expect(actualResult._unsafeUnwrapErr()).toEqual(expectedError) + }) + + it('should return FormDeletedError when form is already archived', async () => { + // Arrange + const mockFormId = new ObjectId().toHexString() + const expectedError = new FormDeletedError('form deleted') + MockFormService.retrieveFullFormById.mockReturnValueOnce( + okAsync({} as IPopulatedForm), + ) + MockAdminFormUtils.assertFormAvailable.mockReturnValueOnce( + err(expectedError), + ) + // Act + const actualResult = await AuthService.getFormAfterPermissionChecks({ + user: MOCK_USER, + formId: mockFormId, + level: PermissionLevel.Delete, + }) + + // Assert + expect(actualResult.isErr()).toEqual(true) + expect(actualResult._unsafeUnwrapErr()).toEqual(expectedError) + }) + + it('should return ForbiddenFormError when user does not have permission', async () => { + // Arrange + const mockFormId = new ObjectId().toHexString() + MockFormService.retrieveFullFormById.mockReturnValueOnce( + okAsync({} as IPopulatedForm), + ) + const expectedError = new ForbiddenFormError('user not allowed') + MockAdminFormUtils.assertFormAvailable.mockReturnValueOnce(ok(true)) + const assertSpy = jest + .fn() + .mockImplementation(() => () => err(expectedError)) + MockAdminFormUtils.getAssertPermissionFn.mockImplementationOnce(assertSpy) + + // Act + const actualResult = await AuthService.getFormAfterPermissionChecks({ + user: MOCK_USER, + formId: mockFormId, + level: PermissionLevel.Write, + }) + + // Assert + expect(actualResult.isErr()).toEqual(true) + expect(actualResult._unsafeUnwrapErr()).toEqual(expectedError) + expect(assertSpy).toHaveBeenCalledWith(PermissionLevel.Write) + }) + + it('should return DatabaseError when error occurs whilst retrieving form', async () => { + // Arrange + const mockFormId = new ObjectId().toHexString() + const expectedError = new DatabaseError('db boom') + MockFormService.retrieveFullFormById.mockReturnValueOnce( + okAsync({} as IPopulatedForm), + ) + MockAdminFormUtils.assertFormAvailable.mockReturnValueOnce( + err(expectedError), + ) + // Act + const actualResult = await AuthService.getFormAfterPermissionChecks({ + user: MOCK_USER, + formId: mockFormId, + level: PermissionLevel.Delete, + }) + + // Assert + expect(actualResult.isErr()).toEqual(true) + expect(actualResult._unsafeUnwrapErr()).toEqual(expectedError) + }) + }) }) diff --git a/src/app/modules/auth/auth.service.ts b/src/app/modules/auth/auth.service.ts index 5d8d0295d3..ba4ffac6fb 100644 --- a/src/app/modules/auth/auth.service.ts +++ b/src/app/modules/auth/auth.service.ts @@ -26,7 +26,7 @@ import { FormDeletedError, FormNotFoundError, } from '../form/form.errors' -import { retrieveFullFormById } from '../form/form.service' +import * as FormService from '../form/form.service' import { InvalidDomainError, InvalidOtpError } from './auth.errors' @@ -288,7 +288,7 @@ export const getFormAfterPermissionChecks = ({ FormNotFoundError | FormDeletedError | DatabaseError | ForbiddenFormError > => { // Step 1: Retrieve full form. - return retrieveFullFormById(formId).andThen((fullForm) => + return FormService.retrieveFullFormById(formId).andThen((fullForm) => // Step 2: Check whether form is available to be retrieved. assertFormAvailable(fullForm).andThen(() => // Step 3: Check required permission levels. From c85eb2cb258c84394b01785b48bcc71cfbd2241f Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Wed, 25 Nov 2020 18:38:58 +0800 Subject: [PATCH 09/12] ref(AdminFormCtl): use getFormAfterPermissionChecks fn in controllers --- .../__tests__/admin-form.controller.spec.ts | 275 +++++++----------- .../form/admin-form/admin-form.controller.ts | 139 +++------ 2 files changed, 160 insertions(+), 254 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 0e66017a18..043b7fe44d 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 @@ -1,9 +1,10 @@ import { PresignedPost } from 'aws-sdk/clients/s3' import { ObjectId } from 'bson-ext' import { merge } from 'lodash' -import { err, errAsync, ok, okAsync } from 'neverthrow' +import { errAsync, okAsync } from 'neverthrow' import { mocked } from 'ts-jest/utils' +import * as AuthService from 'src/app/modules/auth/auth.service' import { DatabaseError } from 'src/app/modules/core/core.errors' import * as FeedbackService from 'src/app/modules/feedback/feedback.service' import * as SubmissionService from 'src/app/modules/submission/submission.service' @@ -18,15 +19,16 @@ import { FormDeletedError, FormNotFoundError, } from '../../form.errors' -import * as FormService from '../../form.service' import * as AdminFormController from '../admin-form.controller' import { CreatePresignedUrlError, InvalidFileTypeError, } from '../admin-form.errors' import * as AdminFormService from '../admin-form.service' -import * as AdminFormUtils from '../admin-form.utils' +import { PermissionLevel } from '../admin-form.types' +jest.mock('src/app/modules/auth/auth.service') +const MockAuthService = mocked(AuthService) jest.mock('src/app/modules/feedback/feedback.service') const MockFeedbackService = mocked(FeedbackService) jest.mock('src/app/modules/submission/submission.service') @@ -35,8 +37,6 @@ jest.mock('../admin-form.service') const MockAdminFormService = mocked(AdminFormService) jest.mock('../../../user/user.service') const MockUserService = mocked(UserService) -jest.mock('../../form.service') -const MockFormService = mocked(FormService) describe('admin-form.controller', () => { beforeEach(() => jest.clearAllMocks()) @@ -301,12 +301,9 @@ describe('admin-form.controller', () => { MockUserService.getPopulatedUserById.mockReturnValueOnce( okAsync(MOCK_USER as IPopulatedUser), ) - MockFormService.retrieveFullFormById.mockReturnValueOnce( + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( okAsync(MOCK_FORM as IPopulatedForm), ) - const readPermsSpy = jest - .spyOn(AdminFormUtils, 'assertHasReadPermissions') - .mockReturnValueOnce(ok(true)) MockSubmissionService.getFormSubmissionsCount.mockReturnValueOnce( okAsync(expectedSubmissionCount), ) @@ -323,10 +320,13 @@ describe('admin-form.controller', () => { expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, ) - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID.toHexString(), + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, ) - expect(readPermsSpy).toHaveBeenCalledWith(MOCK_USER, MOCK_FORM) expect( MockSubmissionService.getFormSubmissionsCount, ).toHaveBeenCalledWith(String(MOCK_FORM._id), { @@ -352,12 +352,9 @@ describe('admin-form.controller', () => { MockUserService.getPopulatedUserById.mockReturnValueOnce( okAsync(MOCK_USER as IPopulatedUser), ) - MockFormService.retrieveFullFormById.mockReturnValueOnce( + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( okAsync(MOCK_FORM as IPopulatedForm), ) - const readPermsSpy = jest - .spyOn(AdminFormUtils, 'assertHasReadPermissions') - .mockReturnValueOnce(ok(true)) MockSubmissionService.getFormSubmissionsCount.mockReturnValueOnce( okAsync(expectedSubmissionCount), ) @@ -374,10 +371,13 @@ describe('admin-form.controller', () => { expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, ) - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID.toHexString(), + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, ) - expect(readPermsSpy).toHaveBeenCalledWith(MOCK_USER, MOCK_FORM) expect( MockSubmissionService.getFormSubmissionsCount, ).toHaveBeenCalledWith(String(MOCK_FORM._id), expectedDateRange) @@ -387,19 +387,16 @@ describe('admin-form.controller', () => { it('should return 403 when ForbiddenFormError is returned when verifying user permissions', async () => { // Arrange + const expectedErrorString = 'no read access' + const mockRes = expressHandler.mockResponse() // Mock various services to return expected results. MockUserService.getPopulatedUserById.mockReturnValueOnce( okAsync(MOCK_USER as IPopulatedUser), ) - MockFormService.retrieveFullFormById.mockReturnValueOnce( - okAsync(MOCK_FORM as IPopulatedForm), + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( + errAsync(new ForbiddenFormError(expectedErrorString)), ) - // Mock error here. - const expectedErrorString = 'no read access' - const readPermsSpy = jest - .spyOn(AdminFormUtils, 'assertHasReadPermissions') - .mockReturnValueOnce(err(new ForbiddenFormError(expectedErrorString))) // Act await AdminFormController.handleCountFormSubmissions( @@ -413,10 +410,13 @@ describe('admin-form.controller', () => { expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, ) - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID.toHexString(), + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, ) - expect(readPermsSpy).toHaveBeenCalledWith(MOCK_USER, MOCK_FORM) expect( MockSubmissionService.getFormSubmissionsCount, ).not.toHaveBeenCalled() @@ -435,7 +435,7 @@ describe('admin-form.controller', () => { ) // Mock error when retrieving form. const expectedErrorString = 'form is not found' - MockFormService.retrieveFullFormById.mockReturnValueOnce( + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( errAsync(new FormNotFoundError(expectedErrorString)), ) @@ -451,8 +451,12 @@ describe('admin-form.controller', () => { expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, ) - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID.toHexString(), + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, ) expect( MockSubmissionService.getFormSubmissionsCount, @@ -472,7 +476,7 @@ describe('admin-form.controller', () => { ) // Mock error when retrieving form. const expectedErrorString = 'form is deleted' - MockFormService.retrieveFullFormById.mockReturnValueOnce( + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( errAsync(new FormDeletedError(expectedErrorString)), ) @@ -488,50 +492,13 @@ describe('admin-form.controller', () => { expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, ) - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID.toHexString(), - ) - expect( - MockSubmissionService.getFormSubmissionsCount, - ).not.toHaveBeenCalled() - expect(mockRes.status).toHaveBeenCalledWith(410) - expect(mockRes.json).toHaveBeenCalledWith({ - message: expectedErrorString, - }) - }) - - it('should return 410 when FormDeletedError is returned when checking form availability', async () => { - // Arrange - const mockRes = expressHandler.mockResponse() - // Mock various services to return expected results. - MockUserService.getPopulatedUserById.mockReturnValueOnce( - okAsync(MOCK_USER as IPopulatedUser), - ) - MockFormService.retrieveFullFormById.mockReturnValueOnce( - okAsync(MOCK_FORM as IPopulatedForm), - ) - // Mock error when checking form availability. - const expectedErrorString = 'form is archived' - const utilSpy = jest - .spyOn(AdminFormUtils, 'assertFormAvailable') - .mockReturnValueOnce(err(new FormDeletedError(expectedErrorString))) - - // Act - await AdminFormController.handleCountFormSubmissions( - MOCK_REQ, - mockRes, - jest.fn(), - ) - - // Assert - // Check all arguments of called services. - expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( - MOCK_USER_ID, - ) - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID.toHexString(), + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, ) - expect(utilSpy).toHaveBeenCalledWith(MOCK_FORM) expect( MockSubmissionService.getFormSubmissionsCount, ).not.toHaveBeenCalled() @@ -562,7 +529,9 @@ describe('admin-form.controller', () => { expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, ) - expect(MockFormService.retrieveFullFormById).not.toHaveBeenCalled() + expect( + MockAuthService.getFormAfterPermissionChecks, + ).not.toHaveBeenCalled() expect( MockSubmissionService.getFormSubmissionsCount, ).not.toHaveBeenCalled() @@ -593,7 +562,9 @@ describe('admin-form.controller', () => { expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, ) - expect(MockFormService.retrieveFullFormById).not.toHaveBeenCalled() + expect( + MockAuthService.getFormAfterPermissionChecks, + ).not.toHaveBeenCalled() expect( MockSubmissionService.getFormSubmissionsCount, ).not.toHaveBeenCalled() @@ -612,7 +583,7 @@ describe('admin-form.controller', () => { ) // Mock error when retrieving form. const expectedErrorString = 'database goes boom' - MockFormService.retrieveFullFormById.mockReturnValueOnce( + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( errAsync(new DatabaseError(expectedErrorString)), ) @@ -628,8 +599,12 @@ describe('admin-form.controller', () => { expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, ) - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID.toHexString(), + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, ) expect( MockSubmissionService.getFormSubmissionsCount, @@ -647,12 +622,9 @@ describe('admin-form.controller', () => { MockUserService.getPopulatedUserById.mockReturnValueOnce( okAsync(MOCK_USER as IPopulatedUser), ) - MockFormService.retrieveFullFormById.mockReturnValueOnce( + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( okAsync(MOCK_FORM as IPopulatedForm), ) - const readPermsSpy = jest - .spyOn(AdminFormUtils, 'assertHasReadPermissions') - .mockReturnValueOnce(ok(true)) const expectedErrorString = 'database goes boom' MockSubmissionService.getFormSubmissionsCount.mockReturnValueOnce( errAsync(new DatabaseError(expectedErrorString)), @@ -670,10 +642,13 @@ describe('admin-form.controller', () => { expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, ) - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID.toHexString(), + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, ) - expect(readPermsSpy).toHaveBeenCalledWith(MOCK_USER, MOCK_FORM) expect( MockSubmissionService.getFormSubmissionsCount, ).toHaveBeenCalledWith(String(MOCK_FORM._id), { @@ -720,12 +695,9 @@ describe('admin-form.controller', () => { MockUserService.getPopulatedUserById.mockReturnValueOnce( okAsync(MOCK_USER as IPopulatedUser), ) - MockFormService.retrieveFullFormById.mockReturnValueOnce( + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( okAsync(MOCK_FORM as IPopulatedForm), ) - const readPermsSpy = jest - .spyOn(AdminFormUtils, 'assertHasReadPermissions') - .mockReturnValueOnce(ok(true)) MockFeedbackService.getFormFeedbackCount.mockReturnValueOnce( okAsync(expectedFeedbackCount), ) @@ -742,10 +714,13 @@ describe('admin-form.controller', () => { expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, ) - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID.toHexString(), + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, ) - expect(readPermsSpy).toHaveBeenCalledWith(MOCK_USER, MOCK_FORM) expect(MockFeedbackService.getFormFeedbackCount).toHaveBeenCalledWith( String(MOCK_FORM._id), ) @@ -760,14 +735,10 @@ describe('admin-form.controller', () => { MockUserService.getPopulatedUserById.mockReturnValueOnce( okAsync(MOCK_USER as IPopulatedUser), ) - MockFormService.retrieveFullFormById.mockReturnValueOnce( - okAsync(MOCK_FORM as IPopulatedForm), - ) - // Mock error here. const expectedErrorString = 'no read access' - const readPermsSpy = jest - .spyOn(AdminFormUtils, 'assertHasReadPermissions') - .mockReturnValueOnce(err(new ForbiddenFormError(expectedErrorString))) + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( + errAsync(new ForbiddenFormError(expectedErrorString)), + ) // Act await AdminFormController.handleCountFormFeedback( @@ -781,10 +752,13 @@ describe('admin-form.controller', () => { expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, ) - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID.toHexString(), + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, ) - expect(readPermsSpy).toHaveBeenCalledWith(MOCK_USER, MOCK_FORM) expect(MockFeedbackService.getFormFeedbackCount).not.toHaveBeenCalled() expect(mockRes.status).toHaveBeenCalledWith(403) expect(mockRes.json).toHaveBeenCalledWith({ @@ -801,7 +775,7 @@ describe('admin-form.controller', () => { ) // Mock error when retrieving form. const expectedErrorString = 'form is not found' - MockFormService.retrieveFullFormById.mockReturnValueOnce( + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( errAsync(new FormNotFoundError(expectedErrorString)), ) @@ -817,8 +791,12 @@ describe('admin-form.controller', () => { expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, ) - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID.toHexString(), + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, ) expect(MockFeedbackService.getFormFeedbackCount).not.toHaveBeenCalled() expect(mockRes.status).toHaveBeenCalledWith(404) @@ -836,7 +814,7 @@ describe('admin-form.controller', () => { ) // Mock error when retrieving form. const expectedErrorString = 'form is deleted' - MockFormService.retrieveFullFormById.mockReturnValueOnce( + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( errAsync(new FormDeletedError(expectedErrorString)), ) @@ -852,48 +830,13 @@ describe('admin-form.controller', () => { expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, ) - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID.toHexString(), - ) - expect(MockFeedbackService.getFormFeedbackCount).not.toHaveBeenCalled() - expect(mockRes.status).toHaveBeenCalledWith(410) - expect(mockRes.json).toHaveBeenCalledWith({ - message: expectedErrorString, - }) - }) - - it('should return 410 when FormDeletedError is returned when checking form availability', async () => { - // Arrange - const mockRes = expressHandler.mockResponse() - // Mock various services to return expected results. - MockUserService.getPopulatedUserById.mockReturnValueOnce( - okAsync(MOCK_USER as IPopulatedUser), - ) - MockFormService.retrieveFullFormById.mockReturnValueOnce( - okAsync(MOCK_FORM as IPopulatedForm), - ) - // Mock error when checking form availability. - const expectedErrorString = 'form is archived' - const utilSpy = jest - .spyOn(AdminFormUtils, 'assertFormAvailable') - .mockReturnValueOnce(err(new FormDeletedError(expectedErrorString))) - - // Act - await AdminFormController.handleCountFormFeedback( - MOCK_REQ, - mockRes, - jest.fn(), - ) - - // Assert - // Check all arguments of called services. - expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( - MOCK_USER_ID, - ) - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID.toHexString(), + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, ) - expect(utilSpy).toHaveBeenCalledWith(MOCK_FORM) expect(MockFeedbackService.getFormFeedbackCount).not.toHaveBeenCalled() expect(mockRes.status).toHaveBeenCalledWith(410) expect(mockRes.json).toHaveBeenCalledWith({ @@ -922,7 +865,9 @@ describe('admin-form.controller', () => { expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, ) - expect(MockFormService.retrieveFullFormById).not.toHaveBeenCalled() + expect( + MockAuthService.getFormAfterPermissionChecks, + ).not.toHaveBeenCalled() expect(MockFeedbackService.getFormFeedbackCount).not.toHaveBeenCalled() expect(mockRes.status).toHaveBeenCalledWith(422) expect(mockRes.json).toHaveBeenCalledWith({ @@ -951,7 +896,9 @@ describe('admin-form.controller', () => { expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, ) - expect(MockFormService.retrieveFullFormById).not.toHaveBeenCalled() + expect( + MockAuthService.getFormAfterPermissionChecks, + ).not.toHaveBeenCalled() expect(MockFeedbackService.getFormFeedbackCount).not.toHaveBeenCalled() expect(mockRes.status).toHaveBeenCalledWith(500) expect(mockRes.json).toHaveBeenCalledWith({ @@ -968,7 +915,7 @@ describe('admin-form.controller', () => { ) // Mock error when retrieving form. const expectedErrorString = 'database goes boom' - MockFormService.retrieveFullFormById.mockReturnValueOnce( + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( errAsync(new DatabaseError(expectedErrorString)), ) @@ -984,8 +931,12 @@ describe('admin-form.controller', () => { expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, ) - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID.toHexString(), + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, ) expect(MockFeedbackService.getFormFeedbackCount).not.toHaveBeenCalled() expect(mockRes.status).toHaveBeenCalledWith(500) @@ -1001,12 +952,9 @@ describe('admin-form.controller', () => { MockUserService.getPopulatedUserById.mockReturnValueOnce( okAsync(MOCK_USER as IPopulatedUser), ) - MockFormService.retrieveFullFormById.mockReturnValueOnce( + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( okAsync(MOCK_FORM as IPopulatedForm), ) - const readPermsSpy = jest - .spyOn(AdminFormUtils, 'assertHasReadPermissions') - .mockReturnValueOnce(ok(true)) const expectedErrorString = 'database goes boom' MockFeedbackService.getFormFeedbackCount.mockReturnValueOnce( errAsync(new DatabaseError(expectedErrorString)), @@ -1024,10 +972,13 @@ describe('admin-form.controller', () => { expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( MOCK_USER_ID, ) - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - MOCK_FORM_ID.toHexString(), + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, ) - expect(readPermsSpy).toHaveBeenCalledWith(MOCK_USER, MOCK_FORM) expect(MockFeedbackService.getFormFeedbackCount).toHaveBeenCalledWith( String(MOCK_FORM._id), ) 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 b86a83dd8b..35576d50ac 100644 --- a/src/app/modules/form/admin-form/admin-form.controller.ts +++ b/src/app/modules/form/admin-form/admin-form.controller.ts @@ -6,10 +6,10 @@ import JSONStream from 'JSONStream' import { createLoggerWithLabel } from '../../../../config/logger' import { AuthType, WithForm } from '../../../../types' import { createReqMeta } from '../../../utils/request' +import * as AuthService from '../../auth/auth.service' import * as FeedbackService from '../../feedback/feedback.service' import * as SubmissionService from '../../submission/submission.service' import * as UserService from '../../user/user.service' -import * as FormService from '../form.service' import { createPresignedPostForImages, @@ -17,11 +17,8 @@ import { getDashboardForms, getMockSpcpLocals, } from './admin-form.service' -import { - assertFormAvailable, - assertHasReadPermissions, - mapRouteError, -} from './admin-form.utils' +import { PermissionLevel } from './admin-form.types' +import { mapRouteError } from './admin-form.utils' const logger = createLoggerWithLabel(module) @@ -155,85 +152,47 @@ export const handleCountFormSubmissions: RequestHandler< } // Step 1: Retrieve currently logged in user. - const adminResult = await UserService.getPopulatedUserById(sessionUserId) - // Step 1a: Error retrieving logged in user. - if (adminResult.isErr()) { - logger.error({ - message: 'Error occurred whilst retrieving user', - meta: logMeta, - error: adminResult.error, - }) - - const { errorMessage, statusCode } = mapRouteError(adminResult.error) - return res.status(statusCode).json({ message: errorMessage }) - } - // Step 1b: Successfully retrieved logged in user. - const admin = adminResult.value + const formResult = await UserService.getPopulatedUserById( + sessionUserId, + ).andThen((user) => + // Step 2: Check whether user has read permissions to form + AuthService.getFormAfterPermissionChecks({ + user, + formId, + level: PermissionLevel.Read, + }), + ) - // Step 2: Retrieve full form. - const formResult = await FormService.retrieveFullFormById(formId) - // Step 2a: Error retrieving form. if (formResult.isErr()) { - logger.error({ - message: 'Failed to retrieve form', + logger.warn({ + message: 'Error occurred when checking user permissions for form', meta: logMeta, error: formResult.error, }) const { errorMessage, statusCode } = mapRouteError(formResult.error) return res.status(statusCode).json({ message: errorMessage }) } - // Step 2b: Successfully retrieved form. - const form = formResult.value - - // Step 3: Check whether form is already archived. - const availableResult = assertFormAvailable(form) - // Step 3a: Form is already archived. - if (availableResult.isErr()) { - logger.warn({ - message: 'User attempted to retrieve archived form', - meta: logMeta, - error: availableResult.error, - }) - const { errorMessage, statusCode } = mapRouteError(availableResult.error) - return res.status(statusCode).json({ message: errorMessage }) - } - - // Step 4: Form is still available for retrieval, check form permissions. - const permissionResult = assertHasReadPermissions(admin, form) - // Step 4a: Read permission error. - if (permissionResult.isErr()) { - logger.warn({ - message: 'User does not have read permissions', - meta: logMeta, - error: permissionResult.error, - }) - const { errorMessage, statusCode } = mapRouteError(permissionResult.error) - return res.status(statusCode).json({ message: errorMessage }) - } - // Step 5: Has permissions, continue to retrieve submission counts. - const countResult = await SubmissionService.getFormSubmissionsCount( - String(form._id), - { startDate, endDate }, - ) - // Step 5a: Error retrieving form submissions counts. - if (countResult.isErr()) { - logger.error({ - message: 'Error retrieving form submission count', - meta: { - action: 'handleCountFormSubmissions', - ...createReqMeta(req), - userId: sessionUserId, - formId, - }, - error: countResult.error, + // Step 3: Has permissions, continue to retrieve submission counts. + return SubmissionService.getFormSubmissionsCount(formId, { + startDate, + endDate, + }) + .map((count) => res.json(count)) + .mapErr((error) => { + logger.error({ + message: 'Error retrieving form submission count', + meta: { + action: 'handleCountFormSubmissions', + ...createReqMeta(req), + userId: sessionUserId, + formId, + }, + error, + }) + const { errorMessage, statusCode } = mapRouteError(error) + return res.status(statusCode).json({ message: errorMessage }) }) - const { errorMessage, statusCode } = mapRouteError(countResult.error) - return res.status(statusCode).json({ message: errorMessage }) - } - - // Successfully retrieved count. - return res.json(countResult.value) } /** @@ -277,16 +236,14 @@ export const handleCountFormFeedback: RequestHandler<{ // Step 1: Retrieve currently logged in user. UserService.getPopulatedUserById(sessionUserId) .andThen((user) => - // Step 2: Retrieve full form. - FormService.retrieveFullFormById(formId).andThen((fullForm) => - // Step 3: Check whether form is active. - assertFormAvailable(fullForm).andThen(() => - // Step 4: Check whether current user has read permissions to form. - assertHasReadPermissions(user, fullForm), - ), - ), + // Step 2: Check whether user has read permissions to form + AuthService.getFormAfterPermissionChecks({ + user, + formId, + level: PermissionLevel.Read, + }), ) - // Step 5: Retrieve form feedback counts. + // Step 3: Retrieve form feedback counts. .andThen(() => FeedbackService.getFormFeedbackCount(formId)) .map((feedbackCount) => res.json(feedbackCount)) // Some error occurred earlier in the chain. @@ -317,14 +274,12 @@ export const handleStreamFormFeedback: RequestHandler<{ const hasReadPermissionResult = await UserService.getPopulatedUserById( sessionUserId, ).andThen((user) => - // Step 2: Retrieve full form. - FormService.retrieveFullFormById(formId).andThen((fullForm) => - // Step 3: Check whether form is active. - assertFormAvailable(fullForm).andThen(() => - // Step 4: Check whether current user has read permissions to form. - assertHasReadPermissions(user, fullForm), - ), - ), + // Step 2: Check whether user has read permissions to form + AuthService.getFormAfterPermissionChecks({ + user, + formId, + level: PermissionLevel.Read, + }), ) const logMeta = { From 14e33751e2841aea0adecb3cbc18802a22813569 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Wed, 25 Nov 2020 21:44:09 +0800 Subject: [PATCH 10/12] test(AdminFormCtl): add tests for handleStreamFormFeedback Note that I have no clue how to test express streams, and my Google-fu failed me this time. Thus, the success case test is just testing that the services return correctly and not whether the stream passes :( --- .../__tests__/admin-form.controller.spec.ts | 284 ++++++++++++++++++ tests/unit/backend/helpers/jest-express.ts | 4 + 2 files changed, 288 insertions(+) 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 043b7fe44d..2dc3eacf40 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 @@ -2,6 +2,7 @@ import { PresignedPost } from 'aws-sdk/clients/s3' import { ObjectId } from 'bson-ext' import { merge } from 'lodash' import { errAsync, okAsync } from 'neverthrow' +import { PassThrough } from 'stream' import { mocked } from 'ts-jest/utils' import * as AuthService from 'src/app/modules/auth/auth.service' @@ -988,4 +989,287 @@ describe('admin-form.controller', () => { }) }) }) + + describe('handleStreamFormFeedback', () => { + const MOCK_USER_ID = new ObjectId() + const MOCK_FORM_ID = new ObjectId() + const MOCK_USER: Partial = { + _id: MOCK_USER_ID, + email: 'somerandom@example.com', + } + const MOCK_FORM: Partial = { + admin: MOCK_USER as IPopulatedUser, + _id: MOCK_FORM_ID, + title: 'mock title', + } + + const MOCK_REQ = expressHandler.mockRequest({ + params: { + formId: MOCK_FORM_ID.toHexString(), + }, + session: { + user: { + _id: MOCK_USER_ID, + }, + }, + }) + + it('should return 200 with feedback stream of given form', async () => { + // Not sure how to really test the stream in Jest, testing to assert that + // the correct services are being called instead. + // Arrange + const mockRes = expressHandler.mockResponse() + // Mock various services to return expected results. + MockUserService.getPopulatedUserById.mockReturnValueOnce( + okAsync(MOCK_USER as IPopulatedUser), + ) + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( + okAsync(MOCK_FORM as IPopulatedForm), + ) + // Mock cursor return. + const mockCursor = new PassThrough() + MockFeedbackService.getFormFeedbackStream.mockReturnValueOnce( + mockCursor as any, + ) + // Act + await AdminFormController.handleStreamFormFeedback( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + // Check all arguments of called services. + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, + ) + expect(MockFeedbackService.getFormFeedbackStream).toHaveBeenCalledWith( + String(MOCK_FORM._id), + ) + }) + + it('should return 403 when ForbiddenFormError is returned when verifying user permissions', async () => { + // Arrange + const mockRes = expressHandler.mockResponse() + // Mock various services to return expected results. + MockUserService.getPopulatedUserById.mockReturnValueOnce( + okAsync(MOCK_USER as IPopulatedUser), + ) + const expectedErrorString = 'no read access' + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( + errAsync(new ForbiddenFormError(expectedErrorString)), + ) + + // Act + await AdminFormController.handleStreamFormFeedback( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + // Check all arguments of called services. + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, + ) + expect(MockFeedbackService.getFormFeedbackStream).not.toHaveBeenCalled() + expect(mockRes.status).toHaveBeenCalledWith(403) + expect(mockRes.json).toHaveBeenCalledWith({ + message: expectedErrorString, + }) + }) + + it('should return 404 when FormNotFoundError is returned when retrieving form', async () => { + // Arrange + const mockRes = expressHandler.mockResponse() + // Mock various services to return expected results. + MockUserService.getPopulatedUserById.mockReturnValueOnce( + okAsync(MOCK_USER as IPopulatedUser), + ) + // Mock error when retrieving form. + const expectedErrorString = 'form is not found' + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( + errAsync(new FormNotFoundError(expectedErrorString)), + ) + + // Act + await AdminFormController.handleStreamFormFeedback( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + // Check all arguments of called services. + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, + ) + expect(MockFeedbackService.getFormFeedbackStream).not.toHaveBeenCalled() + expect(mockRes.status).toHaveBeenCalledWith(404) + expect(mockRes.json).toHaveBeenCalledWith({ + message: expectedErrorString, + }) + }) + + it('should return 410 when FormDeletedError is returned when retrieving form', async () => { + // Arrange + const mockRes = expressHandler.mockResponse() + // Mock various services to return expected results. + MockUserService.getPopulatedUserById.mockReturnValueOnce( + okAsync(MOCK_USER as IPopulatedUser), + ) + // Mock error when retrieving form. + const expectedErrorString = 'form is deleted' + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( + errAsync(new FormDeletedError(expectedErrorString)), + ) + + // Act + await AdminFormController.handleStreamFormFeedback( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + // Check all arguments of called services. + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, + ) + expect(MockFeedbackService.getFormFeedbackStream).not.toHaveBeenCalled() + expect(mockRes.status).toHaveBeenCalledWith(410) + expect(mockRes.json).toHaveBeenCalledWith({ + message: expectedErrorString, + }) + }) + + it('should return 422 when MissingUserError is returned when retrieving logged in user', async () => { + // Arrange + const mockRes = expressHandler.mockResponse() + // Mock various services to return expected results. + const expectedErrorString = 'user is not found' + MockUserService.getPopulatedUserById.mockReturnValueOnce( + errAsync(new MissingUserError(expectedErrorString)), + ) + + // Act + await AdminFormController.handleStreamFormFeedback( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + // Check all arguments of called services. + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect( + MockAuthService.getFormAfterPermissionChecks, + ).not.toHaveBeenCalled() + expect(MockFeedbackService.getFormFeedbackStream).not.toHaveBeenCalled() + expect(mockRes.status).toHaveBeenCalledWith(422) + expect(mockRes.json).toHaveBeenCalledWith({ + message: expectedErrorString, + }) + }) + + it('should return 500 when database error occurs whilst retrieving user in session', async () => { + // Arrange + const mockRes = expressHandler.mockResponse() + // Mock various services to return expected results. + const expectedErrorString = 'database goes boom' + MockUserService.getPopulatedUserById.mockReturnValueOnce( + errAsync(new DatabaseError(expectedErrorString)), + ) + + // Act + await AdminFormController.handleStreamFormFeedback( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + // Check all arguments of called services. + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect( + MockAuthService.getFormAfterPermissionChecks, + ).not.toHaveBeenCalled() + expect(MockFeedbackService.getFormFeedbackStream).not.toHaveBeenCalled() + expect(mockRes.status).toHaveBeenCalledWith(500) + expect(mockRes.json).toHaveBeenCalledWith({ + message: expectedErrorString, + }) + }) + + it('should return 500 when database error occurs whilst retrieving populated form', async () => { + // Arrange + const mockRes = expressHandler.mockResponse() + // Mock various services to return expected results. + MockUserService.getPopulatedUserById.mockReturnValueOnce( + okAsync(MOCK_USER as IPopulatedUser), + ) + // Mock error when retrieving form. + const expectedErrorString = 'database goes boom' + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( + errAsync(new DatabaseError(expectedErrorString)), + ) + + // Act + await AdminFormController.handleStreamFormFeedback( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + // Check all arguments of called services. + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID.toHexString(), + level: PermissionLevel.Read, + }, + ) + expect(MockFeedbackService.getFormFeedbackStream).not.toHaveBeenCalled() + expect(mockRes.status).toHaveBeenCalledWith(500) + expect(mockRes.json).toHaveBeenCalledWith({ + message: expectedErrorString, + }) + }) + }) }) diff --git a/tests/unit/backend/helpers/jest-express.ts b/tests/unit/backend/helpers/jest-express.ts index 8f6b1d7867..46e571fafb 100644 --- a/tests/unit/backend/helpers/jest-express.ts +++ b/tests/unit/backend/helpers/jest-express.ts @@ -39,6 +39,10 @@ const mockResponse = ( send: jest.fn().mockReturnThis(), sendStatus: jest.fn().mockReturnThis(), type: jest.fn().mockReturnThis(), + pipe: jest.fn().mockReturnThis(), + emit: jest.fn().mockReturnThis(), + on: jest.fn().mockReturnThis(), + end: jest.fn(), json: jest.fn(), render: jest.fn(), redirect: jest.fn(), From ef8cd282d4ff81f352e97427b3807fc16f7f46d5 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Wed, 25 Nov 2020 21:50:29 +0800 Subject: [PATCH 11/12] feat: remove unused code and add jsdoc for handler --- .../admin-forms.server.controller.js | 56 ------------------- .../form/admin-form/admin-form.controller.ts | 11 ++++ 2 files changed, 11 insertions(+), 56 deletions(-) diff --git a/src/app/controllers/admin-forms.server.controller.js b/src/app/controllers/admin-forms.server.controller.js index 14c33e280e..30380c315c 100644 --- a/src/app/controllers/admin-forms.server.controller.js +++ b/src/app/controllers/admin-forms.server.controller.js @@ -6,7 +6,6 @@ const mongoose = require('mongoose') const moment = require('moment-timezone') const _ = require('lodash') -const JSONStream = require('JSONStream') const { StatusCodes } = require('http-status-codes') const logger = require('../../config/logger').createLoggerWithLabel(module) @@ -463,61 +462,6 @@ function makeModule(connection) { } }) }, - /** - * Stream download feedback for a form - * @param {Object} req - Express request object - * @param {Object} req.form - the form to download - * @param {Object} res - Express response object - */ - streamFeedback: function (req, res) { - let FormFeedback = getFormFeedbackModel(connection) - FormFeedback.find({ formId: req.form._id }) - .cursor() - .on('error', function (err) { - logger.error({ - message: 'Error streaming feedback from MongoDB', - meta: { - action: 'makeModule.streamFeedback', - ...createReqMeta(req), - }, - error: err, - }) - res.status(StatusCodes.INTERNAL_SERVER_ERROR).json({ - message: 'Error retrieving from database.', - }) - }) - .pipe(JSONStream.stringify()) - .on('error', function (err) { - logger.error({ - message: 'Error converting feedback to JSON', - meta: { - action: 'makeModule.streamFeedback', - ...createReqMeta(req), - }, - error: err, - }) - res.status(StatusCodes.INTERNAL_SERVER_ERROR).json({ - message: 'Error converting feedback to JSON', - }) - }) - .pipe(res.type('json')) - .on('error', function (err) { - logger.error({ - message: 'Error writing feedback to HTTP stream', - meta: { - action: 'makeModule.streamFeedback', - ...createReqMeta(req), - }, - error: err, - }) - res.status(StatusCodes.INTERNAL_SERVER_ERROR).json({ - message: 'Error writing feedback to HTTP stream', - }) - }) - .on('end', function () { - res.end() - }) - }, /** * Submit feedback when previewing forms * Preview feedback is not stored 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 35576d50ac..45fce0e6e6 100644 --- a/src/app/modules/form/admin-form/admin-form.controller.ts +++ b/src/app/modules/form/admin-form/admin-form.controller.ts @@ -264,6 +264,17 @@ export const handleCountFormFeedback: RequestHandler<{ ) } +/** + * Handler for GET /{formId}/adminform/feedback/download. + * @security session + * + * @returns 200 with feedback stream + * @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 + */ export const handleStreamFormFeedback: RequestHandler<{ formId: string }> = async (req, res) => { From 48538c965e88072bb0922500a3ecfec18dd47a1b Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Thu, 26 Nov 2020 13:58:43 +0800 Subject: [PATCH 12/12] test(AuthSvc): replace assertSpy with inlined mock --- .../auth/__tests__/auth.service.spec.ts | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/app/modules/auth/__tests__/auth.service.spec.ts b/src/app/modules/auth/__tests__/auth.service.spec.ts index 67e80a4252..6c7710c9c3 100644 --- a/src/app/modules/auth/__tests__/auth.service.spec.ts +++ b/src/app/modules/auth/__tests__/auth.service.spec.ts @@ -219,9 +219,10 @@ describe('auth.service', () => { MockFormService.retrieveFullFormById.mockReturnValueOnce( okAsync(expectedForm), ) - const assertSpy = jest.fn().mockImplementation(() => () => ok(true)) MockAdminFormUtils.assertFormAvailable.mockReturnValueOnce(ok(true)) - MockAdminFormUtils.getAssertPermissionFn.mockImplementationOnce(assertSpy) + MockAdminFormUtils.getAssertPermissionFn.mockReturnValueOnce(() => + ok(true), + ) // Act const actualResult = await AuthService.getFormAfterPermissionChecks({ @@ -233,7 +234,9 @@ describe('auth.service', () => { // Assert expect(actualResult.isOk()).toEqual(true) expect(actualResult._unsafeUnwrap()).toEqual(expectedForm) - expect(assertSpy).toHaveBeenCalledWith(PermissionLevel.Write) + expect(MockAdminFormUtils.getAssertPermissionFn).toHaveBeenCalledWith( + PermissionLevel.Write, + ) }) it('should return FormNotFoundError when form does not exist in the database', async () => { @@ -285,10 +288,9 @@ describe('auth.service', () => { ) const expectedError = new ForbiddenFormError('user not allowed') MockAdminFormUtils.assertFormAvailable.mockReturnValueOnce(ok(true)) - const assertSpy = jest - .fn() - .mockImplementation(() => () => err(expectedError)) - MockAdminFormUtils.getAssertPermissionFn.mockImplementationOnce(assertSpy) + MockAdminFormUtils.getAssertPermissionFn.mockReturnValueOnce(() => + err(expectedError), + ) // Act const actualResult = await AuthService.getFormAfterPermissionChecks({ @@ -300,7 +302,9 @@ describe('auth.service', () => { // Assert expect(actualResult.isErr()).toEqual(true) expect(actualResult._unsafeUnwrapErr()).toEqual(expectedError) - expect(assertSpy).toHaveBeenCalledWith(PermissionLevel.Write) + expect(MockAdminFormUtils.getAssertPermissionFn).toHaveBeenCalledWith( + PermissionLevel.Write, + ) }) it('should return DatabaseError when error occurs whilst retrieving form', async () => {