Add Event types #289

tribble · 2024-07-24T19:08:46Z

Description

Initial PR to add two event types. It's most crucial to get feedback on the approach, then we can add more types in following PRs.

Documentation

Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.

[ ] Yes

If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.

mattgd · 2024-07-24T19:20:15Z

workos/events.py

+            # TODO: This is a hack, and it's wrong. Events does not support before or order
+            "before": None,


I'm a bit unclear - why do we need to define before here?

Our current list params typing forces it. We need to refactor but I'm kicking that can down the road to a future PR.

mattgd · 2024-07-24T19:21:34Z

workos/events.py

+        return WorkOsListResource(
+            list_method=self.list_events,
+            list_args=params,
+            **ListPage[Event](**response).model_dump(),
+        )


Oh interesting. Does this work without issue for async? I may have to revisit this in my PR.

I doubt it. We probably need to parametrize the list resource to use an async client. But I haven't gotten there yet

tribble · 2024-07-24T19:26:17Z

@mattgd Switching this back to draft. Need to fix some tests and there may be some real work to make the auto paging iter async

mattgd · 2024-07-24T19:29:04Z

@mattgd Switching this back to draft. Need to fix some tests and there may be some real work to make the auto paging iter async

@tribble Before you troubleshoot, I have an addition for that in my PR - let me know what you think: https://github.com/workos/workos-python/pull/288/files#diff-fd823a1e532828a74cb86c58012687832231229d0985952f4580c40ab33c3191R206-R225.

tribble · 2024-07-24T23:41:31Z

@mattgd OK, PR ready for review. Events has enough quirks this required some changes. Will comment inline.

tribble · 2024-07-24T23:42:11Z

tests/conftest.py

