Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(j-s): Indictment sent or withdrawn from prison notifications #16943

Merged
merged 88 commits into from
Nov 27, 2024

Conversation

unakb
Copy link
Member

@unakb unakb commented Nov 19, 2024

Senda tilkynningu til FMST - fullnusta
Senda tilkynningu til FMST - afturköllun

What

Send notifications to a group of prison admin staff when an indictment is sent or withdrawn .

Why

To let them know that something new has happened in case they need to react to it.

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Formatting passes locally with my changes
  • I have rebased against main before asking for a review

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced new notification types for indictments related to prison administration.
    • Added functionality to send notifications when a defendant is sent to or withdrawn from prison administration.
    • New secret added for managing prison administration indictment emails.
  • Bug Fixes

    • Improved handling of notification messages for defendant status changes.
  • Documentation

    • Updated configuration files to include new secret for prison administration email notifications.
  • Tests

    • Added unit tests for new notification functionalities related to indictments.
  • Chores

    • Restructured import paths for better organization within the notification services.

oddsson and others added 30 commits November 11, 2024 14:48
@unakb unakb changed the title J s/indictment to prison notifications feat(j-s): Indictment sent or withdrawn from prison notifications Nov 21, 2024
@unakb unakb marked this pull request as ready for review November 25, 2024 12:02
@unakb unakb requested review from a team as code owners November 25, 2024 12:02
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Outside diff range and nitpick comments (24)
apps/judicial-system/backend/src/app/modules/notification/notification.module.ts (1)

20-24: LGTM! Consider updating documentation.

The reorganization of notification services into a dedicated services directory follows NestJS best practices for module organization and improves code maintainability.

Consider updating the module's documentation to reflect this new directory structure, particularly if there's a README.md file in the notification module.

apps/judicial-system/backend/src/app/modules/notification/services/defendantNotification/defendantNotification.strings.ts (2)

30-41: LGTM! Consider improving type safety for message placeholders.

The implementation follows i18n best practices and maintains consistency with existing patterns. However, consider enhancing type safety by defining TypeScript interfaces for the message placeholders.

Example implementation:

interface IndictmentMessagePlaceholders {
  courtCaseNumber: string;
  linkStart: string;
  linkEnd: string;
}

// Usage
defineMessage<IndictmentMessagePlaceholders>({
  id: 'judicial.system.backend:defendant_notifications.indictment_sent_to_prison_admin_body',
  defaultMessage: '...',
  description: '...',
})

42-54: Consider adding a case overview link for consistency.

For consistency with other notification messages, consider including a link to the case overview in the withdrawal notification body. This would help prison administrators access the case details if needed.

Apply this diff to add the case overview link:

   indictmentWithdrawnFromPrisonAdminBody: defineMessage({
     id: 'judicial.system.backend:defendant_notifications.indictment_withdrawn_from_prison_admin_body',
     defaultMessage:
-      'Ríkissaksóknari hefur afturkallað mál {courtCaseNumber} úr fullnustu.',
+      'Ríkissaksóknari hefur afturkallað mál {courtCaseNumber} úr fullnustu.<br /><br />Sjá nánar á {linkStart}yfirlitssíðu málsins í Réttarvörslugátt{linkEnd}.',
     description:
       'Texti í tilkynningu til FMST þegar mál er afturkallað úr fullnustu',
   }),
libs/judicial-system/types/src/lib/notification.ts (1)

ADVOCATE_ASSIGNED is actively used and requires careful deprecation

The enum value ADVOCATE_ASSIGNED is currently used in multiple critical locations:

  • Used in notification type definitions
  • Referenced in case notification service for handling advocate assignment notifications
  • Active usage in notification service for user-related functionality

Before removal:

  1. Add @deprecated JSDoc comment with migration path
  2. Create tracking issue for coordinated removal
  3. Update all identified usage locations:
    • apps/judicial-system/backend/src/app/modules/notification/services/caseNotification/caseNotification.service.ts
    • apps/judicial-system/backend/src/app/modules/notification/notification.service.ts
🔗 Analysis chain

Line range hint 8-8: Consider proper deprecation process for ADVOCATE_ASSIGNED.

Instead of using a comment to indicate future removal, consider:

  1. Adding a proper @deprecated JSDoc comment with migration instructions
  2. Creating a tracking issue for the removal
  3. Ensuring all usages are identified before removal

This helps maintain backward compatibility and provides clear migration paths.

Let's check for existing usage:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for ADVOCATE_ASSIGNED usage in the codebase
# Expected: Find all code that needs to be updated before removal

rg -A 3 "ADVOCATE_ASSIGNED" \
  --type ts \
  --glob '!*.test.ts' \
  --glob '!*.spec.ts'

Length of output: 3888

apps/judicial-system/backend/infra/judicial-system-backend.ts (1)

73-74: Consider adding environment-specific configuration if needed.

If the email recipients differ across environments (dev/staging/prod), consider adding environment-specific configuration in the env section similar to other configurable values.

Example:

 .env({
   // ... existing env vars ...
+  PRISON_ADMIN_INDICTMENT_EMAILS: {
+    dev: '[email protected]',
+    staging: '[email protected]',
+    prod: ref((h) => h.secrets.PRISON_ADMIN_INDICTMENT_EMAILS),
+  },
 })
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/defendantNotification/sendIndictmentWithdrawnFromPrisonAdminNotifications.spec.ts (2)

1-32: Consider reorganizing imports and extracting common test types.

The imports could be better organized, and the test types could be made more reusable.

Consider:

  1. Grouping imports:
// External dependencies
import { uuid } from 'uuidv4'
import { EmailService } from '@island.is/email-service'

// Domain types
import {
  CaseType,
  DefendantNotificationType,
} from '@island.is/judicial-system/types'

// Test utilities
import { createTestingNotificationModule } from '../../createTestingNotificationModule'

// Internal modules
import { Case } from '../../../../case'
import { Defendant } from '../../../../defendant'
import { DefendantNotificationDto } from '../../../dto/defendantNotification.dto'
import { DeliverResponse } from '../../../models/deliver.response'
import { Notification } from '../../../models/notification.model'
  1. Consider moving the Then interface and GivenWhenThen type to a shared test utilities file as they could be reused across similar notification test files.

1-1: Add test file documentation.

Consider adding a descriptive comment block at the beginning of the file to explain:

  • The purpose of these tests
  • The requirements being verified
  • Any specific setup requirements

Example:

