feat: fga adding base types and module registraiton#556
feat: fga adding base types and module registraiton#556swaroopAkkineniWorkos wants to merge 43 commits intomainfrom
Conversation
|
@greptile review |
| from workos.types.workos_model import WorkOSModel | ||
| from workos.typing.literals import LiteralOrUntyped | ||
|
|
||
| OrganizationMembershipStatus = Literal["active", "inactive", "pending"] |
There was a problem hiding this comment.
Duplicated OrganizationMembershipStatus literal
This exact same Literal["active", "inactive", "pending"] type alias is already defined in src/workos/types/user_management/organization_membership.py:7. Consider importing from a shared location or re-exporting from one module to the other to avoid the definitions drifting apart over time.
There was a problem hiding this comment.
Just pushed up a change where I moved "OrganizationMembershipStatus" to it's own file to be imported
|
Change branch name before merging |
src/workos/authorization.py
Outdated
| if cascade_delete is not None: | ||
| await self._http_client.delete_with_body( | ||
| f"{AUTHORIZATION_RESOURCES_PATH}/{resource_id}", | ||
| json={"cascade_delete": cascade_delete}, |
There was a problem hiding this comment.
This adds cascade_delete to the body, but I believe it should be passed as a query param instead. Which is what delete_resource_by_external_id does here
There was a problem hiding this comment.
updated to be a param
src/workos/authorization.py
Outdated
| if description is not None: | ||
| json["description"] = description |
There was a problem hiding this comment.
Does description here need the same treatment as in update_resource?
|
|
||
| # --- create_resource --- | ||
|
|
||
| def test_create_resource_required_fields_only( |
There was a problem hiding this comment.
Should we update this test since parent isn't a required field?
There was a problem hiding this comment.
updated and added a test
| id: str | ||
| user_id: str | ||
| organization_id: str | ||
| organization_name: str |
There was a problem hiding this comment.
It looks like we don't include organization_name in the responses where this gets used. For instance listOrganizationMembershipsForResource calls serializeBase, which doesn't include organization_name.
|
@greptile plz re-review |
Greptile SummaryThis PR adds foundational types and CRUD operations for Fine-Grained Authorization (FGA) resources to the WorkOS Python SDK. The implementation introduces resource management capabilities with support for both ID-based and external-ID-based operations. Key Changes:
Implementation Quality:
Minor Issue:
Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 954bb37 |
src/workos/authorization.py
Outdated
| # TODO RENAME | ||
| ResourcesListResource = WorkOSListResource[ | ||
| AuthorizationResource, ResourceListFilters, ListMetadata | ||
| ] |
There was a problem hiding this comment.
TODO comment needs resolution
Either rename ResourcesListResource to something more descriptive (like AuthorizationResourceList) or remove the TODO if the current name is acceptable.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
updated and changed name
atainter
left a comment
There was a problem hiding this comment.
Looks good to me. Should probably get a review from python code owners as well
…rce, list_memberships_for_resource_by_external_id (#571)
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.