Forward director changes to open project memberships #3447
Conversation
CarsonF
force-pushed
the
membership-filters
branch
from
f56060b to
505df9f
Compare
CarsonF
force-pushed
the
director-change-members
branch
from
7edd4d3 to
89fd43e
Compare
CarsonF
force-pushed
the
director-change-members
branch
from
89fd43e to
5d9aceb
Compare
|
Warning Rate limit exceeded@CarsonF has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis update introduces a new mechanism for automatically updating project memberships when the director of a region or zone changes. It adds a handler that listens for director update events, and a repository method that replaces memberships for open projects accordingly. Supporting changes include parameter order adjustments in event constructors, new Cypher query helpers, and an end-to-end test. 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/components/field-region/events/field-region-updated.event.ts(1 hunks)src/components/field-zone/events/field-zone-updated.event.ts(1 hunks)src/components/project/project-member/handlers/director-change-apply-to-project-members.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(2 hunks)src/core/database/query-augmentation/subquery.ts(1 hunks)test/features/director-change-replaces-memberships-on-open-projects.e2e-spec.ts(1 hunks)
🔇 Additional comments (9)
src/components/project/project-member/project-member.module.ts (2)
28-28: LGTM! Handler correctly registered in providers.The handler is properly added to the module's providers array.
7-7:✅ Verification successful
LGTM! Verify handler implementation exists.
The import follows standard conventions.
🏁 Script executed:
#!/bin/bash # Verify that the DirectorChangeApplyToProjectMembersHandler class exists fd "director-change-apply-to-project-members.handler.ts" --type fLength of output: 164
LGTM – Handler Implementation Confirmed
The handler file exists and exports the expected class:
• src/components/project/project-member/handlers/director-change-apply-to-project-members.handler.ts
All changes look good.
src/components/field-zone/events/field-zone-updated.event.ts (1)
5-9:✅ Verification successful
Breaking change: Verify all constructor usages are updated.
The parameter order change from
(updated, previous, input)to(previous, updated, input)is a breaking change that could cause runtime errors if existing code constructs this event with the old parameter order.
🏁 Script executed:
#!/bin/bash # Search for all usages of FieldZoneUpdatedEvent constructor rg -A 3 "new FieldZoneUpdatedEvent\(" --type tsLength of output: 353
No action needed: Constructor usages updated to new signature
All occurrences of
new FieldZoneUpdatedEvent(...)have been updated to match the new(previous, updated, input)order. A search across the codebase found only one usage:
- src/components/field-zone/field-zone.service.ts:
new FieldZoneUpdatedEvent(fieldZone /* previous */, updated /* updated */, { … } /* input */)No further changes are required.
src/components/field-region/events/field-region-updated.event.ts (1)
5-9:✅ Verification successful
Breaking change: Verify all constructor usages are updated.
The parameter order change maintains consistency with
FieldZoneUpdatedEventbut is a breaking change that requires verification of all constructor usages.
🏁 Script executed:
#!/bin/bash # Search for all usages of FieldRegionUpdatedEvent constructor rg -A 3 "new FieldRegionUpdatedEvent\(" --type tsLength of output: 375
Breaking change reconciled: FieldRegionUpdatedEvent constructor call sites updated
Verified the sole instantiation in
src/components/field-region/field-region.service.tsuses the new parameter order (previous,updated,input). This matches the updated constructor signature and is consistent withFieldZoneUpdatedEvent. No further action required.src/core/database/query-augmentation/subquery.ts (1)
69-72:❌ Incorrect review comment
Potential issue: Regex now allows empty string matches.
The change from
[^.]+to[^.]*in the capturing group means the function can now return empty strings, which may not be the intended behavior.
🏁 Script executed:
#!/bin/bash # Test the regex change behavior with various inputs python3 << 'EOF' import re old_regex = re.compile(r'(?:.+\()?([^.]+)\.?.*') new_regex = re.compile(r'(?:.+\()?\)?([^.]*)\.?.*') test_cases = [ "variable", "obj.property", "func()", "func().property", "complex(arg).property", "()property", "()", "", "." ] print("Testing regex changes:") print("Input | Old Result | New Result") print("-" * 40) for test in test_cases: old_match = old_regex.search(test) new_match = new_regex.search(test) old_result = old_match.group(1) if old_match else "None" new_result = new_match.group(1) if new_match else "None" print(f"{test:15} | {old_result:10} | {new_result}") EOFLength of output: 426
Regex change is intentional and correct—empty-group matches are desired
The updated pattern consumes the trailing
)and returns''(not')') when there’s no identifier to capture. In practice:
varInExp('func()')now returns''(old regex returned')', which was erroneous)- For inputs with no valid segment (e.g.
'','.'), both before and after the change default to''Tests confirm the new behavior matches the intended extraction logic. No action needed.
Likely an incorrect or invalid review comment.
src/components/project/project-member/project-member.gel.repository.ts (1)
76-125: Well-implemented membership replacement logic.The implementation correctly handles the atomic replacement of project memberships:
- Properly filters for active members on open projects only
- Uses transaction datetime for consistency
- Returns useful statistics for logging
- Follows GEL best practices with parameterized queries
src/components/project/project-member/handlers/director-change-apply-to-project-members.handler.ts (1)
32-39: Good logging implementation for audit trail.The logging includes all relevant context (location, directors, role) and spreads the stats from the repository operation, providing excellent observability.
test/features/director-change-replaces-memberships-on-open-projects.e2e-spec.ts (1)
28-186: Excellent comprehensive test coverage.The test thoroughly validates the director replacement functionality with:
- Multiple project scenarios (needs swap, no member, inactive member, role filled, closed)
- Clear setup/validation/assertion phases
- Both positive and negative test cases
- Proper isolation between test scenarios
src/components/project/project-member/project-member.repository.ts (1)
23-23: LGTM!The added imports are appropriate for the new functionality.
Also applies to: 25-26
...mponents/project/project-member/handlers/director-change-apply-to-project-members.handler.ts
Show resolved
Hide resolved
src/components/project/project-member/project-member.repository.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/core/config/config.service.ts (1)
178-180: Consider UTC for consistent timestamp handling across environments.The test database naming scheme is well-structured, but using local time with
DateTime.now()could lead to inconsistent behavior across different deployment environments and time zones.Consider using UTC for consistent timestamp handling:
- ? `test.${DateTime.now().toFormat( + ? `test.${DateTime.utc().toFormat( 'y-MM-dd.HH-mm-ss', )}.${customAlphabet('abcdefghjkmnpqrstuvwxyz', 7)()}`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/components/project/project-member/project-member.repository.ts(3 hunks)src/core/config/config.service.ts(2 hunks)src/core/database/database.module.ts(1 hunks)src/core/database/database.service.ts(2 hunks)src/core/database/query/cypher-functions.ts(2 hunks)src/core/database/query/properties/update-properties.ts(4 hunks)src/core/database/query/properties/update-property.ts(4 hunks)test/features/director-change-replaces-memberships-on-open-projects.e2e-spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/core/database/query/cypher-functions.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/features/director-change-replaces-memberships-on-open-projects.e2e-spec.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/core/database/query/properties/update-properties.ts (1)
src/core/database/query-augmentation/condition-variables.ts (1)
Variable(7-15)
src/components/project/project-member/project-member.repository.ts (7)
src/common/id-field.ts (1)
ID(24-25)src/common/temporal/calendar-date.ts (1)
now(108-110)src/core/database/query-augmentation/condition-variables.ts (1)
variable(20-20)src/core/database/query/cypher-functions.ts (2)
randomUUID(69-69)apoc(71-110)src/core/database/query/matching.ts (1)
ACTIVE(33-33)src/core/database/query/filters.ts (1)
sub(243-272)src/core/database/query/create-relationships.ts (1)
createRelationships(87-186)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: lint
- GitHub Check: Generate (base)
- GitHub Check: Generate (head)
- 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: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: Unit
- GitHub Check: Clean
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
src/core/database/query/properties/update-properties.ts (1)
10-10: LGTM! Clean implementation of flexible timestamp handling.The changes correctly support both
VariableandDateTimetypes for thenowparameter, with proper conditional logic and sensible fallbacks.Also applies to: 24-24, 39-39, 66-69
src/core/database/query/properties/update-property.ts (1)
39-39: LGTM! Well-designed generic enhancement for customizable stats variable names.The implementation maintains backward compatibility with the default
'stats'while enabling flexible output variable naming. All return statements are correctly updated to use the dynamicoutputStatsVar.Also applies to: 46-46, 67-71, 103-108, 121-125
src/components/project/project-member/project-member.repository.ts (1)
2-8: LGTM! Imports align with the new method's requirements.The additional imports (
not,path,apoc,randomUUID,updateProperty,variable) are all utilized in the newreplaceMembershipsOnOpenProjectsmethod.Also applies to: 22-35
src/core/config/config.service.ts (1)
8-9: Import changes look good for the new test database naming scheme.The replacement of
nanoidwithcustomAlphabetand addition ofDateTimesupport the enhanced test database lifecycle management.src/core/database/database.service.ts (1)
6-6: Import addition is appropriate for the new functionality.Adding
DateTimeto the luxon import supports the timestamp parsing indropStaleTestDbs.
CarsonF
force-pushed
the
director-change-members
branch
from
90a9355 to
0b2efc2
Compare
CarsonF
force-pushed
the
director-change-members
branch
from
0b2efc2 to
36b8d39
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements the functionality to update project memberships when a FieldZone/Region director changes by deactivating the outgoing director’s membership and adding the new director on open projects. Key changes include:
- Adding and updating end‐to‐end tests for director changes.
- Extending and adjusting query functions (updateProperty, updateProperties, cypher functions) to support configurable output stat variables and additional parameters.
- Introducing new repository methods and an event handler to automate membership replacement in both standard and GEL repositories.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/features/director-change-replaces-memberships-on-open-projects.e2e-spec.ts | Establishes e2e tests for the new director change behavior. |
| src/core/database/query/properties/update-property.ts | Adjusts generic types to support a configurable output stats variable. |
| src/core/database/query/properties/update-properties.ts | Introduces an optional “now” parameter and passes it through the update query. |
| src/core/database/query/cypher-functions.ts | Adds a new two-argument function (fn2) and extends the apoc.coll functions. |
| src/core/database/query-augmentation/subquery.ts | Modifies the regular expression used to extract variable names. |
| src/components/project/project-member/project-member.repository.ts | Implements the replaceMembershipsOnOpenProjects method to update memberships on open projects. |
| src/components/project/project-member/project-member.gel.repository.ts | Adds a GEL repository variant for replacing memberships. |
| src/components/project/project-member/handlers/director-change-apply-to-project-members.handler.ts | Implements an event handler that forwards director-changed events to update project memberships. |
| src/components/field-zone/events/field-zone-updated.event.ts & src/components/field-region/events/field-region-updated.event.ts | Updates event payload ordering for consistency. |
Comments suppressed due to low confidence (1)
src/components/project/project-member/project-member.repository.ts:264
- The outputStatsVar for the updateProperty call on the 'roles' property is set to 'inactiveStats', which may be confusing. Consider renaming it to a more descriptive identifier like 'rolesStats' to match the property being updated.
outputStatsVar: 'inactiveStats',
There was a problem hiding this comment.
Look at those tests! 🤩
When a FieldZone/Region director changes: