Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -61,5 +61,93 @@ describe('form_feedback.server.model', () => {
mongoose.Error.ValidationError,
)
})

it('should save successfully with triggerSource and formId', async () => {
const paramsWithTrigger = {
...DEFAULT_PARAMS,
triggerSource: 'publish',
formId: new Types.ObjectId(),
}
const actual = await FeedbackModel.create(paramsWithTrigger)

expect(actual).toEqual(
expect.objectContaining({
...paramsWithTrigger,
created: expect.any(Date),
lastModified: expect.any(Date),
}),
)
})

it('should save successfully without triggerSource and formId (backwards compat)', async () => {
const actual = await FeedbackModel.create(DEFAULT_PARAMS)

expect(actual.triggerSource).toBeUndefined()
expect(actual.formId).toBeUndefined()
})

it('should throw validation error for invalid triggerSource enum value', async () => {
const paramsWithInvalidTrigger = {
...DEFAULT_PARAMS,
triggerSource: 'invalid-source',
}
const actualPromise = new FeedbackModel(paramsWithInvalidTrigger).save()

await expect(actualPromise).rejects.toThrow(
mongoose.Error.ValidationError,
)
})

it('should accept all valid triggerSource values', async () => {
for (const source of ['field-edit', 'publish', 'workflow']) {
const actual = await FeedbackModel.create({
...DEFAULT_PARAMS,
triggerSource: source,
})
expect(actual.triggerSource).toEqual(source)
}
})

it('should accept rating values 1 through 5', async () => {
for (const rating of [1, 2, 3, 4, 5]) {
const actual = await FeedbackModel.create({
...DEFAULT_PARAMS,
rating,
})
expect(actual.rating).toEqual(rating)
}
})

it('should reject rating value of 6', async () => {
const actualPromise = new FeedbackModel({
...DEFAULT_PARAMS,
rating: 6,
}).save()

await expect(actualPromise).rejects.toThrow(
mongoose.Error.ValidationError,
)
})

it('should still accept rating value of 0 (backwards compat with old thumbs-down records)', async () => {
const actual = await FeedbackModel.create({
...DEFAULT_PARAMS,
rating: 0,
})
expect(actual.rating).toEqual(0)
})

it('should default ratingChanged to false', async () => {
const actual = await FeedbackModel.create(DEFAULT_PARAMS)
expect(actual.ratingChanged).toEqual(false)
})

