From 7316443e6bc8810f6f255109740f055d781e7514 Mon Sep 17 00:00:00 2001 From: Lawrence Lau Date: Wed, 14 Aug 2024 14:46:13 -0700 Subject: [PATCH 1/5] initial commit --- .../notifications/notificationsController.ts | 3 +- .../notifications/notificationServices.ts | 53 ++++++++++++++++++- .../src/constants/chesNotificationStatus.ts | 6 +++ 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/express-api/src/controllers/notifications/notificationsController.ts b/express-api/src/controllers/notifications/notificationsController.ts index ab65197fe..d69b877ba 100644 --- a/express-api/src/controllers/notifications/notificationsController.ts +++ b/express-api/src/controllers/notifications/notificationsController.ts @@ -46,7 +46,8 @@ export const getNotificationsByProjectId = async (req: Request, res: Response) = return res.status(200).send(notificationsResult); } catch (error) { - // not sure if the error codes can be handled better here? + // not sure if the 401 and 500 error codes can be handled better here? + // if the ches credentials are incorrect, we don't seem to catch the 401 error in the notificationService return res.status(500).send({ message: 'Error fetching notifications' }); } }; diff --git a/express-api/src/services/notifications/notificationServices.ts b/express-api/src/services/notifications/notificationServices.ts index 5f3353e31..9461bbba5 100644 --- a/express-api/src/services/notifications/notificationServices.ts +++ b/express-api/src/services/notifications/notificationServices.ts @@ -33,6 +33,9 @@ export enum NotificationStatus { Cancelled = 2, Failed = 3, Completed = 4, + NotFound = 5, + Invalid = 6, + Unauthorized = 7, } export enum NotificationAudience { @@ -442,6 +445,12 @@ const convertChesStatusToNotificationStatus = (chesStatus: string): Notification return NotificationStatus.Failed; case 'completed': return NotificationStatus.Completed; + case '404': + return NotificationStatus.NotFound; + case '422': + return NotificationStatus.Invalid; + case '401': + return NotificationStatus.Unauthorized; default: return null; } @@ -465,7 +474,6 @@ const updateNotificationStatus = async (notificationId: number, user: User) => { } const statusResponse = await chesServices.getStatusByIdAsync(notification.ChesMessageId); - if (typeof statusResponse?.status === 'string') { const notificationStatus = convertChesStatusToNotificationStatus(statusResponse.status); // If the CHES status is non-standard, don't update the notification. @@ -481,7 +489,48 @@ const updateNotificationStatus = async (notificationId: number, user: User) => { query.release(); return updatedNotification; } else if (typeof statusResponse?.status === 'number') { - //If we get number type then this wound up being some HTTP code. + // handle 404, 422 code here.... + switch (statusResponse.status) { + case 404: { + notification.Status = NotificationStatus.NotFound; + notification.UpdatedOn = new Date(); + notification.UpdatedById = user.Id; + const updatedNotification = await query.manager.save(NotificationQueue, notification); + logger.error(`Notification with id ${notificationId} not found on CHES.`); + query.release(); + return updatedNotification; + } + case 422: { + notification.Status = NotificationStatus.Invalid; + notification.UpdatedOn = new Date(); + notification.UpdatedById = user.Id; + const updatedNotification = await query.manager.save(NotificationQueue, notification); + logger.error( + `Notification with id ${notificationId} could not be processed, some of the data could be formatted incorrectly.`, + ); + query.release(); + return updatedNotification; + } + case 401: { + notification.Status = NotificationStatus.Unauthorized; + notification.UpdatedOn = new Date(); + notification.UpdatedById = user.Id; + const updatedNotification = await query.manager.save(NotificationQueue, notification); + logger.error( + `Cannot authorize the request to the CHES server, check your CHES credentials.`, + ); + query.release(); + return updatedNotification; + } + case 500: + logger.error( + `Internal server error while retrieving status for notification with id ${notificationId}.`, + ); + break; + default: + logger.error(`Received unexpected status code ${statusResponse.status}.`); + break; + } query.release(); return notification; } else { diff --git a/react-app/src/constants/chesNotificationStatus.ts b/react-app/src/constants/chesNotificationStatus.ts index 9f66b5c0c..919831271 100644 --- a/react-app/src/constants/chesNotificationStatus.ts +++ b/react-app/src/constants/chesNotificationStatus.ts @@ -4,6 +4,8 @@ export enum NotificationStatus { Cancelled = 2, Failed = 3, Completed = 4, + NotFound = 5, + Invalid = 6, } export const getStatusString = (status: number): string => { @@ -18,6 +20,10 @@ export const getStatusString = (status: number): string => { return 'Failed'; case NotificationStatus.Completed: return 'Completed'; + case NotificationStatus.NotFound: + return 'Not Found'; + case NotificationStatus.Invalid: + return 'Invalid'; default: return 'Unknown'; } From 6878f856dc36375c440bd934b3eba8097b48fb9f Mon Sep 17 00:00:00 2001 From: Lawrence Lau Date: Thu, 15 Aug 2024 10:08:15 -0700 Subject: [PATCH 2/5] removing invalid status --- .../notifications/notificationsController.ts | 2 -- .../notifications/notificationServices.ts | 23 ++++++------------- .../src/constants/chesNotificationStatus.ts | 2 -- 3 files changed, 7 insertions(+), 20 deletions(-) diff --git a/express-api/src/controllers/notifications/notificationsController.ts b/express-api/src/controllers/notifications/notificationsController.ts index d69b877ba..df822c299 100644 --- a/express-api/src/controllers/notifications/notificationsController.ts +++ b/express-api/src/controllers/notifications/notificationsController.ts @@ -46,8 +46,6 @@ export const getNotificationsByProjectId = async (req: Request, res: Response) = return res.status(200).send(notificationsResult); } catch (error) { - // not sure if the 401 and 500 error codes can be handled better here? - // if the ches credentials are incorrect, we don't seem to catch the 401 error in the notificationService return res.status(500).send({ message: 'Error fetching notifications' }); } }; diff --git a/express-api/src/services/notifications/notificationServices.ts b/express-api/src/services/notifications/notificationServices.ts index 9461bbba5..29818cb40 100644 --- a/express-api/src/services/notifications/notificationServices.ts +++ b/express-api/src/services/notifications/notificationServices.ts @@ -500,28 +500,19 @@ const updateNotificationStatus = async (notificationId: number, user: User) => { query.release(); return updatedNotification; } - case 422: { - notification.Status = NotificationStatus.Invalid; - notification.UpdatedOn = new Date(); - notification.UpdatedById = user.Id; - const updatedNotification = await query.manager.save(NotificationQueue, notification); + + case 422: logger.error( `Notification with id ${notificationId} could not be processed, some of the data could be formatted incorrectly.`, ); - query.release(); - return updatedNotification; - } - case 401: { - notification.Status = NotificationStatus.Unauthorized; - notification.UpdatedOn = new Date(); - notification.UpdatedById = user.Id; - const updatedNotification = await query.manager.save(NotificationQueue, notification); + break; + + case 401: logger.error( `Cannot authorize the request to the CHES server, check your CHES credentials.`, ); - query.release(); - return updatedNotification; - } + break; + case 500: logger.error( `Internal server error while retrieving status for notification with id ${notificationId}.`, diff --git a/react-app/src/constants/chesNotificationStatus.ts b/react-app/src/constants/chesNotificationStatus.ts index 919831271..9f2e53640 100644 --- a/react-app/src/constants/chesNotificationStatus.ts +++ b/react-app/src/constants/chesNotificationStatus.ts @@ -22,8 +22,6 @@ export const getStatusString = (status: number): string => { return 'Completed'; case NotificationStatus.NotFound: return 'Not Found'; - case NotificationStatus.Invalid: - return 'Invalid'; default: return 'Unknown'; } From 547f83d4b15097d8b9302e56dfb94a7a84d28740 Mon Sep 17 00:00:00 2001 From: Lawrence Lau Date: Thu, 15 Aug 2024 10:11:37 -0700 Subject: [PATCH 3/5] removing 422 and 401 statuses --- .../src/services/notifications/notificationServices.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/express-api/src/services/notifications/notificationServices.ts b/express-api/src/services/notifications/notificationServices.ts index 29818cb40..79b9acca8 100644 --- a/express-api/src/services/notifications/notificationServices.ts +++ b/express-api/src/services/notifications/notificationServices.ts @@ -34,8 +34,6 @@ export enum NotificationStatus { Failed = 3, Completed = 4, NotFound = 5, - Invalid = 6, - Unauthorized = 7, } export enum NotificationAudience { @@ -447,10 +445,6 @@ const convertChesStatusToNotificationStatus = (chesStatus: string): Notification return NotificationStatus.Completed; case '404': return NotificationStatus.NotFound; - case '422': - return NotificationStatus.Invalid; - case '401': - return NotificationStatus.Unauthorized; default: return null; } From 719fdcf64b06706bc1679fb7adaf18fdf34d168e Mon Sep 17 00:00:00 2001 From: dbarkowsky Date: Thu, 15 Aug 2024 11:35:58 -0700 Subject: [PATCH 4/5] remove invalid option from enum --- react-app/src/constants/chesNotificationStatus.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/react-app/src/constants/chesNotificationStatus.ts b/react-app/src/constants/chesNotificationStatus.ts index 9f2e53640..392f30432 100644 --- a/react-app/src/constants/chesNotificationStatus.ts +++ b/react-app/src/constants/chesNotificationStatus.ts @@ -5,7 +5,6 @@ export enum NotificationStatus { Failed = 3, Completed = 4, NotFound = 5, - Invalid = 6, } export const getStatusString = (status: number): string => { From f49c6c2f44580b3d641260027a934e7c1adcad69 Mon Sep 17 00:00:00 2001 From: Lawrence Lau Date: Thu, 15 Aug 2024 15:03:29 -0700 Subject: [PATCH 5/5] adding coverage --- .../notificationServices.test.ts | 122 ++++++++++++++++++ 1 file changed, 122 insertions(+) diff --git a/express-api/tests/unit/services/notifications/notificationServices.test.ts b/express-api/tests/unit/services/notifications/notificationServices.test.ts index 5cec3bad9..8de46a33a 100644 --- a/express-api/tests/unit/services/notifications/notificationServices.test.ts +++ b/express-api/tests/unit/services/notifications/notificationServices.test.ts @@ -24,6 +24,7 @@ import { ProjectStatusNotification } from '@/typeorm/Entities/ProjectStatusNotif import { User } from '@/typeorm/Entities/User'; import { Agency } from '@/typeorm/Entities/Agency'; import { ProjectStatusHistory } from '@/typeorm/Entities/ProjectStatusHistory'; +import logger from '@/utilities/winstonLogger'; const _notifQueueSave = jest .fn() @@ -271,6 +272,121 @@ describe('updateNotificationStatus', () => { // Expect that notification was not updated. expect(response.Status).toBe(notification.Status); }); + it('should handle CHES status 404 and update notification status to NotFound', async () => { + const user = produceUser(); + const notifQueue = produceNotificationQueue(); + notifQueue.ChesMessageId = randomUUID(); + + _getStatusByIdAsync.mockResolvedValueOnce({ + status: 404 as unknown as string, + tag: 'sampleTag', + txId: randomUUID(), + updatedTS: Date.now(), + createdTS: Date.now(), + msgId: randomUUID(), + }); + + const response = await notificationServices.updateNotificationStatus(notifQueue.Id, user); + + expect(response.Status).toBe(NotificationStatus.NotFound); + expect(response.UpdatedById).toBe(user.Id); + }); + + it('should handle CHES status 422 and log an error', async () => { + const user = produceUser(); + const notifQueue = produceNotificationQueue(); + notifQueue.ChesMessageId = randomUUID(); + + _getStatusByIdAsync.mockResolvedValueOnce({ + status: 422 as unknown as string, + tag: 'sampleTag', + txId: randomUUID(), + updatedTS: Date.now(), + createdTS: Date.now(), + msgId: randomUUID(), + }); + + const loggerErrorSpy = jest.spyOn(logger, 'error'); + + const response = await notificationServices.updateNotificationStatus(notifQueue.Id, user); + + expect(response.Status).toBe(notifQueue.Status); + expect(loggerErrorSpy).toHaveBeenCalledWith( + `Notification with id ${notifQueue.Id} could not be processed, some of the data could be formatted incorrectly.`, + ); + }); + + it('should handle CHES status 401 and log an error', async () => { + const user = produceUser(); + const notifQueue = produceNotificationQueue(); + notifQueue.ChesMessageId = randomUUID(); + + _getStatusByIdAsync.mockResolvedValueOnce({ + status: 401 as unknown as string, + tag: 'sampleTag', + txId: randomUUID(), + updatedTS: Date.now(), + createdTS: Date.now(), + msgId: randomUUID(), + }); + + const loggerErrorSpy = jest.spyOn(logger, 'error'); + + const response = await notificationServices.updateNotificationStatus(notifQueue.Id, user); + + expect(response.Status).toBe(notifQueue.Status); + expect(loggerErrorSpy).toHaveBeenCalledWith( + `Cannot authorize the request to the CHES server, check your CHES credentials.`, + ); + }); + + it('should handle CHES status 500 and log an error', async () => { + const user = produceUser(); + const notifQueue = produceNotificationQueue(); + notifQueue.ChesMessageId = randomUUID(); + + _getStatusByIdAsync.mockResolvedValueOnce({ + status: 500 as unknown as string, + tag: 'sampleTag', + txId: randomUUID(), + updatedTS: Date.now(), + createdTS: Date.now(), + msgId: randomUUID(), + }); + + const loggerErrorSpy = jest.spyOn(logger, 'error'); + + const response = await notificationServices.updateNotificationStatus(notifQueue.Id, user); + + expect(response.Status).toBe(notifQueue.Status); + expect(loggerErrorSpy).toHaveBeenCalledWith( + `Internal server error while retrieving status for notification with id ${notifQueue.Id}.`, + ); + }); + it('should throw an error when getNotificationStatusById fails to retrieve the status', async () => { + const mockInvalidStatusResponse: IChesStatusResponse = { + status: null, // Simulating an invalid status + tag: '', + txId: '', + updatedTS: 1234, + createdTS: 1234, + msgId: '', + }; + const user = produceUser(); + + // Mock the response of `getStatusByIdAsync` to return the invalid status response + jest.spyOn(chesServices, 'getStatusByIdAsync').mockResolvedValueOnce(mockInvalidStatusResponse); + + // Mock the query manager findOne method to return a valid notification + const mockNotification = { Id: 1, ChesMessageId: 'some-message-id' }; + const mockFindOne = jest.spyOn(AppDataSource.createQueryRunner().manager, 'findOne'); + mockFindOne.mockResolvedValueOnce(mockNotification); + + // Expect the updateNotificationStatus to throw an error + await expect(notificationServices.updateNotificationStatus(1, user)).rejects.toThrow( + 'Failed to retrieve status for notification with id 1.', + ); + }); }); describe('convertChesStatusToNotificationStatus', () => { it('should return NotificationStatus.Accepted for "accepted"', () => { @@ -297,6 +413,12 @@ describe('convertChesStatusToNotificationStatus', () => { ); }); + it('should return NotificationStatus.NotFound when chesStatus is "404"', () => { + expect(notificationServices.convertChesStatusToNotificationStatus('404')).toBe( + NotificationStatus.NotFound, + ); + }); + it('should return NotificationStatus.Completed for "completed"', () => { expect(notificationServices.convertChesStatusToNotificationStatus('completed')).toBe( NotificationStatus.Completed,