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): Deliver subpoena revocation to police #17291

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

unakb
Copy link
Member

@unakb unakb commented Dec 19, 2024

Afturköllun á fyrirkalli - kalla í endapunkt þegar afturköllun er gerð.

What

Deliver subpoena revocation to police

Why

So they know that an indictment has been cancelled and can send out notifications to defendants and revoke the subpoenas

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

  • New Features

    • Introduced asynchronous methods for managing subpoenas, including revocation and retrieval by case ID.
    • Added functionality to deliver revoked subpoenas to the police with appropriate guards and logging.
    • New enumeration value for tracking subpoena revocation delivery to police.
  • Bug Fixes

    • Enhanced error handling mechanisms for subpoena creation and revocation processes.
  • Tests

    • Added unit tests for the functionality of delivering revoked subpoenas to the police.
    • Introduced a test suite to validate guard configurations for the delivery method.

@unakb unakb requested a review from a team as a code owner December 19, 2024 11:03
Copy link
Contributor

coderabbitai bot commented Dec 19, 2024

Walkthrough

This pull request introduces enhancements to the subpoena and case management functionality in the judicial system backend. The changes primarily focus on adding new methods for handling subpoena revocation, including the ability to deliver revoked subpoenas to the police. The modifications span multiple services and controllers, introducing asynchronous methods for finding and revoking subpoenas, updating error handling, and adding new message type endpoints for communication.

Changes

File Change Summary
apps/judicial-system/backend/src/app/modules/case/case.service.ts Updated addMessagesForRevokedIndictmentCaseToQueue to be an async method with improved subpoena handling.
apps/judicial-system/backend/src/app/modules/police/police.service.ts Added revokeSubpoena method for handling subpoena revocation with error logging.
apps/judicial-system/backend/src/app/modules/subpoena/internalSubpoena.controller.ts Added deliverSubpoenaRevokedToPolice method with appropriate guards and routing.
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts Added findByCaseId and deliverSubpoenaRevokedToPolice methods.
libs/judicial-system/message/src/lib/message.ts Added new message type DELIVERY_TO_POLICE_SUBPOENA_REVOCATION.

Possibly related PRs

Suggested labels

automerge

Suggested reviewers

  • gudjong

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

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: 2

🧹 Nitpick comments (3)
apps/judicial-system/backend/src/app/modules/subpoena/internalSubpoena.controller.ts (1)

93-125: LGTM! Implementation follows established patterns

The new endpoint is well-structured with appropriate guards and logging. The implementation follows the same pattern as the existing deliverSubpoenaToPolice endpoint.

Consider adding JSDoc documentation to describe the endpoint's purpose and parameters:

