Default directors memberships when the project's region is assigned #3504
Conversation
📝 Walkthrough## Walkthrough
A new handler was introduced to automatically add director roles as project members when a project's region is updated. Supporting repository methods were added to ensure only one active member per role. A migration was added to backfill missing director roles on open projects. End-to-end tests were created to verify this behavior, and GraphQL fragments and test utilities were updated to support the new data requirements.
## Changes
| Files/Paths | Change Summary |
|------------------------------------------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------|
| src/components/project/project-member/handlers/project-region-defaults-director-membership.handler.ts | Added handler to listen for project region updates and add director roles as default project members if needed. |
| src/components/project/project-member/project-member.repository.ts<br>src/components/project/project-member/project-member.gel.repository.ts | Added `addDefaultForRole` method to both repositories to conditionally add a project member for a given role if not present. |
| src/components/project/project-member/migrations/backfill-missing-directors.migration.ts | Added migration to backfill missing RegionalDirector and FieldOperationsDirector roles on active or in-development projects. |
| src/components/project/project-member/project-member.module.ts | Registered the new handler and migration as providers in the project member module. |
| test/features/project-region-defaults-director-membership.e2e-spec.ts | Introduced end-to-end tests for automatic director membership assignment on region updates. |
| test/utility/create-region.ts | Simplified the GraphQL mutation for creating a field region by removing nested zone/director fields from the response. |
| test/utility/fragments.ts | Updated and refactored GraphQL fragments for field region and zone to include deeper nesting and director IDs. |
## Possibly related PRs
- [SeedCompany/cord-api-v3#3495](https://github.com/SeedCompany/cord-api-v3/pull/3495): Adds a handler for replacing FieldOperationsDirector memberships when a FieldRegion's zone changes, which is related as both PRs manage project member roles based on region or zone updates.✨ 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 (2)
src/components/project/project-member/project-member.repository.ts (1)
209-209: Fix timestamp consistency.The method uses
DateTime.now()but thecreatemethod on line 108 usesDateTime.local(). For consistency, this should useDateTime.local()to match the existing pattern in the codebase.- const now = DateTime.now(); + const now = DateTime.local();src/components/project/project-member/handlers/project-region-defaults-director-membership.handler.ts (1)
19-26: Consider error handling for resource loading failures.The resource loading and director assignment logic is correct. However, consider adding error handling for scenarios where the field region might not exist or the resource loading fails.
- const fieldRegion = await this.resources.load('FieldRegion', fieldRegionId); + const fieldRegion = await this.resources.load('FieldRegion', fieldRegionId); + if (!fieldRegion) { + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/project/project-member/handlers/project-region-defaults-director-membership.handler.ts(1 hunks)src/components/project/project-member/project-member.gel.repository.ts(1 hunks)src/components/project/project-member/project-member.module.ts(2 hunks)src/components/project/project-member/project-member.repository.ts(1 hunks)test/features/project-region-defaults-director-membership.e2e-spec.ts(1 hunks)test/utility/create-region.ts(1 hunks)test/utility/fragments.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/components/project/project-member/handlers/project-region-defaults-director-membership.handler.ts (2)
src/components/project/events/project-updated.event.ts (1)
ProjectUpdatedEvent(4-14)test/utility/fragments.ts (4)
fieldRegion(919-946)fieldRegion(947-947)fieldZone(899-916)fieldZone(917-917)
test/utility/fragments.ts (1)
src/graphql.ts (1)
graphql(6-18)
src/components/project/project-member/project-member.repository.ts (6)
src/common/id-field.ts (1)
ID(24-25)src/common/temporal/calendar-date.ts (1)
now(108-110)src/core/database/query/matching.ts (1)
ACTIVE(33-33)src/core/database/query/create-node.ts (1)
createNode(53-90)src/core/database/query/create-relationships.ts (1)
createRelationships(87-186)src/core/database/query-augmentation/condition-variables.ts (1)
variable(20-20)
test/features/project-region-defaults-director-membership.e2e-spec.ts (5)
test/utility/create-app.ts (2)
TestApp(20-22)createTestApp(24-51)test/utility/create-session.ts (1)
createSession(4-18)test/utility/login.ts (1)
loginAsAdmin(21-24)test/utility/create-region.ts (1)
createRegion(9-42)src/graphql.ts (2)
VariablesOf(20-20)graphql(6-18)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: Generate (base)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: Unit
- GitHub Check: Generate (head)
- GitHub Check: lint
- GitHub Check: Clean
- GitHub Check: Analyze (javascript)
🔇 Additional comments (16)
src/components/project/project-member/project-member.repository.ts (1)
204-251: The conditional project member creation logic looks solid.The implementation correctly:
- Checks for existing active members with the specified role
- Only creates a new member if none exists (count = 0)
- Uses proper Neo4j query patterns with
createNodeandcreateRelationships- Sets appropriate timestamps and relationships
The subquery logic and filtering approach aligns well with the existing codebase patterns.
test/utility/fragments.ts (2)
906-908: Good enhancement to include director ID.Adding the nested
value { id }structure provides access to the director's ID, which is needed for the new project member default functionality.
919-946: Proper refactoring to tagged template literal with dependencies.The conversion from a plain template string to a tagged template literal with explicit
fieldZonedependency follows the correct GraphQL fragment pattern and supports the enhanced data retrieval requirements.src/components/project/project-member/project-member.module.ts (1)
8-8: Proper module integration of the new handler.The new
ProjectRegionDefaultsDirectorMembershipHandleris correctly imported and added to the providers array, following standard NestJS module patterns.Also applies to: 32-32
test/utility/create-region.ts (1)
54-54: Good simplification aligned with fragment restructuring.Removing the explicit
fieldZoneanduserdependencies makes sense since the enhancedfieldRegionfragment now includes the necessary nested data through its own dependencies.src/components/project/project-member/project-member.gel.repository.ts (2)
76-86: Clean GEL repository method implementation.The method signature matches the Neo4j counterpart and properly delegates to the parameterized query, maintaining consistency between repository implementations.
87-122: Solid EdgeDB conditional logic implementation.The GEL query correctly:
- Casts input parameters to appropriate EdgeDB types
- Checks for existing active members with the specified role
- Uses EdgeDB's
if-then-elseconstruct to conditionally insert or return existing members- Maintains the same logical flow as the Neo4j implementation
The EdgeDB patterns are properly applied.
src/components/project/project-member/handlers/project-region-defaults-director-membership.handler.ts (3)
1-11: LGTM! Clean dependency injection and event handler setup.The imports, decorator, and constructor follow proper NestJS patterns with appropriate dependency injection.
13-17: Good early return pattern for unchanged field region.The handler correctly exits early when
fieldRegionIdhasn't changed, avoiding unnecessary processing.
28-40: Consistent pattern for field zone processing.The field zone loading and director assignment follows the same pattern as the field region, which is good for consistency.
test/features/project-region-defaults-director-membership.e2e-spec.ts (6)
1-24: LGTM! Proper test setup and cleanup.The imports, test app setup, and cleanup hooks follow proper testing patterns.
26-47: Good test coverage for basic director assignment.This test correctly verifies that both regional and field operations directors are added as active members when a region is assigned to a project.
49-99: Excellent test for inactive member handling.This test thoroughly validates that when inactive directors exist for a different region, new directors are added as active while old ones remain inactive. The setup with
inactiveAttimestamps is well-designed.
131-145: Excellent duplicate member detection in helper function.The helper function includes proper validation to detect duplicate members, which is crucial for ensuring data integrity in the tests.
147-169: Well-structured GraphQL mutation document.The mutation correctly updates the project's field region and fetches the necessary team member data for validation.
101-129: Gathering the full implementation ofaddDefaultForRoleto confirm its logic:#!/bin/bash # Print the full addDefaultForRole method body sed -n '200,300p' src/components/project/project-member/project-member.repository.ts
CarsonF
force-pushed
the
project-member/project-region-defaults-directors
branch
from
41dd844 to
0da4bd3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/components/project/project-member/handlers/project-region-defaults-director-membership.handler.ts(1 hunks)src/components/project/project-member/migrations/backfill-missing-directors.migration.ts(1 hunks)src/components/project/project-member/project-member.gel.repository.ts(1 hunks)src/components/project/project-member/project-member.module.ts(2 hunks)src/components/project/project-member/project-member.repository.ts(6 hunks)test/features/project-region-defaults-director-membership.e2e-spec.ts(1 hunks)test/utility/create-region.ts(1 hunks)test/utility/fragments.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/components/project/project-member/project-member.module.ts
- test/utility/create-region.ts
- src/components/project/project-member/project-member.gel.repository.ts
- src/components/project/project-member/handlers/project-region-defaults-director-membership.handler.ts
- test/utility/fragments.ts
- test/features/project-region-defaults-director-membership.e2e-spec.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/components/project/project-member/project-member.repository.ts (8)
src/common/id-field.ts (1)
ID(24-25)src/common/temporal/calendar-date.ts (1)
now(108-110)src/core/database/query/matching.ts (1)
ACTIVE(33-33)src/core/database/query-augmentation/condition-variables.ts (2)
variable(20-20)Variable(7-15)src/core/database/query/create-node.ts (1)
createNode(53-90)src/core/database/query/cypher-functions.ts (1)
randomUUID(69-69)src/core/database/query-augmentation/subquery.ts (1)
varInExp(69-72)src/core/database/query/create-relationships.ts (1)
createRelationships(87-186)
src/components/project/project-member/migrations/backfill-missing-directors.migration.ts (4)
src/components/project/project-member/project-member.repository.ts (2)
upsertMember(284-373)projectMemberFilters(376-393)src/components/project/project-filters.query.ts (1)
projectFilters(22-131)src/core/database/query/matching.ts (1)
ACTIVE(33-33)src/core/database/query-augmentation/condition-variables.ts (1)
variable(20-20)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: Unit
- GitHub Check: Clean
- GitHub Check: lint
- GitHub Check: Generate (base)
- GitHub Check: Generate (head)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (9)
src/components/project/project-member/migrations/backfill-missing-directors.migration.ts (3)
25-53: LGTM! Well-structured helper function.The
openProjectsMissingRolehelper function correctly identifies open projects missing specific roles. The logic properly filters projects by status and uses a subquery to count existing members with the role.
73-93: LGTM! Consistent implementation for FieldOperationsDirector.The second query follows the same pattern as the RegionalDirector query but correctly navigates through the region->zone->director relationship chain.
55-71: Verify that all projects have associated field regions.The query assumes projects have
fieldRegionrelationships. If some projects lack this relationship, they will be silently skipped without logging.Run this script to check if there are open projects without field regions:
#!/bin/bash # Description: Check for open projects without field regions that would be skipped by the migration ast-grep --pattern $'match([ node($_, "Project"), relation("out", $_, "fieldRegion", $_), node($_, "FieldRegion") ])'src/components/project/project-member/project-member.repository.ts (6)
35-37: LGTM! Required imports for new functionality.The new imports
VariableandvarInExpare needed to support the enhanced upsert functionality that handles both ID and Variable cases.
206-239: LGTM! Well-implemented conditional role assignment.The
addDefaultForRolemethod correctly checks for existing active members with the role before adding a new one. The parameter handling and subquery structure are appropriate.
250-251: Improved parameter handling.Good refactoring from
.raw()to.apply()with proper parameter binding usingaddParam. This is more consistent with the rest of the codebase.
275-282: Enhanced return value structure.The method now returns both project IDs and timestamp, which provides better information for callers. The refactoring to use the
upsertMemberhelper eliminates code duplication.
284-373: Complex but correct upsert implementation.The
upsertMembermethod correctly implements the upsert pattern using union logic:
- First branch: Updates existing member's roles and reactivates them
- Second branch: Creates new member if none exists
The handling of both
VariableandIDtypes for the user parameter is well-implemented.One minor suggestion for maintainability:
+ // Try to update existing member first, create new one if none exists return (query: Query) => query.subQuery(scope, (sub) => sub + // Branch 1: Update existing member .match([
297-301: Verify varInExp utility usage.The
varInExputility is used to extract variable names for the subquery scope. Ensure this handles edge cases correctly, particularly with complex variable expressions.#!/bin/bash # Description: Check the implementation of varInExp to ensure it handles the use cases correctly ast-grep --pattern $'export const varInExp = ($_) => $_'
src/components/project/project-member/migrations/backfill-missing-directors.migration.ts
Show resolved
Hide resolved
No description provided.