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): Enable public prosecutors to properly handle fines #17023

Merged
merged 51 commits into from
Nov 26, 2024

Conversation

oddsson
Copy link
Member

@oddsson oddsson commented Nov 26, 2024

Enable public prosecutors to properly handle fines

Asana

What

Enable public prosecutors to properly handle fines. In cases where there is a fine the following applies

  • There is no service done in these cases and no áfrýjun
  • The defendant can appeal the fine, but only has three days to do so from the ruling date, instead of 28.

This PR also gives prison admin users access to cases where there is a fine.

Why

So that the public prosecutor can properly handle all S-cases

Screenshots / Gifs

Screenshot 2024-11-26 at 12 53 57

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 a new BlueBoxWithDate component for better date display in the overview.
    • Added support for handling fine-related appeal deadlines across various components.
    • Enhanced message definitions to support dynamic rendering based on ruling decisions.
    • Added a new message for "Kært" in reviewed cases.
    • Updated the reviewerSubtitle to dynamically reflect appeal status and deadlines.
  • Bug Fixes

    • Improved access control for prison admin users based on ruling decisions.
  • Documentation

    • Updated message IDs and default messages for clarity and consistency.
  • Style

    • Adjusted CSS grid layout for improved responsiveness in the Review Decision component.
  • Tests

    • Added new test cases to validate the handling of fine ruling decisions and appeal deadlines.

oddsson and others added 30 commits November 5, 2024 15:25
…d-is/island.is into j-s/service-announcement-defenders
@datadog-island-is
Copy link

datadog-island-is bot commented Nov 26, 2024

Datadog Report

All test runs 51f3e11 🔗

9 Total Test Services: 0 Failed, 9 Passed
⬆️ Test Sessions change in coverage: 5 increased, 6 no change

Test Services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Code Coverage Change Test Service View
judicial-system-api 0 0 0 61 0 6.99s 1 increased (+0.14%) Link
judicial-system-backend 0 0 0 21070 0 0s N/A Link
judicial-system-formatters 0 0 0 38 0 5.65s 1 increased (+0.02%) Link
judicial-system-message 0 0 0 36 0 11.7s 1 no change Link
judicial-system-message-handler 0 0 0 4 0 3.92s 1 no change Link
judicial-system-scheduler 0 0 0 4 0 4.38s 1 increased (+0.05%) Link
judicial-system-types 0 0 0 23 0 8s 1 no change Link
judicial-system-web 0 0 0 335 0 1m 12.41s 1 increased (+0.51%) Link
judicial-system-xrd-api 0 0 0 6 0 5.46s 1 increased (+0.03%) Link

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.

Minor comments

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 (2)
apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesReviewed.tsx (1)

35-47: Add JSDoc comments to document the mapping function.

The function handles important business logic for review decisions. Consider adding JSDoc comments to document:

  • The purpose of the function
  • Parameter descriptions
  • Return value possibilities (including the null case)
  • The business rules behind each decision mapping

Example:

/**
 * Maps indictment review decisions to localized messages based on the decision type and fine status.
 * @param reviewDecision - The review decision to map
 * @param isFine - Whether the case involves a fine
 * @returns Formatted message or null for unhandled cases
 */
apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx (1)

262-263: Refactor complex conditional expressions for better readability.

The conditional expression starting at line 262 is quite complex:

defendant.isVerdictAppealDeadlineExpired || isFine ? null : !serviceRequired || defendant.verdictViewDate ? ( ... )

Consider simplifying this condition by:

  • Breaking it into multiple smaller conditions with descriptive variable names.
  • Using nested conditionals or early returns to reduce complexity.

This will improve readability and make future maintenance easier.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between f619af5 and c044079.

