-
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): Indictment Name #16729
fix(j-s): Indictment Name #16729
Conversation
WalkthroughThe changes in this pull request involve updates to message definitions 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
|
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: 0
🧹 Outside diff range and nitpick comments (1)
apps/judicial-system/backend/src/app/messages/courtUpload.ts (1)
Line range hint
1-30
: Consider standardizing version numbering across messages.I notice inconsistent version suffixes in the message IDs:
indictment
uses_v2
ruling
uses_v3
- Some messages have no version suffix
Consider adopting a consistent versioning strategy across all messages to make maintenance easier.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/judicial-system/backend/src/app/messages/courtUpload.ts
(1 hunks)apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/backend/src/app/messages/courtUpload.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/internalCase.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."
🔇 Additional comments (2)
apps/judicial-system/backend/src/app/messages/courtUpload.ts (1)
10-11
: LGTM! Verify placeholder usage across consumers.
The addition of the courtCaseNumber
placeholder in the indictment message aligns with the PR objective to fix the indictment naming issue.
Let's verify that all consumers properly handle the new placeholder:
✅ Verification successful
Placeholder usage verified and correctly implemented
The courtCaseNumber
placeholder is properly handled in the only consumer of this message (internalCase.service.ts). The service correctly passes the required parameter when formatting the message:
formatMessage(courtUpload.indictment, {
courtCaseNumber: theCase.courtCaseNumber,
})
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of the indictment message to ensure they provide the courtCaseNumber parameter
# Search for direct references to courtUpload.indictment
rg -l "courtUpload\.indictment"
# Search for formatMessage calls that might use this ID
ast-grep --pattern 'formatMessage($args)'
Length of output: 151
Script:
#!/bin/bash
# Let's examine the actual usage in internalCase.service.ts
rg -A 5 -B 5 "courtUpload\.indictment" apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts
# Also check for any formatMessage calls in this file
ast-grep --pattern 'formatMessage({ id: $_, $$ })' apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts
Length of output: 572
apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts (1)
546-548
: LGTM! The filename enhancement improves document identification.
Including the courtCaseNumber
in the indictment filename makes it easier to identify and track documents in the court system.
Let's verify the message format string:
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16729 +/- ##
=======================================
Coverage 36.53% 36.53%
=======================================
Files 6890 6890
Lines 143640 143640
Branches 40926 40926
=======================================
Hits 52486 52486
Misses 91154 91154
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ❌ 18 Total Test Services: 1 Failed, 16 Passed Test ServicesThis report shows up to 10 services
❌ Failed Tests (2)
🔻 Code Coverage Decreases vs Default Branch (2) |
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: 0
🧹 Outside diff range and nitpick comments (2)
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverIndictmentToCourt.spec.ts (2)
106-107
: LGTM! Consider extracting the duplicated string literal.The changes correctly implement the dynamic indictment naming with courtCaseNumber. However, the string literal 'Ákæra' is duplicated across test cases.
Consider extracting it to a constant at the top of the file:
+ const INDICTMENT_BASE_NAME = 'Ákæra'
Then update the test cases:
- `Ákæra ${courtCaseNumber}`, - `Ákæra ${courtCaseNumber}.pdf`, + `${INDICTMENT_BASE_NAME} ${courtCaseNumber}`, + `${INDICTMENT_BASE_NAME} ${courtCaseNumber}.pdf`,Also applies to: 131-132
106-107
: Add explicit assertion for courtCaseNumber inclusion.While the test verifies the complete filename, it would be more robust to explicitly assert that the courtCaseNumber is included in the document name.
Add these assertions after the existing expect calls:
expect(mockCourtService.createDocument.mock.calls[0][5]).toContain(courtCaseNumber) expect(mockCourtService.createDocument.mock.calls[0][6]).toContain(courtCaseNumber)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverIndictmentToCourt.spec.ts
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverIndictmentToCourt.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."
Indictment Name
Nafn á ákæru í Auði
What
Why
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes