feat: implement expense management module with receipt upload, new Pr…

[JahnaviVeera]

…isma schema, and payments service.

[github-actions]

🤖 AI Code Review

🤖 AI Code Review

📌 Summary

• The documentation is comprehensive and well-structured, covering workflows, endpoints, and payloads for various features.
• Found potential issues related to security, maintainability, and clarity in the API design and workflows.
• Suggestions provided to improve security, error handling, and endpoint consistency.

---

💬 Inline Comments (File-wise)

.agents/workflows/shr-homes-application-workflow.md

• Line 15: The Base URL is hardcoded as http://localhost:3000/api. This could lead to confusion in production environments.

  • ✅ Suggestion: Use environment variables or placeholders (e.g., <BASE_URL>), and clarify that this is for local development.

• Line 25: The Roles & Permissions Matrix does not specify how unauthorized access is handled for restricted actions.

  • ✅ Suggestion: Add a note on how unauthorized access is managed (e.g., HTTP 403 responses or role-based middleware).

• Line 70: JWT tokens are stored in localStorage, which is vulnerable to XSS attacks.

  • ✅ Suggestion: Consider using HttpOnly cookies for storing tokens to mitigate XSS risks.

• Line 75: The token refresh endpoint (/auth/refresh) does not specify how refresh tokens are secured or stored.

  • ✅ Suggestion: Clarify whether refresh tokens are stored securely (e.g., HttpOnly cookies) and how they are invalidated upon logout.

• Line 120: The DELETE /user/:userId endpoint allows deletion of leads but does not mention safeguards against accidental deletions.

  • ✅ Suggestion: Implement soft deletes or confirmation dialogs to prevent accidental data loss.

• Line 150: The Convert to Customer Flow mentions invalidating queries but does not specify how errors (e.g., network issues) are handled during this process.

  • ✅ Suggestion: Add error handling mechanisms for failed API calls, such as retry logic or user notifications.

• Line 250: The Customer Dashboard Stats Response includes sensitive financial data (paidAmount, pendingAmount) but does not mention encryption or secure transmission.

  • ✅ Suggestion: Ensure all sensitive data is transmitted over HTTPS and consider encrypting financial data at rest.

• Line 400: The Supervisor Creation Flow does not mention validation for duplicate supervisors or projects.

  • ✅ Suggestion: Add validation checks to prevent duplicate supervisor accounts or project assignments.

• Line 500: The Project Fields include expectedCompletion but do not specify how delays or changes are handled.

  • ✅ Suggestion: Add fields or workflows for tracking delays and updating completion dates.

• Line 650: The Daily Update Approval Flow mentions email and WebSocket notifications but does not specify fallback mechanisms for failed notifications.

  • ✅ Suggestion: Implement retry logic or alternative notification methods (e.g., SMS) for critical updates.

• Line 700: File uploads for daily updates (image, video) are stored in Supabase Storage but do not mention size limits or file type restrictions.

  • ✅ Suggestion: Define size limits and allowed file types to prevent abuse or storage overflow.

• Line 850: The Quotation Flow mentions file attachments but does not specify validation for file types or sizes.

  • ✅ Suggestion: Add validation for file uploads to ensure only supported formats (e.g., PDF) are allowed.

• Line 900: The Payments Workflow mentions multi-mode payments but does not specify how partial payments or refunds are handled.

  • ✅ Suggestion: Clarify workflows for handling partial payments, refunds, or disputes.

---

⚠️ High-Risk Issues

• JWT Storage in localStorage: Storing JWT tokens in localStorage exposes them to XSS attacks, which can compromise user accounts.

  • ✅ Suggestion: Use HttpOnly cookies for token storage to mitigate XSS risks.

• Hardcoded Base URL: Using a hardcoded http://localhost:3000/api could lead to accidental exposure of sensitive endpoints in production.

  • ✅ Suggestion: Replace with environment variables or placeholders to ensure flexibility and security.

• File Upload Validation: Lack of validation for file uploads (e.g., size, type) could lead to storage abuse or security vulnerabilities.

  • ✅ Suggestion: Implement strict validation rules for file uploads and enforce size limits.