+/**
+ * Delivers a subpoena revocation notification to the police.
+ * @param caseId - The ID of the case
+ * @param defendantId - The ID of the defendant
+ * @param subpoenaId - The ID of the subpoena to be revoked
+ * @param theCase - The current case
+ * @param subpoena - The subpoena to be revoked
+ * @param deliverDto - The delivery information including user details
+ * @returns A promise that resolves to a DeliverResponse
+ */
 @UseGuards(
   CaseExistsGuard,
   new CaseTypeGuard(indictmentCases),
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (1)

365-391: Consider enhancing error handling pattern.

While the error handling is good, consider extracting the success/failure logging into separate methods to improve readability and maintainability. This would also make the success/failure messaging more consistent across the service.

Consider this refactor:

 async deliverSubpoenaRevokedToPolice(
   theCase: Case,
   subpoena: Subpoena,
   user: TUser,
 ): Promise<DeliverResponse> {
   if (!subpoena.subpoenaId) {
     this.logger.warn(
       `Attempted to revoke a subpoena with id ${subpoena.id} that had not been delivered to the police`,
     )
     return { delivered: true }
   }

   const subpoenaRevoked = await this.policeService.revokeSubpoena(
     theCase,
     subpoena,
     user,
   )

-  if (subpoenaRevoked) {
-    this.logger.info(
-      `Subpoena ${subpoena.subpoenaId} successfully revoked from police`,
-    )
-    return { delivered: true }
-  } else {
-    return { delivered: false }
-  }
+  const success = Boolean(subpoenaRevoked)
+  if (success) {
+    this.logSuccessfulRevocation(subpoena.subpoenaId)
+  }
+  return { delivered: success }
 }

+private logSuccessfulRevocation(subpoenaId: string): void {
+  this.logger.info(
+    `Subpoena ${subpoenaId} successfully revoked from police`,
+  )
+}
apps/judicial-system/backend/src/app/modules/case/case.service.ts (1)

Line range hint 1055-1084: Consider improving message handling and error recovery.

The message queue handling for revoked indictments is good, but could benefit from:

  1. Error handling for failed subpoena lookups
  2. Batch processing of messages to reduce queue operations
  3. Message priority handling

Consider this improvement:

 private async addMessagesForRevokedIndictmentCaseToQueue(
   theCase: Case,
   user: TUser,
 ): Promise<void> {
   const messages = this.getRevokeNotificationMessages(user, theCase)

   if (theCase.courtCaseNumber) {
     messages.push({
       type: MessageType.DELIVERY_TO_COURT_INDICTMENT_CANCELLATION_NOTICE,
       user,
       caseId: theCase.id,
       body: { withCourtCaseNumber: true },
     })
   }

+  try {
     const subpoenasToRevoke = await this.subpoenaService.findByCaseId(
       theCase.id,
     )

     if (subpoenasToRevoke?.length > 0) {
+      // Group messages by priority
+      const priorityMessages = []
+      const normalMessages = []
+
       subpoenasToRevoke.forEach((subpoena) => {
-        messages.push({
+        const message = {
           type: MessageType.DELIVERY_TO_POLICE_SUBPOENA_REVOCATION,
           user,
           caseId: theCase.id,
           elementId: [subpoena.defendantId, subpoena.id],
-        })
+        }
+        if (this.isHighPrioritySubpoena(subpoena)) {
+          priorityMessages.push(message)
+        } else {
+          normalMessages.push(message)
+        }
       })
+
+      // Send high priority messages first
+      messages.push(...priorityMessages, ...normalMessages)
     }
+  } catch (error) {
+    this.logger.error(
+      `Failed to fetch subpoenas for case ${theCase.id}`,
+      { error }
+    )
+    // Continue with other messages even if subpoena fetch fails
+  }

   return this.messageService.sendMessagesToQueue(messages)
 }

+private isHighPrioritySubpoena(subpoena: Subpoena): boolean {
+  // Add logic to determine high priority subpoenas
+  return false
+}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 15edb19 and 0a9cd3f.

📒 Files selected for processing (6)
  • apps/judicial-system/backend/src/app/modules/case/case.service.ts (2 hunks)
  • apps/judicial-system/backend/src/app/modules/police/police.service.ts (2 hunks)
  • apps/judicial-system/backend/src/app/modules/subpoena/internalSubpoena.controller.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (2 hunks)
  • apps/judicial-system/backend/src/app/modules/subpoena/test/internalSubpoenaController/deliverSubpoenaRevokedToPolice.spec.ts (1 hunks)
  • libs/judicial-system/message/src/lib/message.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
libs/judicial-system/message/src/lib/message.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."
apps/judicial-system/backend/src/app/modules/subpoena/internalSubpoena.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/case/case.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/police/police.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/subpoena/test/internalSubpoenaController/deliverSubpoenaRevokedToPolice.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/subpoena/subpoena.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."
📓 Learnings (4)
apps/judicial-system/backend/src/app/modules/subpoena/internalSubpoena.controller.ts (2)
Learnt from: unakb
PR: island-is/island.is#16393
File: apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts:164-169
Timestamp: 2024-11-12T15:15:11.835Z
Learning: Ensure that suggested code changes in `subpoena.response.ts` are accurate and necessary, avoiding redundant suggestions.
Learnt from: gudjong
PR: island-is/island.is#16863
File: apps/judicial-system/backend/src/app/modules/subpoena/subpoena.controller.ts:86-98
Timestamp: 2024-11-27T15:27:52.059Z
Learning: In `apps/judicial-system/backend/src/app/modules/subpoena/subpoena.controller.ts`, user action auditing and exception handling are performed at a higher level, so explicit error handling and logging within the `getSubpoena` method is unnecessary.
apps/judicial-system/backend/src/app/modules/police/police.service.ts (1)
Learnt from: unakb
PR: island-is/island.is#16393
File: apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts:164-169
Timestamp: 2024-11-12T15:15:11.835Z
Learning: Ensure that suggested code changes in `subpoena.response.ts` are accurate and necessary, avoiding redundant suggestions.
apps/judicial-system/backend/src/app/modules/subpoena/test/internalSubpoenaController/deliverSubpoenaRevokedToPolice.spec.ts (1)
Learnt from: oddsson
PR: island-is/island.is#16463
File: apps/judicial-system/backend/src/app/modules/subpoena/test/limitedAccessSubpoenaController/limitedAccessSubpoenaControllerGuards.spec.ts:17-17
Timestamp: 2024-11-12T15:15:20.158Z
Learning: In `LimitedAccessSubpoenaController`, `SubpoenaExistsOptionalGuard` is still used on specific endpoints not covered by the test file `limitedAccessSubpoenaControllerGuards.spec.ts`.
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (2)
Learnt from: unakb
PR: island-is/island.is#16393
File: apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts:164-169
Timestamp: 2024-11-12T15:15:11.835Z
Learning: Ensure that suggested code changes in `subpoena.response.ts` are accurate and necessary, avoiding redundant suggestions.
Learnt from: oddsson
PR: island-is/island.is#16329
File: apps/judicial-system/backend/src/app/modules/notification/subpoenaNotification.service.ts:0-0
Timestamp: 2024-11-12T15:15:26.274Z
Learning: In `apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts`, the method `findBySubpoenaId` throws an error if the subpoena is not found. Therefore, downstream code does not need to check if the result is undefined.
🔇 Additional comments (2)
libs/judicial-system/message/src/lib/message.ts (1)

26-26: LGTM! Message type and endpoint mapping are well-structured

The new message type DELIVERY_TO_POLICE_SUBPOENA_REVOCATION and its corresponding endpoint mapping follow the established naming conventions and are logically grouped with other police-related messages.

Also applies to: 67-67

apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (1)

286-291: LGTM! Well-structured database query method.

The method follows established patterns and properly uses the shared include constant for consistent eager loading of related models.

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

🧹 Nitpick comments (1)
apps/judicial-system/backend/src/app/modules/subpoena/test/internalSubpoenaController/deliverSubpoenaRevokedToPoliceGuards.spec.ts (1)

9-17: Consider using proper typing instead of disabling ESLint.

While the current setup follows the established pattern for guard tests, we could improve type safety:

-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  let guards: any[]
+  let guards: Array<new () => unknown>
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0a9cd3f and 161892f.

📒 Files selected for processing (1)
  • apps/judicial-system/backend/src/app/modules/subpoena/test/internalSubpoenaController/deliverSubpoenaRevokedToPoliceGuards.spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/subpoena/test/internalSubpoenaController/deliverSubpoenaRevokedToPoliceGuards.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."
📓 Learnings (1)
apps/judicial-system/backend/src/app/modules/subpoena/test/internalSubpoenaController/deliverSubpoenaRevokedToPoliceGuards.spec.ts (3)
Learnt from: oddsson
PR: island-is/island.is#16463
File: apps/judicial-system/backend/src/app/modules/subpoena/test/limitedAccessSubpoenaController/limitedAccessSubpoenaControllerGuards.spec.ts:17-17
Timestamp: 2024-11-12T15:15:20.158Z
Learning: In `LimitedAccessSubpoenaController`, `SubpoenaExistsOptionalGuard` is still used on specific endpoints not covered by the test file `limitedAccessSubpoenaControllerGuards.spec.ts`.
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getIndictmentPdfGuards.spec.ts:24-25
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In certain scenarios within the judicial-system backend, the `RolesGuard` may intentionally follow the `CaseExistsGuard` when specific roles rules require the guard order to be reversed, as seen in tests like `getIndictmentPdfGuards.spec.ts`.
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts:175-185
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In the Jest tests for the `LimitedAccessViewCaseFileGuard` in `apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts`, code duplication in the `beforeEach` blocks is acceptable and should remain unchanged.
🔇 Additional comments (2)
apps/judicial-system/backend/src/app/modules/subpoena/test/internalSubpoenaController/deliverSubpoenaRevokedToPoliceGuards.spec.ts (2)

1-7: LGTM! Clean and well-organized imports.

The imports follow NextJS best practices using path aliases and are properly organized.


19-28: LGTM! Comprehensive guard verification.

The test thoroughly verifies the guard chain for the deliverSubpoenaRevokedToPolice endpoint. The guard order follows the expected pattern where CaseExistsGuard precedes other guards.

Let's verify the guard implementation in the controller:

✅ Verification successful

✓ Guard implementation matches the test expectations

The controller implementation perfectly matches the test assertions. The @UseGuards decorator shows the guards in the exact order and configuration being tested:

  1. CaseExistsGuard
  2. CaseTypeGuard with indictmentCases
  3. DefendantExistsGuard
  4. SubpoenaExistsGuard
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify guard decorators on the deliverSubpoenaRevokedToPolice method

# Search for the method implementation and its decorators
ast-grep --pattern 'deliverSubpoenaRevokedToPolice($$$) {
  $$$
}'

