diff --git a/apps/backend/src/app/models/__tests__/admin_feedback.server.model.spec.ts b/apps/backend/src/app/models/__tests__/admin_feedback.server.model.spec.ts index b8ddc85a3d..ffef0f0054 100644 --- a/apps/backend/src/app/models/__tests__/admin_feedback.server.model.spec.ts +++ b/apps/backend/src/app/models/__tests__/admin_feedback.server.model.spec.ts @@ -50,16 +50,18 @@ describe('form_feedback.server.model', () => { ) }) - it('should throw validation error when rating param is missing', async () => { + it('should save successfully when rating param is missing (now optional; new rows use csat)', async () => { // Arrange const paramsWithoutRating = omit(DEFAULT_PARAMS, 'rating') // Act - const actualPromise = new FeedbackModel(paramsWithoutRating).save() + const actual = await FeedbackModel.create({ + ...paramsWithoutRating, + csat: 4, + }) // Assert - await expect(actualPromise).rejects.toThrow( - mongoose.Error.ValidationError, - ) + expect(actual.rating).toBeUndefined() + expect(actual.csat).toEqual(4) }) it('should save successfully with triggerSource and formId', async () => { @@ -137,6 +139,38 @@ describe('form_feedback.server.model', () => { expect(actual.rating).toEqual(0) }) + it('should accept csat values 1 through 5', async () => { + for (const csat of [1, 2, 3, 4, 5] as const) { + const actual = await FeedbackModel.create({ + ...omit(DEFAULT_PARAMS, 'rating'), + csat, + }) + expect(actual.csat).toEqual(csat) + } + }) + + it('should reject csat value of 6', async () => { + const actualPromise = new FeedbackModel({ + ...omit(DEFAULT_PARAMS, 'rating'), + csat: 6, + }).save() + + await expect(actualPromise).rejects.toThrow( + mongoose.Error.ValidationError, + ) + }) + + it('should reject csat value of 0 (csat is 1-5, unlike legacy rating)', async () => { + const actualPromise = new FeedbackModel({ + ...omit(DEFAULT_PARAMS, 'rating'), + csat: 0, + }).save() + + await expect(actualPromise).rejects.toThrow( + mongoose.Error.ValidationError, + ) + }) + it('should default ratingChanged to false', async () => { const actual = await FeedbackModel.create(DEFAULT_PARAMS) expect(actual.ratingChanged).toEqual(false) diff --git a/apps/backend/src/app/models/admin_feedback.server.model.ts b/apps/backend/src/app/models/admin_feedback.server.model.ts index 76f0f73a6f..c2423e6920 100644 --- a/apps/backend/src/app/models/admin_feedback.server.model.ts +++ b/apps/backend/src/app/models/admin_feedback.server.model.ts @@ -17,11 +17,20 @@ const AdminFeedbackSchema = new Schema< ref: USER_SCHEMA_ID, required: true, }, + // Legacy thumbs (0/1) key. Optional so new rows can omit it in favour of `csat`. rating: { type: Number, min: 0, max: 5, - required: true, + required: false, + }, + // CSAT: the new 1-5 star satisfaction score. Own key to keep it separate from + // legacy `rating` data. + csat: { + type: Number, + min: 1, + max: 5, + required: false, }, comment: { type: String, diff --git a/apps/backend/src/app/modules/admin-feedback/__tests__/admin_feedback.service.spec.ts b/apps/backend/src/app/modules/admin-feedback/__tests__/admin_feedback.service.spec.ts index 0ae2acd461..965bb76ff6 100644 --- a/apps/backend/src/app/modules/admin-feedback/__tests__/admin_feedback.service.spec.ts +++ b/apps/backend/src/app/modules/admin-feedback/__tests__/admin_feedback.service.spec.ts @@ -26,7 +26,7 @@ describe('feedback.service', () => { const MOCK_COMMENT = 'mock comment' const MOCK_ADMIN_FEEDBACK = new AdminFeedbackModel({ userId: MOCK_USER_ID, - rating: MOCK_RATING, + csat: MOCK_RATING, comment: MOCK_COMMENT, }) @@ -41,7 +41,7 @@ describe('feedback.service', () => { // Act const actualResult = await AdminFeedbackService.insertAdminFeedback({ userId: MOCK_USER_ID, - rating: MOCK_RATING, + csat: MOCK_RATING, comment: MOCK_COMMENT, }) @@ -54,7 +54,7 @@ describe('feedback.service', () => { it('should return Admin Feedback document on successful insertion without comment', async () => { const mock_feedback_no_comment = new AdminFeedbackModel({ userId: MOCK_USER_ID, - rating: MOCK_RATING, + csat: MOCK_RATING, }) // Arrange // Mock success. @@ -66,7 +66,7 @@ describe('feedback.service', () => { // Act const actualResult = await AdminFeedbackService.insertAdminFeedback({ userId: MOCK_USER_ID, - rating: MOCK_RATING, + csat: MOCK_RATING, }) // Assert @@ -80,7 +80,7 @@ describe('feedback.service', () => { const mockFormId = new ObjectId().toHexString() const mockFeedbackWithTrigger = new AdminFeedbackModel({ userId: MOCK_USER_ID, - rating: MOCK_RATING, + csat: MOCK_RATING, comment: MOCK_COMMENT, triggerSource: mockTriggerSource, formId: mockFormId, @@ -93,7 +93,7 @@ describe('feedback.service', () => { const actualResult = await AdminFeedbackService.insertAdminFeedback({ userId: MOCK_USER_ID, - rating: MOCK_RATING, + csat: MOCK_RATING, comment: MOCK_COMMENT, triggerSource: mockTriggerSource, formId: mockFormId, @@ -102,7 +102,7 @@ describe('feedback.service', () => { expect(insertSpy).toHaveBeenCalledTimes(1) expect(insertSpy).toHaveBeenCalledWith({ userId: MOCK_USER_ID, - rating: MOCK_RATING, + csat: MOCK_RATING, comment: MOCK_COMMENT, triggerSource: mockTriggerSource, formId: mockFormId, @@ -122,7 +122,7 @@ describe('feedback.service', () => { // Act const actualResult = await AdminFeedbackService.insertAdminFeedback({ userId: MOCK_USER_ID, - rating: MOCK_RATING, + csat: MOCK_RATING, }) // Assert @@ -143,7 +143,7 @@ describe('feedback.service', () => { await AdminFeedbackModel.create({ _id: MOCK_FEEDBACK_ID, userId: MOCK_USER_ID, - rating: MOCK_RATING, + csat: MOCK_RATING, comment: MOCK_COMMENT, }) }) @@ -157,7 +157,7 @@ describe('feedback.service', () => { const actualResult = await AdminFeedbackService.updateAdminFeedback({ feedbackId: MOCK_FEEDBACK_ID, userId: MOCK_USER_ID, - rating: newRating, + csat: newRating, comment: newComment, }) @@ -166,7 +166,7 @@ describe('feedback.service', () => { // Assert expect(actualResult.isOk()).toEqual(true) expect(newFeedback?.comment).toEqual(newComment) - expect(newFeedback?.rating).toEqual(newRating) + expect(newFeedback?.csat).toEqual(newRating) }) it('should set ratingChanged to true when rating is updated', async () => { @@ -175,13 +175,13 @@ describe('feedback.service', () => { await AdminFeedbackService.updateAdminFeedback({ feedbackId: MOCK_FEEDBACK_ID, userId: MOCK_USER_ID, - rating: newRating, + csat: newRating, }) const newFeedback = await AdminFeedbackModel.findById(MOCK_FEEDBACK_ID) expect(newFeedback?.ratingChanged).toEqual(true) - expect(newFeedback?.rating).toEqual(newRating) + expect(newFeedback?.csat).toEqual(newRating) }) it('should not set ratingChanged when only comment is updated', async () => { @@ -200,7 +200,7 @@ describe('feedback.service', () => { const newRating = 3 const expectedResult = new AdminFeedbackModel({ userId: MOCK_USER_ID, - rating: newRating, + csat: newRating, comment: MOCK_COMMENT, }) @@ -208,7 +208,7 @@ describe('feedback.service', () => { const actualResult = await AdminFeedbackService.updateAdminFeedback({ feedbackId: MOCK_FEEDBACK_ID, userId: MOCK_USER_ID, - rating: newRating, + csat: newRating, }) const newFeedback = await AdminFeedbackModel.findById(MOCK_FEEDBACK_ID) @@ -216,14 +216,14 @@ describe('feedback.service', () => { // Assert expect(actualResult.isOk()).toEqual(true) expect(newFeedback?.comment).toEqual(expectedResult.comment) - expect(newFeedback?.rating).toEqual(expectedResult.rating) + expect(newFeedback?.csat).toEqual(expectedResult.csat) }) it('should update feedback successfully with only comment changes', async () => { const newComment = 'new comment' const expectedResult = new AdminFeedbackModel({ userId: MOCK_USER_ID, - rating: MOCK_RATING, + csat: MOCK_RATING, comment: newComment, }) @@ -239,7 +239,7 @@ describe('feedback.service', () => { // Assert expect(actualResult.isOk()).toEqual(true) expect(newFeedback?.comment).toEqual(expectedResult.comment) - expect(newFeedback?.rating).toEqual(expectedResult.rating) + expect(newFeedback?.csat).toEqual(expectedResult.csat) }) it('should return ok if there are no changes to be made', async () => { diff --git a/apps/backend/src/app/modules/admin-feedback/admin-feedback.controller.ts b/apps/backend/src/app/modules/admin-feedback/admin-feedback.controller.ts index 2bb64e8cf6..2bf0115205 100644 --- a/apps/backend/src/app/modules/admin-feedback/admin-feedback.controller.ts +++ b/apps/backend/src/app/modules/admin-feedback/admin-feedback.controller.ts @@ -45,20 +45,22 @@ const submitAdminFeedback: ControllerHandler< const { rating, comment, triggerSource, formId } = req.body // Dual-emit: old metric kept so existing dashboards don't break during - // migration. New metric (.v2) for the 1-5 scale. Remove the old metric - // once report card has migrated to .v2. + // migration. New `.csat` metric is the 1-5 CSAT measure. Remove the old + // metric once the report card has migrated to `.csat`. statsdClient.distribution('formsg.users.feedback.rating', rating, 1, { rating: `${rating}`, ...(triggerSource ? { triggerSource } : {}), }) - statsdClient.distribution('formsg.users.feedback.rating.v2', rating, 1, { - rating: `${rating}`, + statsdClient.distribution('formsg.users.feedback.csat', rating, 1, { + csat: `${rating}`, ...(triggerSource ? { triggerSource } : {}), }) + // The wire keeps sending `rating`; store it under its own `csat` key so the + // new measure never mixes with legacy thumbs data. return AdminFeedbackService.insertAdminFeedback({ userId: sessionUserId, - rating, + csat: rating, comment, triggerSource, formId, @@ -109,11 +111,12 @@ const updateAdminFeedback: ControllerHandler< const { rating, comment } = req.body + // The wire keeps sending `rating`; store it under its own `csat` key. return AdminFeedbackService.updateAdminFeedback({ feedbackId, userId: sessionUserId, comment, - rating, + csat: rating, }) .map(() => res diff --git a/apps/backend/src/app/modules/admin-feedback/admin-feedback.service.ts b/apps/backend/src/app/modules/admin-feedback/admin-feedback.service.ts index 61f6728645..9f9a919073 100644 --- a/apps/backend/src/app/modules/admin-feedback/admin-feedback.service.ts +++ b/apps/backend/src/app/modules/admin-feedback/admin-feedback.service.ts @@ -14,20 +14,20 @@ const logger = createLoggerWithLabel(module) /** * Inserts given admin feedback to the database. * @param userId the userId of the admin that provided the feedback - * @param rating the feedback rating to insert (0 for thumbs down, 1 for thumbs up) + * @param csat the 1-5 star satisfaction score to insert * @param comment the feedback comment to insert if available * @returns ok(IAdminFeedbackSchema) if successfully inserted * @returns err(DatabaseError) on database error */ export const insertAdminFeedback = ({ userId, - rating, + csat, comment, triggerSource, formId, }: { userId: string - rating: number + csat: number comment?: string triggerSource?: string formId?: string @@ -35,7 +35,7 @@ export const insertAdminFeedback = ({ return ResultAsync.fromPromise( AdminFeedbackModel.create({ userId, - rating, + csat, comment, triggerSource, formId, @@ -57,11 +57,11 @@ export const insertAdminFeedback = ({ /** * Updates admin feedback in the database. - * Will not update previous value if comment or rating is undefined + * Will not update previous value if comment or csat is undefined * @param feedbackId the id of the admin feedback to update * @param userId the id of the admin * @param comment the feedback comment to insert - * @param rating the feedback rating to insert (0 for thumbs down, 1 for thumbs up) + * @param csat the 1-5 star satisfaction score to insert * @returns ok(IAdminFeedbackSchema) if successfully inserted * @returns err(MissingAdminFeedbackError) if feedback document with the same feedbackId and userId is not found * @returns err(DatabaseError) on database error @@ -70,17 +70,18 @@ export const updateAdminFeedback = ({ feedbackId, userId, comment, - rating, + csat, }: { feedbackId: string userId: string comment?: string - rating?: number + csat?: number }) => { const updateObj: Record = {} if (comment !== undefined) updateObj.comment = comment - if (rating !== undefined) { - updateObj.rating = rating + if (csat !== undefined) { + updateObj.csat = csat + // `ratingChanged` is a pre-existing flag; it now reflects a `csat` edit. updateObj.ratingChanged = true } diff --git a/apps/backend/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.form.routes.spec.ts b/apps/backend/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.form.routes.spec.ts index dfd79b1bd9..6f16e545a5 100644 --- a/apps/backend/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.form.routes.spec.ts +++ b/apps/backend/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.form.routes.spec.ts @@ -24,6 +24,7 @@ import mongoose, { Types } from 'mongoose' import { err, errAsync, okAsync } from 'neverthrow' import supertest, { Session } from 'supertest-session' +import { statsdClient } from 'src/app/config/datadog-statsd-client' import getAdminFeedbackModel from 'src/app/models/admin_feedback.server.model' import getFormModel, { getEmailFormModel, @@ -3165,6 +3166,9 @@ describe('admin-form.form.routes', () => { const MOCK_RATING = 3 it('should return 200 when the request is successful', async () => { + // Arrange + const distributionSpy = jest.spyOn(statsdClient, 'distribution') + // Act const resp = await request .post(`/admin/forms/feedback`) @@ -3175,8 +3179,16 @@ describe('admin-form.form.routes', () => { expect(resp.body.message).toContain( 'Successfully submitted admin feedback', ) - expect(resp.body.feedback.rating).toEqual(MOCK_RATING) + // Wire sends `rating`; it is stored and returned under the `csat` key. + expect(resp.body.feedback.csat).toEqual(MOCK_RATING) + expect(resp.body.feedback.rating).toBeUndefined() expect(resp.body.feedback.comment).toEqual(MOCK_COMMENT) + expect(distributionSpy).toHaveBeenCalledWith( + 'formsg.users.feedback.csat', + MOCK_RATING, + 1, + expect.objectContaining({ csat: `${MOCK_RATING}` }), + ) }) it('should return 200 when the request is successful without comments', async () => { @@ -3190,7 +3202,7 @@ describe('admin-form.form.routes', () => { expect(resp.body.message).toContain( 'Successfully submitted admin feedback', ) - expect(resp.body.feedback.rating).toEqual(MOCK_RATING) + expect(resp.body.feedback.csat).toEqual(MOCK_RATING) expect(resp.body.feedback.comment).toBeUndefined() }) @@ -3279,7 +3291,8 @@ describe('admin-form.form.routes', () => { expect(resp.status).toEqual(200) expect(resp.body.message).toEqual('Successfully updated admin feedback') expect(updatedFeedback?.comment).toEqual(MOCK_NEW_COMMENT) - expect(updatedFeedback?.rating).toEqual(MOCK_NEW_RATING) + // Wire sends `rating`; it is stored under the `csat` key. + expect(updatedFeedback?.csat).toEqual(MOCK_NEW_RATING) }) it('should return 200 without changes if request is successful without a body', async () => { diff --git a/packages/shared/types/admin_feedback.ts b/packages/shared/types/admin_feedback.ts index c0cdd2ef73..06b9d4669e 100644 --- a/packages/shared/types/admin_feedback.ts +++ b/packages/shared/types/admin_feedback.ts @@ -5,7 +5,10 @@ export type AdminFeedbackTriggerSource = 'field-edit' | 'publish' | 'workflow' export type AdminFeedbackRating = 1 | 2 | 3 | 4 | 5 export type AdminFeedbackBase = { - rating: AdminFeedbackRating + /** @deprecated Legacy thumbs (0/1) key. Kept for historical rows; new rows use `csat`. */ + rating?: number + /** CSAT: the 1-5 star satisfaction score. Own key so it never mixes with legacy `rating`. */ + csat?: AdminFeedbackRating comment?: string triggerSource?: AdminFeedbackTriggerSource formId?: string