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

refactor(j-s): Transition Side Effects #16660

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

Conversation

gudjong
Copy link
Member

@gudjong gudjong commented Oct 30, 2024

Transition Side Effects

Rannsaka villur í R-málum

What

  • Moves transition side effects from controllers to the case state machine.

Why

  • Cleaner code.

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

    • Streamlined case transition handling for improved clarity and maintainability.
    • Enhanced error handling for court record and ruling signature requests.
    • Simplified transition logic in controllers for better usability.
  • Bug Fixes

    • Improved handling of invalid state transitions with structured error responses.
  • Tests

    • Expanded test coverage for case transitions, including appeal scenarios and notification checks.
    • Enhanced user role context in tests for more accurate behavior validation.

Copy link
Contributor

coderabbitai bot commented Oct 30, 2024

Walkthrough

The pull request introduces significant modifications across multiple files, primarily focusing on the CaseController, CaseService, and related state management logic. Key changes include the removal of extensive case transition logic, consolidation of parameters in method signatures, and enhancements to error handling. The update process is streamlined, with a focus on improved clarity and maintainability. Additionally, test cases are refined to better reflect the new transition logic. Overall, the changes aim to simplify the case management system while preserving core functionalities.

Changes

File Change Summary
apps/judicial-system/backend/src/app/modules/case/case.controller.ts Removed specific case transition logic; consolidated parameters in transition and update methods; enhanced error handling for signature requests.
apps/judicial-system/backend/src/app/modules/case/case.service.ts Refactored update method to improve clarity in state transitions; retained handleDateUpdates and handleCommentUpdates within transaction blocks; refined error handling.
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.controller.ts Simplified transition method by removing appeal-related checks; updated imports and method parameters.
apps/judicial-system/backend/src/app/modules/case/state/case.state.spec.ts Restructured test parameters for transitionCase; modified assertions for better flexibility; enhanced coverage for state transitions and error handling.
apps/judicial-system/backend/src/app/modules/case/state/case.state.ts Updated transition logic with new types and functions; improved error handling for state transitions; introduced new side effect management for request cases.
apps/judicial-system/backend/src/app/modules/case/test/caseController/transition.spec.ts Expanded defaultUser context; refined case transition conditions; organized tests for various transition scenarios and notifications.
apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/transition.spec.ts Enhanced user context in tests; structured tests for appeal and withdrawal scenarios; validated case state updates and notification messages.

Possibly related PRs

  • feat(j-s): Prosecutor Asks for Cancellation #15105: The main PR modifies the CaseController class to streamline case transition handling, which relates to the addition of a new case transition ASK_FOR_CANCELLATION in the same class in this PR.
  • feat(j-s): Complete Cancellation #15154: This PR also involves changes to the case.state.ts file, specifically allowing transitions from WAITING_FOR_CANCELLATION to COMPLETED, which connects to the overall case transition logic being refined in the main PR.
  • fix(j-s): Withdraw Indictment Case #16525: The main PR introduces a new ruling decision type WITHDRAWAL, which is relevant to the changes made in this PR that enhance the handling of indictment cases when they are withdrawn.
  • fix(j-s): State Machine Corruption #16595: The modifications in the main PR to prevent state machine corruption align with the changes in this PR that improve the immutability of state objects in the case.state.ts file.

Suggested reviewers

  • gudjong
  • unakb

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.

@datadog-island-is
Copy link

datadog-island-is bot commented Oct 30, 2024

Datadog Report

All test runs 3f4bbd2 🔗

22 Total Test Services: 0 Failed, 21 Passed
🔻 Test Sessions change in coverage: 1 decreased (-0.1%), 1 increased (+0.03%), 90 no change

Test Services
This report shows up to 10 services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Code Coverage Change Test Service View
air-discount-scheme-web 0 0 0 2 0 8.79s 1 no change Link
api 0 0 0 4 0 2.55s 1 no change Link
application-api-files 0 0 0 12 0 5.57s 1 no change Link
application-core 0 0 0 97 0 14.94s 1 decreased (-0.1%) Link
application-system-api 0 0 0 120 2 3m 26.92s 1 no change Link
application-template-api-modules 0 0 0 123 0 2m 2.4s 1 no change Link
application-templates-accident-notification 0 0 0 148 0 16.02s 1 no change Link
application-templates-criminal-record 0 0 0 2 0 9.88s 1 no change Link
application-templates-driving-license 0 0 0 13 0 13.16s 1 no change Link
application-templates-example-payment 0 0 0 2 0 10.55s 1 no change Link

