From 486354ea4e5fc182974a3052c972e0d01503d27f Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Mon, 9 Dec 2024 13:11:21 -0500 Subject: [PATCH 1/2] Add `Tag.update() -> Tag` method --- integration/tests/posit/connect/test_tags.py | 11 +- src/posit/connect/tags.py | 120 +++++++++++++----- .../connect/__api__/v1/tags/33-patched.json | 7 + tests/posit/connect/test_tags.py | 53 +++++--- 4 files changed, 140 insertions(+), 51 deletions(-) create mode 100644 tests/posit/connect/__api__/v1/tags/33-patched.json diff --git a/integration/tests/posit/connect/test_tags.py b/integration/tests/posit/connect/test_tags.py index 37aae32b..1d58263c 100644 --- a/integration/tests/posit/connect/test_tags.py +++ b/integration/tests/posit/connect/test_tags.py @@ -150,6 +150,16 @@ def test_tag_content_items(self): self.contentC["guid"], } + # Update tag + tagDName = tagD.update(name="tagD_updated") + assert tagDName["name"] == "tagD_updated" + assert self.client.tags.get(tagD["id"])["name"] == "tagD_updated" + + tagDParent = tagDName.update(parent=tagB) + assert tagDParent["parent_id"] == tagB["id"] + assert self.client.tags.get(tagD["id"])["parent_id"] == tagB["id"] + + # Cleanup self.contentA.tags.delete(tagRoot) self.contentB.tags.delete(tagRoot) self.contentC.tags.delete(tagRoot) @@ -159,6 +169,5 @@ def test_tag_content_items(self): assert len(tagC.content_items.find()) == 0 assert len(tagD.content_items.find()) == 0 - # cleanup tagRoot.destroy() assert len(self.client.tags.find()) == 0 diff --git a/src/posit/connect/tags.py b/src/posit/connect/tags.py index 659088a2..be1aec32 100644 --- a/src/posit/connect/tags.py +++ b/src/posit/connect/tags.py @@ -18,6 +18,36 @@ def find(self) -> list[Tag]: pass +def _update_parent_kwargs(kwargs: dict) -> dict: + """ + Sets the `parent_id` key in the kwargs if `parent` is provided. + + Asserts that the `parent=` and `parent_id=` keys are not both provided. + """ + parent = kwargs.get("parent", None) + if parent is None: + # No parent to upgrade, return the kwargs as is + return kwargs + + if not isinstance(parent, Tag): + raise TypeError( + "`parent=` must be a Tag instance. If using a string, please use `parent_id=`" + ) + + parent_id = kwargs.get("parent_id", None) + if parent_id: + raise ValueError("Cannot provide both `parent=` and `parent_id=`") + + ret_kwargs = {**kwargs} + + # Remove `parent` from ret_kwargs + # and store the `parent_id` in the ret_kwargs below + del ret_kwargs["parent"] + + ret_kwargs["parent_id"] = parent["id"] + return ret_kwargs + + class Tag(Active): """Tag resource.""" @@ -135,6 +165,63 @@ def destroy(self) -> None: url = self._ctx.url + self._path self._ctx.session.delete(url) + # Allow for every combination of `name` and (`parent` or `parent_id`) + @overload + def update(self, /, *, name: str = ..., parent: Tag | None = ...) -> Tag: ... + @overload + def update(self, /, *, name: str = ..., parent_id: str | None = ...) -> Tag: ... + + def update( # pyright: ignore[reportIncompatibleMethodOverride] ; This method returns `Tag`. Parent method returns `None` + self, + **kwargs, + ) -> Tag: + """ + Update the tag. + + Parameters + ---------- + name : str + The name of the tag. + parent : Tag | None, optional + The parent `Tag` object. If there is no parent, the tag is a top-level tag. To remove + the parent tag, set the value to `None`. Only one of `parent` or `parent_id` can be + provided. + parent_id : str | None, optional + The identifier for the parent tag. If there is no parent, the tag is a top-level tag. + To remove the parent tag, set the value to `None`. + + Returns + ------- + Tag + Updated tag object. + + Examples + -------- + ```python + import posit + + client = posit.connect.Client() + last_tag = client.tags.find()[-1] + + # Update the tag's name + updated_tag = last_tag.update(name="new_name") + + # Remove the tag's parent + updated_tag = last_tag.update(parent=None) + updated_tag = last_tag.update(parent_id=None) + + # Update the tag's parent + parent_tag = client.tags.find()[0] + updated_tag = last_tag.update(parent=parent_tag) + updated_tag = last_tag.update(parent_id=parent_tag["id"]) + ``` + """ + updated_kwargs = _update_parent_kwargs(kwargs) + url = self._ctx.url + self._path + response = self._ctx.session.patch(url, json=updated_kwargs) + result = response.json() + return Tag(self._ctx, self._path, **result) + class TagContentItems(ContextManager): def __init__(self, ctx: Context, path: str) -> None: @@ -303,35 +390,6 @@ def get(self, tag_id: str) -> Tag: response = self._ctx.session.get(url) return Tag(self._ctx, path, **response.json()) - def _update_parent_kwargs(self, kwargs: dict) -> dict: - """ - Sets the `parent_id` key in the kwargs if `parent` is provided. - - Asserts that the `parent=` and `parent_id=` keys are not both provided. - """ - parent = kwargs.get("parent", None) - if parent is None: - # No parent to upgrade, return the kwargs as is - return kwargs - - if not isinstance(parent, Tag): - raise TypeError( - "`parent=` must be a Tag instance. If using a string, please use `parent_id=`" - ) - - parent_id = kwargs.get("parent_id", None) - if parent_id: - raise ValueError("Cannot provide both `parent=` and `parent_id=`") - - ret_kwargs = {**kwargs} - - # Remove `parent` from ret_kwargs - # and store the `parent_id` in the ret_kwargs below - del ret_kwargs["parent"] - - ret_kwargs["parent_id"] = parent["id"] - return ret_kwargs - # Allow for every combination of `name` and (`parent` or `parent_id`) @overload def find(self, /, *, name: str = ..., parent: Tag = ...) -> list[Tag]: ... @@ -379,7 +437,7 @@ def find(self, /, **kwargs) -> list[Tag]: subtags = client.tags.find(name="sub_name", parent=mytag["id"]) ``` """ - updated_kwargs = self._update_parent_kwargs( + updated_kwargs = _update_parent_kwargs( kwargs, # pyright: ignore[reportArgumentType] ) url = self._ctx.url + self._path @@ -425,7 +483,7 @@ def create(self, /, **kwargs) -> Tag: tag = client.tags.create(name="tag_name", parent=category_tag) ``` """ - updated_kwargs = self._update_parent_kwargs( + updated_kwargs = _update_parent_kwargs( kwargs, # pyright: ignore[reportArgumentType] ) diff --git a/tests/posit/connect/__api__/v1/tags/33-patched.json b/tests/posit/connect/__api__/v1/tags/33-patched.json new file mode 100644 index 00000000..a68103d6 --- /dev/null +++ b/tests/posit/connect/__api__/v1/tags/33-patched.json @@ -0,0 +1,7 @@ +{ + "id": "33", + "name": "academy-updated", + "parent_id": null, + "created_time": "2021-10-18T18:37:56Z", + "updated_time": "2021-10-18T18:37:56Z" +} diff --git a/tests/posit/connect/test_tags.py b/tests/posit/connect/test_tags.py index 324d88ad..9f4f7fb4 100644 --- a/tests/posit/connect/test_tags.py +++ b/tests/posit/connect/test_tags.py @@ -252,10 +252,6 @@ def test_content_with_tag(self): @responses.activate def test_destroy(self): # behavior - # mock_all_tags = responses.get( - # "https://connect.example/__api__/v1/tags", - # json=load_mock_list("v1/tags.json"), - # ) mock_29_tag = responses.get( "https://connect.example/__api__/v1/tags/29", json=load_mock_dict("v1/tags/29.json"), @@ -264,32 +260,51 @@ def test_destroy(self): "https://connect.example/__api__/v1/tags/29", json={}, # empty response ) - # post_destroy_json = load_mock_list("v1/tags.json") - # for tag in post_destroy_json: - # if tag["id"] in {"29", "30"}: - # post_destroy_json.remove(tag) - # mock_all_tags_after_destroy = responses.get( - # "https://connect.example/__api__/v1/tags", - # json=post_destroy_json, - # ) # setup client = Client(api_key="12345", url="https://connect.example") # invoke - # tags = client.tags.find() - # assert len(tags) == 28 tag29 = client.tags.get("29") tag29.destroy() - # tags = client.tags.find() - # # All children tags are removed - # assert len(tags) == 26 # assert - # assert mock_all_tags.call_count == 1 assert mock_29_tag.call_count == 1 assert mock_29_destroy.call_count == 1 - # assert mock_all_tags_after_destroy.call_count == 1 + + @responses.activate + def test_update(self): + # behavior + mock_get_33_tag = responses.get( + "https://connect.example/__api__/v1/tags/33", + json=load_mock_dict("v1/tags/33.json"), + ) + mock_update_33_tag = responses.patch( + "https://connect.example/__api__/v1/tags/33", + json=load_mock_dict("v1/tags/33-patched.json"), + ) + + # setup + client = Client(api_key="12345", url="https://connect.example") + tag33 = client.tags.get("33") + + # invoke + updated_tag33_0 = tag33.update(name="academy-updated", parent_id=None) + updated_tag33_1 = tag33.update(name="academy-updated", parent=None) + + parent_tag = Tag(client._ctx, "/v1/tags/1", id="42", name="Parent") + updated_tag33_2 = tag33.update(name="academy-updated", parent=parent_tag) + updated_tag33_3 = tag33.update(name="academy-updated", parent_id=parent_tag["id"]) + + # assert + assert mock_get_33_tag.call_count == 1 + assert mock_update_33_tag.call_count == 4 + + for tag in [updated_tag33_0, updated_tag33_1, updated_tag33_2, updated_tag33_3]: + assert isinstance(tag, Tag) + + # Asserting updated values are deferred to integration testing + # to avoid agreening with the mocked data class TestContentItemTags: From 62d6b379bd2f068cdaf8953a13436fe842d9ea84 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Mon, 9 Dec 2024 14:17:46 -0500 Subject: [PATCH 2/2] Rearrange code logic to return early, sooner --- src/posit/connect/tags.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/posit/connect/tags.py b/src/posit/connect/tags.py index be1aec32..4d3c0b20 100644 --- a/src/posit/connect/tags.py +++ b/src/posit/connect/tags.py @@ -25,26 +25,22 @@ def _update_parent_kwargs(kwargs: dict) -> dict: Asserts that the `parent=` and `parent_id=` keys are not both provided. """ parent = kwargs.get("parent", None) + parent_id = kwargs.get("parent_id", None) if parent is None: # No parent to upgrade, return the kwargs as is return kwargs - + if parent and parent_id: + raise ValueError("Cannot provide both `parent=` and `parent_id=`") if not isinstance(parent, Tag): raise TypeError( "`parent=` must be a Tag instance. If using a string, please use `parent_id=`" ) - parent_id = kwargs.get("parent_id", None) - if parent_id: - raise ValueError("Cannot provide both `parent=` and `parent_id=`") - + # Remove `parent` from a copy of `kwargs` and replace it with `parent_id` ret_kwargs = {**kwargs} - - # Remove `parent` from ret_kwargs - # and store the `parent_id` in the ret_kwargs below del ret_kwargs["parent"] - ret_kwargs["parent_id"] = parent["id"] + return ret_kwargs