Rewrite Printing #128
Rewrite Printing #128
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR refactors and consolidates PDF report generation across multiple modules. Individual controller methods in expense, healthcare cost, and travel reports now use a unified Changes
Sequence Diagram(s)sequenceDiagram
participant User as Client
participant Controller as Controller Method
participant ReportPrinter as ReportPrinter Instance
participant ReportPrint as Internal Report Generator
participant PDFDrawer as PDF Drawing Engine
participant Storage as Data Storage
User->>Controller: Request report generation
Controller->>ReportPrinter: Call print(report, language)
ReportPrinter->>ReportPrint: Initialize report printing
ReportPrint->>PDFDrawer: Render report elements
ReportPrint->>Storage: Retrieve assets (document & logo)
PDFDrawer-->>ReportPrint: Return rendered sections
ReportPrinter-->>Controller: Return generated PDF
Controller-->>User: Send PDF response
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ Finishing Touches
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
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: 3
🔭 Outside diff range comments (2)
backend/pdf/helper.ts (2)
85-94: 🛠️ Refactor suggestionUse optional chaining to simplify checks.
Currently:
if (cost.cost && cost.cost.receipts) { ... }This can be made more concise:
Proposed diff:
- if (cost.cost && cost.cost.receipts) { + if (cost.cost?.receipts) {
722-730: 🛠️ Refactor suggestionAddress control character (\u000B) in regex.
Static analysis flags “Unexpected control character” in your regex. Consider removing or safely handling
\u000B. This diff removes the vertical tab from the split pattern:- const lineSplit = (text: string) => text.split(/[\n\f\r\u000B]/) + const lineSplit = (text: string) => text.split(/[\n\f\r]/)
🧹 Nitpick comments (4)
backend/pdf/printer.ts (1)
139-253: Consider splitting therun()method for improved readability.The
run()method inReportPrintis quite long. Extracting separate functions (like for advanced text formatting or specialized drawing) can make the code more modular and easier to maintain.backend/models/helper.ts (1)
36-47: Add JSDoc documentation and improve type safety.The
logObjectfunction is well-structured, but could benefit from documentation and type improvements.Apply this diff to enhance the implementation:
+/** + * Creates a logging structure for tracking state changes with dates and editors. + * @param states - Array of valid states to track + * @returns Schema configuration for the log object + */ export function logObject<T extends AnyState>(states: readonly T[]) { const logEntry = { type: { date: { type: Date, required: true }, editor: { type: Schema.Types.ObjectId, ref: 'User', required: true } } } - const log: { type: { [key in T]?: typeof logEntry }; required: true; default: () => {} } = { + const log: { type: { [key in T]?: typeof logEntry }; required: true; default: () => Record<string, never> } = { type: {}, required: true, - default: () => ({}) + default: () => Object.create(null) } for (const state of states) { log.type[state] = logEntry } return log }🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
backend/models/expenseReport.ts (1)
91-91: Consider using a type-safe log entry creation.The log entry creation could benefit from type safety.
Apply this diff to improve type safety:
- this.log[this.state] = { date: new Date(), editor: this.editor } + type LogEntry = { date: Date; editor: typeof this.editor } + this.log[this.state] = { date: new Date(), editor: this.editor } as LogEntrycommon/types.ts (1)
493-494: Consider expanding font options.The
fontNamesarray is limited to only 'NotoSans'. Consider adding more font options to support diverse formatting needs.-export const fontNames = ['NotoSans'] as const +export const fontNames = ['NotoSans', 'Arial', 'TimesNewRoman', 'Helvetica'] as const
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
common/fonts/NotoSans.ttfis excluded by!**/*.ttf
📒 Files selected for processing (23)
backend/controller/expenseReportController.ts(4 hunks)backend/controller/healthCareCostController.ts(6 hunks)backend/controller/travelController.ts(5 hunks)backend/controller/types.ts(1 hunks)backend/data/settings.json(1 hunks)backend/factory.ts(1 hunks)backend/migrations.ts(1 hunks)backend/models/expenseReport.ts(4 hunks)backend/models/healthCareCost.ts(4 hunks)backend/models/helper.ts(2 hunks)backend/models/settings.ts(2 hunks)backend/models/travel.ts(4 hunks)backend/pdf/advance.ts(0 hunks)backend/pdf/expenseReport.ts(0 hunks)backend/pdf/healthCareCost.ts(0 hunks)backend/pdf/helper.ts(2 hunks)backend/pdf/printer.ts(1 hunks)backend/pdf/travel.ts(0 hunks)common/locales/de.json(3 hunks)common/locales/en.json(2 hunks)common/types.ts(5 hunks)frontend/src/components/expenseReport/forms/ExpenseReportForm.vue(1 hunks)frontend/src/components/travel/forms/TravelApplyForm.vue(1 hunks)
💤 Files with no reviewable changes (4)
- backend/pdf/advance.ts
- backend/pdf/healthCareCost.ts
- backend/pdf/expenseReport.ts
- backend/pdf/travel.ts
✅ Files skipped from review due to trivial changes (3)
- backend/data/settings.json
- frontend/src/components/expenseReport/forms/ExpenseReportForm.vue
- frontend/src/components/travel/forms/TravelApplyForm.vue
🧰 Additional context used
🪛 Biome (1.9.4)
backend/models/helper.ts
[error] 38-38: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
backend/models/travel.ts
[error] 194-194: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 197-197: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
backend/factory.ts
[error] 31-31: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
backend/pdf/printer.ts
[error] 87-87: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 722-722: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (23)
common/locales/en.json (2)
176-176: New Label Addition: "advanceFromEmployer"
A new label"advanceFromEmployer": "Advance From Employer"has been added under the"labels"section. This enhances clarity by clearly distinguishing the advance that comes from the employer. Verify that all UI components (e.g., ExpenseReportForm.vue and TravelApplyForm.vue) reference this new key correctly.
578-580: Report String Update for Approved Advances
The report string key in the"report"section has been updated from the former"approvedXY"to"approvedXonYbyZ"with the value"Advance in the amount of {X} approved by {Z} on {Y}."This change improves the clarity by including the advance amount, the approver, and the date. Please ensure that all components using this key are updated to supply parameters in the correct order.common/locales/de.json (3)
87-87: Update "lastPlaceOfWork" Description
The description for"lastPlaceOfWork"in the"info"section has been updated to:
"Wähle hier deinen letzten Tätigkeitsort aus, dieser ist maßgebend für die Pauschalen deiner Rückreise."
This change shifts the context to better reflect return travel expense scenarios. Ensure that the front-end displays this new description as intended.
177-177: New Label Addition: "advanceFromEmployer"
A new key"advanceFromEmployer": "Vorschuss vom Arbeitgeber"has been introduced in the"labels"section. This mirrors the English version and enhances context specificity for employer advances. Verify that all instances where this label is used are correctly updated in the UI.
579-580: Report String Update for Approved Advances
The string for approved advances in the"report"section is updated to
"approvedXonYbyZ": "Vorschuss in Höhe von {X} am {Y} durch {Z} genehmigt."
This reformulation clarifies the details by explicitly stating the amount, date, and approver. Please confirm that the placeholder ordering in both the English and German versions remains consistent and that the associated code correctly handles these variables.backend/pdf/helper.ts (4)
2-2: All good here.No issues spotted in this import statement.
4-4: ImportingreportPrinterlooks fine.Thanks for consolidating the report generation in a single location.
44-60: Centralized PDF generation approach is clear.Great job consolidating the logic into
reportPrinter.print(report, lng). This helps maintain DRY principles throughout the codebase.
200-212: Verify merging of receipt maps for potential key collisions.When combining multiple receipt maps into a single object, ensure there are no unintended overwrites of receipts with the same ID or other conflicts. If you need further help verifying the correctness, let me know.
backend/pdf/printer.ts (2)
1-35: Imports and basic setup look correct.No immediate concerns. If needed, confirm that your
pdf-libversion is compatible with these features.
36-83: Interfaces definition is well-structured.They provide clear type information and will help maintain clarity as the codebase grows.
backend/factory.ts (1)
1-7: Initial imports look good.No concerns about the usage of
pdf-liband internal modules here.backend/controller/types.ts (1)
28-30: LGTM! Interface updates align with logging enhancements.The addition of 'log' to the omitted properties in both interfaces is consistent with the logging capabilities being added across models.
Also applies to: 32-36
backend/models/settings.ts (1)
22-22: LGTM! Consistent settings update across services.The addition ensures that both the travel calculator and report printer are updated with the latest distance refund settings.
Also applies to: 128-130
backend/models/expenseReport.ts (2)
26-26: LGTM! Log field added to schema.The log field is properly initialized using the logObject helper with expenseReportStates.
61-61: LGTM! Comprehensive population of log editors.The populate function correctly includes all state log editors using array mapping.
backend/models/healthCareCost.ts (1)
11-11: LGTM! The logging implementation enhances state tracking.The changes introduce a robust logging mechanism that tracks state transitions with timestamps and editor information, which is valuable for audit trails and debugging.
Also applies to: 35-35, 72-72, 102-102
backend/migrations.ts (1)
137-155: LGTM! The migration properly handles historical data.The migration correctly:
- Iterates through non-historic reports
- Constructs log entries from history data
- Uses appropriate MongoDB operations for updates
backend/models/travel.ts (1)
21-21: LGTM! The logging implementation enhances state tracking.The changes introduce a robust logging mechanism that tracks state transitions with timestamps and editor information.
Also applies to: 60-60, 151-151
backend/controller/expenseReportController.ts (1)
5-5: LGTM! The report generation is properly consolidated.The changes consistently replace the old report generation with the new centralized
reportPrinter.printmethod.Also applies to: 143-143, 278-278, 307-307
common/types.ts (1)
375-380: LGTM! Well-designed logging type.The generic
Logtype effectively tracks state changes with proper type safety and essential metadata (date and editor).backend/controller/healthCareCostController.ts (1)
150-154: LGTM! Consistent report generation refactoring.The replacement of legacy report generation with
reportPrinter.printis consistent across all controllers while maintaining proper error handling and response headers.Also applies to: 325-326, 374-375, 415-416
backend/controller/travelController.ts (1)
222-223: LGTM! Unified report generation implementation.The migration to
reportPrinter.printis consistently applied across all travel-related controllers, maintaining the same behavior and error handling patterns.Also applies to: 283-284, 451-452, 504-505
backend/models/travel.ts
Outdated
| this.log[this.state] = { date: new Date(), editor: this.editor } | ||
|
|
||
| if (this.state === 'approved') { | ||
| // move vehicle registration of owner as receipt to 'ownCar' stages | ||
| const receipts = [] | ||
| for (const stage of this.stages) { | ||
| if (stage.transport.type == 'ownCar') { | ||
| if (receipts.length == 0) { | ||
| const owner = await User.findOne({ _id: this.owner._id }).lean() | ||
| if (owner && owner.vehicleRegistration) { | ||
| for (const vr of owner.vehicleRegistration) { | ||
| const doc = await DocumentFile.findOne({ _id: vr._id }).lean() | ||
| delete (doc as unknown as any)._id | ||
| receipts.push(await DocumentFile.create(doc)) | ||
| } | ||
| stage.cost.receipts = receipts | ||
| } | ||
| } | ||
| stage.cost.receipts = receipts | ||
| } | ||
|
|
||
| break | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve vehicle registration handling.
The current implementation has several potential issues:
- Unsafe delete operation
- Potential memory leak from not cleaning up old receipts
- Missing error handling for document creation
Apply this diff to improve the implementation:
if (this.state === 'approved') {
// move vehicle registration of owner as receipt to 'ownCar' stages
const receipts = []
for (const stage of this.stages) {
if (stage.transport.type == 'ownCar') {
if (receipts.length == 0) {
const owner = await User.findOne({ _id: this.owner._id }).lean()
- if (owner && owner.vehicleRegistration) {
+ if (owner?.vehicleRegistration) {
for (const vr of owner.vehicleRegistration) {
const doc = await DocumentFile.findOne({ _id: vr._id }).lean()
- delete (doc as unknown as any)._id
+ const { _id, ...docWithoutId } = doc as unknown as any
try {
- receipts.push(await DocumentFile.create(doc))
+ receipts.push(await DocumentFile.create(docWithoutId))
+ } catch (error) {
+ console.error('Failed to create document:', error)
+ // Clean up any receipts created so far
+ for (const receipt of receipts) {
+ await DocumentFile.deleteOne({ _id: receipt._id })
+ }
+ throw error
+ }
}
}
}
stage.cost.receipts = receipts
}
}
}📝 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.
| this.log[this.state] = { date: new Date(), editor: this.editor } | |
| if (this.state === 'approved') { | |
| // move vehicle registration of owner as receipt to 'ownCar' stages | |
| const receipts = [] | |
| for (const stage of this.stages) { | |
| if (stage.transport.type == 'ownCar') { | |
| if (receipts.length == 0) { | |
| const owner = await User.findOne({ _id: this.owner._id }).lean() | |
| if (owner && owner.vehicleRegistration) { | |
| for (const vr of owner.vehicleRegistration) { | |
| const doc = await DocumentFile.findOne({ _id: vr._id }).lean() | |
| delete (doc as unknown as any)._id | |
| receipts.push(await DocumentFile.create(doc)) | |
| } | |
| stage.cost.receipts = receipts | |
| } | |
| } | |
| stage.cost.receipts = receipts | |
| } | |
| break | |
| } | |
| this.log[this.state] = { date: new Date(), editor: this.editor } | |
| if (this.state === 'approved') { | |
| // move vehicle registration of owner as receipt to 'ownCar' stages | |
| const receipts = [] | |
| for (const stage of this.stages) { | |
| if (stage.transport.type == 'ownCar') { | |
| if (receipts.length == 0) { | |
| const owner = await User.findOne({ _id: this.owner._id }).lean() | |
| if (owner?.vehicleRegistration) { | |
| for (const vr of owner.vehicleRegistration) { | |
| const doc = await DocumentFile.findOne({ _id: vr._id }).lean() | |
| const { _id, ...docWithoutId } = doc as unknown as any | |
| try { | |
| receipts.push(await DocumentFile.create(docWithoutId)) | |
| } catch (error) { | |
| console.error('Failed to create document:', error) | |
| // Clean up any receipts created so far | |
| for (const receipt of receipts) { | |
| await DocumentFile.deleteOne({ _id: receipt._id }) | |
| } | |
| throw error | |
| } | |
| } | |
| } | |
| } | |
| stage.cost.receipts = receipts | |
| } | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 194-194: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 197-197: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
fixes #125
Summary by CodeRabbit
New Features
Refactor
Chores