🔻 Code Coverage Decreases vs Default Branch (1)

  • application-core - jest 79.62% (-0.1%) - Details

Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 77.41935% with 21 lines in your changes missing coverage. Please review.

Project coverage is 36.66%. Comparing base (dc89848) to head (41c2232).

Files with missing lines Patch % Lines
...m/backend/src/app/modules/case/state/case.state.ts 76.66% 21 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16660      +/-   ##
==========================================
+ Coverage   36.63%   36.66%   +0.02%     
==========================================
  Files        6873     6870       -3     
  Lines      143137   142912     -225     
  Branches    40806    40721      -85     
==========================================
- Hits        52445    52405      -40     
+ Misses      90692    90507     -185     
Flag Coverage Δ
air-discount-scheme-web 0.00% <ø> (ø)
api 3.37% <ø> (ø)
application-api-files 56.78% <ø> (ø)
application-core 71.61% <ø> (-0.25%) ⬇️
application-system-api 41.33% <ø> (ø)
application-template-api-modules 27.83% <ø> (ø)
application-templates-accident-notification 29.21% <ø> (ø)
application-templates-car-recycling 3.12% <ø> (ø)
application-templates-criminal-record 26.26% <ø> (ø)
application-templates-driving-license 18.40% <ø> (ø)
application-templates-estate 12.19% <ø> (ø)
application-templates-example-payment 25.17% <ø> (ø)
application-templates-financial-aid 15.56% <ø> (ø)
application-templates-general-petition 23.40% <ø> (ø)
application-templates-inheritance-report 6.52% <ø> (ø)
application-templates-marriage-conditions 15.20% <ø> (ø)
application-templates-mortgage-certificate 43.52% <ø> (ø)
application-templates-parental-leave 29.93% <ø> (ø)
application-types 6.62% <ø> (ø)
application-ui-components 1.27% <ø> (ø)
application-ui-shell 20.87% <ø> (ø)
clients-charge-fjs-v2 24.11% <ø> (ø)
judicial-system-backend 55.27% <77.41%> (-0.02%) ⬇️
web 1.80% <ø> (ø)

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

Files with missing lines Coverage Δ
...em/backend/src/app/modules/case/case.controller.ts 86.95% <100.00%> (+2.53%) ⬆️
...ystem/backend/src/app/modules/case/case.service.ts 90.40% <100.00%> (ø)
...c/app/modules/case/limitedAccessCase.controller.ts 97.47% <100.00%> (-0.15%) ⬇️
...m/backend/src/app/modules/case/state/case.state.ts 80.86% <76.66%> (-16.19%) ⬇️

... and 29 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 dc89848...41c2232. Read the comment docs.

@gudjong gudjong marked this pull request as ready for review November 1, 2024 07:49
@gudjong gudjong requested a review from a team as a code owner November 1, 2024 07:49
coderabbitai[bot]
coderabbitai bot previously requested changes Nov 1, 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: 4

🧹 Outside diff range and nitpick comments (6)
apps/judicial-system/backend/src/app/modules/case/test/caseController/transition.spec.ts (1)

364-366: Consider improving readability of the condition.

The parentCaseId condition could be more readable by extracting the complex condition into a descriptive variable.

-                isRequestCase(type) && transition === CaseTransition.DELETE
-                  ? null
-                  : undefined,
+                const shouldClearParentCase = isRequestCase(type) && transition === CaseTransition.DELETE;
+                shouldClearParentCase ? null : undefined,
apps/judicial-system/backend/src/app/modules/case/state/case.state.spec.ts (3)

Line range hint 39-39: Enhance error assertions with specific error messages.

The error assertions could be more specific by checking the error message content to ensure the correct error is being thrown for the right reason.

Update the error assertions to include message checks:

-expect(act).toThrow(ForbiddenException)
+expect(act).toThrow(new ForbiddenException(
+  `Cannot transition case from ${fromState} to ${expectedState}`
+))

