Correct FieldRegion's zone change to only migrate zone directors for that region #3502
Conversation
CarsonF
force-pushed
the
project-member/region-zone-change-bug
branch
from
a579fe1 to
124b401
Compare
CarsonF
changed the base branch from
project-member/no-auto-inactivate-member-with-roles
to
develop
🗞 GraphQL SummaryView schema changes@@ -1736,11 +1736,18 @@
id: ID!
name: SecuredString!
}
+input FieldRegionFilters {
+ director: UserFilters
+ fieldZone: FieldZoneFilters
+ id: ID
+}
+
input FieldRegionListInput {
"""The number of items to return in a single page"""
count: Int! = 25
+ filter: FieldRegionFilters
"""The order in which to sort the list"""
order: Order! = ASC
@@ -1774,11 +1781,17 @@
id: ID!
name: SecuredString!
}
+input FieldZoneFilters {
+ director: UserFilters
+ id: ID
+}
+
input FieldZoneListInput {
"""The number of items to return in a single page"""
count: Int! = 25
+ filter: FieldZoneFilters
"""The order in which to sort the list"""
order: Order! = ASC
@@ -4845,8 +4858,9 @@
input ProjectFilters {
"""Only projects created within this time range"""
createdAt: DateTimeFilter
+ fieldRegion: FieldRegionFilters
"""
Only projects that the requesting user is an active member of. false does nothing.
"""
|
CarsonF
force-pushed
the
project-member/region-zone-change-bug
branch
from
124b401 to
c56489f
Compare
📝 WalkthroughWalkthroughThis change introduces enhanced filtering and sorting capabilities for FieldRegion, FieldZone, and Project entities. It adds nested filter and sorter support for related director and field zone/user entities, updates DTOs and repositories to use these new filters and sorters, and extends project member replacement logic to support region-based filtering. Changes
Possibly related PRs
✨ 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 (4)
src/components/project/project.repository.ts (1)
460-476: Sorter duplication – consider extracting a helperThe new
'fieldRegion.*'sorter duplicates the “match / union / null fallback” pattern already present forprimaryLocation.*andprimaryPartnership.*. Extracting a small util (e.g.nestedSorter('fieldRegion', 'FieldRegion', fieldRegionSorters)) would DRY this file and reduce future maintenance.src/components/field-zone/dto/list-field-zone.dto.ts (1)
15-20: Redundant& {}intersection – safe to drop.
director?: UserFilters & {}is equivalent toUserFilters; the empty intersection adds no value and slightly clutters the schema.- readonly director?: UserFilters & {}; + readonly director?: UserFilters;src/components/field-region/dto/list-field-region.dto.ts (1)
16-24: Same& {}nit & public exposure comment as FieldZone DTO.
- Drop the meaningless
& {}intersections ondirectorandfieldZone.- Confirm that making
filterpublic is deliberate; otherwise addinternal: true.- readonly director?: UserFilters & {}; - readonly fieldZone?: FieldZoneFilters & {}; + readonly director?: UserFilters; + readonly fieldZone?: FieldZoneFilters;src/components/field-region/field-region.repository.ts (1)
141-180: Variable re-use (node) may cause accidental shadowing.Inside both custom sorters we alias
node➜region, then immediately re-introduce a newnodefor the related entity.
Cypher allows this but readability & future edits suffer.Consider distinct aliases:
- .with('node as region') - .match([ - node('region'), - relation('out', '', 'director', ACTIVE), - node('node', 'User'), + .with('node as region') + .match([ + node('region'), + relation('out', '', 'director', ACTIVE), + node('directorNode', 'User'), ]) - .apply(sortWith(userSorters, input)), + .apply(sortWith(userSorters, { ...input, base: 'directorNode' })),You’ll avoid confusion and accidental clashes with other chained helpers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/components/field-region/dto/list-field-region.dto.ts(2 hunks)src/components/field-region/field-region.repository.ts(3 hunks)src/components/field-zone/dto/list-field-zone.dto.ts(2 hunks)src/components/field-zone/field-zone.repository.ts(3 hunks)src/components/project/dto/list-projects.dto.ts(2 hunks)src/components/project/project-filters.query.ts(2 hunks)src/components/project/project-member/handlers/regions-zone-changes-applies-director-change-to-project-members.handler.ts(1 hunks)src/components/project/project-member/project-member.gel.repository.ts(2 hunks)src/components/project/project-member/project-member.repository.ts(2 hunks)src/components/project/project.repository.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/components/project/dto/list-projects.dto.ts (1)
src/common/filter-field.ts (1)
FilterField(11-31)
src/components/project/project-member/project-member.repository.ts (1)
src/common/id-field.ts (1)
ID(24-25)
src/components/field-zone/dto/list-field-zone.dto.ts (4)
src/components/field-region/dto/list-field-region.dto.ts (1)
InputType(26-34)src/components/user/dto/list-users.dto.ts (1)
InputType(35-41)src/common/id-field.ts (2)
IdField(9-19)ID(24-25)src/common/filter-field.ts (1)
FilterField(11-31)
src/components/field-zone/field-zone.repository.ts (4)
src/core/database/query/sorting.ts (2)
sortWith(61-85)defineSorters(129-159)src/components/user/user.repository.ts (2)
userFilters(391-410)userSorters(412-426)src/core/database/query/filters.ts (1)
sub(243-272)src/core/database/query/matching.ts (1)
ACTIVE(33-33)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Generate (head)
- GitHub Check: Generate (base)
- GitHub Check: lint
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: Clean
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: Unit
- GitHub Check: Analyze (javascript)
🔇 Additional comments (10)
src/components/project/project-member/handlers/regions-zone-changes-applies-director-change-to-project-members.handler.ts (1)
32-37: Let’s retry with a PCRE2-based grep to catch any 3-arg calls:#!/bin/bash # Find any replaceMembershipsOnOpenProjects calls with exactly 3 args rg --pcre2 --no-heading --line-number \ 'replaceMembershipsOnOpenProjects\(\s*[^,]+,\s*[^,]+,\s*[^,]+\s*\)' \ srcThis will list any invocations still using the old 3-argument signature.
src/components/project/dto/list-projects.dto.ts (1)
138-141: FieldRegion filter added cleanly
@FilterField(() => FieldRegionFilters)integrates seamlessly with existing filters and keeps type-safety. No concerns.src/components/project/project.repository.ts (1)
37-38: Missing unused-import lint suppression
fieldRegionSortersis only referenced inside the new sorter block; import is fine. Just make sure your linter isn’t warning about the default export name clash (fieldRegionSortersvs existing sorters).src/components/project/project-filters.query.ts (1)
111-117: Filter wired correctlyFilter definition mirrors other nested filters and leverages
fieldRegionFilters– looks solid.src/components/project/project-member/project-member.repository.ts (1)
208-241: Region-scoped replacement logic LGTMAppending the optional
fieldRegionconstraint keeps the Cypher query consistent and backward compatible (undefinedspreads to{}). Nice touch making the status filter explicit too.src/components/project/project-member/project-member.gel.repository.ts (1)
80-87: Confirm all call-sites pass an explicitregion(orundefined).
db.runrequires every parameter declared ine.paramsto be present in the object you pass.
If any existing caller omits the fourth argument altogether, TypeScript will compile (because it’s optional), but the runtime parameter object will lack theregionkey, causing EdgeDB to throw “Parameter region is missing”.Two quick mitigations:
- return await this.db.run(this.replaceMembershipsOnOpenProjectsQuery, { - oldDirector, - newDirector, - role, - region, - }); + return await this.db.run(this.replaceMembershipsOnOpenProjectsQuery, { + oldDirector, + newDirector, + role, + // Ensure the key is always present. + region: region ?? null, + });and (more importantly) audit every invocation of
replaceMembershipsOnOpenProjectsafter this change.src/components/field-zone/dto/list-field-zone.dto.ts (1)
28-29: Re-expose filter publicly – verify GraphQL surface area.Removing
{ internal: true }makesfiltervisible to API consumers.
If that is intended – great. If not, restore the flag:- @FilterField(() => FieldZoneFilters) + @FilterField(() => FieldZoneFilters, { internal: true })Double-check the API contract before merging.
src/components/field-region/field-region.repository.ts (1)
113-125: Good swap to explicitfilter/sortWithpipeline.Moving away from the generic sorter improves clarity and keeps pagination deterministic.
Nice job.src/components/field-zone/field-zone.repository.ts (2)
156-178: Missing default sorter for base props (e.g.name).
fieldZoneSorterscurrently only handlesdirector.*.
Callinglistwithsort: 'name'will fall back to the generic base-prop branch withindefineSorters; however that branch relies onFieldZone.BaseNodePropsbeing populated. Ifnameisn’t inBaseNodeProps, sorting will silently break.Either:
- Ensure
nameis inBaseNodeProps, or- Add an explicit matcher:
export const fieldZoneSorters = defineSorters(FieldZone, { + name: (query) => query.return('node.name as sortValue'), 'director.*': (query, input) =>
128-140: 👍 Explicit filters & sorters before pagination.The new pipeline (
fieldZoneFilters➜fieldZoneSorters➜ paginate) is clearer and prevents accidental cartesian products.
No description provided.