From 27d8aa7f43ae78120760744b068e8b4c191b31cf Mon Sep 17 00:00:00 2001 From: Renato Byrro Date: Mon, 27 Jan 2025 12:23:00 -0800 Subject: [PATCH] Fix bug in slack tool pagination (#232) The `get_conversation_metadata_by_name` tool retrieves conversation metadata from another tool, `list_conversations_metadata`, but was accessing the `next_cursor` using the Slack API response dict structure, instead of the tool response structure. As a result, in that tool, the tool would never actually paginate to the second page. This PR fixes it and also adjust tests to capture the issue appropriately. --- toolkits/slack/arcade_slack/tools/chat.py | 2 +- toolkits/slack/tests/test_chat.py | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/toolkits/slack/arcade_slack/tools/chat.py b/toolkits/slack/arcade_slack/tools/chat.py index 31c508cd..17349395 100644 --- a/toolkits/slack/arcade_slack/tools/chat.py +++ b/toolkits/slack/arcade_slack/tools/chat.py @@ -489,7 +489,7 @@ async def get_conversation_metadata_by_name( while should_continue: response = await list_conversations_metadata(context, next_cursor=next_cursor) - next_cursor = response.get("response_metadata", {}).get("next_cursor") + next_cursor = response["next_cursor"] for conversation in response["conversations"]: response_conversation_name = ( diff --git a/toolkits/slack/tests/test_chat.py b/toolkits/slack/tests/test_chat.py index 48f34995..19111b4d 100644 --- a/toolkits/slack/tests/test_chat.py +++ b/toolkits/slack/tests/test_chat.py @@ -316,7 +316,7 @@ async def test_get_conversation_metadata_by_name( sample_conversation = extract_conversation_metadata(mock_channel_info) mock_list_conversations_metadata.return_value = { "conversations": [sample_conversation], - "response_metadata": {"next_cursor": None}, + "next_cursor": None, } response = await get_conversation_metadata_by_name(mock_context, sample_conversation["name"]) @@ -336,11 +336,11 @@ async def test_get_conversation_metadata_by_name_triggering_pagination( mock_list_conversations_metadata.side_effect = [ { "conversations": [another_conversation], - "response_metadata": {"next_cursor": "123"}, + "next_cursor": "123", }, { "conversations": [target_conversation], - "response_metadata": {"next_cursor": None}, + "next_cursor": None, }, ] @@ -365,11 +365,11 @@ async def test_get_conversation_metadata_by_name_not_found( mock_list_conversations_metadata.side_effect = [ { "conversations": [second_conversation], - "response_metadata": {"next_cursor": "123"}, + "next_cursor": "123", }, { "conversations": [first_conversation], - "response_metadata": {"next_cursor": None}, + "next_cursor": None, }, ] @@ -505,11 +505,11 @@ async def test_get_members_in_conversation_by_name_triggering_pagination( mock_list_conversations_metadata.side_effect = [ { "conversations": [extract_conversation_metadata(conversation1)], - "response_metadata": {"next_cursor": "123"}, + "next_cursor": "123", }, { "conversations": [extract_conversation_metadata(conversation2)], - "response_metadata": {"next_cursor": None}, + "next_cursor": None, }, ]