From 6efde5b0a910538fc0b476a5a13fd040773f6633 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Wed, 25 Nov 2020 22:38:08 +0800 Subject: [PATCH 1/7] feat(FormFeedback): add timestamp typings to model types --- src/types/form_feedback.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/types/form_feedback.ts b/src/types/form_feedback.ts index 8ab08cac7d..31a8d00d3f 100644 --- a/src/types/form_feedback.ts +++ b/src/types/form_feedback.ts @@ -11,6 +11,8 @@ export interface IFormFeedback { export interface IFormFeedbackSchema extends IFormFeedback, Document { _id: Document['_id'] + created?: Date + lastModified?: Date } export interface IFormFeedbackDocument extends IFormFeedbackSchema { created: Date From 479eef6040050c533bf6403216dc337b5d19bdf0 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Wed, 25 Nov 2020 22:56:50 +0800 Subject: [PATCH 2/7] feat(FeedbackSvc): add getFormFeedbacks fn (and tests) --- .../__tests__/feedback.service.spec.ts | 55 ++++++++++++++++ src/app/modules/feedback/feedback.service.ts | 62 +++++++++++++++++++ src/app/modules/feedback/feedback.types.ts | 14 +++++ 3 files changed, 131 insertions(+) create mode 100644 src/app/modules/feedback/feedback.types.ts diff --git a/src/app/modules/feedback/__tests__/feedback.service.spec.ts b/src/app/modules/feedback/__tests__/feedback.service.spec.ts index 4dd4730873..37b9e31049 100644 --- a/src/app/modules/feedback/__tests__/feedback.service.spec.ts +++ b/src/app/modules/feedback/__tests__/feedback.service.spec.ts @@ -1,5 +1,6 @@ import { ObjectId } from 'bson-ext' import { times } from 'lodash' +import moment from 'moment-timezone' import mongoose from 'mongoose' import getFormFeedbackModel from 'src/app/models/form_feedback.server.model' @@ -103,4 +104,58 @@ describe('feedback.service', () => { expect(streamSpy).toHaveBeenCalledWith(mockFormId) }) }) + + describe('getFormFeedbacks', () => { + it('should return feedback response successfully', async () => { + // Arrange + const expectedCount = 3 + const mockFormId = new ObjectId().toHexString() + const feedbackPromises = times(expectedCount, (count) => + FormFeedback.create({ + formId: mockFormId, + comment: `cool form ${count}`, + rating: 5 - count, + }), + ) + const createdFeedbacks = await Promise.all(feedbackPromises) + const expectedFeedbackList = createdFeedbacks.map((fb, idx) => ({ + index: idx + 1, + timestamp: moment(fb.created).valueOf(), + rating: fb.rating, + comment: fb.comment, + date: moment(fb.created).tz('Asia/Singapore').format('D MMM YYYY'), + dateShort: moment(fb.created).tz('Asia/Singapore').format('D MMM'), + })) + + // Act + const actualResult = await FeedbackService.getFormFeedbacks(mockFormId) + + // Assert + const expectedAverage = ( + createdFeedbacks.reduce((acc, curr) => acc + curr.rating, 0) / + expectedCount + ).toFixed(2) + expect(actualResult.isOk()).toEqual(true) + expect(actualResult._unsafeUnwrap()).toEqual({ + average: expectedAverage, + count: expectedCount, + feedback: expectedFeedbackList, + }) + }) + + it('should return feedback response with zero count and empty array when no feedback is available', async () => { + // Arrange + const mockFormId = new ObjectId().toHexString() + + // Act + const actualResult = await FeedbackService.getFormFeedbacks(mockFormId) + + // Assert + expect(actualResult.isOk()).toEqual(true) + expect(actualResult._unsafeUnwrap()).toEqual({ + count: 0, + feedback: [], + }) + }) + }) }) diff --git a/src/app/modules/feedback/feedback.service.ts b/src/app/modules/feedback/feedback.service.ts index 6bada6b8aa..8156a8624d 100644 --- a/src/app/modules/feedback/feedback.service.ts +++ b/src/app/modules/feedback/feedback.service.ts @@ -1,3 +1,5 @@ +import { isEmpty } from 'lodash' +import moment from 'moment-timezone' import mongoose from 'mongoose' import { ResultAsync } from 'neverthrow' @@ -7,6 +9,8 @@ import getFormFeedbackModel from '../../models/form_feedback.server.model' import { getMongoErrorMessage } from '../../utils/handle-mongo-error' import { DatabaseError } from '../core/core.errors' +import { FeedbackResponse, ProcessedFeedback } from './feedback.types' + const FormFeedbackModel = getFormFeedbackModel(mongoose) const logger = createLoggerWithLabel(module) @@ -47,3 +51,61 @@ export const getFormFeedbackStream = ( ): mongoose.QueryCursor => { return FormFeedbackModel.getFeedbackCursorByFormId(formId) } + +/** + * Returned processed object containing count of feedback, average rating, and + * list of feedback. + * @param formId the form to retrieve feedback for + * @returns ok(feedback response object) on success + * @returns err(DatabaseError) if database error occurs during query + */ +export const getFormFeedbacks = ( + formId: string, +): ResultAsync => { + return ResultAsync.fromPromise( + FormFeedbackModel.find({ formId }).sort({ created: 1 }).exec(), + (error) => { + logger.error({ + message: 'Error retrieving feedback documents from database', + meta: { + action: 'getFormFeedbacks', + formId, + }, + error, + }) + + return new DatabaseError(getMongoErrorMessage(error)) + }, + ).map((feedbacks) => { + if (isEmpty(feedbacks)) { + return { + count: 0, + feedback: [], + } + } + + // Process retrieved feedback. + const totalFeedbackCount = feedbacks.length + let totalRating = 0 + const processedFeedback = feedbacks.map((fb, idx) => { + totalRating += fb.rating + const response: ProcessedFeedback = { + // 1-based indexing. + index: idx + 1, + timestamp: moment(fb.created).valueOf(), + rating: fb.rating, + comment: fb.comment ?? '', + date: moment(fb.created).tz('Asia/Singapore').format('D MMM YYYY'), + dateShort: moment(fb.created).tz('Asia/Singapore').format('D MMM'), + } + + return response + }) + const averageRating = (totalRating / totalFeedbackCount).toFixed(2) + return { + average: averageRating, + count: totalFeedbackCount, + feedback: processedFeedback, + } + }) +} diff --git a/src/app/modules/feedback/feedback.types.ts b/src/app/modules/feedback/feedback.types.ts new file mode 100644 index 0000000000..36db83d845 --- /dev/null +++ b/src/app/modules/feedback/feedback.types.ts @@ -0,0 +1,14 @@ +export type ProcessedFeedback = { + index: number + timestamp: number + rating: number + comment: string + date: string + dateShort: string +} + +export type FeedbackResponse = { + average?: string + count: number + feedback: ProcessedFeedback[] +} From 285975df7c0b96c907591eb0c229196e8aeb43f7 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Thu, 26 Nov 2020 10:57:33 +0800 Subject: [PATCH 3/7] feat(AdminFormCtl): add handleGetFormFeedbacks handler fn --- .../form/admin-form/admin-form.controller.ts | 43 +++++++++++++++++++ 1 file changed, 43 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 45fce0e6e6..8f8a9490ec 100644 --- a/src/app/modules/form/admin-form/admin-form.controller.ts +++ b/src/app/modules/form/admin-form/admin-form.controller.ts @@ -357,3 +357,46 @@ export const handleStreamFormFeedback: RequestHandler<{ return res.end() }) } + +/** + * Handler for GET /{formId}/adminform/feedback. + * @security session + * + * @returns 200 with feedback response + * @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 + */ +export const handleGetFormFeedbacks: RequestHandler<{ + formId: string +}> = (req, res) => { + const { formId } = req.params + const sessionUserId = (req.session as Express.AuthedSession).user._id + + return UserService.getPopulatedUserById(sessionUserId) + .andThen((user) => + AuthService.getFormAfterPermissionChecks({ + user, + formId, + level: PermissionLevel.Read, + }), + ) + .andThen(() => FeedbackService.getFormFeedbacks(formId)) + .map((fbResponse) => res.json(fbResponse)) + .mapErr((error) => { + logger.error({ + message: 'Error retrieving form feedbacks', + meta: { + action: 'handleGetFormFeedbacks', + ...createReqMeta(req), + userId: sessionUserId, + formId, + }, + error, + }) + const { errorMessage, statusCode } = mapRouteError(error) + return res.status(statusCode).json({ message: errorMessage }) + }) +} From 1b2fef7a3d5f0fbb51da9dd973756f77436e5dac Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Thu, 26 Nov 2020 10:58:03 +0800 Subject: [PATCH 4/7] test(AdminFormCtl): add test cases for handleGetFormFeedbacks --- .../__tests__/admin-form.controller.spec.ts | 308 ++++++++++++++++++ 1 file changed, 308 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 2dc3eacf40..89bfd00249 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 @@ -8,6 +8,7 @@ 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 { FeedbackResponse } from 'src/app/modules/feedback/feedback.types' import * as SubmissionService from 'src/app/modules/submission/submission.service' import { MissingUserError } from 'src/app/modules/user/user.errors' import { IPopulatedForm, IPopulatedUser } from 'src/types' @@ -1272,4 +1273,311 @@ describe('admin-form.controller', () => { }) }) }) + + describe('handleGetFormFeedbacks', () => { + const MOCK_USER_ID = new ObjectId().toHexString() + const MOCK_FORM_ID = new ObjectId().toHexString() + const MOCK_USER = { + _id: MOCK_USER_ID, + email: 'yetanothertest@example.com', + } as IPopulatedUser + const MOCK_FORM = { + admin: MOCK_USER as IPopulatedUser, + _id: MOCK_FORM_ID, + title: 'mock title again', + } as IPopulatedForm + + const MOCK_REQ = expressHandler.mockRequest({ + params: { + formId: MOCK_FORM_ID, + }, + session: { + user: { + _id: MOCK_USER_ID, + }, + }, + }) + + it('should return 200 with feedback response successfully', async () => { + // Arrange + const mockRes = expressHandler.mockResponse() + const expectedFormFeedback: FeedbackResponse = { + count: 212, + feedback: [ + { + comment: 'test feedback', + rating: 5, + date: 'some date', + dateShort: 'some short date', + index: 1, + timestamp: Date.now(), + }, + ], + average: '5.00', + } + // Mock success on all service invocations. + MockUserService.getPopulatedUserById.mockReturnValueOnce( + okAsync(MOCK_USER), + ) + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( + okAsync(MOCK_FORM), + ) + MockFeedbackService.getFormFeedbacks.mockReturnValueOnce( + okAsync(expectedFormFeedback), + ) + + // Act + await AdminFormController.handleGetFormFeedbacks( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + expect(mockRes.json).toHaveBeenCalledWith(expectedFormFeedback) + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID, + level: PermissionLevel.Read, + }, + ) + expect(MockFeedbackService.getFormFeedbacks).toHaveBeenCalledWith( + MOCK_FORM_ID, + ) + }) + + it('should return 403 when user does not have permissions to access form', async () => { + // Arrange + const mockRes = expressHandler.mockResponse() + MockUserService.getPopulatedUserById.mockReturnValueOnce( + okAsync(MOCK_USER), + ) + const mockErrorString = 'not allowed' + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( + errAsync(new ForbiddenFormError(mockErrorString)), + ) + + // Act + await AdminFormController.handleGetFormFeedbacks( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + expect(mockRes.status).toHaveBeenCalledWith(403) + expect(mockRes.json).toHaveBeenCalledWith({ message: mockErrorString }) + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID, + level: PermissionLevel.Read, + }, + ) + expect(MockFeedbackService.getFormFeedbacks).not.toHaveBeenCalled() + }) + + it('should return 404 when form cannot be found', async () => { + // Arrange + const mockRes = expressHandler.mockResponse() + MockUserService.getPopulatedUserById.mockReturnValueOnce( + okAsync(MOCK_USER), + ) + const mockErrorString = 'not found' + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( + errAsync(new FormNotFoundError(mockErrorString)), + ) + + // Act + await AdminFormController.handleGetFormFeedbacks( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + expect(mockRes.status).toHaveBeenCalledWith(404) + expect(mockRes.json).toHaveBeenCalledWith({ message: mockErrorString }) + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID, + level: PermissionLevel.Read, + }, + ) + expect(MockFeedbackService.getFormFeedbacks).not.toHaveBeenCalled() + }) + + it('should return 410 when form is archived', async () => { + // Arrange + const mockRes = expressHandler.mockResponse() + MockUserService.getPopulatedUserById.mockReturnValueOnce( + okAsync(MOCK_USER), + ) + const mockErrorString = 'form gone' + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( + errAsync(new FormDeletedError(mockErrorString)), + ) + + // Act + await AdminFormController.handleGetFormFeedbacks( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + expect(mockRes.status).toHaveBeenCalledWith(410) + expect(mockRes.json).toHaveBeenCalledWith({ message: mockErrorString }) + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID, + level: PermissionLevel.Read, + }, + ) + expect(MockFeedbackService.getFormFeedbacks).not.toHaveBeenCalled() + }) + + it('should return 422 when user in session does not exist in database', async () => { + // Arrange + const mockRes = expressHandler.mockResponse() + const mockErrorString = 'user gone' + MockUserService.getPopulatedUserById.mockReturnValueOnce( + errAsync(new MissingUserError(mockErrorString)), + ) + + // Act + await AdminFormController.handleGetFormFeedbacks( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + expect(mockRes.status).toHaveBeenCalledWith(422) + expect(mockRes.json).toHaveBeenCalledWith({ message: mockErrorString }) + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect( + MockAuthService.getFormAfterPermissionChecks, + ).not.toHaveBeenCalled() + expect(MockFeedbackService.getFormFeedbacks).not.toHaveBeenCalled() + }) + + it('should return 500 when database error occurs whilst retrieving user', async () => { + // Arrange + const mockRes = expressHandler.mockResponse() + const mockErrorString = 'db gone' + MockUserService.getPopulatedUserById.mockReturnValueOnce( + errAsync(new DatabaseError(mockErrorString)), + ) + + // Act + await AdminFormController.handleGetFormFeedbacks( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + expect(mockRes.status).toHaveBeenCalledWith(500) + expect(mockRes.json).toHaveBeenCalledWith({ message: mockErrorString }) + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect( + MockAuthService.getFormAfterPermissionChecks, + ).not.toHaveBeenCalled() + expect(MockFeedbackService.getFormFeedbacks).not.toHaveBeenCalled() + }) + + it('should return 500 when database error occurs whilst retrieving form', async () => { + // Arrange + const mockRes = expressHandler.mockResponse() + MockUserService.getPopulatedUserById.mockReturnValueOnce( + okAsync(MOCK_USER), + ) + const mockErrorString = 'db error' + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( + errAsync(new DatabaseError(mockErrorString)), + ) + + // Act + await AdminFormController.handleGetFormFeedbacks( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + expect(mockRes.status).toHaveBeenCalledWith(500) + expect(mockRes.json).toHaveBeenCalledWith({ message: mockErrorString }) + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID, + level: PermissionLevel.Read, + }, + ) + expect(MockFeedbackService.getFormFeedbacks).not.toHaveBeenCalled() + }) + + it('should return 500 when database error occurs whilst retrieving form feedback', async () => { + // Arrange + const mockRes = expressHandler.mockResponse() + // Mock success on all service invocations. + MockUserService.getPopulatedUserById.mockReturnValueOnce( + okAsync(MOCK_USER), + ) + MockAuthService.getFormAfterPermissionChecks.mockReturnValueOnce( + okAsync(MOCK_FORM), + ) + const mockErrorString = 'db boom' + MockFeedbackService.getFormFeedbacks.mockReturnValueOnce( + errAsync(new DatabaseError(mockErrorString)), + ) + + // Act + await AdminFormController.handleGetFormFeedbacks( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + expect(mockRes.status).toHaveBeenCalledWith(500) + expect(mockRes.json).toHaveBeenCalledWith({ message: mockErrorString }) + expect(MockUserService.getPopulatedUserById).toHaveBeenCalledWith( + MOCK_USER_ID, + ) + expect(MockAuthService.getFormAfterPermissionChecks).toHaveBeenCalledWith( + { + user: MOCK_USER, + formId: MOCK_FORM_ID, + level: PermissionLevel.Read, + }, + ) + expect(MockFeedbackService.getFormFeedbacks).toHaveBeenCalledWith( + MOCK_FORM_ID, + ) + }) + }) }) From bcb4aeb2dc8b932daaad850fc0616d9a471e0c00 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Thu, 26 Nov 2020 11:02:06 +0800 Subject: [PATCH 5/7] test(FeedbackSvc): improve robustness of success test case --- .../feedback/__tests__/feedback.service.spec.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/app/modules/feedback/__tests__/feedback.service.spec.ts b/src/app/modules/feedback/__tests__/feedback.service.spec.ts index 37b9e31049..7d3f56f360 100644 --- a/src/app/modules/feedback/__tests__/feedback.service.spec.ts +++ b/src/app/modules/feedback/__tests__/feedback.service.spec.ts @@ -106,19 +106,25 @@ describe('feedback.service', () => { }) describe('getFormFeedbacks', () => { - it('should return feedback response successfully', async () => { + it('should return correct feedback responses', async () => { // Arrange const expectedCount = 3 const mockFormId = new ObjectId().toHexString() - const feedbackPromises = times(expectedCount, (count) => + const expectedFbPromises = times(expectedCount, (count) => FormFeedback.create({ formId: mockFormId, comment: `cool form ${count}`, rating: 5 - count, }), ) - const createdFeedbacks = await Promise.all(feedbackPromises) - const expectedFeedbackList = createdFeedbacks.map((fb, idx) => ({ + // Add another feedback with a different form id. + await FormFeedback.create({ + formId: new ObjectId(), + comment: 'boo this form sux', + rating: 1, + }) + const expectedCreatedFbs = await Promise.all(expectedFbPromises) + const expectedFeedbackList = expectedCreatedFbs.map((fb, idx) => ({ index: idx + 1, timestamp: moment(fb.created).valueOf(), rating: fb.rating, @@ -131,8 +137,9 @@ describe('feedback.service', () => { const actualResult = await FeedbackService.getFormFeedbacks(mockFormId) // Assert + // Should only average from the feedbacks for given formId. const expectedAverage = ( - createdFeedbacks.reduce((acc, curr) => acc + curr.rating, 0) / + expectedCreatedFbs.reduce((acc, curr) => acc + curr.rating, 0) / expectedCount ).toFixed(2) expect(actualResult.isOk()).toEqual(true) From 6910cc76b51c5fdc847a4d5dfab9fc4a00c00aa6 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Thu, 26 Nov 2020 11:18:34 +0800 Subject: [PATCH 6/7] ref(AdminFormsRoutes): use new AdminFormCtl#handleGetFormFeedbacks fn also remove now unused code --- .../admin-forms.server.controller.js | 48 ---------------- src/app/routes/admin-forms.server.routes.js | 2 +- .../admin-forms.server.controller.spec.js | 57 ------------------- 3 files changed, 1 insertion(+), 106 deletions(-) diff --git a/src/app/controllers/admin-forms.server.controller.js b/src/app/controllers/admin-forms.server.controller.js index 30380c315c..9c5a2560eb 100644 --- a/src/app/controllers/admin-forms.server.controller.js +++ b/src/app/controllers/admin-forms.server.controller.js @@ -4,7 +4,6 @@ * Module dependencies. */ const mongoose = require('mongoose') -const moment = require('moment-timezone') const _ = require('lodash') const { StatusCodes } = require('http-status-codes') @@ -22,8 +21,6 @@ const { getEmailFormModel, } = require('../models/form.server.model') const getFormModel = require('../models/form.server.model').default -const getFormFeedbackModel = require('../models/form_feedback.server.model') - .default const getSubmissionModel = require('../models/submission.server.model').default const { ResponseMode } = require('../../types') @@ -417,51 +414,6 @@ function makeModule(connection) { } }) }, - /** - * Return form feedback matching query - * @param {Object} req - Express request object - * @param {Object} res - Express response object - */ - getFeedback: function (req, res) { - let FormFeedback = getFormFeedbackModel(connection) - let query = FormFeedback.find({ formId: req.form._id }).sort({ - created: 1, - }) - query.exec(function (err, feedback) { - if (err) { - return respondOnMongoError(req, res, err) - } else if (!feedback) { - return res - .status(StatusCodes.NOT_FOUND) - .json({ message: 'No feedback found' }) - } else { - let sum = 0 - let count = 0 - feedback = feedback.map(function (element) { - sum += element.rating - count += 1 - return { - index: count, - timestamp: moment(element.created).valueOf(), - rating: element.rating, - comment: element.comment, - date: moment(element.created) - .tz('Asia/Singapore') - .format('D MMM YYYY'), - dateShort: moment(element.created) - .tz('Asia/Singapore') - .format('D MMM'), - } - }) - let average = count > 0 ? (sum / count).toFixed(2) : undefined - return res.json({ - average: average, - count: count, - feedback: feedback, - }) - } - }) - }, /** * Submit feedback when previewing forms * Preview feedback is not stored diff --git a/src/app/routes/admin-forms.server.routes.js b/src/app/routes/admin-forms.server.routes.js index 211382112d..881116cedb 100644 --- a/src/app/routes/admin-forms.server.routes.js +++ b/src/app/routes/admin-forms.server.routes.js @@ -269,7 +269,7 @@ module.exports = function (app) { app .route('/:formId([a-fA-F0-9]{24})/adminform/feedback') - .get(authActiveForm(PermissionLevel.Read), adminForms.getFeedback) + .get(withUserAuthentication, AdminFormController.handleGetFormFeedbacks) .post(authActiveForm(PermissionLevel.Read), adminForms.passThroughFeedback) /** diff --git a/tests/unit/backend/controllers/admin-forms.server.controller.spec.js b/tests/unit/backend/controllers/admin-forms.server.controller.spec.js index f28ca9165f..1e24985f16 100644 --- a/tests/unit/backend/controllers/admin-forms.server.controller.spec.js +++ b/tests/unit/backend/controllers/admin-forms.server.controller.spec.js @@ -6,10 +6,6 @@ const roles = require('../helpers/roles') const User = dbHandler.makeModel('user.server.model', 'User') const Form = dbHandler.makeModel('form.server.model', 'Form') -const FormFeedback = dbHandler.makeModel( - 'form_feedback.server.model', - 'FormFeedback', -) const EmailForm = mongoose.model('email') const Controller = spec( @@ -291,57 +287,4 @@ describe('Admin-Forms Controller', () => { Controller.transferOwner(req, res) }) }) - - describe('getFeedback', () => { - it('should retrieve correct response based on saved FormFeedbacks', (done) => { - // Define feedback to be added to MongoMemoryServer db - let p1 = new FormFeedback({ - formId: mongoose.Types.ObjectId('4edd40c86762e0fb12000003'), - rating: 2, - comment: 'nice', - created: '2018-05-18 09:12:07.126Z', - }).save() - let p2 = new FormFeedback({ - formId: mongoose.Types.ObjectId('4edd40c86762e0fb12000003'), - rating: 5, - comment: 'great', - created: '2018-03-15 03:52:07.126Z', - }).save() - - Promise.all([p1, p2]).then(([_r1, _r2]) => { - req.form = { _id: '4edd40c86762e0fb12000003' } - let expected = { - average: '3.50', - count: 2, - feedback: [ - { - index: 2, - timestamp: 1526634727126, - rating: 2, - comment: 'nice', - date: '18 May 2018', - dateShort: '18 May', - }, - { - index: 1, - timestamp: 1521085927126, - rating: 5, - comment: 'great', - date: '15 Mar 2018', - dateShort: '15 Mar', - }, - ], - } - res.json.and.callFake((args) => { - expect(args.average).toEqual(expected.average) - expect(args.count).toEqual(expected.count) - expect(args.feedback.length).toEqual(2) - expect(args.feedback).toContain(expected.feedback[0]) - expect(args.feedback).toContain(expected.feedback[1]) - done() - }) - Controller.getFeedback(req, res) - }) - }) - }) }) From fe73da90fbcec868bb8df9f40079758ee12fe440 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Thu, 26 Nov 2020 11:19:13 +0800 Subject: [PATCH 7/7] test(FeedbackSvc): add missing test cases --- .../__tests__/feedback.service.spec.ts | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/src/app/modules/feedback/__tests__/feedback.service.spec.ts b/src/app/modules/feedback/__tests__/feedback.service.spec.ts index 7d3f56f360..242d133c21 100644 --- a/src/app/modules/feedback/__tests__/feedback.service.spec.ts +++ b/src/app/modules/feedback/__tests__/feedback.service.spec.ts @@ -9,6 +9,7 @@ import dbHandler from 'tests/unit/backend/helpers/jest-db' import { DatabaseError } from '../../core/core.errors' import * as FeedbackService from '../feedback.service' +import { FeedbackResponse } from '../feedback.types' const FormFeedback = getFormFeedbackModel(mongoose) @@ -164,5 +165,65 @@ describe('feedback.service', () => { feedback: [], }) }) + + it('should return feedback response with empty string comment if feedback comment is undefined', async () => { + // Arrange + const mockFormId = new ObjectId().toHexString() + const createdFb = await FormFeedback.create({ + formId: mockFormId, + // Missing comment key value. + rating: 3, + }) + + // Act + const actualResult = await FeedbackService.getFormFeedbacks(mockFormId) + + // Assert + const expectedResult: FeedbackResponse = { + count: 1, + average: '3.00', + feedback: [ + { + index: 1, + timestamp: moment(createdFb.created).valueOf(), + rating: createdFb.rating, + // Empty comment string + comment: '', + date: moment(createdFb.created) + .tz('Asia/Singapore') + .format('D MMM YYYY'), + dateShort: moment(createdFb.created) + .tz('Asia/Singapore') + .format('D MMM'), + }, + ], + } + expect(actualResult.isOk()).toEqual(true) + expect(actualResult._unsafeUnwrap()).toEqual(expectedResult) + }) + + it('should return DatabaseError when error occurs whilst querying database', async () => { + // Arrange + const mockFormId = new ObjectId().toHexString() + const sortSpy = jest.fn().mockReturnThis() + const findSpy = jest.spyOn(FormFeedback, 'find').mockImplementationOnce( + () => + (({ + sort: sortSpy, + exec: () => Promise.reject(new Error('boom')), + } as unknown) as mongoose.Query), + ) + + // Act + const actualResult = await FeedbackService.getFormFeedbacks(mockFormId) + + // Assert + expect(findSpy).toHaveBeenCalledWith({ + formId: mockFormId, + }) + expect(sortSpy).toHaveBeenCalledWith({ created: 1 }) + expect(actualResult.isErr()).toEqual(true) + expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(DatabaseError) + }) }) })