it('should save with ratingChanged set to true', async () => {
const actual = await FeedbackModel.create({
...DEFAULT_PARAMS,
ratingChanged: true,
})
expect(actual.ratingChanged).toEqual(true)
})
})
})
13 changes: 12 additions & 1 deletion apps/backend/src/app/models/admin_feedback.server.model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,25 @@ const AdminFeedbackSchema = new Schema<
rating: {
type: Number,
min: 0,
max: 1,
max: 5,
required: true,
},
comment: {
type: String,
required: false,
trim: true,
},
triggerSource: {
type: String,
enum: ['field-edit', 'publish', 'workflow'],
},
formId: {
type: Schema.Types.ObjectId,
},
ratingChanged: {
type: Boolean,
default: false,
},
},
{
timestamps: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,42 @@ describe('feedback.service', () => {
expect(actualResult._unsafeUnwrap()).toEqual(mock_feedback_no_comment)
})

it('should return Admin Feedback document on successful insertion with triggerSource and formId', async () => {
const mockTriggerSource = 'publish'
const mockFormId = new ObjectId().toHexString()
const mockFeedbackWithTrigger = new AdminFeedbackModel({
userId: MOCK_USER_ID,
rating: MOCK_RATING,
comment: MOCK_COMMENT,
triggerSource: mockTriggerSource,
formId: mockFormId,
})

const insertSpy = jest
.spyOn(AdminFeedbackModel, 'create')
// @ts-ignore
.mockResolvedValueOnce(mockFeedbackWithTrigger)

const actualResult = await AdminFeedbackService.insertAdminFeedback({
userId: MOCK_USER_ID,
rating: MOCK_RATING,
comment: MOCK_COMMENT,
triggerSource: mockTriggerSource,
formId: mockFormId,
})

expect(insertSpy).toHaveBeenCalledTimes(1)
expect(insertSpy).toHaveBeenCalledWith({
userId: MOCK_USER_ID,
rating: MOCK_RATING,
comment: MOCK_COMMENT,
triggerSource: mockTriggerSource,
formId: mockFormId,
})
expect(actualResult.isOk()).toEqual(true)
expect(actualResult._unsafeUnwrap()).toEqual(mockFeedbackWithTrigger)
})

it('should return DatabaseError when error occurs whilst inserting feedback', async () => {
// Arrange
// Mock failure
Expand Down Expand Up @@ -114,13 +150,8 @@ describe('feedback.service', () => {
afterEach(() => jest.clearAllMocks())

it('should update feedback successfully with both rating and comment changes', async () => {
const newRating = 0 as number
const newRating = 4
const newComment = 'new comment'
const expectedResult = new AdminFeedbackModel({
userId: MOCK_USER_ID,
rating: newRating,
comment: newComment,
})

// Act
const actualResult = await AdminFeedbackService.updateAdminFeedback({
Expand All @@ -134,12 +165,39 @@ describe('feedback.service', () => {

// Assert
expect(actualResult.isOk()).toEqual(true)
expect(newFeedback?.comment).toEqual(expectedResult.comment)
expect(newFeedback?.rating).toEqual(expectedResult.rating)
expect(newFeedback?.comment).toEqual(newComment)
expect(newFeedback?.rating).toEqual(newRating)
})

it('should set ratingChanged to true when rating is updated', async () => {
const newRating = 5

await AdminFeedbackService.updateAdminFeedback({
feedbackId: MOCK_FEEDBACK_ID,
userId: MOCK_USER_ID,
rating: newRating,
})

const newFeedback = await AdminFeedbackModel.findById(MOCK_FEEDBACK_ID)

expect(newFeedback?.ratingChanged).toEqual(true)
expect(newFeedback?.rating).toEqual(newRating)
})

it('should not set ratingChanged when only comment is updated', async () => {
await AdminFeedbackService.updateAdminFeedback({
feedbackId: MOCK_FEEDBACK_ID,
userId: MOCK_USER_ID,
comment: 'just a comment',
})

const newFeedback = await AdminFeedbackModel.findById(MOCK_FEEDBACK_ID)

expect(newFeedback?.ratingChanged).toEqual(false)
})

it('should update feedback successfully with only rating change', async () => {
const newRating = 0 as number
const newRating = 3
const expectedResult = new AdminFeedbackModel({
userId: MOCK_USER_ID,
rating: newRating,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,12 @@ const logger = createLoggerWithLabel(module)

const valdiateSubmitAdminFeedbackParams = celebrate({
[Segments.BODY]: Joi.object().keys({
rating: Joi.number().min(0).max(1).required(),
rating: Joi.number().integer().min(0).max(5).required(),
comment: Joi.string(),
triggerSource: Joi.string()
.valid('field-edit', 'publish', 'workflow')
.optional(),
formId: Joi.string().optional(),
}),
})

Expand All @@ -35,20 +39,29 @@ const valdiateSubmitAdminFeedbackParams = celebrate({
const submitAdminFeedback: ControllerHandler<
unknown,
{ message: string; feedback: IAdminFeedbackSchema } | ErrorDto,
{ rating: number; comment?: string }
{ rating: number; comment?: string; triggerSource?: string; formId?: string }
> = async (req, res) => {
const sessionUserId = (req.session as AuthedSessionData).user._id
const { rating, comment } = req.body
const { rating, comment, triggerSource, formId } = req.body

// send rating to DD
// 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.
statsdClient.distribution('formsg.users.feedback.rating', rating, 1, {
rating: `${rating}`,
...(triggerSource ? { triggerSource } : {}),
})
statsdClient.distribution('formsg.users.feedback.rating.v2', rating, 1, {
rating: `${rating}`,
...(triggerSource ? { triggerSource } : {}),
})

return AdminFeedbackService.insertAdminFeedback({
userId: sessionUserId,
rating,
comment,
triggerSource,
formId,
})
.map((adminFeedback) =>
res.status(StatusCodes.OK).json({
Expand Down Expand Up @@ -79,11 +92,13 @@ export const handleSubmitAdminFeedback = [

const validateUpdateAdminFormFeedback = celebrate({
[Segments.BODY]: Joi.object().keys({
rating: Joi.number().min(0).max(1),
rating: Joi.number().integer().min(1).max(5),
comment: Joi.string(),
}),
})

// No Datadog metric here. Metric fires on create only to avoid double-counting
// when an admin changes their star rating.
const updateAdminFeedback: ControllerHandler<
{ feedbackId: string },
{ message: string } | ErrorDto,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,22 @@ export const insertAdminFeedback = ({
userId,
rating,
comment,
triggerSource,
formId,
}: {
userId: string
rating: number
comment?: string
triggerSource?: string
formId?: string
}) => {
return ResultAsync.fromPromise(
AdminFeedbackModel.create({
userId,
rating,
comment,
triggerSource,
formId,
}),
(error) => {
logger.error({
Expand Down Expand Up @@ -71,14 +77,12 @@ export const updateAdminFeedback = ({
comment?: string
rating?: number
}) => {
const updateObj = { rating, comment }

// filter out undefined properties
Object.keys(updateObj).forEach(
(key) =>
updateObj[key as keyof typeof updateObj] === undefined &&
delete updateObj[key as keyof typeof updateObj],
)
const updateObj: Record<string, unknown> = {}
if (comment !== undefined) updateObj.comment = comment
if (rating !== undefined) {
updateObj.rating = rating
updateObj.ratingChanged = true
}

// if no update to be done, return ok
if (isEmpty(updateObj)) return okAsync(true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3162,7 +3162,7 @@ describe('admin-form.form.routes', () => {

describe('POST /admin/forms/feedback', () => {
const MOCK_COMMENT = 'mock comment'
const MOCK_RATING = 0
const MOCK_RATING = 3

it('should return 200 when the request is successful', async () => {
// Act
Expand Down Expand Up @@ -3205,7 +3205,7 @@ describe('admin-form.form.routes', () => {
expect(resp.body.message).toEqual('Validation failed')
})

it('should return 400 when rating is not 0 or 1', async () => {
it('should return 400 when rating is out of 1-5 range', async () => {
// Act
const resp = await request
.post(`/admin/forms/feedback`)
Expand Down Expand Up @@ -3251,9 +3251,9 @@ describe('admin-form.form.routes', () => {
})
describe('PATCH /admin/forms/feedback/:feedbackId', () => {
const MOCK_COMMENT = 'mock comment'
const MOCK_RATING = 0
const MOCK_RATING = 3
const MOCK_NEW_COMMENT = 'new comment'
const MOCK_NEW_RATING = 1
const MOCK_NEW_RATING = 5

let MOCK_ADMIN_FEEDBACK: IAdminFeedbackSchema

Expand Down
2 changes: 0 additions & 2 deletions apps/frontend/src/app/AdminNavBar/AdminNavBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import {
ROLLOUT_ANNOUNCEMENT_KEY_PREFIX,
} from '~constants/localStorage'
import { DASHBOARD_ROUTE } from '~constants/routes'
import { ADMIN_FEEDBACK_SESSION_KEY } from '~constants/sessionStorage'
import { useIsMobile } from '~hooks/useIsMobile'
import { useLocalStorage } from '~hooks/useLocalStorage'
import { useToast } from '~hooks/useToast'
Expand Down Expand Up @@ -151,7 +150,6 @@ export const AdminNavBar = ({ isMenuOpen }: AdminNavBarProps): JSX.Element => {
}, [getWogadLogoutUrlMutation])

const handleLogout = useCallback(() => {
sessionStorage.removeItem(ADMIN_FEEDBACK_SESSION_KEY)
logout()
removeQuery()
if (emergencyContactKey) {
Expand Down
8 changes: 0 additions & 8 deletions apps/frontend/src/constants/sessionStorage.ts

This file was deleted.

Loading
Loading