fix(regulations-admin): Format ministry name #16822
Conversation
thordurhhh
added
the
deprecated:automerge
WalkthroughThe changes introduce a new function, 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16822 +/- ##
=======================================
Coverage 36.44% 36.44%
=======================================
Files 6852 6852
Lines 143515 143429 -86
Branches 40959 40911 -48
=======================================
- Hits 52300 52275 -25
+ Misses 91215 91154 -61
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportBranch report: ✅ 0 Failed, 1 Passed, 0 Skipped, 19.48s Total Time 🔻 Code Coverage Decreases vs Default Branch (1)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/download-service/src/app/modules/regulation-documents/regulation-documents.controller.ts (1)
72-81: Consider adding debug logging for ministry name transformationThe transformation looks correct, but adding debug logging would help track any unexpected ministry name formats.
+ console.debug( + `Transforming ministry name from "${draftRegulation.ministry}" to "${draftMinistry}"`, + ) const input: RegulationPdfInput = {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/download-service/src/app/modules/regulation-documents/regulation-documents.controller.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/download-service/src/app/modules/regulation-documents/regulation-documents.controller.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 (1)
apps/download-service/src/app/modules/regulation-documents/regulation-documents.controller.ts (1)
58-81: Verify transformation for all ministry names in the codebase
Let's ensure this transformation works correctly for all ministry names used in the system.
✅ Verification successful
Transformation correctly applied to all ministry names
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other ministry name occurrences to verify the transformation will work
# Look for strings ending in 'ráðuneyti' in TypeScript/JavaScript files
echo "Searching for ministry names in the codebase:"
rg -t ts -t js "\\w+ráðuneyti" --no-filename | sort -u
echo -e "\nSearching for existing transformed names (should be 'ráðuneytinu'):"
rg -t ts -t js "\\w+ráðuneytinu" --no-filename | sort -u
Length of output: 39757
apps/download-service/src/app/modules/regulation-documents/regulation-documents.controller.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/download-service/src/app/modules/regulation-documents/regulation-documents.controller.ts (1)
58-70: Consider extracting and enhancing the ministry name formatter.While the function correctly handles the suffix replacement, consider these improvements for better maintainability and type safety:
- Extract the function outside the
getDraftRegulationPdfmethod- Add input validation for empty strings
- Define the suffixes as typed constants
Here's a suggested implementation:
const MINISTRY_SUFFIXES = { OLD: 'ráðuneyti', NEW: 'ráðuneytinu' } as const; function formatMinistryName(ministryRaw?: string): string | undefined { if (!ministryRaw?.trim()) { return undefined; } return ministryRaw.endsWith(MINISTRY_SUFFIXES.OLD) ? ministryRaw.slice(0, -MINISTRY_SUFFIXES.OLD.length) + MINISTRY_SUFFIXES.NEW : ministryRaw; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/download-service/src/app/modules/regulation-documents/regulation-documents.controller.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/download-service/src/app/modules/regulation-documents/regulation-documents.controller.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/download-service/src/app/modules/regulation-documents/regulation-documents.controller.ts (2)
72-81: LGTM! Clean integration of the ministry name formatting.
The changes correctly integrate the ministry name transformation into the PDF generation flow while maintaining the existing error handling.
58-81: Verify comprehensive testing of ministry name formatting.
The implementation correctly handles the specific case of "Fjármálaráðuneyti" → "Fjármálaráðuneytinu". Let's verify other ministry names in the codebase.
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Fjármálaráðuneyti ➡️ Fjármálaráðuneytinu
Why
Ministry. þgf með greini.
Checklist:
Summary by CodeRabbit