Skip to content

feat: add daily update and quotation service modules. - #48

Merged
JahnaviVeera merged 1 commit into
spotmies:masterfrom
JahnaviVeera:master
Mar 12, 2026
Merged

JahnaviVeera merged 1 commit into
spotmies:masterfrom
JahnaviVeera:master

Conversation

@JahnaviVeera

Copy link
Copy Markdown
Collaborator

No description provided.

@JahnaviVeera
JahnaviVeera merged commit 4356376 into spotmies:master Mar 12, 2026
1 check passed
@github-actions

Copy link
Copy Markdown

🤖 AI Code Review

🤖 AI Code Review

📌 Summary

  • The changes improve flexibility and handle edge cases better, but there are some potential issues with maintainability, error handling, and clarity.
  • A few areas could benefit from additional validation or refactoring for better readability and robustness.

💬 Inline Comments (File-wise)

src/modules/daily-updates/daily-updates.services.ts

  • Line 119: Changing the default statusEnum from DailyUpdateStatus.draft to DailyUpdateStatus.pending alters the behavior of the system. Ensure this change aligns with business requirements and does not introduce unintended side effects.

    • ✅ Suggestion: Add a comment explaining why the default status was changed to pending to help future developers understand the rationale.
  • Line 137: The comment mentions "we don't use [draft] anymore by default," but the code still checks for DailyUpdateStatus.draft. This could lead to confusion.

    • ✅ Suggestion: If draft is truly deprecated, consider removing it from the codebase or adding a deprecation warning.
  • Lines 1051-1063: The logic for determining finalStatus is becoming complex and harder to follow, especially with nested conditions.

    • ✅ Suggestion: Refactor this block into a separate function, such as determineFinalStatus(dailyUpdate), to improve readability and maintainability.
  • Line 1053: The comment "If it was just pending (normal update), customer feedback approves it immediately" is helpful, but the logic assumes dailyUpdate.status will always be valid. If dailyUpdate.status is undefined or invalid, this could lead to unexpected behavior.

    • ✅ Suggestion: Add validation to ensure dailyUpdate.status is a valid DailyUpdateStatus before proceeding.

src/modules/quotations/quotations.services.ts

  • Line 11: Changing projectId to be optional (projectId?: string | null) introduces flexibility but also increases the risk of null/undefined values causing issues downstream.

    • ✅ Suggestion: Add validation to ensure that projectId is either provided or handled appropriately in all cases.
  • Lines 54-63: The logic for checking if a project exists is now conditional on data.projectId. While this is an improvement, the error message when projectId is missing is less clear.

    • ✅ Suggestion: Throw a specific error if data.projectId is null or undefined, such as throw new Error("Project ID is required to create a quotation").
  • Line 83: The optional chaining (projectExists?.customer) is a good addition, but the logic for determining userIdToUse could fail silently if projectExists is null.

    • ✅ Suggestion: Add a fallback or log a warning if userIdToUse cannot be determined.
  • Line 98: The notification message now handles the case where projectExists is null, but the fallback message ("New quotation received.") is generic and might not provide enough context to the user.

    • ✅ Suggestion: Include more details in the fallback message, such as the quotation ID or customer name, to make it more informative.

⚠️ High-Risk Issues

  1. daily-updates.services.ts (Line 1053): The logic assumes dailyUpdate.status is always valid, which could lead to undefined behavior if the status is invalid or missing.

    • ✅ Suggestion: Add validation to ensure dailyUpdate.status is a valid DailyUpdateStatus before proceeding.
  2. quotations.services.ts (Line 11): Making projectId optional increases the risk of null/undefined values causing issues downstream.

    • ✅ Suggestion: Add validation to ensure projectId is either provided or handled appropriately in all cases.

By addressing the above issues, the code will become more robust, maintainable, and aligned with expected behavior. Let me know if you need further clarification or assistance!


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

1 participant