Length of output: 68


Script:

#!/bin/bash
# Let's try a broader search for the controller implementation
rg -l "deliverSubpoenaRevokedToPolice" --type ts

# Then search for the specific file content
rg -A 10 "deliverSubpoenaRevokedToPolice" --type ts

Length of output: 9894


Script:

#!/bin/bash
# Let's search for the guard decorators on the controller method
rg -B 10 "deliverSubpoenaRevokedToPolice\(" apps/judicial-system/backend/src/app/modules/subpoena/internalSubpoena.controller.ts

Length of output: 867


Script:

#!/bin/bash
# Let's check the class-level decorators and imports that might contain the guards
rg -B 20 "class InternalSubpoenaController" apps/judicial-system/backend/src/app/modules/subpoena/internalSubpoena.controller.ts

Length of output: 1188


Script:

#!/bin/bash
# Let's check the method-level decorators for the specific endpoint
rg -B 15 "deliverSubpoenaRevokedToPolice" apps/judicial-system/backend/src/app/modules/subpoena/internalSubpoena.controller.ts --context-separator=""

Length of output: 1044

Copy link

codecov bot commented Dec 19, 2024

Codecov Report

Attention: Patch coverage is 30.55556% with 25 lines in your changes missing coverage. Please review.

Project coverage is 35.68%. Comparing base (f31b613) to head (f09bc69).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...ckend/src/app/modules/subpoena/subpoena.service.ts 0.00% 12 Missing ⚠️
...m/backend/src/app/modules/police/police.service.ts 8.33% 11 Missing ⚠️
...ystem/backend/src/app/modules/case/case.service.ts 50.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #17291      +/-   ##
==========================================
- Coverage   35.70%   35.68%   -0.02%     
==========================================
  Files        6923     6952      +29     
  Lines      148597   148899     +302     
  Branches    42450    42547      +97     
