-
Notifications
You must be signed in to change notification settings - Fork 61
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
fix(j-s): Render sent to prison admin files for FMST #17264
Conversation
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. WalkthroughThis pull request introduces a new case file category Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17264 +/- ##
==========================================
+ Coverage 35.72% 35.73% +0.01%
==========================================
Files 6939 6941 +2
Lines 148368 148401 +33
Branches 42325 42334 +9
==========================================
+ Hits 53009 53037 +28
- Misses 95359 95364 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this 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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts
(1 hunks)apps/judicial-system/backend/src/app/modules/file/guards/caseFileCategory.ts
(1 hunks)apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts
(1 hunks)apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.strings.ts
(1 hunks)apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.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/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.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."
apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.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/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.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/backend/src/app/modules/file/guards/caseFileCategory.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/file/guards/caseFileCategory.ts (1)
Learnt from: oddsson
PR: island-is/island.is#16138
File: apps/judicial-system/backend/src/app/modules/file/guards/caseFileCategory.ts:74-87
Timestamp: 2024-11-12T15:15:26.274Z
Learning: User prefers to keep similar conditional statements separate rather than combining them into a single conditional with logical operators.
🔇 Additional comments (6)
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.strings.ts (1)
61-65
: LGTM! The new localization string follows established patterns.
The new string entry for prison admin files maintains consistency with existing localization patterns and provides clear context through its description.
apps/judicial-system/backend/src/app/modules/file/guards/caseFileCategory.ts (1)
47-47
: LGTM! Access control is properly implemented.
The new file category is correctly added to prisonAdminCaseFileCategories
, maintaining the existing security model where access is granted only to prison admin users for completed cases.
Let's verify the access control implementation:
✅ Verification successful
Access control implementation is properly verified
The verification confirms that:
SENT_TO_PRISON_ADMIN_FILE
is correctly added toprisonAdminCaseFileCategories
array- Access control is enforced through the guard system that checks file categories against user roles
- The implementation maintains consistency with other prison admin file categories (APPEAL_RULING, RULING)
- The category is properly used in both frontend components and backend guards
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that SENT_TO_PRISON_ADMIN_FILE is only accessible by prison admin users
# Search for all usages of SENT_TO_PRISON_ADMIN_FILE to ensure proper access control
rg -A 5 "SENT_TO_PRISON_ADMIN_FILE"
# Search for any test cases validating the access control
rg -A 10 "describe.*prison.*admin.*file"
Length of output: 5996
Script:
#!/bin/bash
# Let's verify the access control implementation more thoroughly
# Check the guard implementation in caseFileCategory.ts
ast-grep --pattern 'const prisonAdminCaseFileCategories = [$$$]'
# Check test cases for prison admin file access
ast-grep --pattern 'describe($$$prison admin$$$, () => {
$$$
})'
# Check the implementation of the createDefendantCaseFile guard
ast-grep --pattern 'class CreateDefendantCaseFileGuard {
$$$
}'
Length of output: 752
Script:
#!/bin/bash
# Let's check how the prison admin file categories are used in the guards
# Check the implementation of access control logic
ast-grep --pattern 'function canPrisonUserViewCaseFile($_) {
$$$
}'
# Check for any validation in the limited access guard
ast-grep --pattern 'class LimitedAccessViewCaseFileGuard {
$$$
}'
# Look for usage of prisonAdminCaseFileCategories in the code
rg -A 5 "prisonAdminCaseFileCategories.includes"
Length of output: 784
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx (1)
102-112
: LGTM! Clean implementation of the prison admin files section.
The UI implementation follows the established patterns in the codebase:
- Consistent with other file sections
- Proper use of island-ui components
- Correct implementation of internationalization
apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts (1)
437-437
: LGTM: Comprehensive test coverage for new prison admin file category.
The addition of SENT_TO_PRISON_ADMIN_FILE
to the allowed categories for prison admin users is well-tested:
- Verifies prison admin access to the new category
- Includes negative test cases
- Maintains consistency with existing test patterns
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts (2)
237-237
: LGTM: Addition of prison admin file category
The addition of SENT_TO_PRISON_ADMIN_FILE
to the allowed case file categories aligns with the PR objective of enabling prison admin file visibility.
237-237
: Verify prison admin file handling
Please ensure that:
- Appropriate access controls are in place for this new file category
- Consider if these files should be included in the zip file generation (currently filtered by
defenderCaseFileCategoriesForRequestCases
)
Let's verify the access controls and related configurations:
✅ Verification successful
Based on the search results, I can now generate the final response:
Access controls and configurations are properly implemented for prison admin files
The verification shows that:
-
Access controls are properly implemented:
- Prison admin file category is defined in the guards (
caseFileCategory.ts
) - Access is restricted through
createDefendantCaseFile.guard.ts
- Proper role checks are in place for prison admin users
- Prison admin file category is defined in the guards (
-
File handling is correctly configured:
- Files are properly categorized with
SENT_TO_PRISON_ADMIN_FILE
- Notifications are set up for prison admin file operations
- Prison admin files are handled separately from defender case files
- Files are properly categorized with
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check access controls and configurations for prison admin files
# Check for access guard configurations
echo "Checking access guard configurations..."
rg -A 5 "SENT_TO_PRISON_ADMIN_FILE" --type ts
# Check for prison admin role definitions and permissions
echo "Checking role definitions..."
rg -A 5 "PRISON_ADMIN|prisonAdmin" --type ts
# Check defender case file categories configuration
echo "Checking defender case file categories..."
ast-grep --pattern 'defenderCaseFileCategoriesForRequestCases = [
$$$
]'
Length of output: 53920
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx
Show resolved
Hide resolved
Datadog ReportAll test runs ✅ 2 Total Test Services: 0 Failed, 2 Passed Test Services
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me!
Laga að FMST sjái uploadað skjal
What
Display prison admin case files to prison admin
Why
They need to see the files
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests