Ensure date range validation logic runs before report syncing logic #3505
Conversation
📝 WalkthroughWalkthroughThe changes update event handler decorators in two handler classes to include an array with a numeric value, likely indicating event handling priority. Additionally, the logic for syncing progress reports to engagements now includes explicit validation and error handling for invalid date intervals, with improved error reporting and exception management. Changes
✨ 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: 0
🧹 Nitpick comments (1)
src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts (1)
93-93: Consider thread safety implications of global Settings manipulation.The global
Settings.throwOnInvalidmodification could affect other concurrent operations using Luxon date parsing. This approach works but may cause unexpected behavior in multi-threaded scenarios.Consider using a more isolated approach:
- Settings.throwOnInvalid = false; - const [updated, prev] = + const [updated, prev] = (() => { + const originalSetting = Settings.throwOnInvalid; + Settings.throwOnInvalid = false; + try { + return event instanceof ProjectUpdatedEvent ? this.intervalsFromProjectChange(engagement, event) : event instanceof EngagementCreatedEvent ? [engagementRange(event.engagement), null] : [ engagementRange(event.updated), engagementRange(event.previous), ]; - Settings.throwOnInvalid = true; + } finally { + Settings.throwOnInvalid = originalSetting; + } + })();Also applies to: 103-103
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/engagement/handlers/validate-eng-date-overrides-on-project-change.handler.ts(1 hunks)src/components/partnership/handlers/validate-partnership-date-overrides-on-project-change.handler.ts(1 hunks)src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/components/engagement/handlers/validate-eng-date-overrides-on-project-change.handler.ts (2)
src/components/partnership/handlers/validate-partnership-date-overrides-on-project-change.handler.ts (1)
EventsHandler(8-67)src/components/project/events/project-updated.event.ts (1)
ProjectUpdatedEvent(4-14)
src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts (3)
src/components/project/events/project-updated.event.ts (1)
ProjectUpdatedEvent(4-14)src/components/engagement/events/engagement-created.event.ts (1)
EngagementCreatedEvent(10-29)src/components/engagement/dto/engagement.dto.ts (1)
engagementRange(243-244)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: lint
- GitHub Check: Unit
- GitHub Check: Clean
- GitHub Check: Generate (head)
- GitHub Check: Generate (base)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
src/components/engagement/handlers/validate-eng-date-overrides-on-project-change.handler.ts (1)
6-6: LGTM! Event handler priority correctly added.The addition of priority value
10to the@EventsHandlerdecorator ensures this validation handler executes with the correct precedence, aligning with the PR objective to run validation before report syncing logic.src/components/partnership/handlers/validate-partnership-date-overrides-on-project-change.handler.ts (1)
8-8: LGTM! Consistent event handler priority applied.The priority value
10matches the engagement handler, ensuring consistent execution order for date validation across both partnership and engagement components.src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts (3)
1-1: LGTM! Required import added for Settings manipulation.The Luxon Settings import is necessary for the date validation logic implemented below.
99-102: LGTM! Proper interval computation for engagement updates.The updated logic correctly computes both the updated and previous engagement intervals for
EngagementUpdatedEvent, which is essential for proper diff calculation.
104-119: LGTM! Robust date validation with comprehensive error reporting.The validation logic effectively catches invalid date ranges before they can cause issues in the syncing process, directly addressing the PR objective. The detailed logging will help with debugging production issues.
Consider if logging the entire
eventobject might expose sensitive information in production logs. You might want to log only essential fields instead:this.logger.error('Found invalid date range for event', { eventType: event.constructor.name, diffSide: 'before', - event, + engagementId: engagement.id, + eventId: event instanceof ProjectUpdatedEvent ? event.updated.id : + event instanceof EngagementCreatedEvent ? event.engagement.id : event.updated.id, });
Attempting to track down & fix some invalid date range errors we are seeing in prod