📒 Files selected for processing (5)
  • apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.spec.tsx (1 hunks)
  • apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.strings.ts (1 hunks)
  • apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx (9 hunks)
  • apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/Overview.tsx (2 hunks)
  • apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesReviewed.tsx (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.strings.ts
  • apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/Overview.tsx
🧰 Additional context used
📓 Path-based instructions (3)
apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.spec.tsx (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/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx (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/web/src/routes/PublicProsecutor/Tables/CasesReviewed.tsx (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 (3)
apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.spec.tsx (1)
Learnt from: oddsson
PR: island-is/island.is#16495
File: apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx:102-103
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In the TypeScript file `apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx`, within the `handleSetDate` function, the else block is intentionally kept to handle an unlikely scenario and should not be removed, even if it appears unnecessary due to prior validation checks.
apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx (3)
Learnt from: oddsson
PR: island-is/island.is#16495
File: apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx:102-103
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In the TypeScript file `apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx`, within the `handleSetDate` function, the else block is intentionally kept to handle an unlikely scenario and should not be removed, even if it appears unnecessary due to prior validation checks.
Learnt from: oddsson
PR: island-is/island.is#16831
File: apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx:204-337
Timestamp: 2024-11-19T09:28:36.448Z
Learning: Buttons in this project are considered accessible as is, and adding additional ARIA attributes like `aria-label` and `aria-disabled` is not required unless specifically needed.
Learnt from: oddsson
PR: island-is/island.is#16495
File: apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx:84-105
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In `apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx`, the `setAndSendDefendantToServer` function handles errors internally, so additional error handling when calling it is not necessary.
apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesReviewed.tsx (2)
Learnt from: oddsson
PR: island-is/island.is#14673
File: apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesForReview.tsx:79-79
Timestamp: 2024-11-12T15:15:11.835Z
Learning: The implementation of `mapIndictmentCaseStateToTagVariant` in `TagCaseState.tsx`, which maps only `CaseState.ACCEPTED` to a specific tag and defaults others to an "unknown" status, is intentional as per the application's requirements.
Learnt from: unakb
PR: island-is/island.is#15378
File: apps/judicial-system/web/src/routes/Court/Indictments/Summary/Summary.tsx:86-100
Timestamp: 2024-11-12T15:15:11.835Z
Learning: User unakb prefers explicit case handling in switch statements for key functionalities like `getRulingDecisionTagColor` to ensure clarity and avoid assumptions that a case was overlooked.
🔇 Additional comments (9)
apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.spec.tsx (3)

1-26: LGTM! Well-structured test setup with proper mocking.

The import statements are well-organized, and the router mock is appropriately configured for the test requirements.


28-36: LGTM! Clean and reusable test data setup.

Good use of faker for generating test data and proper setup of reusable mock objects.


1-80: LGTM! Follows NextJS testing best practices.

The test file is well-structured and follows NextJS testing conventions with proper use of testing utilities and context providers.

apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesReviewed.tsx (3)

7-7: LGTM: Import changes align with requirements.

The added import supports the new fine-handling functionality while maintaining type safety.


124-128: LGTM: Table cell rendering properly implements the new mapping logic.

The implementation correctly uses the new mapping function and maintains type safety through proper TypeScript usage.


59-61: LGTM: Early return for fine cases aligns with requirements.

The implementation correctly handles the special case for fines by returning null, which aligns with the PR objective stating that no service is performed for fine cases.

Let's verify that this change doesn't affect other cases:

✅ Verification successful

Verified: Early return for FINE cases is consistent with codebase patterns

The implementation is correct and aligns with the broader codebase handling of FINE cases. The search results show that:

  • The getVerdictViewTag function is only implemented in this component
  • FINE case handling is consistently treated as a special case across multiple components
  • Backend and API layers also handle FINE cases distinctly
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for similar verdict tag handling in other components
ast-grep --pattern 'getVerdictViewTag = ($params) {
  $$$
  if ($condition === CaseIndictmentRulingDecision.FINE) {
    $$$
  }
  $$$
}'

Length of output: 138


Script:

#!/bin/bash
# Let's try a broader search for the function and its usage patterns
rg -A 10 "getVerdictViewTag" 

# Also search for FINE decision handling
rg "CaseIndictmentRulingDecision.FINE"

Length of output: 6122

apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx (3)

54-56: Good use of the isFine variable for clarity.

Introducing the isFine variable enhances readability by clearly indicating when the case is a fine, making the conditional logic easier to understand.


138-176: Ensure all states are properly handled in conditional rendering.

The addition of the isFine condition in the textItems array construction effectively handles fine cases. However, please verify that all possible combinations of isFine, serviceRequired, and defendant dates are accurately accounted for to prevent any unintended omissions or overlaps in the displayed information.


349-352: Validate the logic for disabling the "Send to Prison Admin" button.

The disabled prop for the button uses the condition:

!workingCase.indictmentReviewDecision || (!isFine && !defendant.verdictViewDate)

Ensure that this logic correctly represents the desired behavior:

  • The button should be disabled if there is no indictmentReviewDecision.
  • For non-fine cases (!isFine), the verdictViewDate must be set to enable the button.
  • For fine cases (isFine), the button should be enabled even if verdictViewDate is not set.

Please verify that this aligns with the application's requirements.

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

🧹 Outside diff range and nitpick comments (4)
libs/judicial-system/types/src/lib/indictmentCase.ts (2)

16-16: Consider using a discriminated union type instead of a boolean flag.

While the isFine parameter works, using a boolean flag makes the code less maintainable if more appeal types are added in the future.

Consider this type-safe alternative:

type AppealType = {
  type: 'FINE' | 'VERDICT';
  windowDays: number;
}

export const getIndictmentVerdictAppealDeadlineStatus = (
  verdictInfo?: [boolean, Date | undefined][],
  appealType?: AppealType,
): [boolean, boolean] => {
  // ... use appealType.windowDays instead of the ternary
}

41-44: Consider extracting the deadline calculation for better readability.

While the logic is correct, the deadline calculation could be more explicit.

Consider this more readable alternative:

+ const getAppealDeadline = (viewDate: Date, isFine: boolean) => 
+   viewDate.getTime() + getDays(isFine ? FINE_APPEAL_WINDOW_DAYS : VERDICT_APPEAL_WINDOW_DAYS)

  return [
    true,
-   Date.now() > newestViewDate.getTime() + getDays(isFine ? 3 : 28),
+   Date.now() > getAppealDeadline(newestViewDate, isFine),
  ]
apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/Overview.strings.ts (1)

27-29: Update the description field to reflect the new message format

The description field should be updated to explain the conditional formatting and the different cases (fines vs. verdicts, past vs. future deadlines).

     defaultMessage:
       'Frestur til að {isFine, select, true {kæra viðurlagaákvörðun} other {áfrýja dómi}} {appealDeadlineIsInThePast, select, true {rann} other {rennur}} út {indictmentAppealDeadline}',
-    description: 'Notaður sem undirtitill á yfirliti ákæru.',
+    description: 'Notaður sem undirtitill á yfirliti ákæru. Sýnir mismunandi texta fyrir sektir (kæra) og dóma (áfrýja), ásamt því hvort frestur sé liðinn eða ekki.',
apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx (1)

138-176: Consider extracting text generation logic to a separate function

The text items generation logic has grown complex with the addition of fine-specific handling. Consider extracting this into a separate function for better maintainability.

- const textItems = useMemo(() => {
-   const texts = []
-   if (isFine) {
-     texts.push(/* ... */)
-   } else {
-     if (serviceRequired) {
-       texts.push(/* ... */)
-     }
-     // ... rest of the logic
-   }
-   return texts
- }, [/* ... */])
+ const getTextItems = useCallback(() => {
+   const texts = []
+   if (isFine) {
+     texts.push(getFineText())
+   } else {
+     if (serviceRequired) {
+       texts.push(getServiceText())
+     }
+     texts.push(getAppealText())
+     texts.push(getAdminText())
+   }
+   return texts
+ }, [/* ... */])
+ 
+ const textItems = useMemo(() => getTextItems(), [getTextItems])
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c044079 and 1494993.

📒 Files selected for processing (5)
  • apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.ts (4 hunks)
  • apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx (9 hunks)
  • apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/Overview.strings.ts (1 hunks)
  • apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/Overview.tsx (4 hunks)
  • libs/judicial-system/types/src/lib/indictmentCase.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.ts
  • apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/Overview.tsx
🧰 Additional context used
📓 Path-based instructions (3)
apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx (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/web/src/routes/PublicProsecutor/Indictments/Overview/Overview.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."
libs/judicial-system/types/src/lib/indictmentCase.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)
apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx (3)
Learnt from: oddsson
PR: island-is/island.is#16495
File: apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx:102-103
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In the TypeScript file `apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx`, within the `handleSetDate` function, the else block is intentionally kept to handle an unlikely scenario and should not be removed, even if it appears unnecessary due to prior validation checks.
Learnt from: oddsson
PR: island-is/island.is#16831
File: apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx:204-337
Timestamp: 2024-11-19T09:28:36.448Z
Learning: Buttons in this project are considered accessible as is, and adding additional ARIA attributes like `aria-label` and `aria-disabled` is not required unless specifically needed.
Learnt from: oddsson
PR: island-is/island.is#16495
File: apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx:84-105
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In `apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx`, the `setAndSendDefendantToServer` function handles errors internally, so additional error handling when calling it is not necessary.
🔇 Additional comments (6)
libs/judicial-system/types/src/lib/indictmentCase.ts (1)

3-3: LGTM! The constant aligns with the business requirements.

The new constant properly defines the 3-day appeal window for fines as specified in the PR objectives.

apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/Overview.strings.ts (1)

25-25: Verify message ID updates across the codebase

The message ID has been updated with a _v2 suffix, indicating a breaking change. Let's verify that all components using this message have been updated accordingly.

✅ Verification successful

Message ID update is properly implemented

The verification shows that:

  1. There are no instances of the old message ID in the codebase
  2. The new message ID with _v2 suffix is only defined in the strings file
  3. The component correctly uses the reviewerSubtitle reference to access the message
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for any remaining usage of the old message ID
# Expected: No results, as all occurrences should use the new ID

rg "judicial\.system\.core:public_prosecutor\.indictments\.overview\.reviewer_subtitle[^_]" \
  --type ts \
  --type tsx

Length of output: 142


Script:

#!/bin/bash
# Search for any usage of the old message ID without the _v2 suffix
# Using ripgrep with just --type-add for TypeScript files
rg --type-add 'ts:*.{ts,tsx}' --type ts "judicial\.system\.core:public_prosecutor\.indictments\.overview\.reviewer_subtitle(?!_v2)" 

Length of output: 508


Script:

#!/bin/bash
# Search for usage of the new message ID with _v2 suffix to ensure it's being used
rg --type-add 'ts:*.{ts,tsx}' --type ts "judicial\.system\.core:public_prosecutor\.indictments\.overview\.reviewer_subtitle_v2"

Length of output: 315


Script:

#!/bin/bash
# List all TypeScript files in the relevant directory to ensure we're searching in the right place
fd -e ts -e tsx . apps/judicial-system/web/src/routes/PublicProsecutor/Indictments

Length of output: 482


Script:

#!/bin/bash
# Search for the old message ID without regex lookbehind
rg --type-add 'ts:*.{ts,tsx}' --type ts "judicial\.system\.core:public_prosecutor\.indictments\.overview\.reviewer_subtitle[^_v2]" -A 2 -B 2

# Also search for the message usage in the Overview.tsx file
rg --type-add 'ts:*.{ts,tsx}' --type ts "reviewerSubtitle" apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/Overview.tsx -A 2 -B 2

Length of output: 498

apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx (4)

54-55: LGTM: Clear and explicit fine status check

The new isFine check clearly identifies cases with fine decisions, aligning with the PR objective of enabling proper fine handling.


224-226: LGTM: Clear conditional title rendering

The title now correctly reflects whether the case involves a fine or regular key dates.


349-352: LGTM: Updated prison admin button logic

The button's disabled state now correctly handles both fine and non-fine cases, ensuring proper workflow for prison administrators.


238-238: Verify the appeal deadline expiration logic

The component now considers fine status for rendering decisions, but we should verify that the appeal deadline expiration logic aligns with the new 3-day requirement mentioned in the PR objectives.

Also applies to: 262-263

✅ Verification successful

The search results show clear evidence that the appeal deadline logic has been properly implemented according to the PR objectives. The constants are defined in libs/judicial-system/types/src/lib/indictmentCase.ts:

  • FINE_APPEAL_WINDOW_DAYS = 3 for fine cases
  • VERDICT_APPEAL_WINDOW_DAYS = 28 for verdict cases

The transformer in apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.ts correctly uses these constants to calculate appeal deadlines based on whether it's a fine case or not:

isFine ? FINE_APPEAL_WINDOW_DAYS : VERDICT_APPEAL_WINDOW_DAYS

Appeal deadline logic aligns with requirements

The code correctly implements the 3-day appeal window for fine cases while maintaining the 28-day window for regular verdicts. The conditional rendering in BlueBoxWithDate.tsx appropriately considers this distinction through the isFine flag.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for appeal window day constants to verify the deadline calculation
rg -A 2 "APPEAL_WINDOW_DAYS|appealDeadline" --type ts

Length of output: 11597

libs/judicial-system/types/src/lib/indictmentCase.ts Outdated Show resolved Hide resolved
@oddsson oddsson added the automerge Merge this PR as soon as all checks pass label Nov 26, 2024
@oddsson oddsson requested a review from unakb November 26, 2024 22:04
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 (3)
libs/judicial-system/types/src/lib/indictmentCase.ts (1)

3-4: LGTM! Consider adding type annotations for better maintainability.

The constant and helper function are well-structured and reusable. The implementation aligns with the PR requirements for a 3-day appeal window for fines.

Consider adding type annotations for better maintainability:

-const getDays = (days: number) => days * DAYS_TO_MILLISECONDS
+const getDays = (days: number): number => days * DAYS_TO_MILLISECONDS
apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/Overview.tsx (2)

Line range hint 73-89: Consider removing unnecessary memoization

Based on previous feedback in PR #16731, since the users data doesn't change frequently, the useMemo for publicProsecutors might be unnecessary. Consider simplifying this to a regular function or computing it directly in the render.

-  const publicProsecutors = useMemo(() => {
+  const publicProsecutors = (() => {
     if (!data?.users || !user) {
       return []
     }
     return data.users.reduce(
       (acc: { label: string; value: string }[], prosecutor) => {
         if (prosecutor.institution?.id === user?.institution?.id) {
           acc.push({
             label: prosecutor.name ?? '',
             value: prosecutor.id,
           })
         }
         return acc
       },
       [],
     )
-  }, [data?.users, user])
+  })()

Line range hint 34-194: Consider extracting reviewer selection logic

The Overview component is handling multiple responsibilities. Consider extracting the reviewer selection logic (including the Select component and related state) into a separate component to improve maintainability and reusability.

This would:

  1. Make the code more modular and easier to test
  2. Reduce the complexity of the Overview component
  3. Make it easier to reuse the reviewer selection functionality elsewhere if needed
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 1494993 and bf5a448.

📒 Files selected for processing (9)
  • apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.spec.ts (1 hunks)
  • apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.ts (4 hunks)
  • apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.spec.tsx (1 hunks)
  • apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.strings.ts (1 hunks)
  • apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/Overview.tsx (4 hunks)
  • apps/judicial-system/web/src/routes/PublicProsecutor/components/ReviewDecision/ReviewDecision.strings.ts (2 hunks)
  • apps/judicial-system/web/src/routes/PublicProsecutor/components/ReviewDecision/ReviewDecision.tsx (4 hunks)
  • apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx (1 hunks)
  • libs/judicial-system/types/src/lib/indictmentCase.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
  • apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.spec.ts
  • apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.ts
  • apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.spec.tsx
  • apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.strings.ts
  • apps/judicial-system/web/src/routes/PublicProsecutor/components/ReviewDecision/ReviewDecision.strings.ts
  • apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx
🧰 Additional context used
📓 Path-based instructions (3)
apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/Overview.tsx (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/web/src/routes/PublicProsecutor/components/ReviewDecision/ReviewDecision.tsx (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/indictmentCase.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)
apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/Overview.tsx (1)
Learnt from: oddsson
PR: island-is/island.is#16731
File: apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx:172-186
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `IndictmentOverview.tsx`, since the defendants data does not change, using `useMemo` to memoize the filtering logic is unnecessary.
🔇 Additional comments (8)
libs/judicial-system/types/src/lib/indictmentCase.ts (1)

16-16: LGTM! Parameter addition maintains backward compatibility.

The optional isFine parameter is well-typed and maintains backward compatibility with existing code.

apps/judicial-system/web/src/routes/PublicProsecutor/components/ReviewDecision/ReviewDecision.tsx (4)

35-38: LGTM!

Props destructuring is clean and follows React best practices.


42-45: LGTM!

Component setup follows NextJS and React best practices with proper hook usage.


64-73: Skipping comment as it was already raised in past reviews

The issue about appeal options being available for fines was already identified in a previous review and remains valid.


84-90: Verify appeal deadline calculation

According to the PR objectives, the appeal timeframe should be reduced from 28 to 3 days. Please ensure that the indictmentAppealDeadline being passed to this component is calculated using the new 3-day timeframe.

Also applies to: 119-124

✅ Verification successful

Based on the search results, I found the appeal deadline calculation in apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.ts:

appealInfo.appealDeadline = new Date(
  theRulingDate.getTime() + getDays(3),
).toISOString()

This confirms that the appeal deadline is indeed set to 3 days from the ruling date, which aligns with the PR objectives to reduce the timeframe from 28 to 3 days.

Appeal deadline calculation verified

The implementation correctly uses the new 3-day timeframe for calculating the appeal deadline. The indictmentAppealDeadline being passed to the component is derived from this calculation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for appeal deadline calculation logic
rg -A 5 "appealDeadline|APPEAL_DEADLINE" --type ts

Length of output: 17565

apps/judicial-system/web/src/routes/PublicProsecutor/Indictments/Overview/Overview.tsx (3)

11-11: LGTM: Import changes align with new requirements.

The new imports support the fine handling functionality and follow NextJS best practices.

Also applies to: 28-28


130-137: LGTM: Fine and appeal deadline checks implemented correctly.

The implementation correctly handles fine decisions and appeal deadline expiration, aligning with the PR objectives.


106-106: Verify appeal deadline handling in BlueBoxWithDate

Please ensure that the BlueBoxWithDate component correctly implements the new 3-day appeal timeframe for fines, as specified in the PR objectives.

Let's verify the implementation:

@oddsson oddsson requested review from unakb and removed request for unakb November 26, 2024 22:24
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/indictmentCase.ts (1)

16-16: Update JSDoc to document the isFine parameter.

The function's documentation should be updated to explain the purpose and impact of the isFine parameter on the appeal deadline calculation.

 /*
   This function takes an array of verdict info tuples:
   - The first element of the tuple is a boolean indicating whether the defendant can appeal the verdict.
   - The second element of the tuple is a Date object indicating when the defendant viewed the verdict. Undefined if the defendant has not viewed the verdict.
+  @param isFine - When true, uses a 3-day appeal window instead of the standard 28-day window
   The function returns a tuple of two booleans:
   - The first boolean indicates whether all defendants who need to see the verdict have seen it.
   - The second boolean indicates whether all defenant appeal deadlines have expired.
 */
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between bf5a448 and 38ae47e.

📒 Files selected for processing (1)
  • libs/judicial-system/types/src/lib/indictmentCase.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/judicial-system/types/src/lib/indictmentCase.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."
🔇 Additional comments (2)
libs/judicial-system/types/src/lib/indictmentCase.ts (2)

3-3: LGTM! The constant is well-defined and matches requirements.

The new constant FINE_APPEAL_WINDOW_DAYS correctly implements the 3-day appeal window for fines as specified in the PR objectives.


39-44: LGTM! Clean implementation of the conditional appeal window.

The implementation correctly:

  • Uses the helper function for time calculations
  • Applies the appropriate appeal window based on the case type
  • Avoids magic numbers by using constants

@kodiakhq kodiakhq bot merged commit f66ba3c into main Nov 26, 2024
37 checks passed
@kodiakhq kodiakhq bot deleted the j-s/fine-refactoring branch November 26, 2024 23:03
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.

3 participants