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

chore(j-s): Update subpoena status from digital-mailbox #16645

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { Controller, Get, Inject, Param, UseGuards } from '@nestjs/common'
import { ApiOkResponse, ApiTags } from '@nestjs/swagger'

import { type Logger, LOGGER_PROVIDER } from '@island.is/logging'

import { TokenGuard } from '@island.is/judicial-system/auth'

import { Case, CaseExistsGuard, CurrentCase } from '../case'
import { Subpoena } from '../subpoena'
import { PoliceService } from './police.service'

@UseGuards(TokenGuard, CaseExistsGuard)
@Controller('api/internal/case/:caseId')
@ApiTags('internal police')
export class InternalPoliceController {
constructor(
private readonly policeService: PoliceService,
@Inject(LOGGER_PROVIDER) private readonly logger: Logger,
) {}

@Get('subpoenaStatus/:subpoenaId')
@ApiOkResponse({
type: Subpoena,
description: 'Gets subpoena status',
})
Comment on lines +21 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add input validation for subpoenaId.

The endpoint should validate the subpoenaId parameter format before processing.

Consider adding a DTO with class-validator:

export class SubpoenaParamsDto {
  @IsString()
  @IsNotEmpty()
  @Matches(/^[a-zA-Z0-9-]+$/) // adjust regex based on expected format
  subpoenaId: string;
}

Then use it in the controller:

-  @Param('subpoenaId') subpoenaId: string,
+  @Param() params: SubpoenaParamsDto,

getSubpoenaStatus(
@Param('subpoenaId') subpoenaId: string,
@CurrentCase() theCase: Case,
): Promise<Subpoena> {
this.logger.debug(`Gets subpoena status in case ${theCase.id}`)

return this.policeService.getSubpoenaStatus(subpoenaId)
}
Comment on lines +21 to +33
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling and improve parameter usage.

Several improvements are needed in the implementation:

  1. The injected theCase parameter is not used in the service call
  2. Missing error handling for invalid subpoenaId or service failures
  3. The debug log could include more context (subpoenaId)

Consider applying these improvements:

 getSubpoenaStatus(
   @Param('subpoenaId') subpoenaId: string,
   @CurrentCase() theCase: Case,
 ): Promise<Subpoena> {
-  this.logger.debug(`Gets subpoena status in case ${theCase.id}`)
+  this.logger.debug(
+    `Getting subpoena status (id: ${subpoenaId}) for case ${theCase.id}`
+  )
 
-  return this.policeService.getSubpoenaStatus(subpoenaId)
+  try {
+    return this.policeService.getSubpoenaStatus(subpoenaId, theCase)
+  } catch (error) {
+    this.logger.error(
+      `Failed to get subpoena status: ${error.message}`,
+      error
+    )
+    throw error
+  }
 }

Committable suggestion was skipped due to low confidence.

}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { forwardRef, Module } from '@nestjs/common'

import { AwsS3Module, CaseModule, EventModule, SubpoenaModule } from '../index'
import { InternalPoliceController } from './internalPolice.controller'
import { PoliceController } from './police.controller'
import { PoliceService } from './police.service'

Expand All @@ -13,6 +14,6 @@ import { PoliceService } from './police.service'
],
providers: [PoliceService],
exports: [PoliceService],
controllers: [PoliceController],
controllers: [PoliceController, InternalPoliceController],
})
export class PoliceModule {}
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ export class PoliceService {
})
}