==========================================
+ Hits        53058    53139      +81     
- Misses      95539    95760     +221     
Flag Coverage Δ
judicial-system-backend 55.93% <30.55%> (-0.05%) ⬇️
judicial-system-message 66.66% <100.00%> (+0.06%) ⬆️
judicial-system-message-handler 48.41% <100.00%> (+0.08%) ⬆️
judicial-system-scheduler 71.17% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...pp/modules/subpoena/internalSubpoena.controller.ts 96.07% <100.00%> (+0.62%) ⬆️
libs/judicial-system/message/src/lib/message.ts 100.00% <100.00%> (ø)
...ystem/backend/src/app/modules/case/case.service.ts 89.23% <50.00%> (-0.28%) ⬇️
...m/backend/src/app/modules/police/police.service.ts 61.73% <8.33%> (-3.11%) ⬇️
...ckend/src/app/modules/subpoena/subpoena.service.ts 38.00% <0.00%> (-3.31%) ⬇️

... and 104 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f31b613...f09bc69. Read the comment docs.

@datadog-island-is
Copy link

datadog-island-is bot commented Dec 19, 2024

Datadog Report

All test runs a12c0e9 🔗

4 Total Test Services: 0 Failed, 4 Passed
🔻 Test Sessions change in coverage: 1 decreased (-0.06%), 3 increased

