chore(j-s): Fixes to ServiceRequirement NOT_REQUIRED #16305
Conversation
WalkthroughThe pull request introduces several modifications across multiple files in the judicial system application. Key changes include updates to the logic for handling verdict information in the Changes
Possibly related PRs
Suggested reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
Datadog ReportAll test runs ❌ 3 Total Test Services: 1 Failed, 2 Passed Test Services
❌ Failed Tests (1)
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16305 +/- ##
==========================================
- Coverage 36.73% 36.72% -0.01%
==========================================
Files 6809 6801 -8
Lines 141086 139985 -1101
Branches 40224 39848 -376
==========================================
- Hits 51825 51411 -414
+ Misses 89261 88574 -687
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 230 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
oddsson
changed the title
oddsson
added
the
deprecated:automerge
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
apps/judicial-system/web/src/components/SectionHeading/SectionHeading.tsx (1)
32-36: Consider simplifying the description renderingThe change from rendering the description directly to wrapping it in a
Boxcomponent provides more flexibility for styling and layout. However, there's a potential for optimization:
- The
component="span"prop on theBoxmight be unnecessary unless there's a specific reason for rendering as a span.- Ensure that this change doesn't unintentionally affect text styling that might have been inherited from the previous
Textcomponent usage.Consider simplifying the code as follows:
{description && ( - <Box component="span" marginTop={1}> + <Box marginTop={1}> {description} </Box> )}This change maintains the added flexibility while simplifying the component structure.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.ts (1 hunks)
- apps/judicial-system/backend/src/app/modules/case/filters/case.filter.ts (1 hunks)
- apps/judicial-system/backend/src/app/modules/case/filters/cases.filter.ts (1 hunks)
- apps/judicial-system/backend/src/app/modules/subpoena/limitedAccessSubpoena.controller.ts (0 hunks)
- apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.css.ts (1 hunks)
- apps/judicial-system/web/src/components/SectionHeading/SectionHeading.tsx (1 hunks)
💤 Files with no reviewable changes (1)
- apps/judicial-system/backend/src/app/modules/subpoena/limitedAccessSubpoena.controller.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.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/filters/case.filter.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/filters/cases.filter.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/web/src/components/InfoCard/DefendantInfo/DefendantInfo.css.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/web/src/components/SectionHeading/SectionHeading.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."
🔇 Additional comments (5)
apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.css.ts (1)
11-11: Improved layout flexibility with auto-sized second columnThe change from
'5fr 1fr'to'5fr auto'for thegridTemplateColumnsproperty in thewithButtonvariant is a good improvement. This modification allows the second column (likely containing a button) to automatically size based on its content, rather than being fixed to 1/6 of the available space. This change enhances the layout's flexibility and responsiveness, particularly when the button's width may vary.apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.ts (1)
150-153: Verify alignment with PR objectives and consider additional logicThe changes align with the PR objectives by modifying how cases marked as "NOT_REQUIRED" are handled. However, the 28-day waiting period mentioned in the PR objectives is not directly implemented in this function.
Consider adding logic to implement the 28-day waiting period for cases marked as "NOT_REQUIRED" before they are sent to FMST. This could involve:
- Adding a new field to track when a "NOT_REQUIRED" case was reviewed by a prosecutor.
- Implementing a check to ensure 28 days have passed since the review date before further processing.
To ensure the changes align with the PR objectives, let's check for any related logic elsewhere in the codebase:
Would you like assistance in implementing the 28-day waiting period logic?
apps/judicial-system/backend/src/app/modules/case/filters/cases.filter.ts (2)
Line range hint
1-365: Overall assessment: Change aligns with PR objectives, but requires verification.The modification to the
getPrisonAdminUserCasesQueryFilterfunction aligns with the PR objectives by implementing a 28-day waiting period for cases marked as not required to be served. The change is localized and doesn't affect other parts of the file.To ensure the change meets all requirements and maintains code clarity:
- Verify that the new logic exactly matches the requirements from the public prosecutors office.
- Add a comment explaining the 28-day waiting period logic.
- Run the suggested verification script to check for any potential impacts on other parts of the codebase.
217-217: Verify alignment with PR objectives and add explanatory comment.The removal of the
service_requirement <> 'NOT_REQUIRED'condition aligns with the PR objective to include a 28-day waiting period for cases marked as not required to be served. This change simplifies the query and potentially includes more cases in the result set.To ensure clarity and maintainability:
- Verify that this change exactly meets the requirements stated in the PR objectives, particularly regarding the handling of cases marked as "NOT_REQUIRED".
- Add a comment explaining the 28-day logic and its purpose. For example:
// Filter out cases viewed within the last 28 days to allow for the waiting period // requested by the public prosecutors office before sending to FMSTTo verify the impact of this change, please run the following query:
This will help ensure that removing the
service_requirementcondition doesn't have unintended consequences elsewhere in the codebase.apps/judicial-system/backend/src/app/modules/case/filters/case.filter.ts (1)
301-302: Verify access denial logic based on appeal deadline expirationThe condition
if (!indictmentVerdictAppealDeadlineExpired)returnsfalse, denying access if the indictment verdict appeal deadline has not expired. Please confirm that this logic aligns with the intended business rules for prison admin users accessing cases.
apps/judicial-system/backend/src/app/modules/case/filters/case.filter.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/judicial-system/backend/src/app/modules/case/filters/test/cases.filter.spec.ts (2)
Line range hint
269-277: Ensure correct user role for prison admin staffIn the test case for the prison admin staff filter, the
roleis set toUserRole.PRISON_SYSTEM_STAFF, while theinstitution.typeisInstitutionType.PRISON_ADMIN. This could lead to confusion or incorrect behavior if the role does not align with the institution type.Consider updating the
roleto a more specific value that matchesInstitutionType.PRISON_ADMIN, such asUserRole.PRISON_ADMIN_STAFF, if it exists. This ensures consistency and clarity in the test setup.
Line range hint
313-316: Simplify redundant array in defender national ID conditionIn the defender filter, the condition for
defender_national_idis specified as:{ defender_national_id: [user.nationalId, user.nationalId], },Listing the same national ID twice in the array is redundant. Simplify the condition by using a single value:
Apply this diff to fix the redundancy:
{ - defender_national_id: [user.nationalId, user.nationalId], + defender_national_id: user.nationalId, },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/judicial-system/backend/src/app/modules/case/filters/test/cases.filter.spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/case/filters/test/cases.filter.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 (2)
apps/judicial-system/backend/src/app/modules/case/filters/test/cases.filter.spec.ts (2)
Line range hint
285-308: Validate the subquery logic for the 28-day waiting periodThe subquery in the
idcondition filters out cases where theverdict_view_dateis eitherNULLor within the last 28 days:(SELECT case_id FROM defendant WHERE (verdict_view_date IS NULL OR verdict_view_date > NOW() - INTERVAL '28 days'))This aims to include cases that have been viewed more than 28 days ago or not viewed at all. Confirm that this logic correctly implements the 28-day waiting period before cases are sent to FMST, as specified in the PR objectives.
Additionally, ensure that the
verdict_view_datefield is correctly populated elsewhere in the application and that there are no timezone-related issues that could affect the date comparisons.
Line range hint
108-124: Reevaluate assistant filter case typesIn the assistant filter for district court roles, only
indictmentCasesare included:{ type: indictmentCases },Verify whether assistants should have access exclusively to indictment cases. If assistants are supposed to access other case types, such as restriction or investigation cases, consider including those in the filter to align with their role requirements.
Fixes to ServiceRequirement NOT_REQUIRED
Asana
What
Today, if district court users mark that a ruling is not required to be served and a prosecutor at the public prosecutors office has reviewed a case, it is directly sent to FMST. According to the public prosecutors office, this should not happen, but rather it should wait 28 days (like with rulings that are required to be served) before being sent to FMST.
Why
This is a request from the public prosecutors office, based on their needs.
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Refactor