• Sensitive Data Transmission: Financial data (paidAmount, pendingAmount) is included in API responses without mention of encryption or secure transmission.

  • ✅ Suggestion: Ensure all sensitive data is transmitted over HTTPS and encrypted at rest.

---

By addressing these issues, the documentation and workflows can be made more secure, maintainable, and robust. Let me know if further clarification is needed!

---

🤖 AI Code Review

📌 Summary

• Found several maintainability and consistency issues in the API design and payloads.
• Identified potential bugs and high-risk areas related to validation, data integrity, and security.
• Suggested improvements for better clarity, validation, and error handling.

---

💬 Inline Comments (File-wise)

Payment Payload (MultiMode)

• Line 8: The paymentBreakup field is a stringified JSON array, which is inconsistent with the rest of the payload structure (e.g., category in expenses uses a proper JSON array).

  • ✅ Suggestion: Use a proper JSON array instead of a stringified JSON to avoid unnecessary parsing and potential errors.

• Line 12: referenceNumber is set to null for MultiMode payments, but validation rules indicate it should be validated per breakup entry. This could lead to confusion or validation bypass.

  • ✅ Suggestion: Remove referenceNumber from the main payload for MultiMode payments and rely solely on paymentBreakup validation.

Payment Response Fields

• Line 28: Duplicate fields receivedBy, recievedBy, and recievedby are included to accommodate frontend spelling variations. This introduces redundancy and potential confusion.

  • ✅ Suggestion: Standardize the field name (receivedBy) and enforce consistent usage across frontend and backend. Avoid aliases in the API response.

• Line 31: paymentDate uses the format DD-MM-YYYY, which is inconsistent with the YYYY-MM-DD format used elsewhere (e.g., expenses payload).

  • ✅ Suggestion: Use a consistent date format (YYYY-MM-DD) across all APIs for better interoperability.

Expenses Workflow

• Line 80: The category field in the expense creation payload is a stringified JSON array, which is inconsistent with the rest of the payload structure.

  • ✅ Suggestion: Use a proper JSON array instead of a stringified JSON to avoid unnecessary parsing and potential errors.

• Line 108: receiptUrl is a permanent public URL, which could expose sensitive data if not properly managed.

  • ✅ Suggestion: Implement access control or token-based authentication for sensitive files, even if they are public URLs.

Materials Management Workflow

• Line 140: Duplicate endpoint /material/getallmaterials is provided as an alias for /material. This redundancy could lead to confusion and maintenance overhead.
  • ✅ Suggestion: Remove the alias endpoint and standardize the usage of /material.

Real-time Events (WebSocket)

• Line 230: Duplicate event names (PAYMENT_RECEIVED and payment_created, PAYMENT_UPDATED and payment_updated) are used for similar triggers. This redundancy could lead to confusion.
  • ✅ Suggestion: Consolidate event names to avoid duplication (e.g., use payment_created and payment_updated consistently).

File Upload Architecture

• Line 270: Permanent public URLs for file storage could expose sensitive data and pose security risks.
  • ✅ Suggestion: Implement signed URLs with expiration or token-based access for sensitive files.

---

⚠️ High-Risk Issues

1. Duplicate Fields in Payment Response:

   • The inclusion of receivedBy, recievedBy, and recievedby introduces redundancy and risks data integrity issues.
   • ✅ Fix: Standardize the field name (receivedBy) and enforce consistent usage across frontend and backend.

2. Inconsistent Date Formats:

   • paymentDate uses DD-MM-YYYY, while other APIs use YYYY-MM-DD. This inconsistency can lead to parsing errors and interoperability issues.
   • ✅ Fix: Use YYYY-MM-DD format consistently across all APIs.

3. Public URLs for Sensitive Files:

   • Permanent public URLs for file storage (e.g., receipts, documents) could expose sensitive data.
   • ✅ Fix: Implement signed URLs with expiration or token-based access for sensitive files.

4. Stringified JSON Arrays:

   • Fields like paymentBreakup and category use stringified JSON arrays, which are prone to parsing errors and inconsistent handling.
   • ✅ Fix: Use proper JSON arrays instead of stringified JSON.

5. Duplicate WebSocket Event Names:

   • Events like PAYMENT_RECEIVED and payment_created are redundant and could lead to confusion.
   • ✅ Fix: Consolidate event names for clarity and consistency.