@@ -153,7 +153,7 @@ def inner(
        http_client: Union[SyncHTTPClient, AsyncHTTPClient],
        data_list: list,
        status_code: int = 200,
-        headers: Mapping[str, str] = None,
+        headers: Optional[Mapping[str, str]] = None,


Maybe we should point mypy at more of our files?

Seemingly we should be able to open it up to all workos/ files. That's how I was running it locally. If we're having a mypy mismatch we should adjust settings to make sure it runs how we'd like in CI.

tribble · 2024-07-24T23:42:39Z

tests/test_directory_sync.py

+    # We normalize this to 'active' in the SDK. This helper function
+    # does this conversion to make make assertions easier.
+    if directory["state"] == "linked":
+        return {**directory, "state": "active"}


Yeah, I bit the bullet and just did this conversion instead of having two types.

tribble · 2024-07-24T23:43:12Z

tests/test_directory_sync.py

@@ -73,7 +87,7 @@ def mock_group(self):

    @pytest.fixture
    def mock_directories(self):
-        directory_list = [MockDirectory(id=str(i)).to_dict() for i in range(20)]
+        directory_list = [MockDirectory(id=str(i)).to_dict() for i in range(10)]


Our list pages default to 10 anyway

tribble · 2024-07-24T23:45:58Z

tests/utils/fixtures/mock_directory.py

@@ -8,8 +8,9 @@ def __init__(self, id):
        self.object = "directory"
        self.id = id
        self.organization_id = "organization_id"
-        self.domain = "crashlanding.com"
-        self.name = "Ri Jeong Hyeok"
+        self.external_key = "ext_123"


This external key is actually required. See our API ref (which isn't consistent, sigh) as well as our API JSON model and Event JSON model.

tribble · 2024-07-24T23:46:44Z

tests/utils/fixtures/mock_directory_activated_payload.py

+from workos.resources.base import WorkOSBaseResource
+
+
+class MockDirectoryActivatedPayload(WorkOSBaseResource):


This was easier than mangling the existing MockDirectory model

tribble · 2024-07-24T23:47:06Z

tests/utils/fixtures/mock_event.py

 from workos.resources.base import WorkOSBaseResource


 class MockEvent(WorkOSBaseResource):
    def __init__(self, id):
        self.object = "event"
        self.id = id
-        self.event = "dsync.user.created"
-        self.data = {"id": "event_01234ABCD", "organization_id": "org_1234"}
+        self.event = "dsync.activated"


Had to switch to an event that actually exists

tribble · 2024-07-24T23:47:25Z

tests/utils/list_resource.py



-def list_data_to_dicts(list_data: List):
+def list_data_to_dicts(list_data: Sequence):


Why not use a more generic duck type?

tribble · 2024-07-24T23:48:34Z

workos/directory_sync.py

@@ -4,7 +4,6 @@
 from workos.utils.http_client import AsyncHTTPClient, SyncHTTPClient
 from workos.utils.pagination_order import PaginationOrder
 from workos.utils.request import REQUEST_METHOD_DELETE, REQUEST_METHOD_GET
-


I hates me rando newlines

tribble · 2024-07-24T23:51:24Z

workos/events.py

            "events": events,
            "limit": limit,
            "after": after,
-            "organization_id": organization_id,
+            "organization_id": organization,


Right now our other API methods just use organization.

I'm conforming for consistency. Happy to have a cleanup PR sweep through and change all of those params to organization_id

tribble · 2024-07-24T23:54:48Z

workos/events.py

+        return WorkOsListResource(
+            list_method=self.list_events,
+            list_args=params,
+            **ListPage[Event](**response).model_dump(exclude_unset=True),


I had to use exclude_unset=True otherwise this would always serialize before in the list_metadata. I think it's because the resource can't tell the difference between ListAfterMetadata and ListMetadata.

The other option is to and a third generic parameter and explicitly pass in the ListMetadata shape. But, since python type hint generics don't set support default values for generics, every list resource would have to pass in the list metadata type.

I realized there is another option. We could have separate WorkOsForwardListResource and WorkOsBidirectionalListResource.

Not sure how much benefit that will bring. It would almost definitely be more code.

tribble · 2024-07-24T23:55:33Z

workos/resources/directory_sync.py

 from workos.typing.literals import LiteralOrUntyped

-DirectoryState = Literal[


Extracted to a shared type which normalizes state

tribble · 2024-07-25T00:00:37Z

workos/resources/event.py

@@ -1,42 +0,0 @@
-from workos.resources.base import WorkOSBaseResource


Wasn't used ¯\_(ツ)_/¯

tribble · 2024-07-25T00:02:39Z

workos/resources/list.py

+        fixed_pagination_params = cast(
+            # Type hints consider this a mismatch because it assume the dictionary is dict[str, int]
+            Dict[str, Union[int, str, None]],
+            {
+                "limit": self.list_args["limit"],
+            },
+        )


Type hints were a bit "too smart". Since self.list_args["limit"] is an int it considers the entire dictionary to be of type Dict[str, int].

tribble · 2024-07-25T00:03:09Z

workos/resources/list.py

+                "limit": self.list_args["limit"],
+            },
+        )
+        if "order" in self.list_args:


Order isn't a thing for events.

mattgd · 2024-07-25T13:30:10Z

workos/types/directory_sync/directory_state.py

+def convert_linked_to_active(value: Any, info: ValidationInfo) -> Any:
+    if isinstance(value, str) and value == "linked":
+        return "active"
+    else:
+        return value


mattgd · 2024-07-25T13:31:54Z

tests/conftest.py

@@ -153,7 +153,7 @@ def inner(
        http_client: Union[SyncHTTPClient, AsyncHTTPClient],
        data_list: list,
        status_code: int = 200,
-        headers: Mapping[str, str] = None,
+        headers: Optional[Mapping[str, str]] = None,


Seemingly we should be able to open it up to all workos/ files. That's how I was running it locally. If we're having a mypy mismatch we should adjust settings to make sure it runs how we'd like in CI.

tribble requested a review from a team as a code owner July 24, 2024 19:08

tribble requested review from gcarvelli and mattgd and removed request for a team July 24, 2024 19:08

mattgd reviewed Jul 24, 2024

View reviewed changes

tribble marked this pull request as draft July 24, 2024 19:26

tribble force-pushed the add-event-types branch from fa18cb6 to 9667433 Compare July 24, 2024 23:40

tribble marked this pull request as ready for review July 24, 2024 23:41

tribble commented Jul 24, 2024

View reviewed changes

tribble commented Jul 25, 2024

View reviewed changes

workos/resources/event.py

@@ -1,42 +0,0 @@

from workos.resources.base import WorkOSBaseResource

Copy link

Author

tribble Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't used ¯\_(ツ)_/¯

tribble commented Jul 25, 2024

View reviewed changes

workos/resources/list.py

"limit": self.list_args["limit"],

},

)

if "order" in self.list_args:

Copy link

Author

tribble Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Order isn't a thing for events.

mattgd approved these changes Jul 25, 2024

View reviewed changes

tribble added 3 commits July 25, 2024 12:25

Add two directory types and update events list methods

fc4b88f

Bring back event_action

ba4ed28

Some initial test fixes

32dd90a

tribble and others added 5 commits July 25, 2024 12:25

Add testing and normalize 'linked' to 'active'

bc2142a

Undo debugging change

ea78424

Handle optional before in list metadata

da24395

Format

177844e

Fix conflicts.

9f954d1

mattgd force-pushed the add-event-types branch from 846c24b to 9f954d1 Compare July 25, 2024 16:26

Compatibility updates.

8c2722e

mattgd merged commit f2ecb38 into beta-5.0 Jul 25, 2024
5 checks passed

mattgd deleted the add-event-types branch July 25, 2024 16:29

tribble added a commit that referenced this pull request Jul 30, 2024

Add Event types (#289)

032981a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Event types #289

Add Event types #289

tribble commented Jul 24, 2024

mattgd Jul 24, 2024

tribble Jul 24, 2024

mattgd Jul 24, 2024

tribble Jul 24, 2024

tribble commented Jul 24, 2024

mattgd commented Jul 24, 2024 •

edited

Loading

tribble commented Jul 24, 2024

tribble Jul 24, 2024

mattgd Jul 25, 2024

tribble Jul 24, 2024

tribble Jul 24, 2024

tribble Jul 24, 2024

tribble Jul 24, 2024

tribble Jul 24, 2024

tribble Jul 24, 2024

tribble Jul 24, 2024

tribble Jul 24, 2024

tribble Jul 24, 2024

tribble Jul 25, 2024

tribble Jul 24, 2024

tribble Jul 25, 2024

tribble Jul 25, 2024 •

edited

Loading

tribble Jul 25, 2024

mattgd Jul 25, 2024

mattgd Jul 25, 2024

		# TODO: This is a hack, and it's wrong. Events does not support before or order
		"before": None,

		from workos.resources.base import WorkOSBaseResource


		class MockDirectoryActivatedPayload(WorkOSBaseResource):



		def list_data_to_dicts(list_data: List):
		def list_data_to_dicts(list_data: Sequence):

		from workos.typing.literals import LiteralOrUntyped

		DirectoryState = Literal[

		@@ -1,42 +0,0 @@
		from workos.resources.base import WorkOSBaseResource

Add Event types #289

Add Event types #289

Conversation

tribble commented Jul 24, 2024

Description

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tribble commented Jul 24, 2024

mattgd commented Jul 24, 2024 • edited Loading

tribble commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tribble Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattgd commented Jul 24, 2024 •

edited

Loading

tribble Jul 25, 2024 •

edited

Loading