/**
 * Tests for prison admin notification functionality when an indictment is withdrawn.
 * 
 * Requirements:
 * - Notifications must be sent to all configured prison admin email addresses
 * - Each notification must include the case number and withdrawal information
 * - All notification attempts must be recorded, whether successful or not
 * 
 * Setup:
 * - Requires PRISON_ADMIN_INDICTMENT_EMAILS environment variable
 * - Uses mocked email service and notification model
 */
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/defendantNotification/sendIndictmentSentToPrisonAdminNotification.spec.ts (3)

21-33: Add JSDoc comments to improve type documentation.

The GivenWhenThen type and Then interface would benefit from JSDoc comments explaining their purpose and usage in the test suite.

+/**
+ * Represents the result of a test case execution
+ */
 interface Then {
   result: DeliverResponse
   error: Error
 }

+/**
+ * Type definition for the Given-When-Then test pattern implementation
+ * @param caseId - The unique identifier of the case
+ * @param defendantId - The unique identifier of the defendant
+ * @param theCase - The case object
+ * @param defendant - The defendant object
+ * @param notificationDto - The notification data transfer object
+ * @returns Promise containing the test result
+ */
 type GivenWhenThen = (
   caseId: string,
   defendantId: string,
   theCase: Case,
   defendant: Defendant,
   notificationDto: DefendantNotificationDto,
 ) => Promise<Then>

37-40: Move test email addresses to constants.

Consider moving the test email addresses to a constants file to improve maintainability and reusability.

+const TEST_PRISON_ADMIN_EMAILS = [
+  '[email protected]',
+  '[email protected]',
+] as const;
-const emails = [
-  '[email protected]',
-  '[email protected]',
-]
+const emails = [...TEST_PRISON_ADMIN_EMAILS]

1-144: Consider simplifying the test file name.

The current file name sendIndictmentSentToPrisonAdminNotification.spec.ts is quite long. Consider a more concise name like prisonAdminIndictment.spec.ts while ensuring it remains descriptive enough to understand its purpose.

charts/services/judicial-system-backend/values.prod.yaml (1)

Line range hint 1-7: Warning: This file should not be edited manually

The file header explicitly states that manual edits should be avoided and yarn charts should be run instead. Please ensure this change was made through the proper workflow.

Consider running yarn charts to regenerate this file instead of manual edits.

charts/services/judicial-system-backend/values.staging.yaml (1)

Line range hint 1-7: Important: Do not edit this file manually.

As indicated by the warning comment, this file is automatically generated. Please ensure you're using yarn charts to make any changes.

apps/judicial-system/backend/src/app/modules/defendant/test/defendantController/update.spec.ts (2)

212-236: Consider enhancing test coverage for prison admin notification.

