Validate partnership dates #3402
Conversation
📝 WalkthroughWalkthroughThis set of changes introduces a centralized approach for validating and handling date override conflicts across related entities such as engagements and partnerships when a project's MOU dates are updated. A new exception class, 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:
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 (4)
src/components/partnership/partnership.service.ts (1)
309-338: Well-implemented date range validation exception.The
PartnershipDateRangeExceptionclass properly encapsulates validation logic for partnership date ranges and is correctly integrated into the create and update methods.One minor suggestion:
Consider making the error message configurable or using a format string for better flexibility:
constructor(readonly value: Range<CalendarDate>, readonly field: string) { - const message = - "Partnership's MOU start date must be before the MOU end date"; + const message = + `Partnership's MOU start date must be before the MOU end date`; super({ message, field }); }src/common/exceptions/date-override-conflict.exception.ts (3)
21-39: Consider adding JSDoc comments for better documentationThe class constructor is well-implemented with appropriate error message formatting for both singular and plural cases. However, adding JSDoc comments would improve code documentation and IDE intellisense support.
+/** + * Exception thrown when date overrides conflict with canonical date ranges + */ export class DateOverrideConflictException extends RangeException { + /** + * Creates a new DateOverrideConflictException + * @param object The resource with the canonical date range + * @param canonical The canonical date range + * @param label Tuple of [singular, plural] labels for error messages + * @param conflicts Non-empty array of conflicts found + */ constructor( readonly object: IdentifiableResource, readonly canonical: Range<CalendarDate | null>, label: [singular: string, plural: string], readonly conflicts: NonEmptyArray<Conflict>, ) { const message = [ conflicts.length === 1 ? `${label[0]} has a date outside the new range` : `${label[1]} have dates outside the new range`, ...conflicts.map(({ date, point, label }) => { const pointStr = point === 'start' ? 'Start' : 'End'; const dateStr = date.toISO(); return ` - ${pointStr} date of ${label} is ${dateStr}`; }), ].join('\n'); super({ message }); }
41-70: Well-implemented conflict detection with proper null handlingThe static
findConflictsmethod effectively identifies date conflicts while handling null values appropriately. The use offlatMapto check both start and end date conflicts in a single operation is elegant.Consider adding JSDoc documentation to explain the logic:
+/** + * Finds conflicts between a canonical date range and a list of items with date ranges + * @param canonical The canonical date range to check against + * @param items Array of items with start and end dates to validate + * @returns A non-empty array of conflicts if any are found, otherwise undefined + */ static findConflicts( canonical: Range<CalendarDate | null>, items: ReadonlyArray<{ __typename: string; id: ID; label: string; start: CalendarDate | null; end: CalendarDate | null; }>, ): NonEmptyArray<Conflict> | undefined {
73-74: Consider adding type annotation to helper functionThe
asNonEmptyhelper function is concise and useful, but would benefit from an explicit return type annotation for improved readability.-const asNonEmpty = <T>(items: readonly T[]) => +const asNonEmpty = <T>(items: readonly T[]): NonEmptyArray<T> | undefined => items.length === 0 ? undefined : (items as NonEmptyArray<T>);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/common/exceptions/date-override-conflict.exception.ts(1 hunks)src/common/exceptions/index.ts(1 hunks)src/components/engagement/handlers/validate-eng-date-overrides-on-project-change.handler.ts(1 hunks)src/components/partnership/handlers/index.ts(1 hunks)src/components/partnership/handlers/validate-partnership-date-overrides-on-project-change.handler.ts(1 hunks)src/components/partnership/partnership.gel.repository.ts(1 hunks)src/components/partnership/partnership.repository.ts(1 hunks)src/components/partnership/partnership.service.ts(6 hunks)src/components/project/events/project-updated.event.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/components/partnership/partnership.gel.repository.ts (1)
src/common/id-field.ts (1)
ID(20-21)
src/components/partnership/partnership.repository.ts (2)
src/common/id-field.ts (1)
ID(20-21)src/core/database/query/matching.ts (1)
ACTIVE(27-27)
src/components/engagement/handlers/validate-eng-date-overrides-on-project-change.handler.ts (3)
src/components/project/events/project-updated.event.ts (1)
ProjectUpdatedEvent(4-15)src/common/exceptions/date-override-conflict.exception.ts (1)
DateOverrideConflictException(21-71)src/common/resource.dto.ts (1)
EnhancedResource(95-298)
src/common/exceptions/date-override-conflict.exception.ts (3)
src/common/id-field.ts (1)
ID(20-21)src/common/temporal/calendar-date.ts (1)
CalendarDate(26-274)src/common/exceptions/range.exception.ts (1)
RangeException(3-7)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: Analyze (javascript)
- GitHub Check: Generate (head)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: Generate (base)
- GitHub Check: lint
- GitHub Check: Clean
- GitHub Check: Unit
🔇 Additional comments (11)
src/components/project/events/project-updated.event.ts (1)
1-2: Enhanced ProjectUpdatedEvent with type-safe resource propertyThe addition of the
resourceproperty withEnhancedResourcetype provides a strongly-typed reference to the project, which will improve type safety when this event is consumed by handlers. This is a good enhancement that aligns with the PR objective of improving validation for partnerships.Also applies to: 5-6, 12-14
src/common/exceptions/index.ts (1)
12-12: Exported the new DateOverrideConflictExceptionThe export of the new exception class makes it available for reuse throughout the codebase. This supports the PR's goal of making the exception shape more generic for validation across different entity types.
src/components/partnership/handlers/index.ts (1)
2-2: Exposed partnership date validation handlerAdding this export makes the new validation handler available to the rest of the application, which is necessary for it to be registered and used by the event system. This aligns with the PR objective of adding validation for partnerships.
src/components/partnership/partnership.gel.repository.ts (1)
52-58: Added method to retrieve all partnerships by project IDThe implementation of
listAllByProjectIdlooks good. It follows the repository pattern consistently with other methods in this class. The GEL query correctly selects all partnerships associated with the given project ID and applies the standard hydration logic.This method is a necessary building block for validating partnership date overrides when project MOU dates change, which directly supports the PR's objective.
src/components/partnership/partnership.repository.ts (1)
186-197: Well implemented repository method for fetching partnerships by project.The implementation follows the established repository pattern in the codebase and uses the existing hydration mechanism. This method supports the new validation flow for partnership date overrides when project MOU dates are updated.
src/components/partnership/partnership.service.ts (1)
153-155: Clean service method implementation.This method correctly delegates to the repository layer, maintaining the separation of concerns while exposing the functionality to handlers.
src/components/partnership/handlers/validate-partnership-date-overrides-on-project-change.handler.ts (1)
1-46:❓ Verification inconclusive
Good implementation of date validation handler.
The handler correctly validates partnership date overrides against project MOU dates, leveraging the centralized
DateOverrideConflictExceptionfor conflict detection and error reporting.Address the TODO comment on line 29 to provide meaningful labels for partnerships in error messages. Currently, it's using just the ID which might not be user-friendly in error messages:
🏁 Script executed:
#!/bin/bash # Look for any existing partnership label implementations in the codebase rg "label.*partnership" --type=tsLength of output: 175
#!/bin/bash # Find the PartnershipService implementation SERVICE_FILE=$(rg -l "class PartnershipService" --type=ts) echo "PartnershipService file: $SERVICE_FILE" # Show the signature and return type of listAllByProjectId to inspect available fields rg -n -A5 -B2 "listAllByProjectId" "$SERVICE_FILE" # Search for the underlying Partnership entity or interface to see its properties rg -n "export (interface|class) .*Partnership" -n
#!/bin/bash # Find the Partnership type or interface definition rg -n "export (interface|class) Partnership" -t ts # Locate the file containing listAllByProjectId and inspect its signature for available properties SERVICE_FILE=$(rg -l "listAllByProjectId" -t ts) echo "PartnershipService file: $SERVICE_FILE" rg -n -B2 -A5 "listAllByProjectId" "$SERVICE_FILE"
Action Required: Provide user-friendly partnership labels in error messages
The TODO on line 29 should be replaced with a descriptive field (e.g.partnership.name,partnership.organizationName, etc.) rather than just the ID so end users can easily identify which partnership caused the conflict.• File to update:
- src/components/partnership/handlers/validate-partnership-date-overrides-on-project-change.handler.ts
• Replace:label: partnership.id, // TODOwith something like:
label: partnership.name, // using the partnership’s display name• Verify the Partnership model or service (
listAllByProjectIdreturn type) exposes a suitable human‐readable property.src/components/engagement/handlers/validate-eng-date-overrides-on-project-change.handler.ts (1)
1-46: Clean refactoring to use centralized date validation.Good job refactoring this handler to use the new centralized
DateOverrideConflictExceptionmechanism. The code is now simpler and more aligned with the new partnership validation approach.src/common/exceptions/date-override-conflict.exception.ts (3)
1-5: LGTM - Well-organized importsThe imports are correctly structured and include all necessary dependencies for the functionality implemented in this file.
7-19: Types are well-defined with proper readonly propertiesThe
Conflicttype andIdentifiableResourceinterface are well-designed:
- Using
Readonly<{...}>ensures immutability- Clear typing for conflict points with discriminated union ('start' | 'end')
- Properly typed ID field using the project's ID type
This establishes a solid foundation for type safety throughout the rest of the implementation.
1-74: LGTM - Overall excellent implementation of reusable conflict detectionThis exception class is well-designed for reuse across the codebase:
- Centralizes date override conflict detection logic in one place
- Produces consistent, well-formatted error messages
- Properly handles null values and edge cases
- Follows TypeScript best practices with immutable types and proper null checks
- Provides utility methods that make implementation in handlers straightforward
Based on the context from other files, this appears to be replacing previous custom implementations across engagement and partnership components with a more generic, reusable approach.
Following up on #3396 to validate Partnerships as well
Honestly I forgot these existed when doing that one.
Re-did the exception shape to be more generic.