Test Services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Code Coverage Change Test Service View
judicial-system-backend 0 0 0 21395 0 22m 51.86s 1 decreased (-0.06%) Link
judicial-system-message 0 0 0 41 0 9.66s 1 increased (+0.06%) Link
judicial-system-message-handler 0 0 0 4 0 3.18s 1 increased (+0.09%) Link
judicial-system-scheduler 0 0 0 4 0 3.72s 1 increased (+0.02%) Link

🔻 Code Coverage Decreases vs Default Branch (1)

  • judicial-system-backend - jest 59.56% (-0.06%) - Details

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 comments (1)
apps/judicial-system/backend/src/app/modules/case/case.service.ts (1)

Line range hint 1065-1094: Add error handling for subpoena fetching.

The implementation correctly handles the message queue for revoked indictment cases, but should include error handling for the subpoena fetch operation.

  private async addMessagesForRevokedIndictmentCaseToQueue(
    theCase: Case,
    user: TUser,
  ): Promise<void> {
    const messages = this.getRevokeNotificationMessages(user, theCase)

    if (theCase.courtCaseNumber) {
      messages.push({
        type: MessageType.DELIVERY_TO_COURT_INDICTMENT_CANCELLATION_NOTICE,
        user,
        caseId: theCase.id,
        body: { withCourtCaseNumber: true },
      })
    }

+   try {
      const subpoenasToRevoke = await this.subpoenaService.findByCaseId(
        theCase.id,
      )

      if (subpoenasToRevoke?.length > 0) {
        messages.push(
          ...subpoenasToRevoke.map((subpoena) => ({
            type: MessageType.DELIVERY_TO_POLICE_SUBPOENA_REVOCATION,
            user,
            caseId: theCase.id,
            elementId: [subpoena.defendantId, subpoena.id],
          })),
        )
      }
+   } catch (error) {
+     this.logger.error(
+       `Failed to fetch subpoenas for revoked case ${theCase.id}`,
+       error
+     )
+   }

    return this.messageService.sendMessagesToQueue(messages)
  }
🧹 Nitpick comments (2)
apps/judicial-system/backend/src/app/modules/subpoena/internalSubpoena.controller.ts (1)

128-161: Consider adding error handling for service failures.

The implementation looks good with proper guards, parameters, and API documentation. However, consider adding a try-catch block to handle potential service failures gracefully and provide appropriate error responses.

  @Post(
    `case/:caseId/${
      messageEndpoint[MessageType.DELIVERY_TO_POLICE_SUBPOENA_REVOCATION]
    }/:defendantId/:subpoenaId`,
  )
  @ApiOkResponse({
    type: DeliverResponse,
    description: 'Delivers subpoena revocation to police',
  })
  async deliverSubpoenaRevokedToPolice(
    @Param('caseId') caseId: string,
    @Param('defendantId') defendantId: string,
    @Param('subpoenaId') subpoenaId: string,
    @CurrentCase() theCase: Case,
    @CurrentSubpoena() subpoena: Subpoena,
    @Body() deliverDto: DeliverDto,
  ): Promise<DeliverResponse> {
+   try {
      this.logger.debug(
        `Delivering subpoena revocation of ${subpoenaId} to police for defendant ${defendantId} of case ${caseId}`,
      )

      return this.subpoenaService.deliverSubpoenaRevokedToPolice(
        theCase,
        subpoena,
        deliverDto.user,
      )
+   } catch (error) {
+     this.logger.error('Failed to deliver subpoena revocation to police', error)
+     throw new InternalServerErrorException('Failed to deliver subpoena revocation')
+   }
  }
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (1)

402-428: Enhance error logging for failed revocations.

The implementation correctly handles the revocation flow, but could benefit from more detailed error logging when the revocation fails.

  async deliverSubpoenaRevokedToPolice(
    theCase: Case,
    subpoena: Subpoena,
    user: TUser,
  ): Promise<DeliverResponse> {
    if (!subpoena.subpoenaId) {
      this.logger.warn(
        `Attempted to revoke a subpoena with id ${subpoena.id} that had not been delivered to the police`,
      )
      return { delivered: true }
    }

    const subpoenaRevoked = await this.policeService.revokeSubpoena(
      theCase,
      subpoena,
      user,
    )

    if (subpoenaRevoked) {
      this.logger.info(
        `Subpoena ${subpoena.subpoenaId} successfully revoked from police`,
      )
      return { delivered: true }
    } else {
+     this.logger.error(
+       `Failed to revoke subpoena ${subpoena.subpoenaId} for case ${theCase.id}`,
+       { caseId: theCase.id, subpoenaId: subpoena.id, userId: user.id }
+     )
      return { delivered: false }
    }
  }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 161892f and f09bc69.

