Format report name in report service and controller : BE #1295
Conversation
WalkthroughThe changes refactor the logic for generating report file names in the report generation controller. The file name is now determined by a new service function, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as reporting.ctrl.ts
participant Service as reportService.ts
Client->>Controller: POST /generateReports (reportName, reportType)
Controller->>Service: getFormattedReportName(reportName, reportType)
Service-->>Controller: Returns formatted file name
Controller->>Service: getProjectRiskMarkdown(...)
Service-->>Controller: Returns markdown report
Controller->>Service: convertMarkdownToDocx(...)
Service-->>Controller: Returns docx buffer
Controller->>Service: uploadFile(...)
Service-->>Controller: Returns file URL
Controller-->>Client: Responds with file URL
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
Servers/services/reportService.ts (1)
14-14: Consider adding type safety for the report type parameterThe
typeparameter could benefit from being typed more specifically to ensure only valid report types are passed.- export function getFormattedReportName(name: string, type: string) { + export function getFormattedReportName(name: string, type: keyof typeof ReportType | string) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Servers/controllers/reporting.ctrl.ts(2 hunks)Servers/services/reportService.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
Servers/services/reportService.ts (1)
Servers/models/reporting.model.ts (2)
ReportType(9-15)DefaultReportName(1-7)
Servers/controllers/reporting.ctrl.ts (1)
Servers/services/reportService.ts (1)
getFormattedReportName(14-48)
🔇 Additional comments (3)
Servers/services/reportService.ts (1)
107-107: LGTM: Improved formatting in report textThe formatting changes in the report template improve readability and consistency by adjusting capitalization and spacing.
Also applies to: 112-114, 116-116
Servers/controllers/reporting.ctrl.ts (2)
4-4: LGTM: Good service function importThe import statement correctly includes the new
getFormattedReportNamefunction from the report service.
33-33: LGTM: Good refactoring to service layerMoving the report name formatting logic to a dedicated service function improves separation of concerns and code maintainability.
Servers/services/reportService.ts
Outdated
| export function getFormattedReportName(name: string, type: string) { | ||
| let reportType; | ||
| switch(type) { | ||
| case ReportType.PROJECTRISK_REPORT: | ||
| reportType = DefaultReportName.PROJECTRISK_REPORT; | ||
| break; | ||
| case ReportType.VENDOR_REPORT: | ||
| reportType = DefaultReportName.VENDOR_REPORT; | ||
| break; | ||
| case ReportType.ASSESSMENT_REPORT: | ||
| reportType = DefaultReportName.ASSESSMENT_REPORT; | ||
| break; | ||
| case ReportType.COMPLIANCE_REPORT: | ||
| reportType = DefaultReportName.COMPLIANCE_REPORT; | ||
| break; | ||
| default: | ||
| reportType = DefaultReportName.ALL_REPORT; | ||
| } | ||
|
|
||
| const date = new Date(); | ||
|
|
||
| const year = date.getFullYear(); | ||
| const month = String(date.getMonth() + 1).padStart(2, '0'); | ||
| const day = String(date.getDate()).padStart(2, '0'); | ||
|
|
||
| const hour = String(date.getHours()).padStart(2, '0'); | ||
| const minute = String(date.getMinutes()).padStart(2, '0'); | ||
| const second = String(date.getSeconds()).padStart(2, '0'); | ||
|
|
||
| if(name.length === 0 ) { | ||
| return `${type}_${year}${month}${day}_${hour}${minute}${second}`; | ||
| } else { | ||
| return `${name}`; | ||
| } |
There was a problem hiding this comment.
Bug in default report name generation
The function correctly handles the case when a user provides a report name, but there's an issue when generating the default name. The function currently uses ${type} as the prefix (the full report type string like "Project risks report"), but it should be using the reportType variable (like "risk-report") that was set in the switch statement.
Fix the default report name generation:
if(name.length === 0 ) {
- return `${type}_${year}${month}${day}_${hour}${minute}${second}`;
+ return `${reportType}_${year}${month}${day}_${hour}${minute}${second}`;
} else {
- return `${name}`;
+ return name;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function getFormattedReportName(name: string, type: string) { | |
| let reportType; | |
| switch(type) { | |
| case ReportType.PROJECTRISK_REPORT: | |
| reportType = DefaultReportName.PROJECTRISK_REPORT; | |
| break; | |
| case ReportType.VENDOR_REPORT: | |
| reportType = DefaultReportName.VENDOR_REPORT; | |
| break; | |
| case ReportType.ASSESSMENT_REPORT: | |
| reportType = DefaultReportName.ASSESSMENT_REPORT; | |
| break; | |
| case ReportType.COMPLIANCE_REPORT: | |
| reportType = DefaultReportName.COMPLIANCE_REPORT; | |
| break; | |
| default: | |
| reportType = DefaultReportName.ALL_REPORT; | |
| } | |
| const date = new Date(); | |
| const year = date.getFullYear(); | |
| const month = String(date.getMonth() + 1).padStart(2, '0'); | |
| const day = String(date.getDate()).padStart(2, '0'); | |
| const hour = String(date.getHours()).padStart(2, '0'); | |
| const minute = String(date.getMinutes()).padStart(2, '0'); | |
| const second = String(date.getSeconds()).padStart(2, '0'); | |
| if(name.length === 0 ) { | |
| return `${type}_${year}${month}${day}_${hour}${minute}${second}`; | |
| } else { | |
| return `${name}`; | |
| } | |
| if (name.length === 0) { | |
| return `${reportType}_${year}${month}${day}_${hour}${minute}${second}`; | |
| } else { | |
| return name; | |
| } |
|
@eieimon thank you |
|
File formating/naming wise look good to me. |
eieimon
force-pushed
the
eem-may-05-report-name-changes
branch
from
6b90396 to
1d0a1cc
Compare
There was a problem hiding this comment.
@eieimon Thank you for applying changes.
There's a problem however.
I'm not able to generate reports on your branch, while your able to do so as shown in the video attached.
Describe your changes
Write your issue number after "Fixes "
Fixes #1283
Please ensure all items are checked off before requesting a review:
Screen.Recording.2025-05-05.at.1.02.15.PM.mov
Summary by CodeRabbit
Refactor
Style