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; |
| @@ -41,6 +41,13 @@ module default { | |||
| multi languagesOfConsulting: Language; | |||
| 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.
We are adding 2 new fields to Partner:
parentandstrategicAlliances. These both capture relationships to other partners.SeedCompany/cord-field#1693