The test case effectively verifies the basic notification flow. However, consider adding:

  1. A negative test case where CaseType is not INDICTMENT
  2. Verification of the complete message payload structure, including any metadata
 describe('defendant in indictment is sent to prison admin', () => {
+  describe.each([
+    { caseType: CaseType.INDICTMENT, shouldQueue: true },
+    { caseType: CaseType.CUSTODY, shouldQueue: false }
+  ])('when case type is $caseType', ({ caseType, shouldQueue }) => {
     const defendantUpdate = { isSentToPrisonAdmin: true }
     const updatedDefendant = { ...defendant, ...defendantUpdate }

     beforeEach(async () => {
       const mockUpdate = mockDefendantModel.update as jest.Mock
       mockUpdate.mockResolvedValueOnce([1, [updatedDefendant]])

-      await givenWhenThen(defendantUpdate, CaseType.INDICTMENT, caseId)
+      await givenWhenThen(defendantUpdate, caseType, caseId)
     })

-    it('should queue messages', () => {
+    it(shouldQueue ? 'should queue messages' : 'should not queue messages', () => {
+      if (shouldQueue) {
         expect(mockMessageService.sendMessagesToQueue).toHaveBeenCalledWith([
           {
             type: MessageType.DEFENDANT_NOTIFICATION,
             caseId,
             elementId: defendantId,
             body: {
               type: DefendantNotificationType.INDICTMENT_SENT_TO_PRISON_ADMIN,
             },
+            metadata: expect.any(Object)
           },
         ])
         expect(mockMessageService.sendMessagesToQueue).toHaveBeenCalledTimes(1)
+      } else {
+        expect(mockMessageService.sendMessagesToQueue).not.toHaveBeenCalled()
+      }
     })
+  })
 })

238-262: Improve test name and coverage for withdrawal notification.

The test case effectively verifies the withdrawal notification. Consider:

  1. Making the test description more concise by removing redundant "for indictment withdrawn from prison admin"
  2. Adding test cases for non-INDICTMENT case types similar to the previous test
 describe('defendant in indictment is withdrawn from prison admin', () => {
+  describe.each([
+    { caseType: CaseType.INDICTMENT, shouldQueue: true },
+    { caseType: CaseType.CUSTODY, shouldQueue: false }
+  ])('when case type is $caseType', ({ caseType, shouldQueue }) => {
     const defendantUpdate = { isSentToPrisonAdmin: false }
     const updatedDefendant = { ...defendant, ...defendantUpdate }

     beforeEach(async () => {
       const mockUpdate = mockDefendantModel.update as jest.Mock
       mockUpdate.mockResolvedValueOnce([1, [updatedDefendant]])

-      await givenWhenThen(defendantUpdate, CaseType.INDICTMENT, caseId)
+      await givenWhenThen(defendantUpdate, caseType, caseId)
     })

-    it('should queue messages for indictment withdrawn from prison admin', () => {
+    it(shouldQueue ? 'should queue messages' : 'should not queue messages', () => {
+      if (shouldQueue) {
         expect(mockMessageService.sendMessagesToQueue).toHaveBeenCalledWith([
           {
             type: MessageType.DEFENDANT_NOTIFICATION,
             caseId,
             elementId: defendantId,
             body: {
               type: DefendantNotificationType.INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN,
             },
+            metadata: expect.any(Object)
           },
         ])
         expect(mockMessageService.sendMessagesToQueue).toHaveBeenCalledTimes(1)
+      } else {
+        expect(mockMessageService.sendMessagesToQueue).not.toHaveBeenCalled()
+      }
     })
+  })
 })
apps/judicial-system/backend/src/app/modules/defendant/defendant.service.ts (2)

77-95: Consider enhancing type safety and method naming

The implementation is clean and follows a consistent pattern, but could benefit from these improvements:

- private getMessagesForIndictmentToPrisonAdminChanges(
-   defendant: Defendant,
- ): Message {
+ private getIndictmentPrisonAdminMessage(
+   defendant: Defendant,
+ ): DefendantNotificationMessage {

Consider creating a type for notification messages to enhance type safety:

type DefendantNotificationMessage = Message & {
  type: MessageType.DEFENDANT_NOTIFICATION;
  body: {
    type: DefendantNotificationType;
  };
};

Line range hint 166-207: Consider splitting responsibilities and improving error handling

The method is handling multiple notification types which increases complexity. Consider refactoring for better separation of concerns.

- private async sendIndictmentCaseUpdateDefendantMessages(
+ private async sendDefenderChoiceNotification(
   theCase: Case,
   updatedDefendant: Defendant,
   oldDefendant: Defendant,
   user: User,
-): Promise<void> {
+): Promise<Message[]> {
   if (!theCase.courtCaseNumber) {
-    return
+    return []
   }

   const messages: Message[] = []
   
   if (updatedDefendant.isDefenderChoiceConfirmed && 
       !oldDefendant.isDefenderChoiceConfirmed) {
     // ... defender choice logic ...
   }
   
   return messages
}

+ private async sendPrisonAdminNotification(
+   theCase: Case,
+   updatedDefendant: Defendant,
+   oldDefendant: Defendant,
+ ): Promise<Message[]> {
+   if (!theCase.courtCaseNumber) {
+     return []
+   }
+   
+   const messages: Message[] = []
+   
+   if (this.hasPrisonAdminStatusChanged(updatedDefendant, oldDefendant)) {
+     messages.push(
+       this.getIndictmentPrisonAdminMessage(updatedDefendant),
+     )
+   }
+   
+   return messages
+ }

+ private hasPrisonAdminStatusChanged(
+   updatedDefendant: Defendant,
+   oldDefendant: Defendant,
+ ): boolean {
+   return updatedDefendant.isSentToPrisonAdmin !== undefined &&
+          updatedDefendant.isSentToPrisonAdmin !== oldDefendant.isSentToPrisonAdmin
+ }

+ private async sendIndictmentCaseUpdateDefendantMessages(
+   theCase: Case,
+   updatedDefendant: Defendant,
+   oldDefendant: Defendant,
+   user: User,
+ ): Promise<void> {
+   try {
+     const messages = [
+       ...(await this.sendDefenderChoiceNotification(theCase, updatedDefendant, oldDefendant, user)),
+       ...(await this.sendPrisonAdminNotification(theCase, updatedDefendant, oldDefendant)),
+     ]
+     
+     if (messages.length > 0) {
+       await this.messageService.sendMessagesToQueue(messages)
+     }
+   } catch (error) {
+     this.logger.error('Failed to send indictment case update notifications', {
+       error,
+       caseId: theCase.id,
+       defendantId: updatedDefendant.id,
+     })
+     throw error
+   }
+ }
charts/judicial-system/values.dev.yaml (1)

Line range hint 89-93: Review the impact of reduced nginx request rate

The nginxRequestsIrate metric for HPA scaling has been reduced from 8 to 5, which might affect the scaling behavior of the backend service. This change could impact the system's ability to handle traffic spikes.

Consider the following recommendations:

  1. Document the reasoning behind this reduction in the PR description
  2. Monitor the scaling behavior after deployment to ensure it meets performance requirements
  3. Consider implementing a gradual reduction if this is a significant change for production
apps/judicial-system/backend/src/app/modules/notification/services/caseNotification/caseNotification.service.ts (5)

Line range hint 1-84: Consider reorganizing imports for better maintainability

The imports could be better organized by grouping them into the following categories:

  1. Node built-in modules
  2. External dependencies (nest, lodash, etc.)
  3. Internal modules
  4. Types and interfaces
+ // Node built-in modules
+
+ // External dependencies
import { ICalendar } from 'datebook'
import _uniqBy from 'lodash/uniqBy'
import {
  Inject,
  Injectable,
  InternalServerErrorException,
} from '@nestjs/common'
import { InjectModel } from '@nestjs/sequelize'
+
+ // Internal modules
import { IntlService } from '@island.is/cms-translations'
...
+
+ // Types and interfaces
import {
  CaseAppealRulingDecision,
  CaseCustodyRestrictions,
  ...
} from '@island.is/judicial-system/types'

Line range hint 92-99: Enhance error handling in sendSms method

The error handling in the catch block could be improved by:

  1. Adding more context to the error message
  2. Including the SMS text in the error event for better debugging
  3. Considering retrying failed SMS sends for critical notifications
  } catch (reason) {
-   this.logger.error('Failed to send sms', { error: reason })
+   this.logger.error('Failed to send SMS notification', {
+     error: reason,
+     mobileNumbers,
+     smsText: smsText.substring(0, 100), // Truncate for logging
+     notificationType: 'SMS'
+   })

    this.eventService.postErrorEvent(
-     'Failed to send sms',
-     { mobileNumbers },
+     'Failed to send SMS notification',
+     { mobileNumbers, notificationType: 'SMS' },
      reason,
    )

Line range hint 1024-1039: Enhance security for sensitive data handling

The email content construction should:

  1. Sanitize HTML content to prevent XSS
  2. Redact sensitive information in logs
  3. Consider adding rate limiting for notifications to prevent abuse
+ import { sanitizeHtml } from '@island.is/judicial-system/utils'

  private async sendRulingEmailNotificationToProsecutor(
    theCase: Case,
  ): Promise<Recipient> {
+   // Sanitize HTML content
+   const sanitizedHtml = sanitizeHtml(
+     isIndictmentCase(theCase.type)
+       ? this.formatMessage(notifications.caseCompleted.prosecutorBody, {
+           courtCaseNumber: theCase.courtCaseNumber,
+           courtName: theCase.court?.name?.replace('dómur', 'dómi'),
+           ...
+         })
+       : this.formatMessage(notifications.signedRuling.prosecutorBodyS3, {
+           ...
+         })
+   )

    return this.sendEmail(
      subject,
-     html,
+     sanitizedHtml,
      theCase.prosecutor?.name,
      theCase.prosecutor?.email,
    )
  }

Line range hint 1583-1612: Optimize notification sending with batching

The sendNotification method could be optimized by:

  1. Implementing batch sending for multiple recipients
  2. Caching frequently used email templates
  3. Adding retry mechanisms with exponential backoff
+ private async batchSendEmails(
+   recipients: Array<{ email: string; name: string }>,
+   subject: string,
+   template: string,
+   attachments?: Attachment[],
+ ): Promise<Recipient[]> {
+   // Implement batched email sending
+   return this.emailService.sendBatch(recipients, subject, template, attachments);
+ }

  private async sendRulingNotifications(
    theCase: Case,
  ): Promise<DeliverResponse> {
-   const promises = [this.sendRulingEmailNotificationToProsecutor(theCase)]
+   const recipients = [{
+     email: theCase.prosecutor?.email,
+     name: theCase.prosecutor?.name,
+   }]

    if (isIndictmentCase(theCase.type)) {
      const uniqueDefendants = _uniqBy(
        theCase.defendants ?? [],
        (d: Defendant) => d.defenderEmail,
      )
-     uniqueDefendants.forEach((defendant) => {
+     recipients.push(...uniqueDefendants.map(defendant => ({
+       email: defendant.defenderEmail,
+       name: defendant.defenderName,
+     })))
-       if (defendant.defenderEmail) {
-         promises.push(
-           this.sendRulingEmailNotificationToDefender(
-             theCase,
-             defendant.defenderNationalId,
-             defendant.defenderName,
-             defendant.defenderEmail,
-           ),
-         )
-       }
-     })
    }

+   const batchResults = await this.batchSendEmails(
+     recipients.filter(r => r.email),
+     subject,
+     template,
+   )

Line range hint 1-1700: Consider splitting the service into smaller, focused services

The CaseNotificationService has grown quite large and handles multiple responsibilities. Consider:

  1. Extracting email and SMS sending logic into separate services
  2. Creating separate services for different notification types (appeal, ruling, etc.)
  3. Implementing a notification factory pattern

Suggested structure:

// notification.factory.ts
export class NotificationFactory {
  createNotification(type: CaseNotificationType): BaseNotificationHandler {
    switch (type) {
      case CaseNotificationType.APPEAL:
        return new AppealNotificationHandler();
      case CaseNotificationType.RULING:
        return new RulingNotificationHandler();
      // ...
    }
  }
}

// handlers/appeal.notification.handler.ts
export class AppealNotificationHandler extends BaseNotificationHandler {
  // Appeal-specific notification logic
}

// handlers/ruling.notification.handler.ts
export class RulingNotificationHandler extends BaseNotificationHandler {
  // Ruling-specific notification logic
}
apps/judicial-system/backend/src/app/modules/notification/services/defendantNotification/defendantNotification.service.ts (2)

182-217: Consider refactoring to reduce code duplication

The methods sendIndictmentSentToPrisonAdminNotification and sendIndictmentWithdrawnFromPrisonAdminNotification have similar structures and duplicate code for formatting messages and preparing recipients. Consider extracting the common logic into a helper method to improve maintainability.

Apply this diff to refactor the code:

+private preparePrisonAdminNotification(
+  theCase: Case,
+  subjectTemplate: string,
+  bodyTemplate: string,
+  notificationType: DefendantNotificationType,
+): Promise<DeliverResponse> {
+  const formattedSubject = this.formatMessage(subjectTemplate, {
+    courtCaseNumber: theCase.courtCaseNumber,
+  })
+
+  const formattedBody = this.formatMessage(bodyTemplate, {
+    courtCaseNumber: theCase.courtCaseNumber,
+    linkStart: `<a href="${this.config.clientUrl}${ROUTE_HANDLER_ROUTE}/${theCase.id}">`,
+    linkEnd: '</a>',
+  })
+
+  const to = this.config.email.prisonAdminIndictmentEmails
+    .split(',')
+    .map((email) => email.trim())
+    .map((email) => ({
+      name: 'Fangelsismálastofnun',
+      email,
+    }))
+
+  return this.sendEmails(
+    theCase,
+    notificationType,
+    formattedSubject,
+    formattedBody,
+    to,
+  )
+}

Then, modify the existing methods to use this helper:

-private sendIndictmentSentToPrisonAdminNotification(theCase: Case) {
-  const formattedSubject = this.formatMessage(
-    strings.indictmentSentToPrisonAdminSubject,
-    {
-      courtCaseNumber: theCase.courtCaseNumber,
-    },
-  )
-  // ... rest of the method
-}
+private sendIndictmentSentToPrisonAdminNotification(theCase: Case) {
+  return this.preparePrisonAdminNotification(
+    theCase,
+    strings.indictmentSentToPrisonAdminSubject,
+    strings.indictmentSentToPrisonAdminBody,
+    DefendantNotificationType.INDICTMENT_SENT_TO_PRISON_ADMIN,
+  )
+}
-private sendIndictmentWithdrawnFromPrisonAdminNotification(theCase: Case) {
-  const formattedSubject = this.formatMessage(
-    strings.indictmentWithdrawnFromPrisonAdminSubject,
-    {
-      courtCaseNumber: theCase.courtCaseNumber,
-    },
-  )
-  // ... rest of the method
-}
+private sendIndictmentWithdrawnFromPrisonAdminNotification(theCase: Case) {
+  return this.preparePrisonAdminNotification(
+    theCase,
+    strings.indictmentWithdrawnFromPrisonAdminSubject,
+    strings.indictmentWithdrawnFromPrisonAdminBody,
+    DefendantNotificationType.INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN,
+  )
+}

205-206: Externalize hardcoded recipient name for better maintainability

The recipient name 'Fangelsismálastofnun' is hardcoded in both methods. Consider externalizing this string into a configuration or localization file to facilitate easier updates and localization in the future.

Also applies to: 242-243

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 78bdd5b and 207d74f.

📒 Files selected for processing (22)
  • apps/judicial-system/backend/infra/judicial-system-backend.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/defendant/defendant.service.ts (3 hunks)
  • apps/judicial-system/backend/src/app/modules/defendant/test/defendantController/update.spec.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/notification/notification.config.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/notification/notification.module.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/notification/services/caseNotification/caseNotification.service.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/notification/services/civilClaimantNotification/civilClaimantNotification.service.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/notification/services/defendantNotification/defendantNotification.service.ts (3 hunks)
  • apps/judicial-system/backend/src/app/modules/notification/services/defendantNotification/defendantNotification.strings.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/notification/services/institutionNotification/institutionNotification.service.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/notification/services/subpoenaNotification/subpoenaNotification.service.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/notification/test/createTestingNotificationModule.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/defendantNotification/sendIndictmentSentToPrisonAdminNotification.spec.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/defendantNotification/sendIndictmentWithdrawnFromPrisonAdminNotifications.spec.ts (1 hunks)
  • charts/judicial-system/values.dev.yaml (1 hunks)
  • charts/judicial-system/values.prod.yaml (1 hunks)
  • charts/judicial-system/values.staging.yaml (1 hunks)
  • charts/services/judicial-system-backend/values.dev.yaml (1 hunks)
  • charts/services/judicial-system-backend/values.prod.yaml (1 hunks)
  • charts/services/judicial-system-backend/values.staging.yaml (1 hunks)
  • libs/judicial-system/types/src/lib/notification.ts (2 hunks)
✅ Files skipped from review due to trivial changes (2)
  • apps/judicial-system/backend/src/app/modules/notification/services/civilClaimantNotification/civilClaimantNotification.service.ts
  • apps/judicial-system/backend/src/app/modules/notification/services/subpoenaNotification/subpoenaNotification.service.ts
🧰 Additional context used
📓 Path-based instructions (14)
apps/judicial-system/backend/infra/judicial-system-backend.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/defendant/defendant.service.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/defendant/test/defendantController/update.spec.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/notification/notification.config.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/notification/notification.module.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/notification/services/caseNotification/caseNotification.service.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/notification/services/defendantNotification/defendantNotification.service.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/notification/services/defendantNotification/defendantNotification.strings.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/notification/services/institutionNotification/institutionNotification.service.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/notification/test/createTestingNotificationModule.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/defendantNotification/sendIndictmentSentToPrisonAdminNotification.spec.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/defendantNotification/sendIndictmentWithdrawnFromPrisonAdminNotifications.spec.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
libs/judicial-system/types/src/lib/notification.ts (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
📓 Learnings (3)
apps/judicial-system/backend/src/app/modules/notification/services/defendantNotification/defendantNotification.service.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16452
File: apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.ts:1845-1854
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In the `sendCaseFilesUpdatedNotifications` method within `internalNotification.service.ts`, when the `user` is a defence user, it is intended to send notifications to the prosecutor.
apps/judicial-system/backend/src/app/modules/notification/services/institutionNotification/institutionNotification.service.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16452
File: apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.ts:1845-1854
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In the `sendCaseFilesUpdatedNotifications` method within `internalNotification.service.ts`, when the `user` is a defence user, it is intended to send notifications to the prosecutor.
libs/judicial-system/types/src/lib/notification.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16556
File: apps/judicial-system/backend/src/app/modules/notification/guards/rolesRules.ts:13-16
Timestamp: 2024-11-12T15:15:26.274Z
Learning: In the `CaseNotificationType` enum in `libs/judicial-system/types/src/lib/notification.ts`, `APPEAL_CASE_FILES_UPDATED` and `CASE_FILES_UPDATED` are distinct notification types that need to remain separate, as they serve different purposes in the notification system.
🔇 Additional comments (20)
apps/judicial-system/backend/src/app/modules/notification/notification.config.ts (1)

17-20: LGTM! Clean implementation of the new configuration.

The addition of prisonAdminIndictmentEmails follows the existing configuration patterns and is well-structured within the email configuration object.

apps/judicial-system/backend/src/app/modules/notification/notification.module.ts (1)

20-24: Verify directory structure consistency.

Let's ensure the new directory structure is consistently implemented.

✅ Verification successful

Let me verify if the baseNotification.service.ts is actually being used by checking its imports and implementation.


The directory structure is consistent, but baseNotification.service.ts is correctly placed at root level

The verification shows that:

  • All notification services are properly organized in their respective directories under services/
  • The baseNotification.service.ts file at the root level is an abstract base class that is actively imported and extended by all notification services
  • Its placement at the root level is appropriate as it's a shared base class
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence and consistency of the new service directory structure

# Check if all service directories exist
echo "Checking service directories..."
for service in caseNotification civilClaimantNotification defendantNotification institutionNotification subpoenaNotification; do
  fd -t d "services/$service" apps/judicial-system/backend/src
done

# Verify no old service files remain at the root level
echo "Checking for old service files..."
fd -t f ".*Notification\.service\.ts$" apps/judicial-system/backend/src/app/modules/notification --exclude "services/*"

Length of output: 1362


Script:

#!/bin/bash
# Check the content and usage of baseNotification.service.ts
echo "Content of baseNotification.service.ts:"
cat apps/judicial-system/backend/src/app/modules/notification/baseNotification.service.ts

echo -e "\nChecking imports of baseNotification.service.ts:"
rg -l "baseNotification.service" apps/judicial-system/backend/src

Length of output: 6575

apps/judicial-system/backend/src/app/modules/notification/services/institutionNotification/institutionNotification.service.ts (2)

15-21: LGTM! Import paths are properly organized.

The import statements have been correctly reorganized to use relative paths, maintaining code clarity and modularity.


Line range hint 1-109: Implementation missing for prison admin notifications.

Based on the PR objectives to implement "Indictment sent or withdrawn from prison notifications", this service should be extended to handle these new notification types. Currently, it only handles INDICTMENTS_WAITING_FOR_CONFIRMATION.

Consider the following architectural improvements:

  1. Add new notification types in the switch statement
  2. Implement methods for handling prison admin notifications
  3. Add documentation for the new notification types

Let's verify if the prison admin notification types are defined:

libs/judicial-system/types/src/lib/notification.ts (1)

69-70: LGTM! Proper integration with NotificationType enum.

The new notification types are correctly aggregated in the NotificationType enum with proper references to DefendantNotificationType.

apps/judicial-system/backend/infra/judicial-system-backend.ts (1)

73-74: LGTM! Verify secret configuration.

The new secret for prison admin indictment emails follows the established naming and path conventions.

Let's verify the secret configuration:

charts/services/judicial-system-backend/values.dev.yaml (2)

141-141: Verify secret configuration across environments

The new secret PRISON_ADMIN_INDICTMENT_EMAILS has been added for prison admin notifications. Please ensure:

  1. The secret is configured in all environments (staging, production)
  2. The secret is documented in the team's secret management documentation
✅ Verification successful

Secret is properly configured across environments

The PRISON_ADMIN_INDICTMENT_EMAILS secret is:

  • Correctly configured in all environments (dev, staging, and production)
  • Properly integrated in the codebase with corresponding config handling and tests
  • Used consistently in the notification service for prison admin communications

While no explicit secrets documentation was found, the secret follows the established pattern across all environment configurations and is well-integrated into the application's notification system.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the secret is configured consistently across environments
# and documented properly

# Check if the secret exists in other environment files
echo "Checking environment files..."
for env in staging prod; do
  rg "PRISON_ADMIN_INDICTMENT_EMAILS" "charts/judicial-system/values.${env}.yaml" || echo "Missing in ${env}"
done

# Check if the secret is documented
echo -e "\nChecking documentation..."
rg -i "prison.*admin.*indictment.*email" "docs/"

Length of output: 617


Script:

#!/bin/bash
# Let's check for documentation in other common locations and README files
echo "Checking for documentation in README files and other common locations..."
rg -i "prison.*admin.*indictment.*email" .

# Also check if there's any documentation about secrets management in general
echo -e "\nChecking for general secrets documentation..."
rg -i "secret.*management|managing.*secret" .

Length of output: 2531


Line range hint 1-5: ⚠️ Note: This is an auto-generated file

As per the warning comment, this file is automatically generated. Ensure you're making changes to the source templates instead of editing this file directly. Run yarn charts to regenerate the configuration files.

✅ Verification successful

⚠️ Do not edit values files directly - use source templates

The warning is correct. The values files in the charts/services/ directory are automatically generated. The source templates and configuration are managed through a Domain-Specific Language (DSL) in the infra/ directory, specifically:

  • Service definitions in infra/src/uber-charts/
  • Chart templates in infra/helm/libs/
  • DSL implementation in infra/src/dsl/

To make changes:

  1. Modify the appropriate service configuration in infra/src/uber-charts/
  2. Run yarn charts from the root directory to regenerate all values files
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find the source template used to generate this file

# Look for chart template files that might generate this configuration
echo "Searching for chart templates..."
fd -e yaml -e yml -e jsonnet -e json "template|values" "charts/" | rg -v "\.dev\.|\.prod\.|\.staging\."

Length of output: 178


Script:

#!/bin/bash
# Let's try a different approach to find template or source files

# First, let's check if there's a script or configuration that mentions "charts" command
echo "Looking for files related to chart generation..."
rg -i "yarn charts|charts:" -A 5 -B 5

# Also check for any package.json scripts
echo -e "\nChecking package.json for chart-related scripts..."
fd "package.json" --exec cat {} | jq '.scripts | select(. != null)'

# Look for any documentation about chart generation
echo -e "\nChecking for documentation about chart generation..."
fd "README" --exec cat {}

Length of output: 113770


Script:

#!/bin/bash
# Let's look for the source of chart generation in the infra directory
echo "Looking for chart generation source files..."
fd -t f -e ts -e js "charts" "infra/"

# Also check for any documentation in the infra directory
echo -e "\nChecking for documentation in infra directory..."
fd -t f "README|CONTRIBUTING" "infra/"

# Look for the chart generation configuration
echo -e "\nChecking for chart configuration files..."
fd -t f -e yaml -e yml -e json "values|chart" "infra/"

Length of output: 943


Script:

#!/bin/bash
# Let's examine the content of key files to understand the chart generation process
echo "Checking infra/README.md for chart generation instructions..."
cat infra/README.md

echo -e "\nChecking chart generation source code..."
cat infra/src/uber-charts/all-charts.ts

echo -e "\nChecking DSL documentation..."
cat infra/src/dsl/README.md

Length of output: 5559

charts/services/judicial-system-backend/values.prod.yaml (1)

141-141: Verify the secret exists in the infrastructure

The new secret PRISON_ADMIN_INDICTMENT_EMAILS needs to be properly configured in the infrastructure before deployment.

apps/judicial-system/backend/src/app/modules/notification/test/createTestingNotificationModule.ts (1)

33-36: LGTM! Import paths are well-organized.

The reorganization of imports follows a clean, domain-driven structure that improves code maintainability.

Let's verify that these import paths are consistently used across the codebase:

✅ Verification successful

Import paths are consistently used across the notification module

The verification shows that all imports of notification services follow the same organized pattern throughout the codebase:

  • All service imports in notification.module.ts use the same path structure
  • The internalNotification.controller.ts follows the same import pattern
  • All notification services properly extend from BaseNotificationService
  • The test module imports match the production code structure
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the consistency of notification service imports
# Expected: All imports should follow the same pattern

# Check for any inconsistent imports of these services
rg -g '!*.{spec,test}.ts' "from '.*/(Case|CivilClaimant|Defendant|Institution)NotificationService'" apps/judicial-system/backend

Length of output: 136


Script:

#!/bin/bash
# Let's try a different approach to verify the import patterns

# First, let's find all notification service files
fd "NotificationService.ts$" apps/judicial-system/backend

# Then, let's check all imports of these services across the codebase
# excluding test files but including the specific test file we're reviewing
rg --type ts "import.*NotificationService" apps/judicial-system/backend

# Also check if there are any other files importing from old paths
rg --type ts "from '.*notification.*Service'" apps/judicial-system/backend

Length of output: 4448

charts/services/judicial-system-backend/values.staging.yaml (1)

142-142: LGTM! Verify secret across environments.

The new secret for prison admin indictment emails follows the established pattern and security model.

✅ Verification successful

✅ Secret configuration verified across all environments

The PRISON_ADMIN_INDICTMENT_EMAILS secret is consistently configured with the same path /k8s/judicial-system/PRISON_ADMIN_INDICTMENT_EMAILS across development, staging, and production environments.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the secret is consistently defined across environments
# Expected: The secret should be present in dev and prod environments

# Check for the secret in dev and prod values files
echo "Checking dev environment..."
fd -t f "values.dev.yaml" -x grep -l "PRISON_ADMIN_INDICTMENT_EMAILS"

echo "Checking prod environment..."
fd -t f "values.prod.yaml" -x grep -l "PRISON_ADMIN_INDICTMENT_EMAILS"

Length of output: 468


Script:

#!/bin/bash
# Let's verify the actual secret configuration across environments
echo "=== Development Environment ==="
grep -A 1 "PRISON_ADMIN_INDICTMENT_EMAILS" ./charts/services/judicial-system-backend/values.dev.yaml

echo -e "\n=== Staging Environment ==="
grep -A 1 "PRISON_ADMIN_INDICTMENT_EMAILS" ./charts/services/judicial-system-backend/values.staging.yaml

echo -e "\n=== Production Environment ==="
grep -A 1 "PRISON_ADMIN_INDICTMENT_EMAILS" ./charts/services/judicial-system-backend/values.prod.yaml

Length of output: 948

apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts (2)

37-41: LGTM: Import paths have been properly restructured.

The reorganization of service imports into a dedicated services directory improves code organization and maintainability.


Line range hint 1-200: Missing implementation for prison indictment notifications.

Based on the PR objectives, this controller should include endpoints for handling prison indictment notifications (sent/withdrawn). However, no such endpoints are implemented.

Consider adding the following:

  1. A new DTO for prison indictment notifications
  2. New endpoint(s) for handling prison admin notifications

Here's a suggested implementation:

// New DTO
export class PrisonIndictmentNotificationDto {
  type: NotificationType;
}

// New endpoints to add to the controller
@Post(`case/:caseId/${messageEndpoint[MessageType.PRISON_INDICTMENT_NOTIFICATION]}`)
@UseGuards(CaseHasExistedGuard)
@ApiCreatedResponse({
  type: DeliverResponse,
  description: 'Sends prison indictment notifications',
})
sendPrisonIndictmentNotification(
  @Param('caseId') caseId: string,
  @CurrentCase() theCase: Case,
  @Body() notificationDto: PrisonIndictmentNotificationDto,
): Promise<DeliverResponse> {
  this.logger.debug(
    `Sending ${notificationDto.type} prison indictment notification for case ${caseId}`,
  )

  return this.institutionNotificationService.sendPrisonIndictmentNotification(
    notificationDto.type,
    theCase,
  )
}

Let's verify if similar notification endpoints exist elsewhere:

charts/judicial-system/values.dev.yaml (2)

Line range hint 1-7: Reminder: This file should not be edited manually

The file header indicates that it should be generated using "yarn charts" command instead of manual edits. Please ensure you're following the correct process for updating this configuration.

✅ Verification successful

Let me analyze the infrastructure setup to verify the chart generation process, since the previous output showed that the charts command is executed from the infra/ directory.


Proper chart generation process is in place

The file warning is valid and correct. The repository has a proper chart generation setup with:

  • A dedicated charts script in package.json that runs from the infra/ directory
  • Chart generation infrastructure in infra/src/cli/generate-chart-values.ts
  • Template files and chart definitions in the infra/helm/ directory

The warning header serves its purpose by directing developers to use the correct tooling (yarn charts) instead of manual edits.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify if the file was generated using the proper tooling

# Check if yarn charts command exists in package.json scripts
cat package.json | jq -r '.scripts | select(.charts != null)'

# Look for chart generation related files
fd -t f "values.template" charts/

Length of output: 2312


Script:

#!/bin/bash
# Check if there's a chart generation setup in the infra directory
fd -t f "values.*yaml" infra/
fd -t f "values.template" infra/

# Check for any chart generation related scripts
fd -t f "chart" infra/

Length of output: 698


227-227: Verify the secret path exists in AWS Parameter Store

The new secret PRISON_ADMIN_INDICTMENT_EMAILS has been added for prison administration indictment notifications, which aligns with the PR objectives. Ensure the corresponding secret value is properly configured in the AWS Parameter Store.

✅ Verification successful

Let me gather more information about how this secret is used in the codebase.


Based on the gathered information, I can now provide a final response:

The secret path for PRISON_ADMIN_INDICTMENT_EMAILS is properly integrated

The secret is correctly referenced in the codebase:

  • Defined in the notification config as a required environment variable
  • Used in the backend infrastructure configuration with the correct path
  • Implemented in the notification system for sending indictment notifications to prison administrators
  • Verified through test cases that demonstrate the expected functionality
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if other components reference this secret

# Search for references to the new secret
rg "PRISON_ADMIN_INDICTMENT_EMAILS" -g '!values.*.yaml'

# Check for related environment variable usage
ast-grep --pattern 'process.env.PRISON_ADMIN_INDICTMENT_EMAILS'

Length of output: 1425


Script:

#!/bin/bash
# Check the notification config and controller implementation
cat apps/judicial-system/backend/src/app/modules/notification/notification.config.ts
cat apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/defendantNotification/sendIndictmentSentToPrisonAdminNotification.spec.ts

Length of output: 5557

charts/judicial-system/values.staging.yaml (2)

228-228: LGTM! Please verify configuration in other environments.

The new secret PRISON_ADMIN_INDICTMENT_EMAILS is properly configured for the staging environment.

✅ Verification successful

Configuration verified across all environments

The secret PRISON_ADMIN_INDICTMENT_EMAILS is properly configured in all environments:

  • Present in dev environment files
    • charts/services/judicial-system-backend/values.dev.yaml
    • charts/judicial-system/values.dev.yaml
  • Present in staging environment (from original review)
  • Present in prod environment files
    • charts/services/judicial-system-backend/values.prod.yaml
    • charts/judicial-system/values.prod.yaml
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify if the secret is configured in other environments
# Expected: The secret should be present in dev and prod values files

# Check dev environment
echo "Checking dev environment..."
fd --type f "values.dev.yaml" | xargs grep "PRISON_ADMIN_INDICTMENT_EMAILS"

# Check prod environment
echo "Checking prod environment..."
fd --type f "values.prod.yaml" | xargs grep "PRISON_ADMIN_INDICTMENT_EMAILS"

Length of output: 828


228-228: Reminder: Ensure AWS Parameter Store is configured.

The secret path /k8s/judicial-system/PRISON_ADMIN_INDICTMENT_EMAILS needs to be populated in AWS Parameter Store for the staging environment.

charts/judicial-system/values.prod.yaml (2)

227-227: LGTM: Secret configuration follows established patterns

The new secret PRISON_ADMIN_INDICTMENT_EMAILS is:

  • Correctly positioned in the judicial-system-backend section
  • Follows the existing naming convention
  • Uses the established pattern for AWS Parameter Store paths
  • Aligns with the PR objective of implementing prison admin notifications
✅ Verification successful

Secret configuration is consistent across all environments

The secret PRISON_ADMIN_INDICTMENT_EMAILS is correctly configured with identical paths in all environments (dev, staging, and prod), following the established pattern /k8s/judicial-system/PRISON_ADMIN_INDICTMENT_EMAILS. The configuration is properly aligned across the deployment pipeline.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify consistent secret configuration across environments
fd -t f "values\.(dev|staging)\.yaml" -x grep -l "PRISON_ADMIN_INDICTMENT_EMAILS" {}

Length of output: 292


Script:

#!/bin/bash
# Check the actual secret configuration in dev and staging environments
echo "=== Dev Environment ==="
rg "PRISON_ADMIN_INDICTMENT_EMAILS" ./charts/judicial-system/values.dev.yaml -A 1
echo -e "\n=== Staging Environment ==="
rg "PRISON_ADMIN_INDICTMENT_EMAILS" ./charts/judicial-system/values.staging.yaml -A 1

Length of output: 583


Line range hint 1-6: Warning: This file is marked as auto-generated

The file header explicitly states not to edit this file manually and suggests running "yarn charts" instead. Please ensure you're following the correct process for updating this configuration.

apps/judicial-system/backend/src/app/modules/notification/services/defendantNotification/defendantNotification.service.ts (1)

200-208: Verify that 'prisonAdminIndictmentEmails' configuration is properly set

Ensure that the prisonAdminIndictmentEmails configuration property is correctly defined and populated with valid email addresses. An undefined or empty value could lead to notifications not being sent to the prison administrators.

Run the following script to check the configuration:

Also applies to: 237-245

✅ Verification successful

Configuration is properly set and validated

The prisonAdminIndictmentEmails configuration is correctly defined and validated:

  • It's required through environment variable PRISON_ADMIN_INDICTMENT_EMAILS in notification.config.ts
  • The configuration is enforced as required using env.required(), which means the application won't start without this value being set
  • The configuration is properly referenced in deployment configurations for all environments (dev, staging, prod) in the Helm charts
  • Test files demonstrate proper validation by setting the environment variable with comma-separated email addresses
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that 'prisonAdminIndictmentEmails' is set in the configuration files.

# Expectation: The property should be defined and contain at least one email address.

# Search for the configuration property in all configuration files
rg 'prisonAdminIndictmentEmails' --glob '**/config/**'

# Check if the value is non-empty
rg 'prisonAdminIndictmentEmails.*=.*[^\'\"\s]' --glob '**/config/**'

Length of output: 172


Script:

#!/bin/bash
# Let's try a different approach to find the configuration
# First, let's find all configuration files
fd -e ts -e js config

# Then search for prisonAdminIndictmentEmails in these files
rg "prisonAdminIndictmentEmails" 

# Also check environment configuration files
fd -e env -e env.example

# And check their content for email configuration
rg "PRISON.*EMAIL" 

Length of output: 48471

Copy link
Member

@gudjong gudjong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realy nice. One suggestion.

@gudjong gudjong added the automerge Merge this PR as soon as all checks pass label Nov 27, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
libs/judicial-system/types/src/lib/notification.ts (1)

Line range hint 8-8: Address the commented removal of ADVOCATE_ASSIGNED.

The comment indicates that ADVOCATE_ASSIGNED will be removed, but it's still present in both CaseNotificationType and NotificationType. Either:

  1. Remove the entry if it's no longer needed, or
  2. Remove the comment if the entry should be kept
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 207d74f and 939e7cf.

📒 Files selected for processing (1)
  • libs/judicial-system/types/src/lib/notification.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/judicial-system/types/src/lib/notification.ts (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
📓 Learnings (1)
libs/judicial-system/types/src/lib/notification.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16556
File: apps/judicial-system/backend/src/app/modules/notification/guards/rolesRules.ts:13-16
Timestamp: 2024-11-12T15:15:26.274Z
Learning: In the `CaseNotificationType` enum in `libs/judicial-system/types/src/lib/notification.ts`, `APPEAL_CASE_FILES_UPDATED` and `CASE_FILES_UPDATED` are distinct notification types that need to remain separate, as they serve different purposes in the notification system.
🔇 Additional comments (2)
libs/judicial-system/types/src/lib/notification.ts (2)

26-27: Consider grouping with other defendant notifications for better readability.

The new notification types are well-named and align with the PR objectives. However, consider grouping them with other defendant-related notifications for better code organization.

 export enum DefendantNotificationType {
   DEFENDANT_SELECTED_DEFENDER = 'DEFENDANT_SELECTED_DEFENDER',
   DEFENDER_ASSIGNED = 'DEFENDER_ASSIGNED',
+  INDICTMENT_SENT_TO_PRISON_ADMIN = 'INDICTMENT_SENT_TO_PRISON_ADMIN',
+  INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN = 'INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN',
 }

65-66: Verify implementation of notification templates.

The notification types are properly added to the enum, but ensure that corresponding email templates are implemented for these new notifications.

✅ Verification successful

Email templates and notification handling are properly implemented

Based on the search results, the notification system for prison admin indictments is fully implemented with:

  • Email templates with subject and body content defined in defendantNotification.strings.ts
  • Notification handling service in defendantNotification.service.ts with dedicated methods:
    • sendIndictmentSentToPrisonAdminNotification
    • sendIndictmentWithdrawnFromPrisonAdminNotification
  • Configuration for prison admin email recipients in notification.config.ts
  • Comprehensive test coverage in dedicated spec files
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for notification templates implementation
# Expected: Find template files for the new notification types

# Search for template files
echo "=== Checking for notification template files ==="
fd -e json -e hbs -e mjml -e html . | rg -i "template|notification|email"

# Search for prison admin notification content
echo -e "\n=== Checking for prison admin notification content ==="
rg -l "INDICTMENT_SENT_TO_PRISON_ADMIN|INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN" \
  --type json \
  --type html \
  --type typescript

Length of output: 29149


Script:

#!/bin/bash
# Description: Check for notification templates and content
# Expected: Find template files and content for the new notification types

# Search for notification templates and content
echo "=== Checking for notification templates and content ==="
rg -l "INDICTMENT_SENT_TO_PRISON_ADMIN|INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN" \
  --type-add 'ts:*.ts' \
  --type-add 'hbs:*.hbs' \
  --type ts \
  --type hbs \
  --type json

# Search for email templates directory
echo -e "\n=== Checking email templates directory ==="
fd -t d "templates|email"

# Search for notification content in email templates
echo -e "\n=== Checking notification content in email templates ==="
rg -i "indictment.*prison.*admin|prison.*admin.*indictment" \
  --type-add 'ts:*.ts' \
  --type-add 'hbs:*.hbs' \
  --type ts \
  --type hbs \
  --type json

Length of output: 16175

@kodiakhq kodiakhq bot merged commit 14a7777 into main Nov 27, 2024
42 checks passed
@kodiakhq kodiakhq bot deleted the j-s/indictment-to-prison-notifications branch November 27, 2024 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this PR as soon as all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants