chore(j-s): extract formatter to apply dative case to court names #17470
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 utility function Changes
Sequence DiagramsequenceDiagram
participant Notification Service
participant Formatters
participant Court Name
Notification Service->>Formatters: applyDativeCaseToCourtName()
Formatters->>Court Name: Transform to dative case
Formatters-->>Notification Service: Formatted court name
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 ✅ 5 Total Test Services: 0 Failed, 5 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (2) |
…nd-is/island.is into j-s/fix-thgf-for-court-name
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/judicial-system/backend/src/app/modules/notification/services/caseNotification/caseNotification.service.ts (1)
Direct string manipulation of court names is widespread across the codebase
The verification confirms multiple instances of
.replace('dómur', 'dómi')in both frontend and backend code:
- Frontend: In Indictment and CourtRecord components
- Backend: In formatters.ts, indictmentCaseNotification.service.ts, and caseNotification.service.ts
Replacing all these instances with
applyDativeCaseToCourtNamewould improve code consistency and maintainability.🔗 Analysis chain
Line range hint
879-921: Consider replacing all direct string manipulations with the new formatterThere are still instances in the code where court names are transformed using direct string replacement (
.replace('dómur', 'dómi')). Consider replacing all such occurrences with the newapplyDativeCaseToCourtNamefunction to ensure consistent formatting throughout the codebase.Let's find other instances of direct string manipulation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct string replacements of 'dómur' with 'dómi' rg "\.replace\('dómur', 'dómi'\)" -A 2Length of output: 8006
🧹 Nitpick comments (2)
libs/judicial-system/formatters/src/lib/formatters.ts (1)
468-475: Enhance the dative case formatter with validation and documentationThe function would benefit from input validation and documentation explaining the Icelandic grammar transformation.
-// þgf to dómur -export const applyDativeCaseToCourtName = (courtName: string) => { +/** + * Transforms a court name to its dative case form in Icelandic. + * Replaces 'dómur' with 'dómi' in court names. + * + * @example + * applyDativeCaseToCourtName('Héraðsdómur Reykjavíkur') // returns 'Héraðsdómi Reykjavíkur' + * + * @param courtName - The court name to transform + * @returns The court name in dative case + * @throws {Error} If courtName is null or undefined + */ +export const applyDativeCaseToCourtName = (courtName: string): string => { + if (!courtName) { + throw new Error('Court name cannot be null or undefined') + } const target = 'dómur' if (courtName.includes(target)) { return courtName?.replace(target, 'dómi') } return courtName }libs/judicial-system/formatters/src/lib/formatters.spec.ts (1)
417-439: Add more test cases for comprehensive coverageThe current test suite should be expanded to cover edge cases and error conditions.
describe('applyDativeCaseToCourtName', () => { + test('should throw error for null or undefined input', () => { + expect(() => applyDativeCaseToCourtName(undefined as unknown as string)).toThrow() + expect(() => applyDativeCaseToCourtName(null as unknown as string)).toThrow() + }) + + test('should handle empty string', () => { + expect(() => applyDativeCaseToCourtName('')).toThrow() + }) + + test('should handle multiple occurrences of "dómur"', () => { + expect(applyDativeCaseToCourtName('Héraðsdómur dómur')).toBe('Héraðsdómi dómi') + }) + test('should return dative case if court name contains "dómur"', () => { // Arrange const courtName = 'Héraðsdómur Reykjavíkur'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/judicial-system/backend/src/app/modules/notification/services/caseNotification/caseNotification.service.ts(3 hunks)apps/judicial-system/backend/src/app/modules/notification/services/indictmentCaseNotification/indictmentCaseNotification.service.ts(2 hunks)libs/judicial-system/formatters/src/lib/formatters.spec.ts(2 hunks)libs/judicial-system/formatters/src/lib/formatters.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
libs/judicial-system/formatters/src/lib/formatters.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."
apps/judicial-system/backend/src/app/modules/notification/services/indictmentCaseNotification/indictmentCaseNotification.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."
libs/judicial-system/formatters/src/lib/formatters.spec.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."
apps/judicial-system/backend/src/app/modules/notification/services/caseNotification/caseNotification.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."
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: prepare
🔇 Additional comments (5)
apps/judicial-system/backend/src/app/modules/notification/services/indictmentCaseNotification/indictmentCaseNotification.service.ts (2)
14-14: LGTM: Clean import additionThe import statement for the new formatter function is properly added.
108-110: LGTM: Robust implementation of court name formattingThe code correctly applies the dative case formatter with proper null checks using optional chaining and provides a fallback to an empty string.
apps/judicial-system/backend/src/app/modules/notification/services/caseNotification/caseNotification.service.ts (3)
28-28: LGTM: Clean import additionThe formatter function is properly added to the existing imports.
879-881: LGTM: Consistent implementation of court name formattingThe code correctly applies the dative case formatter with proper null checks and empty string fallback, maintaining consistency with other implementations.
919-921: LGTM: Consistent formatting across notification typesThe dative case formatting is consistently applied across different notification types, maintaining uniform behavior.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17470 +/- ##
==========================================
- Coverage 35.66% 35.66% -0.01%
==========================================
Files 6977 6977
Lines 149788 149842 +54
Branches 42832 42855 +23
==========================================
+ Hits 53424 53438 +14
- Misses 96364 96404 +40
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
…s/fix-thgf-for-court-name
…nd-is/island.is into j-s/fix-thgf-for-court-name
...m/backend/src/app/modules/notification/services/caseNotification/caseNotification.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/judicial-system/backend/src/app/modules/notification/services/caseNotification/caseNotification.service.ts (1)
Line range hint
879-924: Consider extracting the court name formatting logic.While the current implementation is correct, there's some duplication in how court names are formatted across different notification types. Consider extracting this into a helper function within the service to maintain DRY principles.
+ private formatCourtNameForNotification(courtName?: string): string { + return applyDativeCaseToCourtName(courtName || "") + } private sendRulingEmailNotificationToProsecutor( theCase: Case, ): Promise<Recipient> { // ... - courtName: applyDativeCaseToCourtName(theCase.court?.name || ""), + courtName: this.formatCourtNameForNotification(theCase.court?.name), // ... } private async sendRulingEmailNotificationToDefender( // ... - courtName: applyDativeCaseToCourtName(theCase.court?.name || ""), + courtName: this.formatCourtNameForNotification(theCase.court?.name), // ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/judicial-system/backend/src/app/modules/notification/services/caseNotification/caseNotification.service.ts(3 hunks)apps/judicial-system/backend/src/app/modules/notification/services/indictmentCaseNotification/indictmentCaseNotification.service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/judicial-system/backend/src/app/modules/notification/services/indictmentCaseNotification/indictmentCaseNotification.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/notification/services/caseNotification/caseNotification.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/modules/notification/services/caseNotification/caseNotification.service.ts (2)
Line range hint
879-883: LGTM! Consistent use of the dative case formatter.The implementation correctly uses the new
applyDativeCaseToCourtNamefunction with a proper fallback for undefined court names, which aligns with the discussed pattern.
Line range hint
917-924: LGTM! Consistent implementation across notification types.The same formatting pattern is correctly applied here, maintaining consistency in how court names are handled across different notification contexts.
Fallbeyging dómstóls í tölvupósti
What
Why
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates