PIMS-2347 Project Agency Response Date Saving Bug

express-api/src/controllers/projects/projectsController.ts

@@ -215,11 +215,21 @@ export const updateProjectAgencyResponses = async (req: Request, res: Response)
     return res.status(403).send('Projects only editable by Administrator role.');
   }
 
-  const notificationsSent = await projectServices.updateProjectAgencyResponses(
+  const updateResults = await projectServices.updateProjectAgencyResponses(
     projectId,
     req.body.responses,
     req.pimsUser,
   );
 
-  return res.status(200).send(notificationsSent);
+  // Let requestor know if any of the responses were not updated due to invalid changes.
+  // This can happen if they are trying to subscribe an agency that is disabled, has no email, or has opted out of notifications.
+  if (updateResults.invalidResponseChanges > 0) {
+    return res
+      .status(400)
+      .send(
+        `${updateResults.invalidResponseChanges} of the responses were not updated due to invalid changes.`,
+      );
+  }
+
+  return res.status(200).send(updateResults);
 };

express-api/src/services/notifications/notificationServices.ts

@@ -677,8 +677,9 @@ const generateProjectWatchNotifications = async (
           const agency = await query.manager.findOne(Agency, {
             where: { Id: response.AgencyId },
           });
-          //No use in queueing an email for an agency with no email address or that doesn't want notifications
-          if (agency?.SendEmail && agency?.Email && agency?.Email.length) {
+          // No use in queueing an email for an agency with no email address or that doesn't want notifications
+          // Don't queue notifications for disabled agencies either.
+          if (agency?.SendEmail && agency?.Email && agency?.Email.length && !agency.IsDisabled) {
             const dateInERP = project.ApprovedOn;
             const daysSinceThisStatus = getDaysBetween(dateInERP, new Date());
             const statusNotifs = await query.manager.find(ProjectStatusNotification, {

express-api/src/services/projects/projectsServices.ts

@@ -800,7 +800,7 @@ const updateProjectAgencyResponses = async (
   id: number,
   updatedResponses: Partial<ProjectAgencyResponse>[],
   user: User,
-): Promise<NotificationQueue[]> => {
+): Promise<{ sentNotifications: NotificationQueue[]; invalidResponseChanges: number }> => {
   if (!(await projectRepo.exists({ where: { Id: id } }))) {
     throw new ErrorWithCode('Project matching this internal ID not found.', 404);
   }
@@ -851,14 +851,39 @@ const updateProjectAgencyResponses = async (
     },
     { DeletedById: user.Id, DeletedOn: new Date() },
   );
+  // Are any of the agencies involved disabled, not wanting notifications, or missing an email?
+  // We can't allow them to subcribe.
+  const relevantAgencies = await AppDataSource.getRepository(Agency).find({
+    where: {
+      Id: In(updatedResponses.map((r) => r.AgencyId)),
+    },
+  });
+  const validReponses = updatedResponses.filter((resp) => {
+    // Only worry about this if subscribing.
+    if (resp.Response === AgencyResponseType.Subscribe) {
+      const agency = relevantAgencies.find((a) => a.Id === resp.AgencyId);
+      // If agency is not found, skip it.
+      if (!agency) return false;
+      // If agency is disabled, skip it.
+      if (agency.IsDisabled) return false;
+      // If agency does not want notifications, skip it.
+      if (!agency.SendEmail) return false;
+      // If agency does not have an email, skip it.
+      if (!agency.Email || agency.Email.trim() === '') return false;
+    }
+    // Otherwise, keep the response.
+    return true;
+  });
+
   // Save the remaining updated responses as current
   await AppDataSource.getRepository(ProjectAgencyResponse).save(
-    updatedResponses.map(
+    validReponses.map(
       (resp) =>
         ({
           ...resp,
           ProjectId: id,
           CreatedById: user.Id,
+          UpdatedById: user.Id,
           DeletedById: null,
           DeletedOn: null,
         }) as ProjectAgencyResponse,
@@ -867,7 +892,7 @@ const updateProjectAgencyResponses = async (
 
   // Identify which incoming responses are different than original ones on the project
   // This counts deleted ones as unsubscribed
-  const changedResponses = await getAgencyResponseChanges(originalResponses, updatedResponses);
+  const changedResponses = await getAgencyResponseChanges(originalResponses, validReponses);
   // For each of these changed/new responses, queue for send or cancel the notification as needed
   // Notifications are cancelled in this function, but only ones to send are returned in the list
   const notifsToSend = await notificationServices.generateProjectWatchNotifications(
@@ -876,9 +901,13 @@ const updateProjectAgencyResponses = async (
   );
 
   // Send new notifcations
-  return await Promise.all(
+  const sentNotifications = await Promise.all(
     notifsToSend.map((notif) => notificationServices.sendNotification(notif, user)),
   );
+  return {
+    sentNotifications,
+    invalidResponseChanges: updatedResponses.length - validReponses.length,
+  };
 };
 
 /**

express-api/tests/unit/controllers/projects/projectsController.test.ts

@@ -52,9 +52,12 @@ const _updateProjectAgencyResponses = jest
   .fn()
   // eslint-disable-next-line @typescript-eslint/no-unused-vars
   .mockImplementation((_id: number, responses: ProjectAgencyResponse[], user: User) => {
-    return responses.map((r) =>
-      produceNotificationQueue({ ProjectId: r.ProjectId, ToAgencyId: r.AgencyId }),
-    ) as NotificationQueue[];
+    return {
+      sentNotifications: responses.map((r) =>
+        produceNotificationQueue({ ProjectId: r.ProjectId, ToAgencyId: r.AgencyId }),
+      ) as NotificationQueue[],
+      invalidResponseChanges: 0,
+    };
   });
 
 jest
@@ -426,7 +429,7 @@ describe('UNIT - Testing controllers for users routes.', () => {
       mockRequest.params.projectId = '1';
       await controllers.updateProjectAgencyResponses(mockRequest, mockResponse);
       expect(mockResponse.statusValue).toBe(200);
-      expect(mockResponse.sendValue).toHaveLength(1);
+      expect(mockResponse.sendValue.sentNotifications).toHaveLength(1);
     });
 
     it('should return 400 when the body of responses is not properly formatted', async () => {
@@ -439,14 +442,35 @@ describe('UNIT - Testing controllers for users routes.', () => {
     });
 
     it('should return 400 when the projectId is not a number', async () => {
-      mockRequest.body = { responses: [produceAgencyResponse()] };
+      mockRequest.body = {
+        responses: [produceAgencyResponse({ Response: 0 })], // Response 0 is Subscribe
+      };
       mockRequest.setPimsUser({ RoleId: Roles.ADMIN, hasOneOfRoles: () => true });
       mockRequest.params.projectId = 'a';
       await controllers.updateProjectAgencyResponses(mockRequest, mockResponse);
       expect(mockResponse.statusValue).toBe(400);
       expect(mockResponse.sendValue).toBe('Invalid Project ID');
     });
 
+    it('should return 400 when some invalid subscribe requests are received', async () => {
+      mockRequest.body = {
+        responses: [produceAgencyResponse({ Response: 0 })], // Response 0 is Subscribe
+      };
+      mockRequest.setPimsUser({ RoleId: Roles.ADMIN, hasOneOfRoles: () => true });
+      mockRequest.params.projectId = '1';
+      _updateProjectAgencyResponses.mockImplementationOnce(() => {
+        return {
+          sentNotifications: [],
+          invalidResponseChanges: 1,
+        };
+      });
+      await controllers.updateProjectAgencyResponses(mockRequest, mockResponse);
+      expect(mockResponse.statusValue).toBe(400);
+      expect(mockResponse.sendValue).toBe(
+        '1 of the responses were not updated due to invalid changes.',
+      );
+    });
+
     it('should return 403 when a non-admin attempts to make this change', async () => {
       mockRequest.body = { responses: [produceAgencyResponse()] };
       mockRequest.setPimsUser({ RoleId: Roles.GENERAL_USER, hasOneOfRoles: () => false });

express-api/tests/unit/services/projects/projectsServices.test.ts

@@ -889,9 +889,6 @@ describe('UNIT - Project Services', () => {
   });
 
   describe('updateProjectAgencyResponses', () => {
-    jest
-      .spyOn(AppDataSource.getRepository(Agency), 'findOne')
-      .mockImplementation(async () => produceAgency());
     jest
       .spyOn(AppDataSource.getRepository(ProjectNote), 'find')
       .mockImplementation(async () => [produceNote()]);
@@ -908,18 +905,153 @@ describe('UNIT - Project Services', () => {
     jest
       .spyOn(AppDataSource.getRepository(ProjectAgencyResponse), 'save')
       .mockImplementation(async () => produceAgencyResponse());
+
     it('should return the list of sent notifications from this update', async () => {
+      const returnAgency = produceAgency({
+        SendEmail: true,
+        Email: '[email protected]',
+        IsDisabled: false,
+      });
+      jest
+        .spyOn(AppDataSource.getRepository(Agency), 'findOne')
+        .mockImplementationOnce(async () => returnAgency);
+      jest
+        .spyOn(AppDataSource.getRepository(Agency), 'find')
+        .mockImplementationOnce(async () => [returnAgency]);
+
       const project = produceProject();
-      const responses = [produceAgencyResponse({ Response: AgencyResponseType.Subscribe })];
+      const responses = [
+        produceAgencyResponse({
+          Response: AgencyResponseType.Subscribe,
+          AgencyId: returnAgency.Id,
+        }),
+      ];
       const user = producePimsRequestUser();
       const result = await projectServices.updateProjectAgencyResponses(
         project.Id,
         responses,
         user,
       );
-      expect(result).toHaveLength(
+      expect(result.sentNotifications).toHaveLength(
         responses.filter((r) => r.Response === AgencyResponseType.Subscribe).length,
       );
+      expect(result.invalidResponseChanges).toBe(0);
+    });
+
+    it('should return a invalid response of 1 if the agency cannot be found', async () => {
+      const returnAgency = produceAgency({
+        SendEmail: true,
+        Email: '[email protected]',
+        IsDisabled: false,
+      });
+      jest
+        .spyOn(AppDataSource.getRepository(Agency), 'findOne')
+        .mockImplementationOnce(async () => returnAgency);
+      jest
+        .spyOn(AppDataSource.getRepository(Agency), 'find')
+        .mockImplementationOnce(async () => []);
+
+      const project = produceProject();
+      const responses = [
+        produceAgencyResponse({
+          Response: AgencyResponseType.Subscribe,
+          AgencyId: returnAgency.Id,
+        }),
+      ];
+      const user = producePimsRequestUser();
+      const result = await projectServices.updateProjectAgencyResponses(
+        project.Id,
+        responses,
+        user,
+      );
+      expect(result.invalidResponseChanges).toBe(1);
+    });
+
+    it('should return a invalid response of 1 if the agency is disabled', async () => {
+      const returnAgency = produceAgency({
+        SendEmail: true,
+        Email: '[email protected]',
+        IsDisabled: true,
+      });
+      jest
+        .spyOn(AppDataSource.getRepository(Agency), 'findOne')
+        .mockImplementationOnce(async () => returnAgency);
+      jest
+        .spyOn(AppDataSource.getRepository(Agency), 'find')
+        .mockImplementationOnce(async () => [returnAgency]);
+
+      const project = produceProject();
+      const responses = [
+        produceAgencyResponse({
+          Response: AgencyResponseType.Subscribe,
+          AgencyId: returnAgency.Id,
+        }),
+      ];
+      const user = producePimsRequestUser();
+      const result = await projectServices.updateProjectAgencyResponses(
+        project.Id,
+        responses,
+        user,
+      );
+      expect(result.invalidResponseChanges).toBe(1);
+    });
+
+    it('should return a invalid response of 1 if the agency has SendEmail disabled', async () => {
+      const returnAgency = produceAgency({
+        SendEmail: false,
+        Email: '[email protected]',
+        IsDisabled: false,
+      });
+      jest
+        .spyOn(AppDataSource.getRepository(Agency), 'findOne')
+        .mockImplementationOnce(async () => returnAgency);
+      jest
+        .spyOn(AppDataSource.getRepository(Agency), 'find')
+        .mockImplementationOnce(async () => [returnAgency]);
+
+      const project = produceProject();
+      const responses = [
+        produceAgencyResponse({
+          Response: AgencyResponseType.Subscribe,
+          AgencyId: returnAgency.Id,
+        }),
+      ];
+      const user = producePimsRequestUser();
+      const result = await projectServices.updateProjectAgencyResponses(
+        project.Id,
+        responses,
+        user,
+      );
+      expect(result.invalidResponseChanges).toBe(1);
+    });
+
+    it('should return a invalid response of 1 if the agency has a blank email', async () => {
+      const returnAgency = produceAgency({
+        SendEmail: true,
+        Email: '',
+        IsDisabled: false,
+      });
+      jest
+        .spyOn(AppDataSource.getRepository(Agency), 'findOne')
+        .mockImplementationOnce(async () => returnAgency);
+      jest
+        .spyOn(AppDataSource.getRepository(Agency), 'find')
+        .mockImplementationOnce(async () => [returnAgency]);
+
+      const project = produceProject();
+      const responses = [
+        produceAgencyResponse({
+          Response: AgencyResponseType.Subscribe,
+          AgencyId: returnAgency.Id,
+        }),
+      ];
+      const user = producePimsRequestUser();
+      const result = await projectServices.updateProjectAgencyResponses(
+        project.Id,
+        responses,
+        user,
+      );
+      expect(result.invalidResponseChanges).toBe(1);
     });
   });
 });