feat: send reports to organisation email #121
Conversation
WalkthroughThe changes in this pull request involve multiple modifications across various files, primarily focusing on enhancing email functionality and updating method signatures. Key updates include the addition of the Changes
Possibly related PRs
Warning Rate limit exceeded@david-loe has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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: 12
🧹 Outside diff range and nitpick comments (23)
backend/models/organisation.ts (2)
7-7: Consider adding trim and index to reportEmail field.While the email validation is good, consider these improvements:
- Add
trim: trueto remove whitespace from email addresses- Consider adding an index if you'll be querying by this field frequently
- reportEmail: { type: String, validate: emailRegex }, + reportEmail: { type: String, trim: true, validate: emailRegex, index: true },
Line range hint
1-11: Consider centralizing email validation logic.Since email validation is being used here and potentially in other models, consider creating a reusable Mongoose custom validator or SchemaType for email fields to maintain consistency across the application.
Example implementation for a custom SchemaType:
// types/mongoose/email.ts import { Schema } from 'mongoose'; import { emailRegex } from '../../common/types.js'; export const EmailSchemaType = { type: String, trim: true, validate: emailRegex, }; // Usage in schema: reportEmail: EmailSchemaType,backend/models/displaySettings.ts (1)
33-33: LGTM! Schema structure maintains proper validation.The required validation for locale settings is essential, especially in the context of email reporting functionality being added in this PR. This ensures that proper localization settings are always available for generating localized email reports.
backend/authStrategies/magiclogin.ts (1)
21-26: Add JSDoc to document sendMail parametersConsider adding documentation for the
sendMailfunction parameters to improve code maintainability. This will help other developers understand the purpose of each parameter, especially the optional ones./** * Sends an email to the specified users * @param users - Array of users to send the email to * @param subject - Email subject * @param paragraph - Email body paragraph * @param button - Button configuration {text: string, link: string} * @param [optional] - Optional parameter description * @param sendImmediately - Whether to send the email immediately */backend/controller/uploadController.ts (1)
Line range hint
76-76: Enhance file upload security measuresThe current implementation using
fileHandler.any()accepts all file types without restriction. Consider:
- Implementing server-side file type validation
- Adding server-side file size checks
- Scanning uploaded files for malware
- Rate limiting upload requests
Would you like assistance in implementing these security measures?
backend/models/connectionSettings.ts (1)
10-17: Consider adding a default locale valueWhile the schema structure is well-designed, consider adding a default value for the
localefield to ensure backward compatibility and smoother migrations.- locale: { type: String, enum: locales, required: true } + locale: { type: String, enum: locales, required: true, default: 'en' }backend/pdf/advance.ts (2)
Line range hint
7-14: Consider adding input validation and resource management.While the function structure is good, consider these improvements:
- Validate the travel object properties before processing
- Implement proper cleanup for large PDF operations
Consider adding validation:
function validateTravel(travel: TravelSimple): void { if (!travel.project?.organisation) { throw new Error('Invalid travel: missing project organization') } if (!travel.owner?.name) { throw new Error('Invalid travel: missing owner information') } // Add other necessary validations }
Line range hint
123-124: Extract magic numbers into named constants.The fontSize adjustments and spacing calculations use magic numbers that could be extracted into named constants for better maintainability.
Consider this approach:
+ const FONT_SIZE_INCREASE = 2 + const SPACING_MULTIPLIER = 1.25 + const BOTTOM_MARGIN = 16 - const fontSize = options.fontSize + 2 + const fontSize = options.fontSize + FONT_SIZE_INCREASE - tabelOptions.yStart -= fontSize * 1.25 + tabelOptions.yStart -= fontSize * SPACING_MULTIPLIERbackend/mail/mail.ts (2)
33-34: Consider adding JSDoc commentsThe new optional parameters are well-structured, but adding JSDoc comments would improve maintainability by documenting the purpose of these parameters.
+/** + * Sends an email to multiple recipients + * @param recipients - Array of users to receive the email + * @param subject - Email subject + * @param paragraph - Main email content + * @param button - Optional button with text and link + * @param lastParagraph - Optional closing paragraph + * @param authenticateLink - Whether to authenticate the button link + */
37-52: Consider adding error handling for failed emailsThe use of Promise.allSettled is good for handling multiple emails concurrently. However, consider adding error logging or handling for failed email sends.
- return await Promise.allSettled(mailPromises) + const results = await Promise.allSettled(mailPromises) + const failures = results.filter(r => r.status === 'rejected') + if (failures.length > 0) { + console.error(`Failed to send ${failures.length} emails:`, failures) + } + return resultsbackend/pdf/expenseReport.ts (2)
Line range hint
82-226: Consider improving helper functions maintainabilityWhile these functions weren't modified in this PR, here are some suggestions for future improvements:
- Add JSDoc documentation for better code documentation
- Consider creating configuration objects for magic numbers
- Extract repeated layout calculations into shared utilities
Example improvement:
/** * Draws the general information section of the expense report * @param page - The PDF page to draw on * @param expenseReport - The expense report data * @param options - Drawing options including font, position, and formatting * @returns The Y coordinate after drawing */ function drawGeneralInformation(page: pdf_lib.PDFPage, expenseReport: ExpenseReport, options: Options) { // ... existing code } // Consider creating a layout constants object const LAYOUT = { TITLE_SCALE: 1.5, SECTION_SPACING: 1.5, DEFAULT_EDGE: 36 } as const;
Line range hint
18-77: Consider PDF generation performance for email attachmentsSince this PDF generator will now be used in the context of email attachments, consider:
- Implementing PDF size limits
- Adding progress tracking for large reports
- Implementing caching for frequently accessed reports
Would you like me to provide specific implementation suggestions for any of these improvements?
backend/pdf/healthCareCost.ts (1)
Line range hint
18-82: Consider implementing caching for generated PDFsSince this PDF generation will now be used for email sending, consider implementing caching for generated PDFs to:
- Prevent regenerating the same PDF multiple times
- Reduce server load
- Improve response times for repeated requests
You could use a temporary storage solution with an appropriate TTL (Time To Live) for the cached PDFs.
backend/templates/mail.ejs (1)
Line range hint
353-369: Consider adding link security attributes.For enhanced security when rendering external links, consider adding security-related attributes to the anchor tag.
- <td><a href="<%= button.link %>" target="_blank"><%= button.text %></a></td> + <td><a href="<%= button.link %>" target="_blank" rel="noopener noreferrer"><%= button.text %></a></td>backend/controller/expenseReportController.ts (1)
Line range hint
275-284: Consider refactoring for better separation of concernsThe callback function handles multiple responsibilities (notifications, email sending, disk operations). Consider:
- Extracting these operations into separate service methods
- Implementing proper error handling for all async operations
- Adding retry mechanisms for critical operations like email sending
Example refactor structure:
class ExpenseReportService { async handleRefundedReport(expenseReport: IExpenseReport) { await this.sendNotifications(expenseReport); await this.sendReportViaEmail(expenseReport); await this.saveReportToDisk(expenseReport); } private async sendNotifications(report: IExpenseReport) { try { await sendNotificationMail(report); } catch (error) { // Handle error } } private async sendReportViaEmail(report: IExpenseReport) { try { await sendViaMail(report); } catch (error) { // Handle error } } private async saveReportToDisk(report: IExpenseReport) { if (process.env.BACKEND_SAVE_REPORTS_ON_DISK.toLowerCase() !== 'true') { return; } try { const filePath = await writeToDiskFilePath(report); const reportContent = await generateExpenseReportReport( report, i18n.language as Locale ); await writeToDisk(filePath, reportContent); } catch (error) { // Handle error } } }common/types.ts (1)
219-219: Consider adding email validation for reportEmail.The optional
reportEmailfield is appropriately typed. Consider validating this field against theemailRegexpattern that's already defined in this file (line 516) to ensure valid email addresses are stored.- reportEmail?: string | null + /** + * @pattern ${emailRegex} + */ + reportEmail?: string | nullbackend/controller/healthCareCostController.ts (3)
Line range hint
254-261: Refactor duplicate code blocksThe same code block appears in both
postUnderExaminationByInsuranceandpostRefundedmethods. Consider extracting this into a shared utility function to improve maintainability.async function handleReportGeneration(healthCareCost: IHealthCareCost) { sendNotificationMail(healthCareCost); sendViaMail(healthCareCost); if (process.env.BACKEND_SAVE_REPORTS_ON_DISK.toLowerCase() === 'true') { await writeToDisk( await writeToDiskFilePath(healthCareCost), await generateHealthCareCostReport(healthCareCost, i18n.language as Locale) ); } }Also applies to: 375-382
256-256: Improve environment variable handlingThe environment variable check
process.env.BACKEND_SAVE_REPORTS_ON_DISK.toLowerCase() === 'true'could be improved:
- Add null check for the environment variable
- Consider using a utility function for boolean environment variables
function getBooleanEnvVar(key: string, defaultValue = false): boolean { const value = process.env[key]; return value ? value.toLowerCase() === 'true' : defaultValue; }Also applies to: 377-377
Line range hint
257-261: Add error handling for file operationsThe file operations (
writeToDisk) lack error handling. Consider wrapping these operations in try-catch blocks to handle potential file system errors gracefully.try { await writeToDisk( await writeToDiskFilePath(healthCareCost), await generateHealthCareCostReport(healthCareCost, i18n.language as Locale) ); } catch (error) { console.error('Failed to write report to disk:', error); // Consider appropriate error handling strategy }Also applies to: 378-382
backend/pdf/travel.ts (2)
Line range hint
31-43: Add error handling for async operations.The function lacks error handling for async operations which could lead to unhandled promise rejections. Consider adding try-catch blocks to handle potential file reading and font embedding errors gracefully.
export async function generateTravelReport(travel: Travel, language: Locale) { formatter.setLocale(language) - const pdfDoc = await pdf_lib.PDFDocument.create() - pdfDoc.registerFontkit(pdf_fontkit) - const fontBytes = await fs.readFile('../common/fonts/NotoSans-Regular.ttf') - const font = await pdfDoc.embedFont(fontBytes, { subset: true }) + try { + const pdfDoc = await pdf_lib.PDFDocument.create() + pdfDoc.registerFontkit(pdf_fontkit) + const fontBytes = await fs.readFile(path.join(__dirname, '..', 'common', 'fonts', 'NotoSans-Regular.ttf')) + const font = await pdfDoc.embedFont(fontBytes, { subset: true }) + } catch (error) { + console.error('Failed to initialize PDF document:', error) + throw new Error('Failed to generate travel report: ' + error.message) + }
Line range hint
31-600: Consider implementing PDF generation as a background job.Given that PDF generation involves complex operations and file I/O, consider:
- Moving the PDF generation to a background job queue
- Implementing progress tracking for large reports
- Caching generated PDFs temporarily to avoid regeneration
This would improve the user experience and system reliability, especially when sending reports via email.
backend/controller/travelController.ts (1)
Line range hint
450-454: Add error handling and await async operations.The email sending and disk operations need proper error handling and async/await usage.
Apply this fix:
const cb = async (travel: ITravel) => { sendNotificationMail(travel) - sendViaMail(travel) - if (process.env.BACKEND_SAVE_REPORTS_ON_DISK.toLowerCase() === 'true') { - await writeToDisk(await writeToDiskFilePath(travel), await generateTravelReport(travel, i18n.language as Locale)) - } + try { + await sendViaMail(travel) + if (process.env.BACKEND_SAVE_REPORTS_ON_DISK?.toLowerCase() === 'true') { + const report = await generateTravelReport(travel, i18n.language as Locale) + const filePath = await writeToDiskFilePath(travel) + await writeToDisk(filePath, report) + } + } catch (error) { + console.error('Failed to process travel report:', error) + // Consider whether to throw or handle silently + } }common/locales/de.json (1)
100-102: Fix German language conventions in translationsPlease update the translations to follow proper German language conventions:
"locale": "Sprache", - "reportEmail": "Email Adresse für Berichte", - "sendPDFReportsToOrganisationEmail": "PDF Berichte an Organisations-Email senden", + "reportEmail": "E-Mail-Adresse für Berichte", + "sendPDFReportsToOrganisationEmail": "PDF-Berichte an Organisations-E-Mail senden",The changes:
- "Email" → "E-Mail" (standard German spelling)
- "Email Adresse" → "E-Mail-Adresse" (proper compound word with hyphens)
- "PDF Berichte" → "PDF-Berichte" (proper compound word with hyphen)
- "Organisations-Email" → "Organisations-E-Mail" (consistent spelling)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
backend/authStrategies/magiclogin.ts(1 hunks)backend/controller/expenseReportController.ts(2 hunks)backend/controller/healthCareCostController.ts(3 hunks)backend/controller/travelController.ts(3 hunks)backend/controller/uploadController.ts(1 hunks)backend/mail/mail.ts(6 hunks)backend/models/connectionSettings.ts(1 hunks)backend/models/displaySettings.ts(1 hunks)backend/models/organisation.ts(1 hunks)backend/pdf/advance.ts(2 hunks)backend/pdf/expenseReport.ts(2 hunks)backend/pdf/healthCareCost.ts(2 hunks)backend/pdf/helper.ts(2 hunks)backend/pdf/travel.ts(2 hunks)backend/retentionpolicy.ts(1 hunks)backend/templates/mail.ejs(2 hunks)common/locales/de.json(1 hunks)common/locales/en.json(1 hunks)common/types.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/retentionpolicy.ts
🔇 Additional comments (23)
backend/models/organisation.ts (2)
1-2: LGTM! Imports are properly structured.
The imports correctly include all necessary dependencies for the schema definition and email validation.
7-7: Verify if reportEmail should be required.
Since this PR implements report emailing functionality, consider whether reportEmail should be a required field for organizations that need to receive reports.
backend/authStrategies/magiclogin.ts (1)
24-24: Verify the sendMail parameter change across the codebase
The change from empty string to undefined looks correct, but please ensure this aligns with the updated sendMail function signature and that all other call sites have been updated consistently.
backend/controller/uploadController.ts (2)
3-3: Consider using the node: prefix for built-in modules
The change from node:fs/promises to fs/promises removes the explicit built-in module prefix. Node.js documentation recommends using the node: prefix for built-in modules as it:
- Makes the module source explicit
- Prevents module naming conflicts
- Ensures consistent module resolution across Node.js versions
-import fs from 'fs/promises'
+import fs from 'node:fs/promises'Additionally, since this controller handles file operations, ensure that:
- Proper file size limits are enforced
- File type validation is implemented
- Temporary files are cleaned up
- Error handling for file operations is robust
Line range hint 82-92: Consider integrating email notifications for uploads
Given that this PR's objective is to implement report sending via email, consider integrating the new email functionality here. After successful file upload, you might want to:
- Notify relevant organization members about new uploads
- Send confirmation emails to uploaders
- Generate and email reports if applicable
Would you like assistance in implementing this integration with the new sendViaMail function?
backend/models/connectionSettings.ts (2)
6-6: LGTM: Appropriate extension of validation logic
The addition of false to the conditional validation logic correctly handles boolean field dependencies.
10-17: Verify email sending integration
The schema changes look good and align with the PR objective. Please ensure:
- The organization's email address is validated before sending reports
- There's proper error handling if email sending fails
- The selected locale is used correctly in email templates
backend/pdf/advance.ts (1)
1-1: LGTM! Good transition to promise-based fs operations.
The switch from fs to fs/promises is a positive change that prevents blocking I/O operations.
backend/mail/mail.ts (1)
2-2: LGTM! Good improvements to module structure
The switch to fs/promises and export of getClient function improves the module's architecture and promotes better async handling.
Also applies to: 20-28
backend/pdf/expenseReport.ts (1)
1-1: LGTM: Good migration to promise-based fs operations
The change from fs to fs/promises aligns well with modern Node.js best practices for asynchronous file operations.
backend/pdf/healthCareCost.ts (2)
1-1: Good improvement using fs/promises!
The switch from synchronous fs to fs/promises is a positive change that promotes better async/await usage and prevents blocking operations.
Line range hint 84-85: Well-structured options handling!
Good implementation of the options parameter with proper TypeScript typing and default value handling for the edge property.
backend/templates/mail.ejs (2)
Line range hint 353-369: LGTM! Button implementation follows email best practices.
The conditional button implementation follows email template best practices:
- Uses table-based layout for maximum email client compatibility
- Maintains proper semantic structure
- Includes mobile-responsive considerations
Please verify that:
- The
buttonobject is properly validated before being passed to the template - The default styling matches your organization's brand guidelines
Line range hint 370-373: LGTM! Clean implementation of optional paragraph.
The conditional rendering of the last paragraph is implemented cleanly and consistently with the template's existing structure.
backend/controller/expenseReportController.ts (1)
11-11: LGTM: Import statement aligns with new email functionality
The addition of sendViaMail and writeToDiskFilePath imports supports the new email reporting feature.
common/types.ts (1)
77-80: LGTM! Well-structured configuration for PDF email reports.
The new PDFReportsViaEmail configuration in ConnectionSettings is well-typed and logically placed. The boolean flag for enabling/disabling the feature combined with locale setting provides good flexibility for email report customization.
backend/controller/healthCareCostController.ts (2)
19-19: LGTM! Import aligns with PR objectives
The addition of sendViaMail and writeToDiskFilePath imports supports the new email reporting functionality.
254-254: Breaking Change: Method Rename Impact
The renaming of deleteExpenese to deleteExpense is a breaking change that could affect API consumers. Please ensure this change is:
- Documented in the changelog
- Communicated to API consumers
- Considered for backward compatibility if needed
Also applies to: 375-375
backend/pdf/travel.ts (2)
1-1: LGTM! Good switch to async file operations.
The change from synchronous to asynchronous file operations is a good practice that can improve performance by preventing I/O operations from blocking the event loop.
Line range hint 89-600: LGTM! Well-structured helper functions with proper type safety.
The helper functions are well-implemented with:
- Consistent patterns for table drawing
- Strong type safety through TypeScript
- Clear separation of concerns for different report sections
backend/controller/travelController.ts (1)
11-11: LGTM! Import changes align with the feature requirements.
The addition of sendViaMail and writeToDiskFilePath imports supports the new email reporting functionality.
common/locales/en.json (1)
100-102: LGTM! Consistent with British English style.
The new labels are well-structured and maintain consistency with the existing translations, including the use of British English spelling ("organisation").
backend/pdf/helper.ts (1)
80-84: Ensure compliance with data privacy policies when including personal data
The email body includes personal information such as the report owner's name. Verify that including this personal data in emails complies with the organization's data privacy policies and regulations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
backend/data/connectionSettings.development.json (1)
28-28: Consider making the locale configurableThe locale is hardcoded to "de". Consider making this value configurable through environment variables or allowing it to inherit from the system/application default locale settings.
- "locale": "de" + "locale": "${REPORT_LOCALE:-de}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
backend/data/connectionSettings.development.json(1 hunks)backend/db.ts(1 hunks)backend/migrations.ts(1 hunks)
🔇 Additional comments (5)
backend/data/connectionSettings.development.json (2)
26-28: Verify schema compatibility
Please ensure that these new configuration properties align with the connectionSettingsSchema in backend/models/connectionSettings.ts and that the migration script properly initializes these values in production environments.
Line range hint 19-24: Security reminder: Protect sensitive credentials
This development configuration file contains sensitive SMTP credentials. Ensure that:
- Production credentials are not committed to version control
- These development credentials are not used in any public/shared environments
backend/migrations.ts (1)
127-136: LGTM! Migration structure is well-organized.
The migration step correctly initializes the PDFReportsViaEmail settings with safe defaults and reuses the locale from display settings.
backend/db.ts (2)
59-59: LGTM! Type assertion improvement
The use of Partial<IConnectionSettings> improves type safety by explicitly indicating that the development settings may contain a subset of the full configuration.
61-64: Verify email configuration initialization
The empty connection settings initialization looks good, and the variable rename fixes the typo. However, since this PR implements email report functionality:
- Consider if any default email-related settings should be initialized here
- Verify if the
PDFReportsViaEmailproperty mentioned in the AI summary should have a default value in this initialization
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
frontend/src/components/settings/elements/SettingsForm.vue (1)
24-30: Enhance keyboard shortcut handling robustness and user feedback.While the Ctrl+S implementation works, consider these improvements for better user experience and reliability:
- Add form validation check before saving
- Provide visual feedback when save is triggered
- Add debouncing to prevent rapid-fire saves
- Add error handling for undefined form reference
Consider this enhanced implementation:
- ctrlS(event: KeyboardEvent) { - if (event.ctrlKey && event.key === 's') { - event.preventDefault() - if (!event.repeat && this.$refs.form$) { - this.postSettings((this.$refs.form$ as any).data) - } - } - } + ctrlS: debounce(function(event: KeyboardEvent) { + if (event.ctrlKey && event.key === 's') { + event.preventDefault() + if (!event.repeat) { + const form = this.$refs.form$ as any + if (!form) { + console.warn('Form reference not found') + return + } + if (form.validate()) { + this.$root.$emit('show-save-indicator') + this.postSettings(form.data) + } + } + } + }, 300)frontend/src/components/settings/elements/DisplaySettingsForm.vue (2)
25-31: Consider enhancing keyboard shortcut handling for better UX.The keyboard shortcut implementation could be improved in several ways:
- Add visual feedback when the shortcut is triggered
- Support Mac users with Command key (⌘)
- Add error handling for undefined form reference
Consider this implementation:
-ctrlS(event: KeyboardEvent) { - if (event.ctrlKey && event.key === 's') { +ctrlS(event: KeyboardEvent) { + if ((event.ctrlKey || event.metaKey) && event.key === 's') { event.preventDefault() - if (!event.repeat && this.$refs.form$) { + if (!event.repeat) { + if (!this.$refs.form$) { + console.warn('Form reference not found') + return + } this.postDisplaySettings((this.$refs.form$ as any).data) + // Show feedback toast/notification } } }
38-38: Consider scoping the keyboard event listener.Global event listeners can cause memory leaks and might interfere with other components. Consider scoping the event listener to the component's template root.
-document.addEventListener('keydown', this.ctrlS) +this.$el.addEventListener('keydown', this.ctrlS)Then update the cleanup accordingly in
beforeUnmount.frontend/src/components/settings/elements/ConnectionSettingsForm.vue (2)
34-40: Consider improving type safety and user experience.The keyboard shortcut implementation could benefit from the following improvements:
- Replace
anytype assertion with proper typing for the form reference- Add error handling for form submission
- Provide visual feedback when saving
- Consider debouncing save operations
Consider this implementation:
- ctrlS(event: KeyboardEvent) { + async ctrlS(event: KeyboardEvent) { if (event.ctrlKey && event.key === 's') { event.preventDefault() - if (!event.repeat && this.$refs.form$) { - this.postConnectionSettings((this.$refs.form$ as any).data) + if (!event.repeat && this.$refs.form$) { + try { + this.$root.$emit('loading', true) // Show loading state + await this.postConnectionSettings((this.$refs.form$ as VueformInstance).data) + this.$root.$emit('success', this.$t('messages.settingsSaved')) + } catch (error) { + this.$root.$emit('error', this.$t('messages.saveFailed')) + } finally { + this.$root.$emit('loading', false) + } } } }Also, consider adding these TypeScript interfaces at the top of the file:
interface VueformInstance { data: ConnectionSettings; load: (data: ConnectionSettings) => void; }
Security measures are partially implemented, but some improvements needed
The codebase already implements several security measures:
- Rate limiting is configured in
app.tsfor POST requests- Input validation is implemented through Mongoose schema in
connectionSettings.tswith detailed validation rules- CORS protection is enabled with specific origin URLs
- Session management with secure cookie settings
- Pre-validation hooks for SMTP and LDAP configurations
However, some recommended improvements:
- Add CSRF protection tokens for the admin endpoints
- Consider implementing request sanitization before validation
- Add specific rate limiting for the admin endpoints (current rate limiting is global)
🔗 Analysis chain
Line range hint
16-32: Consider adding security measures for sensitive settings.The
postConnectionSettingsmethod handles sensitive connection data (SMTP, LDAP, Microsoft auth). Consider implementing:
- Input validation before sending data
- Rate limiting for save operations
- CSRF protection for the admin endpoint
- Sanitization of connection settings before submission
Let's verify if these security measures are implemented elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for CSRF protection implementation rg -l "csrf|xsrf" --type vue --type ts # Check for rate limiting implementation rg -l "rate.?limit|throttle" --type vue --type ts # Check for input validation rg -l "validate|sanitize" --type vue --type tsLength of output: 225
Script:
#!/bin/bash # Check for CSRF protection implementation rg -l "csrf|xsrf" # Check for rate limiting implementation rg -l "rate.?limit|throttle" # Check for input validation in connection settings related files rg -l "validate|sanitize|schema" -g "*connection*" # Look for security middleware or interceptors rg -l "middleware|interceptor" # Check for axios or fetch configuration files that might contain security headers fd -g "*api*" -g "*http*" -g "*axios*" -g "*fetch*" -t f | xargs catLength of output: 531
Script:
#!/bin/bash # Check rate limiting implementation in app.ts cat backend/app.ts # Check connection settings validation schema cat backend/models/connectionSettings.ts # Let's try a different approach for API/HTTP config files fd "api|http|axios|fetch" -t f | xargs cat # Check if there's any security middleware in the backend fd -t f -e ts -p "middleware" | xargs catLength of output: 7075
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
frontend/src/components/settings/elements/ConnectionSettingsForm.vue(1 hunks)frontend/src/components/settings/elements/DisplaySettingsForm.vue(1 hunks)frontend/src/components/settings/elements/SettingsForm.vue(1 hunks)
🔇 Additional comments (3)
frontend/src/components/settings/elements/SettingsForm.vue (1)
Line range hint 1-52: Verify the relationship between settings and email functionality.
While this file handles general settings, the PR's objective is about sending reports to organization email. It would be helpful to understand how these settings relate to the email functionality.
frontend/src/components/settings/elements/DisplaySettingsForm.vue (2)
44-51: Verify the purpose of adding submit button ID.
The addition of id: 'submit-button' seems unrelated to the PR's objective of sending reports via email. Please clarify if this ID is needed for specific functionality or testing purposes.
Line range hint 1-62: Clarify relationship with email reporting feature.
The changes in this file (keyboard shortcuts and button ID) seem unrelated to the PR's objective of "sending reports to organisation email". Please clarify how these changes contribute to the email reporting feature.
frontend/src/components/settings/elements/DisplaySettingsForm.vue
Outdated
Show resolved
Hide resolved
frontend/src/components/settings/elements/ConnectionSettingsForm.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
frontend/src/components/settings/elements/OrganisationList.vue (1)
Line range hint
82-92: Add email validation before saving organization dataThe postOrganisation method should validate the reportEmail field before saving to ensure it contains a valid email address.
Add email validation:
async postOrganisation(organisation: Organisation) { + if (organisation.reportEmail && !this.isValidEmail(organisation.reportEmail)) { + this.$root.notify('error', this.$t('errors.invalidEmail')) + return + } let headers = {} if (organisation.logo && organisation.logo.data) { headers = { 'Content-Type': 'multipart/form-data' } } const result = await this.$root.setter<Organisation>('admin/organisation', organisation, { headers }) if (result.ok) { this.getOrganisations() this._showForm = false } this.organisationToEdit = undefined }Add the validation helper method:
isValidEmail(email: string): boolean { const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/ return emailRegex.test(email) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
backend/models/organisation.ts(1 hunks)frontend/src/components/settings/elements/OrganisationList.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/models/organisation.ts
🔇 Additional comments (2)
frontend/src/components/settings/elements/OrganisationList.vue (2)
Line range hint 58-67: LGTM: Interface and data structure changes are well implemented
The Filter interface and corresponding data properties are properly structured to support email filtering.
Line range hint 1-1: Verify integration with email sending feature
Let's verify that this component properly integrates with the email sending functionality mentioned in the PR objectives.
✅ Verification successful
Integration with email sending feature is properly implemented
The verification shows that the email functionality is correctly integrated:
- The
reportEmailfield is properly defined in the organization model (backend/models/organisation.ts) - Email sending implementation exists in
backend/mail/mail.tsandbackend/pdf/helper.ts - The
reportEmailfield is used inbackend/pdf/helper.tsto send reports to the organization's email address - The UI component correctly displays and manages the
reportEmailfield with proper translations in place
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for files that implement the email sending functionality
echo "Searching for email sending implementation..."
rg -l "sendViaMail|sendMail" --type vue --type ts
# Search for usage of reportEmail field
echo "Searching for reportEmail usage..."
rg "reportEmail" --type vue --type ts
Length of output: 325
Script:
#!/bin/bash
# Search for files that implement the email sending functionality
echo "Searching for email sending implementation..."
rg -l "sendViaMail|sendMail"
# Search for usage of reportEmail field
echo "Searching for reportEmail usage..."
rg "reportEmail"
# Look for Vue components and TypeScript files
echo "Listing Vue and TypeScript files..."
fd "\.(vue|ts)$"
Length of output: 7219
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
frontend/src/i18n.ts (1)
13-15: Consider improving locale fallback strategyUsing the same locale for both
localeandfallbackLocalemight not provide the best user experience. Consider:
- Using a more widely-used language (e.g., 'en') as the fallback
- Using an array of fallback locales for better coverage
Also, the
anytype parameter increateI18n<any, Locale>could be more specific to improve type safety.-export default createI18n<any, Locale>({ +export default createI18n<{[K in Locale]: typeof defaultMessages[K]}, Locale>({ legacy: false, locale: defaultLocale, - fallbackLocale: defaultLocale, + fallbackLocale: ['en', defaultLocale], messages: defaultMessages, globalInjection: true })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
backend/db.ts(1 hunks)backend/models/connectionSettings.ts(2 hunks)backend/models/displaySettings.ts(2 hunks)common/types.ts(3 hunks)frontend/src/formatter.ts(1 hunks)frontend/src/i18n.ts(1 hunks)frontend/src/vueform.config.ts(2 hunks)frontend/vue.config.ts(0 hunks)
💤 Files with no reviewable changes (1)
- frontend/vue.config.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/db.ts
- backend/models/displaySettings.ts
- common/types.ts
🔇 Additional comments (10)
frontend/src/formatter.ts (2)
3-3: Verify the defaultLocale constant exists
Let's ensure the defaultLocale constant is properly exported from the common/types module.
✅ Verification successful
Import of defaultLocale is correctly implemented
The constant defaultLocale is properly exported from common/types.ts with the value 'de' as a typed constant (Locale type), making this a valid and safe import.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the defaultLocale export in common/types
# Expected: Find the defaultLocale constant declaration
# Search for defaultLocale export
ast-grep --pattern 'export const defaultLocale = $_'
# Backup search using ripgrep
rg -p "export.*defaultLocale" common/types.ts
Length of output: 148
7-7: LGTM! Note: Breaking change in locale initialization
The change from hardcoded 'de' to defaultLocale improves flexibility in locale management. However, this is a breaking change that affects the entire application's default localization behavior. Ensure this aligns with the expected default locale in the backend/models/displaySettings.ts changes.
frontend/src/i18n.ts (3)
6-8: LGTM! Clean implementation of locale messages
The implementation follows Vue i18n best practices by directly mapping locale codes to their respective message bundles.
2-3: Verify completeness of translations
Ensure both locale files contain all required translations to prevent missing translation keys.
2-3: Verify TypeScript version compatibility
The JSON import assertions (with { type: 'json' }) require TypeScript 4.5 or higher.
✅ Verification successful
TypeScript version is compatible with JSON import assertions
The codebase is using TypeScript version ^5.6.3, which fully supports the with { type: 'json' } import assertion syntax (required TS 4.5+). Additionally, the tsconfig.json files confirm that JSON module resolution is enabled via "resolveJsonModule": true. The syntax usage is correct and compatible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check TypeScript version in package.json
# Expected: Version should be >= 4.5
jq -r '.dependencies.typescript, .devDependencies.typescript' package.json 2>/dev/null | grep -v null
Length of output: 91
Script:
#!/bin/bash
# Let's try different approaches to find TypeScript version:
# 1. Look for TypeScript version in package.json with less strict path
fd package.json --type f --exec jq -r '.dependencies.typescript, .devDependencies.typescript' {} 2>/dev/null
# 2. Look for tsconfig.json to confirm TypeScript usage
fd tsconfig.json --type f --exec cat {} 2>/dev/null
# 3. Look for any TypeScript configuration files
fd -e json -e jsonc --type f -d 1 | grep -i ts
Length of output: 1134
frontend/src/vueform.config.ts (2)
6-6: LGTM! Clean import of defaultLocale
The import statement is well-placed and follows a logical import ordering.
42-42: Verify defaultLocale configuration
The change from hardcoded locale to defaultLocale improves maintainability. However, let's verify that the default locale value is properly set and matches the available locales.
✅ Verification successful
Default locale configuration is properly set and consistently used
The verification confirms that:
defaultLocaleis correctly defined as 'de' incommon/types.ts- It's consistently imported and used across both frontend and backend:
- Frontend: Used in vueform config, formatter, and i18n setup
- Backend: Used as default value in connection and display settings models
- The usage aligns with the type system (
Localetype) and is properly integrated with locale-related configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify defaultLocale configuration and usage
# Expected results:
# 1. defaultLocale should be defined as either 'de' or 'en'
# 2. defaultLocale should be consistently used across the application
# Check defaultLocale definition
echo "Checking defaultLocale definition:"
rg -A 1 "defaultLocale.*=.*['\"](?:de|en)['\"]" "common/types.ts"
# Check other uses of defaultLocale
echo -e "\nChecking defaultLocale usage:"
rg "defaultLocale" --type ts
Length of output: 1476
backend/models/connectionSettings.ts (3)
6-6: LGTM: Enhanced validation logic
The addition of false to the validation conditions properly handles boolean fields in the conditional validation logic.
105-106: LGTM: Safe auth property enhancement
Making the auth property required with a default empty object is a good practice. It ensures consistent document structure while maintaining backward compatibility.
Line range hint 1-107: Verify database migration for schema changes
The schema changes require existing documents to be updated with the new required fields.
Let's verify if proper migrations are in place:
✅ Verification successful
Migration is properly handled for PDFReportsViaEmail field
The migration for the PDFReportsViaEmail field is properly implemented in backend/migrations.ts. Specifically, in version 1.4.1 migration:
if (semver.lte(migrateFrom, '1.4.1')) {
console.log('Apply migration from v1.4.1: Add PDFReportsViaEmail Settings')
const displaySettings = await mongoose.connection.collection('displaysettings').findOne({})
await mongoose.connection
.collection('connectionsettings')
.updateOne(
{},
{ $set: { PDFReportsViaEmail: { sendPDFReportsToOrganisationEmail: false, locale: displaySettings?.locale?.default || 'de' } } }
)
}The migration:
- Sets default values for all required fields
- Uses the display settings locale or falls back to 'de'
- Matches the schema definition in
connectionSettings.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for migration handling of the new PDFReportsViaEmail field
# Look for migration files that handle the new field
rg -l "PDFReportsViaEmail" --type ts
# Check for any mongoose migration scripts
fd -e ts -e js migration
Length of output: 182
Script:
#!/bin/bash
# Let's check the content of migrations.ts to verify if it handles the PDFReportsViaEmail field
cat backend/migrations.ts
# Also check the default values and types definition
cat common/types.ts
Length of output: 20268
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
backend/templates/cache.ts (1)
1-4: Consider adding error recovery mechanism for cached templatesThe current implementation might return stale data if templates are modified or deleted after initial caching. Consider adding a mechanism to detect and reload templates when they become invalid.
import fs from 'fs/promises' -let mail: string | null = null -let upload: string | null = null +interface CachedTemplate { + content: string | null; + lastModified?: number; +} + +let mail: CachedTemplate = { content: null }; +let upload: CachedTemplate = { content: null };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
backend/controller/uploadController.ts(2 hunks)backend/mail/mail.ts(5 hunks)backend/templates/cache.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/controller/uploadController.ts
- backend/mail/mail.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
backend/pdf/helper.ts (2)
86-98: Improve type safety for email optionsThe email options should be type-safe to prevent runtime errors. Consider creating an interface for the email options.
Add this interface before the function:
interface EmailOptions { from: string to: string subject: string text: string attachments: Array<{ content: Buffer contentType: string filename: string }> }Then use it in the
sendMailcall:-return await mailClient.sendMail({ +const emailOptions: EmailOptions = { from: '"' + appName + '" <' + mailClient.options.from + '>', to: org?.reportEmail, subject: subject, text, attachments: [ { content: Buffer.from(pdf), contentType: 'application/pdf', filename: 'report.pdf' } ] -}) +} +return await mailClient.sendMail(emailOptions)
59-75: Extract PDF generation logic into a separate functionThe PDF generation logic could be extracted into a separate function to improve readability and reusability.
Apply this diff to extract the logic:
+async function generateReportPDF( + report: Travel | ExpenseReport | HealthCareCost, + lng: string +): Promise<{ pdf: Uint8Array; subject: string }> { + let subject = '🧾 ' + let pdf: Uint8Array + + if (reportIsTravel(report)) { + if (report.state == 'refunded') { + subject = subject + i18n.t('labels.travel', { lng }) + pdf = await generateTravelReport(report, lng) + } else { + subject = subject + i18n.t('labels.advance', { lng }) + pdf = await generateAdvanceReport(report, lng) + } + } else if (reportIsHealthCareCost(report)) { + subject = subject + i18n.t('labels.healthCareCost', { lng }) + pdf = await generateHealthCareCostReport(report, lng) + } else { + subject = subject + i18n.t('labels.expenseReport', { lng }) + pdf = await generateExpenseReportReport(report, lng) + } + + return { pdf, subject } +} export async function sendViaMail(report: Travel | ExpenseReport | HealthCareCost) { try { const connectionSettings = await getConnectionSettings() if (connectionSettings.PDFReportsViaEmail.sendPDFReportsToOrganisationEmail) { const org = await Organisation.findOne({ _id: report.project.organisation._id }) if (org?.reportEmail) { const mailClient = await getClient() const lng = connectionSettings.PDFReportsViaEmail.locale - let subject = '🧾 ' - let pdf: Uint8Array - if (reportIsTravel(report)) { - if (report.state == 'refunded') { - subject = subject + i18n.t('labels.travel', { lng }) - pdf = await generateTravelReport(report, lng) - } else { - subject = subject + i18n.t('labels.advance', { lng }) - pdf = await generateAdvanceReport(report, lng) - } - } else if (reportIsHealthCareCost(report)) { - subject = subject + i18n.t('labels.healthCareCost', { lng }) - pdf = await generateHealthCareCostReport(report, lng) - } else { - subject = subject + i18n.t('labels.expenseReport', { lng }) - pdf = await generateExpenseReportReport(report, lng) - } + const { pdf, subject } = await generateReportPDF(report, lng)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
backend/pdf/helper.ts(2 hunks)
🔇 Additional comments (1)
backend/pdf/helper.ts (1)
17-25: LGTM! Well-organized imports
The new imports are logically grouped and provide the necessary functionality for email sending and PDF generation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
frontend/src/components/settings/elements/DisplaySettingsForm.vue (3)
2-7: Refactor keyboard shortcut handler for better maintainability.The inline keyboard event handler is complex and contains repeated type casting. Consider extracting it to a method for better maintainability and reusability.
<Vueform :schema="schema" ref="form$" :endpoint="false" @submit="(form$: any) => postDisplaySettings(form$.data)" - @keydown.ctrl.s.prevent="(e: KeyboardEvent) => {e.repeat ? null: postDisplaySettings(($refs.form$ as any).data)}"></Vueform> + @keydown.ctrl.s.prevent="handleShortcutSave"></Vueform> + // Add to methods: + handleShortcutSave(e: KeyboardEvent): void { + if (!e.repeat) { + const form = this.$refs.form$ as any; + this.postDisplaySettings(form.data); + } + }
Line range hint
22-28: Add error handling and loading state management.The
postDisplaySettingsmethod lacks error handling and loading state management. Also, the type safety could be improved.+ interface ApiResponse<T> { + ok?: T; + error?: string; + } - async postDisplaySettings(displaySettings: DisplaySettings) { + async postDisplaySettings(displaySettings: DisplaySettings) { + try { + this.isLoading = true; const result = await this.$root.setter<DisplaySettings>('admin/displaySettings', displaySettings) if (result.ok) { this.displaySettings = result.ok ;(this.$refs.form$ as any).load(this.displaySettings) + } else { + this.$emit('error', result.error || 'Failed to save settings') + } + } catch (error) { + this.$emit('error', 'An unexpected error occurred') + } finally { + this.isLoading = false + } }
37-44: Enhance button accessibility attributes.While adding an id is good, consider enhancing the button's accessibility by adding ARIA attributes and a more descriptive label.
submit: { type: 'button', submits: true, buttonLabel: this.$t('labels.save'), full: true, columns: { container: 6 }, - id: 'submit-button' + id: 'submit-button', + attrs: { + 'aria-label': this.$t('labels.save_display_settings'), + 'data-test': 'display-settings-submit' + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
frontend/src/components/settings/elements/ConnectionSettingsForm.vue(2 hunks)frontend/src/components/settings/elements/DisplaySettingsForm.vue(2 hunks)frontend/src/components/settings/elements/SettingsForm.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/settings/elements/ConnectionSettingsForm.vue
- frontend/src/components/settings/elements/SettingsForm.vue
fixes #117
Summary by CodeRabbit
Release Notes
New Features
sendViaMailto send reports via email.reportEmailfield to organization settings for storing email addresses related to reports.PDFReportsViaEmailsettings for managing PDF report email configurations.Bug Fixes
deleteExpenesetodeleteExpensefor consistency across controllers.Improvements
Documentation