Also applies to: 92-92, 171-171, 247-247, 323-323, 378-378, 409-409, 457-457, 485-485, 533-533, 588-588, 619-619, 667-667, 695-695, 746-746, 774-774, 799-799, 864-864, 889-889, 954-954, 979-979, 1044-1044, 1095-1095, 1155-1155, 1186-1186, 1211-1211, 1280-1280, 1305-1305, 1365-1365, 1396-1396, 1421-1421, 1488-1488, 1518-1518, 1543-1543, 1605-1605, 1635-1635, 1660-1660, 1727-1727, 1757-1757, 1782-1782, 1844-1844, 1874-1874


Line range hint 22-22: Standardize test descriptions for better readability.

The test descriptions could follow a more consistent format that includes the initial state, action, and expected outcome.

Update the test descriptions to follow this format:

-describe.each(indictmentCases)('open %s', (type) => {
+describe.each(indictmentCases)('when opening a %s case', (type) => {
-'state %s - should open'
+'given initial state %s, should transition to OPEN state'

Also applies to: 43-43, 177-177, 253-253, 329-329, 384-384, 415-415, 463-463, 491-491, 539-539, 594-594, 625-625, 673-673, 701-701, 752-752, 780-780, 805-805, 870-870, 895-895, 960-960, 985-985, 1050-1050, 1101-1101, 1161-1161, 1192-1192, 1217-1217, 1286-1286, 1311-1311, 1371-1371, 1402-1402, 1427-1427, 1494-1494, 1524-1524, 1549-1549, 1611-1611, 1641-1641, 1666-1666, 1733-1733, 1763-1763, 1788-1788, 1850-1850, 1880-1880


Line range hint 1-1880: Add edge case tests for improved coverage.

While the test coverage is comprehensive for happy paths and basic error cases, consider adding tests for the following scenarios:

  • Concurrent state transitions
  • Invalid user roles
  • Missing or malformed case/user objects

Example test cases to add:

it('should handle concurrent transitions correctly', async () => {
  const theCase = createTestCase({ state: CaseState.NEW });
  const user = createTestUser();
  
  await Promise.all([
    transitionCase(CaseTransition.OPEN, theCase, user),
    transitionCase(CaseTransition.OPEN, theCase, user)
  ]);
});

it('should reject transitions with invalid user roles', () => {
  const theCase = createTestCase({ state: CaseState.NEW });
  const user = createTestUser({ role: undefined });
  
  expect(() => 
    transitionCase(CaseTransition.OPEN, theCase, user)
  ).toThrow(ForbiddenException);
});
apps/judicial-system/backend/src/app/modules/case/state/case.state.ts (2)

175-175: Address the TODO: Define behavior for correcting cases

There is a TODO comment indicating that a decision needs to be made on how to handle correcting cases during the transition. Please specify the intended behavior or implement the necessary logic to address this scenario.

Would you like assistance in defining and implementing the behavior for correcting cases in this transition? I can help develop the required logic or open a GitHub issue to track this task.


393-393: Address the TODO: Define behavior for correcting appeals

A TODO comment indicates uncertainty on handling corrections during appeals. Please clarify the intended functionality or provide the implementation to handle correcting appeals appropriately.

Do you need assistance in designing the logic for correcting appeals in this context? I can help provide an implementation or open a GitHub issue to ensure this is addressed.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 1beceb8 and 00a9cb7.

📒 Files selected for processing (6)
  • apps/judicial-system/backend/src/app/modules/case/case.controller.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/case/case.service.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.controller.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/case/state/case.state.spec.ts (67 hunks)
  • apps/judicial-system/backend/src/app/modules/case/state/case.state.ts (16 hunks)
  • apps/judicial-system/backend/src/app/modules/case/test/caseController/transition.spec.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/judicial-system/backend/src/app/modules/case/case.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/case/limitedAccessCase.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/state/case.state.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/case/state/case.state.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/test/caseController/transition.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."
🔇 Additional comments (4)
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.controller.ts (1)

171-172: LGTM! Verify state machine transition handling

The simplification of the transition logic by moving it to the state machine aligns well with the PR objectives and improves code maintainability. This change follows the Single Responsibility Principle by keeping transition logic in one place.

Let's verify that all transition cases are properly handled in the state machine:

✅ Verification successful

Based on the examination of the codebase, I can now provide a final response:

Transition logic properly handled in state machine

The case state transitions are well-managed through dedicated state machines:

  • indictmentCaseStateMachine handles indictment case transitions with proper state validation
  • requestCaseStateMachine manages request case transitions with state and appeal state validation
  • Each transition is guarded by validation rules that check valid state combinations
  • The implementation throws appropriate exceptions for invalid transitions

The changes in the controller align with good practices by delegating transition logic to the state machine, which maintains proper state management and validation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the transitionCase implementation and its test coverage
echo "Searching for transitionCase implementation:"
ast-grep --pattern 'function transitionCase($_) {
  $$$
}'

echo -e "\nSearching for transition test cases:"
rg -l "describe.*transition" --type ts

Length of output: 1586


Script:

#!/bin/bash
# Let's search for transitionCase implementation with a broader pattern
echo "Searching for transitionCase implementation in case-related files:"
rg "transitionCase" -A 5 --type ts

echo -e "\nSearching for state machine transition logic:"
rg "transition.*case" -A 5 --type ts

echo -e "\nSearching for case state machine files:"
fd "case.*state.*machine" --type f

Length of output: 117202


Script:

#!/bin/bash
# Let's examine the case state machine implementation
echo "Searching for case state machine implementation:"
rg -A 10 "const.*CaseStateMachine" --type ts

echo -e "\nSearching for case state transitions:"
ast-grep --pattern 'const $_ = {
  fromStates: $_,
  transition: $_
}'

Length of output: 4848

apps/judicial-system/backend/src/app/modules/case/test/caseController/transition.spec.ts (1)

54-59: LGTM! Enhanced user context for tests.

The addition of role and institution properties to defaultUser provides a more realistic test context, properly reflecting the relationship between PROSECUTOR role and PROSECUTORS_OFFICE institution type.

apps/judicial-system/backend/src/app/modules/case/case.controller.ts (1)

305-309: Refactored transition logic enhances maintainability

Consolidating the parameters in the transitionCase function call simplifies the code and improves readability. This change aligns with best practices for clean and maintainable code.

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

1692-1692: Ensure 'update' reassignment does not introduce unintended side effects

Reassigning update with the result of transitionCase may alter the properties of the update object. Please verify that the updated update object includes all necessary fields required by subsequent methods such as handleDateUpdates, handleCommentUpdates, and handleEventLogs, ensuring no essential data is lost or overwritten during the reassignment.

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)
apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/transition.spec.ts (1)

Line range hint 95-190: Consider adding error case scenarios

While the happy path scenarios for appeals and withdrawals are well covered, consider adding test cases for potential error scenarios such as:

  • Invalid state transitions
  • Missing required fields
  • Unauthorized role attempts

Example test structure:

describe('invalid transitions', () => {
  it('should reject transition when case is in invalid state', async () => {
    const then = await givenWhenThen(
      CaseState.DRAFT,  // Invalid initial state
      CaseTransition.APPEAL
    )
    expect(then.error).toBeDefined()
  })
})
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 00a9cb7 and 5bac552.

📒 Files selected for processing (1)
  • apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/transition.spec.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/transition.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/case/test/limitedAccessCaseController/transition.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCaseFilesRecordPdfRolesRules.spec.ts:1-1
Timestamp: 2024-10-17T11:30:12.482Z
Learning: The usage of `defenderRule` in the following test files is intentional and should remain unchanged:

- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createPresignedPostRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/deleteCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/getCaseFileSignedUrlRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRulingPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRequestPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCourtRecordPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getByIdRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getAllFilesZipRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/caseController/getAllRolesRules.spec.ts`
🔇 Additional comments (2)
apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/transition.spec.ts (2)

15-15: LGTM: Import addition is well-organized

The addition of UserRole to the imports is properly grouped with other types from the judicial system module.


39-39: LGTM: User role definition enhances test context

The addition of role: UserRole.DEFENDER to the user object improves test clarity by explicitly defining the user's role, which is consistent with the role-based access patterns used across the codebase.

coderabbitai[bot]
coderabbitai bot previously requested changes Nov 1, 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: 2

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

175-175: Address TODO comment regarding case correction

The TODO comment indicates an unresolved decision about case correction handling. This should be addressed before the code is merged.

Would you like me to help create a GitHub issue to track this decision and implementation?

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 5bac552 and f28af3e.

📒 Files selected for processing (1)
  • apps/judicial-system/backend/src/app/modules/case/state/case.state.ts (16 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/case/state/case.state.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."
🔇 Additional comments (1)
apps/judicial-system/backend/src/app/modules/case/state/case.state.ts (1)

30-36: LGTM: Well-structured type definitions

The Actor and Transition type definitions are clear, type-safe, and align well with the domain model.

@oddsson oddsson added the automerge Merge this PR as soon as all checks pass label Nov 1, 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: 2

🧹 Outside diff range and nitpick comments (4)
apps/judicial-system/backend/src/app/modules/case/state/case.state.ts (1)

175-179: Address pending architectural decisions in appeal handling

There are two important TODOs that need to be addressed:

  1. Decision on handling correcting cases
  2. Decision on setting both appeal dates when both parties appeal

Would you like me to help create GitHub issues to track these architectural decisions? This would help ensure these important decisions are properly documented and tracked.

apps/judicial-system/backend/src/app/modules/case/case.controller.ts (3)

Line range hint 305-315: Consider handling case deletion more explicitly

While the transition logic has been successfully moved to the state machine, the null coalescing operator (??) usage in the event posting might mask potential issues. Consider making the deletion case more explicit:

-    const updatedCase = await this.caseService.update(
-      theCase,
-      update,
-      user,
-      update.state !== CaseState.DELETED,
-    )
-
-    // No need to wait
-    this.eventService.postEvent(transition.transition, updatedCase ?? theCase)
-
-    return updatedCase ?? theCase
+    const updatedCase = await this.caseService.update(
+      theCase,
+      update,
+      user,
+      update.state !== CaseState.DELETED,
+    )
+
+    const finalCase = update.state === CaseState.DELETED ? theCase : updatedCase
+
+    // No need to wait
+    this.eventService.postEvent(transition.transition, finalCase)
+
+    return finalCase

Line range hint 305-315: Extract common error handling logic

The error handling for signature requests is duplicated. Consider extracting it into a utility function:

private handleSignatureError(error: unknown, caseId: string, documentType: 'court record' | 'ruling'): never {
  if (error instanceof DokobitError) {
    throw new HttpException(
      {
        statusCode: error.status,
        message: `Failed to request a ${documentType} signature for case ${caseId}`,
        code: error.code,
        error: error.message,
      },
      error.status,
    )
  }
  throw error;
}

Then use it in both methods:

.catch((error) => this.handleSignatureError(error, caseId, 'court record'))

Line range hint 1-24: Consider further separation of concerns

While the transition side effects have been successfully moved to the state machine, there are still opportunities to improve the separation of concerns:

  1. Move user validation logic to the UserService
  2. Extract history text formatting to a dedicated service
  3. Move case resend logic to the CaseService

This would make the controller more focused on its primary responsibility of handling HTTP requests and responses.

Example of how the user validation could be moved:

// user.service.ts
async validateAssignedUser(userId: string, allowedRoles: UserRole[], institutionId?: string): Promise<void> {
  const user = await this.findById(userId);
  if (!allowedRoles.includes(user.role)) {
    throw new ForbiddenException(`User ${userId} does not have an acceptable role ${allowedRoles}`);
  }
  if (institutionId && user.institutionId !== institutionId) {
    throw new ForbiddenException(`User ${userId} belongs to the wrong institution`);
  }
}

// case.controller.ts
if (update.prosecutorId) {
  await this.userService.validateAssignedUser(
    update.prosecutorId,
    [UserRole.PROSECUTOR],
    theCase.prosecutorsOfficeId,
  );
}

Also applies to: 305-315

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between f28af3e and cb656cd.

📒 Files selected for processing (3)
  • apps/judicial-system/backend/src/app/modules/case/case.controller.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.controller.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/case/state/case.state.ts (16 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.controller.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/backend/src/app/modules/case/case.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/state/case.state.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."
🔇 Additional comments (1)
apps/judicial-system/backend/src/app/modules/case/state/case.state.ts (1)

30-36: Well-structured type definitions for actor-based transitions!

The introduction of Actor and Transition types provides clear type safety and makes the role-based transition logic explicit.

@@ -325,25 +526,33 @@ const transitionRequestCase = (
)
}

// Since the state machine is a global constant, we spread the result before returning it to avoid accidental mutations
return { ...rule.to } as CaseStates
return rule.transition(update, theCase, actor) as UpdateCase
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

Consider improving type safety in transition functions

The type assertion as UpdateCase could be avoided by properly typing the state machine's transition results. Additionally, the case type checks could be made type-safe using type predicates.

- return rule.transition(update, theCase, actor) as UpdateCase
+ return rule.transition(update, theCase, actor)

- if (isIndictmentCase(theCase.type)) {
+ if (isIndictmentCase(theCase.type) && isIndictmentCaseState(theCase.state)) {
    return transitionIndictmentCase(transition, theCase, update, actor)
  }

- if (isRequestCase(theCase.type)) {
+ if (isRequestCase(theCase.type) && isRequestCaseState(theCase.state)) {
    return transitionRequestCase(transition, theCase, update, actor)
  }

Also applies to: 547-552

Comment on lines +387 to +417
if (
isRestrictionCase(theCase.type) &&
currentState === CaseState.ACCEPTED &&
(currentDecision === CaseDecision.ACCEPTING ||
currentDecision === CaseDecision.ACCEPTING_PARTIALLY)
) {
// TODO: Decide what to do if correcting appeal
const currentAppealRulingDecision =
newUpdate.appealRulingDecision ?? theCase.appealRulingDecision

if (
currentAppealRulingDecision ===
CaseAppealRulingDecision.CHANGED ||
currentAppealRulingDecision ===
CaseAppealRulingDecision.CHANGED_SIGNIFICANTLY
) {
// The court of appeals has modified the ruling of a restriction case
newUpdate.validToDate =
update.appealValidToDate ?? theCase.appealValidToDate
newUpdate.isCustodyIsolation =
update.isAppealCustodyIsolation ??
theCase.isAppealCustodyIsolation
newUpdate.isolationToDate =
update.appealIsolationToDate ?? theCase.appealIsolationToDate
} else if (
currentAppealRulingDecision === CaseAppealRulingDecision.REPEAL
) {
// The court of appeals has repealed the ruling of a restriction case
newUpdate.validToDate = nowFactory()
}
}
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

Consider extracting restriction case appeal handling logic

The appeal ruling logic for restriction cases is complex and handles multiple concerns. Consider extracting this into a separate function for better maintainability.

+ const handleRestrictionCaseAppealRuling = (
+   update: UpdateCase,
+   theCase: Case,
+   currentAppealRulingDecision: CaseAppealRulingDecision
+ ): UpdateCase => {
+   if (
+     currentAppealRulingDecision === CaseAppealRulingDecision.CHANGED ||
+     currentAppealRulingDecision === CaseAppealRulingDecision.CHANGED_SIGNIFICANTLY
+   ) {
+     return {
+       ...update,
+       validToDate: update.appealValidToDate ?? theCase.appealValidToDate,
+       isCustodyIsolation: update.isAppealCustodyIsolation ?? theCase.isAppealCustodyIsolation,
+       isolationToDate: update.appealIsolationToDate ?? theCase.appealIsolationToDate,
+     }
+   }
+   
+   if (currentAppealRulingDecision === CaseAppealRulingDecision.REPEAL) {
+     return {
+       ...update,
+       validToDate: nowFactory()
+     }
+   }
+   
+   return update
+ }

  if (
    isRestrictionCase(theCase.type) &&
    currentState === CaseState.ACCEPTED &&
    (currentDecision === CaseDecision.ACCEPTING ||
      currentDecision === CaseDecision.ACCEPTING_PARTIALLY)
  ) {
    const currentAppealRulingDecision =
      newUpdate.appealRulingDecision ?? theCase.appealRulingDecision

-   if (
-     currentAppealRulingDecision === CaseAppealRulingDecision.CHANGED ||
-     currentAppealRulingDecision === CaseAppealRulingDecision.CHANGED_SIGNIFICANTLY
-   ) {
-     newUpdate.validToDate = update.appealValidToDate ?? theCase.appealValidToDate
-     newUpdate.isCustodyIsolation = update.isAppealCustodyIsolation ?? theCase.isAppealCustodyIsolation
-     newUpdate.isolationToDate = update.appealIsolationToDate ?? theCase.appealIsolationToDate
-   } else if (currentAppealRulingDecision === CaseAppealRulingDecision.REPEAL) {
-     newUpdate.validToDate = nowFactory()
-   }
+   return handleRestrictionCaseAppealRuling(newUpdate, theCase, currentAppealRulingDecision)
  }

Committable suggestion skipped: line range outside the PR's diff.

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.

2 participants