---

Additional Notes

• Consider adding stricter validation rules for fields like referenceNumber to ensure compliance with the specified formats.
• Review the use of aliases and redundant endpoints to reduce maintenance overhead and improve API clarity.
• Ensure consistent error handling and validation across all workflows to prevent unexpected behavior.

---

🤖 AI Code Review

📌 Summary

• The diff introduces new API endpoints, schema updates, and backend logic enhancements for handling payments, expenses, and receipts.
• Overall, the code is well-structured, but there are areas for improvement in security, maintainability, and error handling.

---

💬 Inline Comments (File-wise)

prisma/schema.prisma

• Line 446: The referenceNumber field is added with a String type and a maximum length of 20 characters. Consider validating this field at the database level to ensure it adheres to the expected format (e.g., numeric-only for certain payment modes).
  • ✅ Suggestion: Add a database constraint or validation logic to enforce the format directly.

src/modules/expense/expense.controller.ts

• Line 148: The fetch call to retrieve the receipt file from a public URL lacks timeout handling. If the external service is slow or unresponsive, this could lead to prolonged request times.
  • ✅ Suggestion: Use a timeout mechanism (e.g., AbortController) to limit the waiting time for the fetch call.
• Line 151: The error message returned when the receipt fetch fails exposes internal details (fetchResponse.statusText). This could be a security risk.
  • ✅ Suggestion: Return a generic error message to avoid exposing internal details.

src/modules/expense/expense.routes.ts

• Line 48: The /receipt endpoint is added before /expenseId to avoid route collision. This is a good practice, but ensure that the route ordering is documented clearly to prevent future confusion.
  • ✅ Suggestion: Add comments or documentation explaining the route precedence.

src/modules/expense/expense.services.ts

• Line 36: The validation logic for referenceNumber assumes specific formats for payment modes. While this is good, consider centralizing the validation logic to avoid duplication across services.
  • ✅ Suggestion: Move the validation logic to a shared utility function or module.
• Line 145: The receiptUrl is exposed directly to the client. Ensure that this URL is sanitized and does not expose sensitive information.
  • ✅ Suggestion: Validate or sanitize the receiptUrl before returning it to the client.

src/modules/payments/payments.services.ts

• Line 53: The validateReferenceNumber function is introduced, but it does not handle edge cases like null or undefined paymentMethod. This could lead to runtime errors.
  • ✅ Suggestion: Add a fallback or default value for paymentMethod to ensure robustness.
• Line 88: Multiple variations of receivedBy are handled to account for typos. While this is helpful for backward compatibility, it could lead to confusion in the long term.
  • ✅ Suggestion: Deprecate incorrect variations (recievedBy, receivedby, etc.) and enforce a single consistent field name (receivedBy).

---

⚠️ High-Risk Issues

1. Security Risk in Receipt Proxy Endpoint:

   • The receipt proxy endpoint (getReceiptByExpenseId) exposes CORS headers and fetches files from a public URL without sanitization or validation. This could lead to unauthorized access or abuse.
   • ✅ Suggestion: Implement stricter validation for receiptUrl and ensure that only authorized users can access the endpoint.

2. Error Handling in External Fetch Calls:

   • The fetch call in getReceiptByExpenseId does not handle network errors or timeouts effectively, which could lead to degraded performance or unresponsive endpoints.
   • ✅ Suggestion: Use AbortController or a similar mechanism to enforce timeouts and handle network errors gracefully.

3. Potential Data Integrity Issues in Payment Reference Validation:

   • The validateReferenceNumber function assumes specific formats but does not enforce these constraints at the database level. This could lead to inconsistent data.
   • ✅ Suggestion: Add database-level constraints or validations to ensure data integrity.

---

🛠️ Additional Suggestions

• API Documentation: Ensure that all new endpoints are documented in Swagger or similar tools, especially for complex routes like /daily-updates and /payments.
• Testing: Add unit tests for the new validation logic (validateReferenceNumber) and endpoint functionality to ensure reliability.
• Deprecation Notices: For fields like recievedBy, consider adding warnings or logs to notify developers about the upcoming removal of deprecated variations.

Let me know if you need further clarification or assistance!

---