Add parent and strategicAlliances fields to Partner#3436
Add parent and strategicAlliances fields to Partner#3436
Conversation
📝 WalkthroughWalkthroughThis change introduces two new relationship fields— Changes
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ 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: 3
🧹 Nitpick comments (6)
src/components/partner/dto/update-partner.dto.ts (1)
54-56: Consider consistent implementation between DTOs.There's an inconsistency in how the
strategicAlliancesfield is defined compared to theCreatePartnerDTO:
CreatePartneruses@Fieldwith explicit@Transformfor duplicates, whileUpdatePartneruses@ListFieldwhich already handles duplicates internallyCreatePartnerprovides a default empty array (= []), butUpdatePartnerdoesn'tWhile both implementations will work correctly, consider making them consistent for better maintainability:
-@ListField(() => IDType, { optional: true }) -@IsId({ each: true }) -readonly strategicAlliances?: ReadonlyArray<ID<'Partner'>>; +@Field(() => [IDType], { nullable: true }) +@IsId({ each: true }) +@Transform(({ value }) => uniq(value)) +readonly strategicAlliances?: ReadonlyArray<ID<'Partner'>> = [];Or alternatively:
-@Field(() => [IDType], { nullable: true }) -@IsId({ each: true }) -@Transform(({ value }) => uniq(value)) -readonly strategicAlliances?: ReadonlyArray<ID<'Partner'>> = []; +@ListField(() => IDType, { optional: true }) +@IsId({ each: true }) +readonly strategicAlliances?: ReadonlyArray<ID<'Partner'>> = [];src/components/partner/dto/partner.dto.ts (2)
70-72:strategicAlliancesis typed asRequired, but the input allows it to be omitted
CreatePartnerandUpdatePartneracceptstrategicAlliancesas optional.
Marking the DTO property asRequired<Secured<...>>makes the runtime object always expose a (possibly empty) list, which is fine, but the semantic difference (optional in the API vs required in the DTO) has bitten us before when spreading objects or doingPartial<Partner>.Consider relaxing the compile-time requirement to mirror the input expectations, e.g.
-readonly strategicAlliances: Required< - Secured<ReadonlyArray<LinkTo<'Partner'>>> ->; +readonly strategicAlliances: Secured< + ReadonlyArray<LinkTo<'Partner'>> +>;or document clearly that consumers may always rely on the field being present (never
undefined).
103-106: Minor naming nit – description could be more specific
SecuredPartnersis a generic helper, but its description strings "a list of partners" will surface in the GraphQL schema documentation.
If the intent is to reuse it elsewhere, keep it. If it will only ever backstrategicAlliances, consider:-@ObjectType({ - description: SecuredPropertyList.descriptionFor('a list of partners'), -}) -export class SecuredPartners extends SecuredPropertyList(Partner) {} +@ObjectType({ + description: SecuredPropertyList.descriptionFor('a partner alliance list'), +}) +export class SecuredPartnerList extends SecuredPropertyList(Partner) {}Purely a doc-string / clarity tweak — no functional change.
src/components/partner/partner.resolver.ts (1)
68-75: Guard against missing IDs inparentresolution
mapSecuredValuewill invoke the mapper when the value is notnull, but it doesn’t guarantee an.idexists.
If hydration ever returns{ id: undefined }(e.g. bad data) we’ll feedundefinedinto the DataLoader, which will throw.-return await mapSecuredValue(partner.parent, ({ id }) => partners.load(id)); +return await mapSecuredValue(partner.parent, ({ id }) => + id ? partners.load(id) : undefined, +);Adds a tiny safeguard without altering happy-path behaviour.
src/components/partner/partner.repository.ts (2)
134-137:parentIdclearing works, but circular parent loops are still possibleUpdating the relation to a new ID (or
null) is handled, yet nothing prevents:A.parent = B B.parent = ASimple depth-one loop detection would prevent the most common mistake:
if (parentId) { const ancestor = await this.readOne(parentId, session); if (ancestor.parent.value?.id === id) { throw new InputException('Circular parent relationship', 'partner.parentId'); } }Not critical for merge but worth a backlog ticket.
286-305: UseACTIVEfilter consistently & avoid duplicates incollectOther list sub-queries (e.g.
languagesOfConsulting) includeACTIVE.
If we only want active alliances, add it here for parity:- relation('out', '', 'strategicAlliances'), + relation('out', '', 'strategicAlliances', ACTIVE),Minor: consider
collect(distinct …)to avoid dupes when multiple parallel relationships are created inadvertently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
dbschema/migrations/00021-m1xsgaf.edgeql(1 hunks)dbschema/partner.gel(1 hunks)src/components/partner/dto/create-partner.dto.ts(1 hunks)src/components/partner/dto/partner.dto.ts(3 hunks)src/components/partner/dto/update-partner.dto.ts(1 hunks)src/components/partner/partner.repository.ts(5 hunks)src/components/partner/partner.resolver.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/components/partner/dto/update-partner.dto.ts (3)
src/common/id-field.ts (2)
IdField(9-19)ID(24-25)src/common/list-field.ts (1)
ListField(12-24)src/common/validators/short-id.validator.ts (1)
IsId(18-24)
src/components/partner/partner.repository.ts (4)
src/components/partner/partner.resolver.ts (1)
strategicAlliances(77-85)src/common/exceptions/input.exception.ts (1)
InputException(14-127)src/core/database/query/cypher-functions.ts (1)
collect(29-29)src/core/database/query/matching.ts (1)
ACTIVE(27-27)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
dbschema/migrations/00021-m1xsgaf.edgeql (1)
4-7: LGTM! Migration looks good.The migration correctly implements the addition of the
parentlink and the multi-valuedstrategicAllianceslink to the Partner type.src/components/partner/dto/create-partner.dto.ts (1)
53-59: LGTM! Field additions are properly implemented.The new fields are properly defined with appropriate decorators:
parentIdis correctly marked as nullablestrategicAlliancesincludes validation for each ID and transformation to remove duplicatesThe implementation aligns well with the schema changes.
src/components/partner/partner.resolver.ts (1)
76-85: Empty-list update: possibleundefined → []mismatch
loadSecuredIdscopes withvalue?: string[], but we’re still mapping?.map(...)which is fine.
However, if the list can legitimately be empty and secured (i.e.value: []), callers must ensure the repository writes an empty array instead ofundefined. See the repository comment below.
rdonigian
force-pushed
the
org-alliance-fields
branch
from
a288608 to
251e29e
Compare
🗞 GraphQL SummaryView schema changes@@ -781,11 +781,13 @@
globalInnovationsClient: Boolean
languageOfWiderCommunicationId: ID
languagesOfConsulting: [ID!] = []
organizationId: ID!
+ parentId: ID
pmcEntityCode: String
pointOfContactId: ID
startDate: Date
+ strategicAlliances: [ID!] = []
types: [PartnerType!] = []
}
input CreatePartnerInput {
@@ -3679,8 +3681,9 @@
languages(input: LanguageListInput = {count: 25, order: ASC, page: 1, sort: "name"}): SecuredLanguageList!
languagesOfConsulting: SecuredLanguages!
modifiedAt: DateTime!
organization: SecuredOrganization!
+ parent: SecuredPartner!
"""Does the requesting user have this pinned?"""
pinned: Boolean!
pmcEntityCode: SecuredStringNullable!
@@ -3694,8 +3697,9 @@
"""Based on the project's sensitivity"""
sensitivity: Sensitivity!
startDate: SecuredDateNullable!
+ strategicAlliances: SecuredPartners!
types: SecuredPartnerTypes!
}
input PartnerFilters {
@@ -6289,8 +6293,19 @@
value: [PartnerType!]!
}
"""
+An object whose `value` is a list of a list of partners and has additional authorization information.
+The value is only given if `canRead` is `true` otherwise it is empty: `[]`.
+These `can*` authorization properties are specific to the user making the request.
+"""
+type SecuredPartners implements Secured {
+ canEdit: Boolean!
+ canRead: Boolean!
+ value: [Partner!]!
+}
+
+"""
An object with a partnership `value` and additional authorization information.
The value is only given if `canRead` is `true` otherwise it is `null`.
These `can*` authorization properties are specific to the user making the request.
"""
@@ -7663,11 +7678,13 @@
globalInnovationsClient: Boolean
id: ID!
languageOfWiderCommunicationId: ID
languagesOfConsulting: [ID!]
+ parentId: ID
pmcEntityCode: String
pointOfContactId: ID
startDate: Date
+ strategicAlliances: [ID!]
types: [PartnerType!]
}
input UpdatePartnerInput {
|
rdonigian
force-pushed
the
org-alliance-fields
branch
from
251e29e to
f774f23
Compare
rdonigian
force-pushed
the
org-alliance-fields
branch
from
f774f23 to
9384da0
Compare
rdonigian
force-pushed
the
org-alliance-fields
branch
from
9384da0 to
5ad7317
Compare
rdonigian
force-pushed
the
org-alliance-fields
branch
from
5ad7317 to
0c7b46d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/partner/partner.service.ts (3)
114-125: Consider adding similar validation in the create methodWhile the update method now validates these relationships, the same validation isn't present in the create method. This could potentially allow creating a partner with invalid self-referential relationships.
async create(input: CreatePartner): Promise<Partner> { this.verifyFinancialReportingType( input.financialReportingTypes, input.types, ); + if (input.strategicAlliances?.includes(input.id)) { + throw new InputException( + 'A partner cannot be its own strategic ally', + 'partner.strategicAlliances', + ); + } + if (input.parentId && input.parentId === input.id) { + throw new InputException( + 'A partner cannot be its own parent organization', + 'partner.parent', + ); + } + if (input.countries) { await this.verifyCountries(input.countries); } const created = await this.repo.create(input);
114-119: Consider handling partial updates for strategicAlliancesThe current validation only checks if
strategicAlliancesis included in the input. For partial updates where the field is not included but already contains invalid data, this validation would be skipped.- if (input.strategicAlliances?.includes(input.id)) { + // Check both input and existing data for partial updates + const alliancesToCheck = input.strategicAlliances ?? partner.strategicAlliances.value; + if (alliancesToCheck?.includes(input.id)) { throw new InputException( 'A partner cannot be its own strategic ally', 'partner.strategicAlliances', ); }
120-125: Consider handling partial updates for parentIdSimilar to strategicAlliances, the parentId validation only runs if included in the input. If the partner already has its own ID as parentId in the database, this validation would be bypassed during a partial update.
- if (input.parentId && input.parentId === input.id) { + // Check both input and existing parent ID for partial updates + const parentIdToCheck = input.parentId ?? partner.parent?.id; + if (parentIdToCheck && parentIdToCheck === input.id) { throw new InputException( 'A partner cannot be its own parent organization', 'partner.parent', ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
dbschema/migrations/00021-m1ud7t6.edgeql(1 hunks)dbschema/partner.gel(1 hunks)src/components/partner/dto/create-partner.dto.ts(1 hunks)src/components/partner/dto/partner.dto.ts(3 hunks)src/components/partner/dto/update-partner.dto.ts(1 hunks)src/components/partner/partner.gel.repository.ts(1 hunks)src/components/partner/partner.repository.ts(5 hunks)src/components/partner/partner.resolver.ts(2 hunks)src/components/partner/partner.service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- dbschema/migrations/00021-m1ud7t6.edgeql
🚧 Files skipped from review as they are similar to previous changes (7)
- src/components/partner/partner.gel.repository.ts
- dbschema/partner.gel
- src/components/partner/dto/partner.dto.ts
- src/components/partner/dto/update-partner.dto.ts
- src/components/partner/partner.resolver.ts
- src/components/partner/partner.repository.ts
- src/components/partner/dto/create-partner.dto.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/partner/partner.service.ts (1)
src/common/exceptions/input.exception.ts (1)
InputException(14-127)
🔇 Additional comments (1)
src/components/partner/partner.service.ts (1)
114-125: Approve: Good validation to prevent self-referential relationshipsThe added validation checks correctly prevent a partner from being its own strategic ally or parent, which maintains data integrity. The error messages are clear and specific field identifiers are used.
| }; | ||
|
|
||
| parent: Partner { | ||
| constraint exclusive; |
There was a problem hiding this comment.
This means that a partner can only a parent to one other partner, which is not what you what. Multiple partners can share a single parent partner.
| constraint exclusive; |
| multi fieldRegions: FieldRegion; | ||
| multi countries: Location; | ||
| multi strategicAlliances: Partner { | ||
| constraint exclusive; |
There was a problem hiding this comment.
This means a partner can only be apart of one strategic alliance.
Are these alliances supposed to bi-directional? I think so.
Right now it is only unidirectional. "SC has an alliance with Wycliffe, but Wycliffe does not have an alliance with SC." That doesn't sound right.
"SC and Wycliffe have an alliance with each other" sounds more correct.
If that's true the implementation here would be different to accommodate.
Also are the alliances transient? A <-> B & B <-> C, meaning additionally A <-> C?
| .subQuery('node', (sub) => | ||
| sub | ||
| .optionalMatch([ | ||
| node('node'), | ||
| relation('out', '', 'parent', ACTIVE), | ||
| node('parent', 'Partner'), | ||
| ]) | ||
| .return('parent { .id } as parent'), | ||
| ) |
There was a problem hiding this comment.
| .subQuery('node', (sub) => | |
| sub | |
| .optionalMatch([ | |
| node('node'), | |
| relation('out', '', 'parent', ACTIVE), | |
| node('parent', 'Partner'), | |
| ]) | |
| .return('parent { .id } as parent'), | |
| ) | |
| .optionalMatch([ | |
| node('node'), | |
| relation('out', '', 'parent', ACTIVE), | |
| node('parent', 'Partner'), | |
| ]) |
| if (parentId === id) { | ||
| throw new InputException( | ||
| 'A partner cannot be its own parent organization', | ||
| 'partner.parent', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Already done in the service
| if (parentId === id) { | |
| throw new InputException( | |
| 'A partner cannot be its own parent organization', | |
| 'partner.parent', | |
| ); | |
| } |
| if (strategicAlliances.includes(changes.id)) { | ||
| throw new InputException( | ||
| 'A partner cannot be its own strategic ally', | ||
| 'partner.strategicAlliances', | ||
| ); | ||
| } |
There was a problem hiding this comment.
| if (strategicAlliances.includes(changes.id)) { | |
| throw new InputException( | |
| 'A partner cannot be its own strategic ally', | |
| 'partner.strategicAlliances', | |
| ); | |
| } |
| if (input.strategicAlliances?.includes(input.id)) { | ||
| throw new InputException( | ||
| 'A partner cannot be its own strategic ally', | ||
| 'partner.strategicAlliances', | ||
| ); | ||
| } | ||
| if (input.parentId && input.parentId === input.id) { | ||
| throw new InputException( | ||
| 'A partner cannot be its own parent organization', | ||
| 'partner.parent', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Technically these constraints should apply on create() as well.
There was a problem hiding this comment.
Pull request overview
Adds two new Partner relationship fields—parent (single) and strategicAlliances (multi)—and wires them through GraphQL, persistence layers, and the EdgeDB schema/migration.
Changes:
- Extend Partner create/update inputs to accept
parentIdandstrategicAlliances. - Add GraphQL resolve fields for
parentandstrategicAllianceswith secured wrappers. - Persist and hydrate the new relationships in Neo4j + EdgeDB, including EdgeDB schema/migration updates.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/partner/partner.service.ts | Validates self-referential parent/alliance updates. |
| src/components/partner/partner.resolver.ts | Adds parent + strategicAlliances GraphQL resolve fields. |
| src/components/partner/partner.repository.ts | Writes/reads new relations in Neo4j queries and updates. |
| src/components/partner/partner.gel.repository.ts | Hydrates new links in EdgeDB repository. |
| src/components/partner/dto/update-partner.dto.ts | Adds parentId + strategicAlliances to update input DTO. |
| src/components/partner/dto/partner.dto.ts | Adds DTO fields + secured wrapper types for Partner relationships. |
| src/components/partner/dto/create-partner.dto.ts | Adds parentId + strategicAlliances to create input DTO. |
| dbschema/partner.gel | Adds EdgeDB links for parent + strategicAlliances. |
| dbschema/migrations/00021-m1ud7t6.edgeql | Creates EdgeDB migration for the new links. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| parent: Partner { | ||
| constraint exclusive; | ||
| }; |
There was a problem hiding this comment.
constraint exclusive on the parent link would prevent a single parent Partner from being referenced by multiple child Partners (i.e., only one child could have a given parent). For an org hierarchy, this is usually not desired; remove the exclusive constraint unless you explicitly want a strict one-to-one parent relationship.
| parent: Partner { | |
| constraint exclusive; | |
| }; | |
| parent: Partner; |
| CREATE LINK parent: default::Partner { | ||
| CREATE CONSTRAINT std::exclusive; | ||
| }; |
There was a problem hiding this comment.
This migration adds an std::exclusive constraint to parent, which would prevent multiple Partners from sharing the same parent. If the intent is a normal parent/child hierarchy, drop this exclusive constraint (the link is already single-valued on the child side).
| CREATE LINK parent: default::Partner { | |
| CREATE CONSTRAINT std::exclusive; | |
| }; | |
| CREATE LINK parent: default::Partner; |
| CREATE MULTI LINK strategicAlliances: default::Partner { | ||
| CREATE CONSTRAINT std::exclusive; | ||
| }; |
There was a problem hiding this comment.
This migration adds an std::exclusive constraint to the strategicAlliances multi link, which would prevent a Partner from appearing as a strategic ally of more than one Partner (breaking many-to-many). Remove the exclusive constraint unless that restriction is intentional.
| CREATE MULTI LINK strategicAlliances: default::Partner { | |
| CREATE CONSTRAINT std::exclusive; | |
| }; | |
| CREATE MULTI LINK strategicAlliances: default::Partner; |
| if (input.strategicAlliances?.includes(input.id)) { | ||
| throw new InputException( | ||
| 'A partner cannot be its own strategic ally', | ||
| 'partner.strategicAlliances', | ||
| ); | ||
| } | ||
| if (input.parentId && input.parentId === input.id) { | ||
| throw new InputException( | ||
| 'A partner cannot be its own parent organization', | ||
| 'partner.parent', | ||
| ); | ||
| } |
There was a problem hiding this comment.
New behavior for parentId/strategicAlliances (setting relationships, clearing them, and rejecting self-references) isn’t covered by the existing Partner e2e tests. Add test cases for create/update to verify the resolvers return the expected secured values and that self-referential inputs produce the expected GraphQL errors.
| departmentIdBlock: 'departmentIdBlock', | ||
| scope: 'scopedRoles', | ||
| pinned, | ||
| parent: 'parent { .id }', |
There was a problem hiding this comment.
parent is already projected to a map (parent { .id } as parent) in the preceding subquery. Projecting it again here (parent { .id }) will fail at runtime (map projection on a map). Either return the raw parent node from the subquery and project here, or keep the subquery projection and reference parent directly in the merge map.
| parent: 'parent { .id }', | |
| parent: 'parent', |
| if (input.parentId && input.parentId === input.id) { | ||
| throw new InputException( | ||
| 'A partner cannot be its own parent organization', | ||
| 'partner.parent', |
There was a problem hiding this comment.
The InputException field path uses partner.parent, but the input field being validated is parentId. Using partner.parentId would align the error with the actual input property and match how other input validation fields are reported.
| 'partner.parent', | |
| 'partner.parentId', |
| if (parentId === id) { | ||
| throw new InputException( | ||
| 'A partner cannot be its own parent organization', | ||
| 'partner.parent', |
There was a problem hiding this comment.
The InputException field path is partner.parent, but the update input field is parentId. Consider changing this to partner.parentId so clients can correctly associate the error with the submitted field.
| 'partner.parent', | |
| 'partner.parentId', |
| multi strategicAlliances: Partner { | ||
| constraint exclusive; | ||
| }; |
There was a problem hiding this comment.
constraint exclusive on a multi link makes the target Partner exclusive across all sources, effectively preventing a Partner from being in more than one other Partner’s strategicAlliances. That turns this into a one-to-many constraint and blocks many-to-many alliances; if that’s not intended, remove the exclusive constraint.
| multi strategicAlliances: Partner { | |
| constraint exclusive; | |
| }; | |
| multi strategicAlliances: Partner; |
We are adding 2 new fields to Partner:
parentandstrategicAlliances. These both capture relationships to other partners.SeedCompany/cord-field#1693