📒 Files selected for processing (3)
  • apps/judicial-system/backend/src/app/modules/case/case.service.ts (2 hunks)
  • apps/judicial-system/backend/src/app/modules/subpoena/internalSubpoena.controller.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/judicial-system/backend/src/app/modules/subpoena/internalSubpoena.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/case/case.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/subpoena/subpoena.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."
📓 Learnings (2)
apps/judicial-system/backend/src/app/modules/subpoena/internalSubpoena.controller.ts (2)
Learnt from: unakb
PR: island-is/island.is#16393
File: apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts:164-169
Timestamp: 2024-11-12T15:15:11.835Z
Learning: Ensure that suggested code changes in `subpoena.response.ts` are accurate and necessary, avoiding redundant suggestions.
Learnt from: gudjong
PR: island-is/island.is#16863
File: apps/judicial-system/backend/src/app/modules/subpoena/subpoena.controller.ts:86-98
Timestamp: 2024-11-27T15:27:52.059Z
Learning: In `apps/judicial-system/backend/src/app/modules/subpoena/subpoena.controller.ts`, user action auditing and exception handling are performed at a higher level, so explicit error handling and logging within the `getSubpoena` method is unnecessary.
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (2)
Learnt from: unakb
PR: island-is/island.is#16393
File: apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts:164-169
Timestamp: 2024-11-12T15:15:11.835Z
Learning: Ensure that suggested code changes in `subpoena.response.ts` are accurate and necessary, avoiding redundant suggestions.
Learnt from: oddsson
PR: island-is/island.is#16329
File: apps/judicial-system/backend/src/app/modules/notification/subpoenaNotification.service.ts:0-0
Timestamp: 2024-11-12T15:15:26.274Z
Learning: In `apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts`, the method `findBySubpoenaId` throws an error if the subpoena is not found. Therefore, downstream code does not need to check if the result is undefined.
🔇 Additional comments (1)
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (1)

288-293: LGTM!

The implementation follows the established pattern for finder methods and correctly includes related models.

Copy link
Member

@thorhildurt thorhildurt left a comment

Choose a reason for hiding this comment

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

Looks good to me 👌 Just left some non-blocking questions ☺️ Tested the new endpoint as well, but I was unable to figure out why the subpoena was not delivered to the police already when revoking the subpoena, resulting in just logging the warning message for the if(!subpoena.subpoenaId)...condition in deliverSubpoenaRevokedToPolicefunction.

return true
}

throw await res.text()
Copy link
Member

Choose a reason for hiding this comment

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

What are the scenarios where this would be triggered? Wouldn't it always go to the catch block on error?


const subpoenaRevoked = await this.policeService.revokeSubpoena(
Copy link
Member

Choose a reason for hiding this comment

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

Just a question for my understanding: So here the only change is that we update the revoked subpoena's at RLS (via the post endpoint)? But we don't yet update the subpoena data or the case data that a subpoena has actually been revoked in our db? Or maybe that happens somewhere already?

Copy link
Member

Choose a reason for hiding this comment

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

Asking also because we could technically call deliverSubpoenaRevokedToPolice endpoint for any case id, right? Or do we ensure somewhere that this case is actually revoked or in the process of being revoked before we trigger deliverSubpoenaRevokedToPolice request ?

Copy link
Member

Choose a reason for hiding this comment

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

bit of a rubber ducking here hehe 🦆 buuut I see that we only trigger adding this event to the message queue if the following applies updatedCase.state === CaseState.WAITING_FOR_CANCELLATION, but I wonder if it makes sense to have an extra check in the controller to ensure that this logic only applies when updatedCase.state === CaseState.WAITING_FOR_CANCELLATION

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants