-
Notifications
You must be signed in to change notification settings - Fork 46
union of parentResourceExternalId & parentResourceTypeSlug #1480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
union of parentResourceExternalId & parentResourceTypeSlug #1480
Conversation
53d03ba
into
ENT-4372-resources-external
Greptile OverviewGreptile SummaryThis PR refactors the Critical Issue: The union is missing a member for creating resources without any parent. Previously, all parent fields were optional, allowing top-level resources. The new type requires EITHER Test Coverage: All updated tests now include parent resource fields. Tests that previously verified behavior when parent fields are omitted have been removed, and some test names are now misleading (e.g., "excludes description when omitted" but always includes Confidence Score: 1/5
Important Files Changed
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 files reviewed, 3 comments
| export type CreateAuthorizationResourceOptions = | ||
| | CreateOptionsWithParentResourceId | ||
| | CreateOptionsWithParentExternalId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing union member for resources without a parent. The type currently requires EITHER parentResourceId OR parentResourceExternalId+parentResourceTypeSlug, but doesn't allow creating top-level resources (no parent at all). Add a third union member with just BaseCreateAuthorizationResourceOptions to support this case.
| export type CreateAuthorizationResourceOptions = | |
| | CreateOptionsWithParentResourceId | |
| | CreateOptionsWithParentExternalId; | |
| export type CreateAuthorizationResourceOptions = | |
| | BaseCreateAuthorizationResourceOptions | |
| | CreateOptionsWithParentResourceId | |
| | CreateOptionsWithParentExternalId; |
| it('excludes description when omitted', async () => { | ||
| fetchOnce(authorizationResourceFixture, { status: 201 }); | ||
|
|
||
| await workos.authorization.createResource({ | ||
| organizationId: testOrgId, | ||
| resourceTypeSlug: 'document', | ||
| externalId: 'doc-456', | ||
| name: 'Q4 Budget Report', | ||
| parentResourceId: 'resource_01HXYZ', | ||
| }); | ||
|
|
||
| const body = fetchBody(); | ||
| expect(body).not.toHaveProperty('description'); | ||
| expect(body).not.toHaveProperty('parent_resource_id'); | ||
| expect(body).toHaveProperty('parent_resource_id', 'resource_01HXYZ'); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test updated to always include parentResourceId, but the test name says "excludes description when omitted". This doesn't test the scenario where parent fields are actually omitted (which would be a top-level resource). Either restore the original test case or update the name to reflect what's actually being tested.
| @@ -822,33 +804,12 @@ describe('Authorization', () => { | |||
| externalId: 'doc-456', | |||
| name: 'Q4 Budget Report', | |||
| description: null, | |||
| parentResourceId: 'resource_01HXYZ', | |||
| }); | |||
|
|
|||
| const body = fetchBody(); | |||
| expect(body).toHaveProperty('description', null); | |||
| expect(body).not.toHaveProperty('parent_resource_id'); | |||
| }); | |||
|
|
|||
| it('sends null when parentResourceId is explicitly set to null', async () => { | |||
| fetchOnce( | |||
| { | |||
| ...authorizationResourceFixture, | |||
| parent_resource_id: null, | |||
| }, | |||
| { status: 201 }, | |||
| ); | |||
|
|
|||
| await workos.authorization.createResource({ | |||
| organizationId: testOrgId, | |||
| resourceTypeSlug: 'document', | |||
| externalId: 'doc-456', | |||
| name: 'Q4 Budget Report', | |||
| parentResourceId: null, | |||
| }); | |||
|
|
|||
| const body = fetchBody(); | |||
| expect(body).toHaveProperty('parent_resource_id', null); | |||
| expect(body).not.toHaveProperty('description'); | |||
| expect(body).toHaveProperty('parent_resource_id', 'resource_01HXYZ'); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test now always sets parentResourceId, making the test name misleading. The test says it "sends null when description is explicitly set to null" but doesn't verify behavior when parentResourceId is omitted or null.
Description
Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.