async getSubpoenaStatus(subpoenaId: string, user: User): Promise<Subpoena> {
async getSubpoenaStatus(subpoenaId: string, user?: User): Promise<Subpoena> {
return this.fetchPoliceDocumentApi(
`${this.xRoadPath}/GetSubpoenaStatus?id=${subpoenaId}`,
)
Expand Down Expand Up @@ -407,8 +407,8 @@ export class PoliceService {
'Failed to get subpoena',
{
subpoenaId,
actor: user.name,
institution: user.institution?.name,
actor: user?.name || 'Digital-mailbox',
institution: user?.institution?.name,
},
reason,
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export class CaseService {
lang?: string,
): Promise<CasesResponse[]> {
const response = await this.fetchCases(nationalId)

return CasesResponse.fromInternalCasesResponse(response, lang)
}

Expand All @@ -95,6 +96,16 @@ export class CaseService {
lang?: string,
): Promise<CaseResponse> {
const response = await this.fetchCase(id, nationalId)
const defendant = response.defendants[0]

if (!defendant) {
throw new NotFoundException('No defendant found for this case')
}

if (defendant.subpoenas && defendant.subpoenas[0].subpoenaId) {
await this.fetchServiceStatus(id, defendant.subpoenas[0].subpoenaId)
}

Comment on lines +99 to +108
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve subpoena status handling and code safety.

While the null check for defendant is good, there are two improvements needed:

  1. The fetched subpoena status is not being used
  2. The conditional check can be simplified using optional chaining

Apply this diff:

    const defendant = response.defendants[0]

    if (!defendant) {
      throw new NotFoundException('No defendant found for this case')
    }

-   if (defendant.subpoenas && defendant.subpoenas[0].subpoenaId) {
-     await this.fetchServiceStatus(id, defendant.subpoenas[0].subpoenaId)
+   if (defendant.subpoenas?.[0]?.subpoenaId) {
+     const statusResponse = await this.fetchServiceStatus(
+       id,
+       defendant.subpoenas[0].subpoenaId
+     )
+     // Update the case response with the fetched status
+     Object.assign(response, statusResponse)
    }

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 Biome

[error] 105-105: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


💡 Codebase verification

Missing test coverage for subpoena status functionality

The implementation of subpoena status checking appears incomplete:

  • The fetchServiceStatus method is implemented but lacks test coverage (no .spec.ts file found)
  • While other services (police.controller, police.resolver) handle subpoena status, this new implementation in case.service.ts needs proper testing
  • The method is marked as private, suggesting it's an internal implementation detail, but its behavior should still be verified through public method tests
🔗 Analysis chain

Verify the subpoena status update mechanism.

Let's verify that the subpoena status update mechanism works as expected with LRH.


Let me gather more information about the subpoena status handling implementation.


Let me check one more aspect to understand the complete flow of subpoena status handling.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other places in the codebase that might need to handle subpoena status updates

# Test 1: Check for other methods that might need to handle subpoena status
echo "Checking for other methods that might need subpoena status handling..."
rg -l "subpoena.*status" --type ts

# Test 2: Check for test coverage of the new functionality
echo "Checking for test coverage..."
fd -e spec.ts -e test.ts case.service

Length of output: 566


Script:

#!/bin/bash
# Check the actual implementation of subpoena status handling in the found files
echo "Checking subpoena status implementations..."
rg "subpoena.*status" -B 2 -A 2 --type ts

# Check for the fetchServiceStatus method implementation
echo "Checking fetchServiceStatus implementation..."
ast-grep --pattern 'fetchServiceStatus($$$) {
  $$$
}'

Length of output: 3760


Script:

#!/bin/bash
# Check for the case service test file and its content
echo "Checking case service test implementation..."
rg "fetchServiceStatus" -B 2 -A 2 apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.service.spec.ts

# Check for the actual implementation of fetchServiceStatus in the service
echo "Checking fetchServiceStatus in case service..."
rg "fetchServiceStatus" -B 2 -A 2 apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.service.ts

Length of output: 795

🧰 Tools
🪛 Biome

[error] 105-105: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

return CaseResponse.fromInternalCaseResponse(response, lang)
}

Expand Down Expand Up @@ -230,6 +241,52 @@ export class CaseService {
}
}

private async fetchServiceStatus(
caseId: string,
subpoenaId: string,
): Promise<InternalCaseResponse> {
try {
const res = await fetch(
`${this.config.backendUrl}/api/internal/case/${caseId}/subpoenaStatus/${subpoenaId}`,
{
method: 'GET',
headers: {
'Content-Type': 'application/json',
authorization: `Bearer ${this.config.secretToken}`,
},
},
)

if (!res.ok) {
if (res.status === 404) {
throw new NotFoundException(`Case ${caseId} not found`)
}

const reason = await res.text()

throw new BadGatewayException(
reason ||
'Unexpected error occurred while fetching serviceStatus by subpoenaID',
)
}

const caseData = await res.json()

return caseData
} catch (reason) {
if (
reason instanceof BadGatewayException ||
reason instanceof NotFoundException
) {
throw reason
}

throw new BadGatewayException(
`Failed to fetch serviceStatus by subpoenaId: ${reason.message}`,
)
}
}
Comment on lines +244 to +288
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Enhance error handling and response validation in fetchServiceStatus.

Several improvements can be made to enhance the robustness of this method:

  1. Use URL class for safer URL construction
  2. Add response type validation
  3. Make error messages more specific
  4. Improve error handling

Apply this diff:

  private async fetchServiceStatus(
    caseId: string,
    subpoenaId: string,
  ): Promise<InternalCaseResponse> {
    try {
+     const url = new URL(
+       `/api/internal/case/${caseId}/subpoenaStatus/${subpoenaId}`,
+       this.config.backendUrl
+     ).toString()
+
      const res = await fetch(
-       `${this.config.backendUrl}/api/internal/case/${caseId}/subpoenaStatus/${subpoenaId}`,
+       url,
        {
          method: 'GET',
          headers: {
            'Content-Type': 'application/json',
            authorization: `Bearer ${this.config.secretToken}`,
          },
        },
      )

      if (!res.ok) {
        if (res.status === 404) {
-         throw new NotFoundException(`Case ${caseId} not found`)
+         throw new NotFoundException(
+           `Case ${caseId} or subpoena ${subpoenaId} not found`
+         )
        }

        const reason = await res.text()

        throw new BadGatewayException(
          reason ||
-           'Unexpected error occurred while fetching serviceStatus by subpoenaID',
+           `Failed to fetch service status for case ${caseId} and subpoena ${subpoenaId}`,
        )
      }

      const caseData = await res.json()
+     
+     // Validate response type
+     if (!caseData || typeof caseData !== 'object') {
+       throw new BadGatewayException('Invalid response format from service status endpoint')
+     }

      return caseData
    } catch (reason) {
      if (
        reason instanceof BadGatewayException ||
        reason instanceof NotFoundException
      ) {
        throw reason
      }

      throw new BadGatewayException(
-       `Failed to fetch serviceStatus by subpoenaId: ${reason.message}`,
+       `Failed to fetch service status for case ${caseId} and subpoena ${subpoenaId}: ${reason.message}`,
      )
    }
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private async fetchServiceStatus(
caseId: string,
subpoenaId: string,
): Promise<InternalCaseResponse> {
try {
const res = await fetch(
`${this.config.backendUrl}/api/internal/case/${caseId}/subpoenaStatus/${subpoenaId}`,
{
method: 'GET',
headers: {
'Content-Type': 'application/json',
authorization: `Bearer ${this.config.secretToken}`,
},
},
)
if (!res.ok) {
if (res.status === 404) {
throw new NotFoundException(`Case ${caseId} not found`)
}
const reason = await res.text()
throw new BadGatewayException(
reason ||
'Unexpected error occurred while fetching serviceStatus by subpoenaID',
)
}
const caseData = await res.json()
return caseData
} catch (reason) {
if (
reason instanceof BadGatewayException ||
reason instanceof NotFoundException
) {
throw reason
}
throw new BadGatewayException(
`Failed to fetch serviceStatus by subpoenaId: ${reason.message}`,
)
}
}
private async fetchServiceStatus(
caseId: string,
subpoenaId: string,
): Promise<InternalCaseResponse> {
try {
const url = new URL(
`/api/internal/case/${caseId}/subpoenaStatus/${subpoenaId}`,
this.config.backendUrl
).toString()
const res = await fetch(
url,
{
method: 'GET',
headers: {
'Content-Type': 'application/json',
authorization: `Bearer ${this.config.secretToken}`,
},
},
)
if (!res.ok) {
if (res.status === 404) {
throw new NotFoundException(
`Case ${caseId} or subpoena ${subpoenaId} not found`
)
}
const reason = await res.text()
throw new BadGatewayException(
reason ||
`Failed to fetch service status for case ${caseId} and subpoena ${subpoenaId}`,
)
}
const caseData = await res.json()
// Validate response type
if (!caseData || typeof caseData !== 'object') {
throw new BadGatewayException('Invalid response format from service status endpoint')
}
return caseData
} catch (reason) {
if (
reason instanceof BadGatewayException ||
reason instanceof NotFoundException
) {
throw reason
}
throw new BadGatewayException(
`Failed to fetch service status for case ${caseId} and subpoena ${subpoenaId}: ${reason.message}`,
)
}
}


private async patchDefenseInfo(
defendantNationalId: string,
caseId: string,
Expand Down
Loading