Async rest client in Channels #1101
Conversation
… instead of requests. 2. Fixed unit and integration test cases for the same.
WalkthroughWalkthroughThe suite of updates predominantly transitions various components from synchronous to asynchronous operations, enhancing the system's efficiency and scalability. The affected areas include interaction with Facebook Messenger, WhatsApp, and Microsoft Teams through their respective APIs, as well as internal message broadcasting and testing frameworks. The move to Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 40
Configuration used: CodeRabbit UI
Files selected for processing (12)
- kairon/chat/handlers/channels/clients/messenger/messenger_client.py (1 hunks)
- kairon/chat/handlers/channels/clients/whatsapp/cloud.py (7 hunks)
- kairon/chat/handlers/channels/clients/whatsapp/dialog360.py (2 hunks)
- kairon/chat/handlers/channels/clients/whatsapp/on_premise.py (3 hunks)
- kairon/chat/handlers/channels/messenger.py (4 hunks)
- kairon/chat/handlers/channels/msteams.py (5 hunks)
- kairon/events/definitions/base.py (1 hunks)
- kairon/events/definitions/message_broadcast.py (2 hunks)
- kairon/shared/channels/broadcast/whatsapp.py (6 hunks)
- tests/integration_test/chat_service_test.py (10 hunks)
- tests/unit_test/channel_client_test.py (11 hunks)
- tests/unit_test/events/events_test.py (19 hunks)
Additional comments: 34
kairon/events/definitions/base.py (1)
- 17-18: The
executemethod has been correctly updated to an asynchronous definition. Ensure that all calls to this method are updated to useawaitand that the event loop is properly managed where this method is invoked.kairon/chat/handlers/channels/clients/whatsapp/dialog360.py (2)
1-1: The import of
loggerfromloguruis appropriate for adding logging capabilities to the module.27-44: The
send_actionandmark_as_readmethods have been correctly updated to be asynchronous. Ensure that all usages of these methods are updated to handle the asynchronous nature, including usingawaitwhen calling them.kairon/chat/handlers/channels/clients/whatsapp/on_premise.py (2)
4-6: The import of
AioRestClientis correct and necessary for the asynchronous operations being introduced in this module.27-68: The methods
send_action,get_attachment,mark_as_read, andsend_template_messagehave been updated to be asynchronous. Ensure that all usages of these methods are updated to handle the asynchronous nature, including usingawaitwhen calling them.kairon/chat/handlers/channels/clients/whatsapp/cloud.py (2)
8-8: The import of
AioRestClientreplaces the synchronousrequestsmodule, aligning with the PR's goal of introducing asynchronous operations.104-144: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [63-163]
The methods
send,send_json,send_action,get_attachment,mark_as_read, andsend_template_messagehave been updated to be asynchronous. Ensure that all usages of these methods are updated to handle the asynchronous nature, including usingawaitwhen calling them.kairon/events/definitions/message_broadcast.py (1)
- 52-58: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [43-58]
The
executemethod within theMessageBroadcastclass has been correctly updated to be asynchronous. Ensure that all calls to this method are updated to useawaitand that the event loop is properly managed where this method is invoked.kairon/shared/channels/broadcast/whatsapp.py (3)
1-10: The imports of
asyncio,uuid6, andDoesNotExistare appropriate for the new asynchronous functionality and error handling being introduced.43-51: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [46-61]
The
sendmethod has been correctly updated to be asynchronous. Ensure that all usages of this method are updated to handle the asynchronous nature, including usingawaitwhen calling it.
- 88-94: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [91-115]
The
__send_using_configurationmethod has been correctly updated to be asynchronous. Ensure that all usages of this method are updated to handle the asynchronous nature, including usingawaitwhen calling it.kairon/chat/handlers/channels/clients/messenger/messenger_client.py (2)
- 64-72: The
make_requestmethod implementation.
Ensure that themake_requestmethod correctly handles different HTTP methods and responses. It's important to check if the response is being processed correctly and if there's proper error handling for failed requests.Verification successful
The
make_requestmethod in themessenger_client.pyfile has been successfully extracted. The method takes several parameters, includingmethod,url,request_body,headers, and additional keyword arguments. It uses these parameters to make an asynchronous request via thesession.requestmethod. The method also supports streaming responses and custom timeouts, and it retries up to three times if necessary.The implementation includes popping of
is_streaming_respandtimeoutfrom the keyword arguments, which are then passed to thesession.requestcall. The response is logged at the debug level and then returned.Based on the provided output, the following observations can be made:
- The method appears to be flexible, handling different HTTP methods as indicated by the
methodparameter.- It allows for both standard and streaming responses.
- Custom headers and request bodies can be provided.
- A custom timeout can be set, and a default of three retries is in place.
- The response is logged for debugging purposes.
However, the output does not show explicit error handling within the
make_requestmethod itself. Error handling might be implemented within thesession.requestmethod or it might be expected that the caller handles any exceptions raised bymake_request.Given the information available, the
make_requestmethod seems to be implemented with consideration for different HTTP methods and responses. For a complete verification, one would need to review thesession.requestmethod or the error handling practices in the calling code to ensure robust error handling is in place.* 204-213: The `generate_appsecret_proof` method. This method is synchronous. If there's no I/O operation or heavy computation, it's fine to keep it synchronous. However, if it's called within an async context, ensure it doesn't block the event loop.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the make_request method is used consistently and correctly throughout the codebase. rg --type python 'await .+\.make_request\('Length of output: 76
Script:
#!/bin/bash # Search for usages of the make_request method across the codebase without specifying file type. rg 'await .+\.make_request\('Length of output: 2598
Script:
#!/bin/bash # Extract the implementation details of the make_request method from the messenger_client.py file. rg --context 10 'async def make_request\(self,' kairon/chat/handlers/channels/clients/messenger/messenger_client.pyLength of output: 1034
kairon/chat/handlers/channels/msteams.py (1)
- 56-62: The
make_requestmethod implementation.
Ensure that themake_requestmethod correctly handles different HTTP methods and responses. It's important to check if the response is being processed correctly and if there's proper error handling for failed requests.Verification successful
The shell script output provides the implementation of the
make_requestmethod from bothmsteams.pyandmessenger_client.py. Here are the key points from the implementation:
- Both methods use
self.session.requestto perform the HTTP request, which is likely a part of an asynchronous HTTP client.- They accept various parameters such as
method,url,request_body,headers, andtimeout.- The
is_streaming_respparameter is used to determine if the response should be streamed, andmax_retriesis set to 3, indicating some level of retry logic.- The response is logged using
logger.debug(resp)before being returned.From this implementation, we can infer the following:
- The method is designed to handle different HTTP methods, as indicated by the
methodparameter.- The method does not directly handle different HTTP responses or errors within its body. Instead, it relies on the caller to handle the response and any exceptions that may arise from
self.session.request.- Proper error handling for failed requests would need to be implemented in the code that calls
make_request, as this method simply returns the response object.Given the above points, the
make_requestmethod appears to be a generic asynchronous request handler that delegates response processing and error handling to the caller. Therefore, the verification of proper error handling and response processing should focus on the code that calls this method, rather than the method itself.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the make_request method is used consistently and correctly throughout the codebase. rg --type python 'await .+\.make_request\('Length of output: 76
Script:
#!/bin/bash # Verify that the make_request method is used consistently and correctly throughout the codebase. rg 'await .+\.make_request\('Length of output: 2598
Script:
#!/bin/bash # Examine the implementation of the make_request method in msteams.py and messenger_client.py # to verify the handling of different HTTP responses and error handling. cat kairon/chat/handlers/channels/msteams.py kairon/chat/handlers/channels/clients/messenger/messenger_client.py | rg 'async def make_request'Length of output: 353
Script:
#!/bin/bash # View the full implementation of the make_request method in msteams.py and messenger_client.py. ast-grep --lang python --pattern $'async def make_request($$$)'Length of output: 409
Script:
#!/bin/bash # Adjust the pattern to match the full implementation of the make_request method. ast-grep --lang python --pattern $'async def make_request(self, method, url, $$$)'Length of output: 428
Script:
#!/bin/bash # Use cat and rg to extract the full implementation of the make_request method from msteams.py and messenger_client.py. cat kairon/chat/handlers/channels/msteams.py kairon/chat/handlers/channels/clients/messenger/messenger_client.py | rg -A 10 'async def make_request'Length of output: 1401
kairon/chat/handlers/channels/messenger.py (4)
5-22: The introduction of
MessengerClientOutputand other related imports aligns with the PR's objective to implement asynchronous operations. Ensure that the newly added imports are used within the file and that there are no unused imports which could lead to unnecessary memory consumption and potential confusion.35-40: The
Messengerclass constructor now initializesMessengerClientOutputwith thepage_access_token. This change is consistent with the PR's goal of using an asynchronous client. Verify that theMessengerClientOutputclass is fully compatible with theMessengerclass and that all methods previously called on the synchronous client are available and correctly implemented in the async client.178-184: The
MessengerBotclass has been updated to acceptMessengerClientOutputas a parameter in its constructor, which is in line with the shift to asynchronous operations. Ensure that all instances ofMessengerBotare updated to pass the newMessengerClientOutputclient and that theMessengerBotclass methods are compatible with the async client.412-417: The
get_output_channelmethod in theMessengerHandlerclass has been updated to instantiateMessengerClientOutput. This change is necessary for the transition to asynchronous operations. Verify that theMessengerClientOutputis correctly instantiated with all required parameters and that theMessengerBotclass is prepared to handle the async client.tests/unit_test/channel_client_test.py (16)
23-33: The test
test_send_actionhas been correctly updated to be asynchronous and usesmock.patch.objectto patch therequestmethod of thewhatsapp_on_premise.client. The assertions and the use ofassert_awaited_once_withare appropriate for the async context.50-60: The test
test_get_attachmentis correctly updated to async and properly mocks therequestmethod. The assertionassert_awaited_once_withis used correctly.64-74: The test
test_get_attachment_failureis correctly updated to async and properly mocks therequestmethod. The assertionassert_awaited_once_withis used correctly.78-88: The test
test_mark_as_readis correctly updated to async and properly mocks therequestmethod. The assertionassert_awaited_once_withis used correctly.92-102: The test
test_mark_as_read_failureis correctly updated to async and properly mocks therequestmethod. The assertionassert_awaited_once_withis used correctly.139-148: The test
test_send_template_message_without_namespacecorrectly raises anAppExceptionwhen the namespace is not provided. The async update is appropriate.191-197: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [163-209]
The test
test_whatsapp_cloud_send_template_messageis correctly updated to async and uses thesendmethod with theautospec=Trueparameter. The assertions are appropriate for the async context.
211-223: The test
test_whatsapp_cloud_send_template_message_without_payloadis correctly updated to async and uses thesendmethod with theautospec=Trueparameter. The assertions are appropriate for the async context.225-239: The test
test_whatsapp_cloud_send_template_message_with_namespaceis correctly updated to async and uses thesendmethod with theautospec=Trueparameter. The assertions are appropriate for the async context.241-249: The test
test_whatsapp_cloud_send_template_message_failureis correctly updated to async and uses thesendmethod with theautospec=Trueparameter. The assertions are appropriate for the async context.278-317: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [252-294]
The test
test_whatsapp_cloud_send_template_message_with_360dialogis correctly updated to async and uses theaioresponsesfixture to mock the async HTTP call. The assertions are appropriate for the async context.
297-311: The test
test_whatsapp_cloud_mark_read_360dialogis correctly updated to async and uses theaioresponsesfixture to mock the async HTTP call. The assertions are appropriate for the async context.353-368: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [314-384]
The test
test_whatsapp_cloud_send_template_message_with_360dialog_failureis correctly updated to async and uses theaioresponsesfixture to mock the async HTTP call. The assertions are appropriate for the async context.
- 401-422: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [387-416]
The test
test_whatsapp_cloud_mark_read_360dialog_failureis correctly updated to async and uses theaioresponsesfixture to mock the async HTTP call. The assertions are appropriate for the async context.
- 425-451: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [419-445]
The test
test_whatsapp_cloud_send_message_with_360dialogis correctly updated to async and uses theaioresponsesfixture to mock the async HTTP call. The assertions are appropriate for the async context.
- 467-487: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [448-487]
The test
test_whatsapp_cloud_send_message_with_360dialog_failureis correctly updated to async and uses theaioresponsesfixture to mock the async HTTP call. The assertions are appropriate for the async context.
| def test_whatsapp_valid_text_message_request(aioresponses): | ||
| def _mock_validate_hub_signature(*args, **kwargs): | ||
| return True | ||
|
|
||
| responses.add( | ||
| "POST", "https://graph.facebook.com/v13.0/12345678/messages", json={} | ||
| aioresponses.add( | ||
| method=responses.POST, | ||
| url="https://graph.facebook.com/v13.0/12345678/messages", | ||
| body="success", | ||
| status=200 |
There was a problem hiding this comment.
The use of responses instead of aioresponses is incorrect for asynchronous tests.
- method=responses.POST,
+ method=aioresponses.POST,Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def test_whatsapp_valid_text_message_request(aioresponses): | |
| def _mock_validate_hub_signature(*args, **kwargs): | |
| return True | |
| responses.add( | |
| "POST", "https://graph.facebook.com/v13.0/12345678/messages", json={} | |
| aioresponses.add( | |
| method=responses.POST, | |
| url="https://graph.facebook.com/v13.0/12345678/messages", | |
| body="success", | |
| status=200 | |
| def test_whatsapp_valid_text_message_request(aioresponses): | |
| def _mock_validate_hub_signature(*args, **kwargs): | |
| return True | |
| aioresponses.add( | |
| method=aioresponses.POST, | |
| url="https://graph.facebook.com/v13.0/12345678/messages", | |
| body="success", | |
| status=200 |
| @pytest.mark.asyncio | ||
| async def test_send_action_failure(self, whatsapp_on_premise): | ||
| with mock.patch.object(whatsapp_on_premise.client, 'request') as mock_post: | ||
| mock_post.return_value = {"error": {"message": "Message Undeliverable", "code": 400}} | ||
| response = await whatsapp_on_premise.send_action(payload={"text": " "}) | ||
| mock_post.assert_called_once_with( | ||
| 'POST', | ||
| 'https://graph.facebook.com/v13.0/messages', | ||
| headers=whatsapp_on_premise.auth_args, | ||
| json={'text': ' '}, timeout=None | ||
| request_body={'text': ' '}, timeout=None, return_json=False, is_streaming_resp=False, | ||
| max_retries=3 |
There was a problem hiding this comment.
The test test_send_action_failure is correctly updated for async execution. However, the assert_called_once_with should be assert_awaited_once_with to match the asynchronous context.
- mock_post.assert_called_once_with(
+ mock_post.assert_awaited_once_with(Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| @pytest.mark.asyncio | |
| async def test_send_action_failure(self, whatsapp_on_premise): | |
| with mock.patch.object(whatsapp_on_premise.client, 'request') as mock_post: | |
| mock_post.return_value = {"error": {"message": "Message Undeliverable", "code": 400}} | |
| response = await whatsapp_on_premise.send_action(payload={"text": " "}) | |
| mock_post.assert_called_once_with( | |
| 'POST', | |
| 'https://graph.facebook.com/v13.0/messages', | |
| headers=whatsapp_on_premise.auth_args, | |
| json={'text': ' '}, timeout=None | |
| request_body={'text': ' '}, timeout=None, return_json=False, is_streaming_resp=False, | |
| max_retries=3 | |
| @pytest.mark.asyncio | |
| async def test_send_action_failure(self, whatsapp_on_premise): | |
| with mock.patch.object(whatsapp_on_premise.client, 'request') as mock_post: | |
| mock_post.return_value = {"error": {"message": "Message Undeliverable", "code": 400}} | |
| response = await whatsapp_on_premise.send_action(payload={"text": " "}) | |
| mock_post.assert_awaited_once_with( | |
| 'POST', | |
| 'https://graph.facebook.com/v13.0/messages', | |
| headers=whatsapp_on_premise.auth_args, | |
| request_body={'text': ' '}, timeout=None, return_json=False, is_streaming_resp=False, | |
| max_retries=3 |
| def test_whatsapp_exception_when_try_to_handle_webhook_for_whatsapp_message(mock_process_message, aioresponses): | ||
| def _mock_validate_hub_signature(*args, **kwargs): | ||
| return True | ||
|
|
||
| responses.add( | ||
| "POST", "https://graph.facebook.com/v13.0/12345678/messages", json={} | ||
| aioresponses.add( | ||
| method=responses.POST, | ||
| url="https://graph.facebook.com/v13.0/12345678/messages", | ||
| body="success", | ||
| status=200 |
There was a problem hiding this comment.
The use of responses instead of aioresponses is incorrect for asynchronous tests.
- method=responses.POST,
+ method=aioresponses.POST,Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def test_whatsapp_exception_when_try_to_handle_webhook_for_whatsapp_message(mock_process_message, aioresponses): | |
| def _mock_validate_hub_signature(*args, **kwargs): | |
| return True | |
| responses.add( | |
| "POST", "https://graph.facebook.com/v13.0/12345678/messages", json={} | |
| aioresponses.add( | |
| method=responses.POST, | |
| url="https://graph.facebook.com/v13.0/12345678/messages", | |
| body="success", | |
| status=200 | |
| def test_whatsapp_exception_when_try_to_handle_webhook_for_whatsapp_message(mock_process_message, aioresponses): | |
| def _mock_validate_hub_signature(*args, **kwargs): | |
| return True | |
| aioresponses.add( | |
| method=aioresponses.POST, | |
| url="https://graph.facebook.com/v13.0/12345678/messages", | |
| body="success", | |
| status=200 |
| def test_whatsapp_valid_button_message_request(aioresponses): | ||
| def _mock_validate_hub_signature(*args, **kwargs): | ||
| return True | ||
|
|
||
| responses.add( | ||
| "POST", "https://graph.facebook.com/v13.0/12345678/messages", json={} | ||
| aioresponses.add( | ||
| method=responses.POST, | ||
| url="https://graph.facebook.com/v13.0/12345678/messages", | ||
| body="success", | ||
| status=200 |
There was a problem hiding this comment.
The use of responses instead of aioresponses is incorrect for asynchronous tests.
- method=responses.POST,
+ method=aioresponses.POST,Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def test_whatsapp_valid_button_message_request(aioresponses): | |
| def _mock_validate_hub_signature(*args, **kwargs): | |
| return True | |
| responses.add( | |
| "POST", "https://graph.facebook.com/v13.0/12345678/messages", json={} | |
| aioresponses.add( | |
| method=responses.POST, | |
| url="https://graph.facebook.com/v13.0/12345678/messages", | |
| body="success", | |
| status=200 | |
| def test_whatsapp_valid_button_message_request(aioresponses): | |
| def _mock_validate_hub_signature(*args, **kwargs): | |
| return True | |
| aioresponses.add( | |
| method=aioresponses.POST, | |
| url="https://graph.facebook.com/v13.0/12345678/messages", | |
| body="success", | |
| status=200 |
| @pytest.mark.asyncio | ||
| async def test_send_template_message(self, whatsapp_on_premise): | ||
| namespace = "test_namespace" | ||
| name = "test_template_name" | ||
| to_phone_number = "9876543210" | ||
| with patch("kairon.chat.handlers.channels.clients.whatsapp.on_premise.WhatsappOnPremise.send", | ||
| autospec=True) as mock_send: | ||
| mock_send.return_value = { | ||
| "contacts": [{"input": "+55123456789", "status": "valid", "wa_id": "55123456789"}]} | ||
| response = whatsapp_on_premise.send_template_message(namespace=namespace, name=name, | ||
| response = await whatsapp_on_premise.send_template_message(namespace=namespace, name=name, | ||
| to_phone_number=to_phone_number) | ||
| assert response == {"contacts": [{"input": "+55123456789", "status": "valid", "wa_id": "55123456789"}]} | ||
| assert mock_send.call_args[0][1] == {'language': {'code': 'en', 'policy': 'deterministic'}, | ||
| 'name': 'test_template_name', 'namespace': 'test_namespace'} | ||
| assert mock_send.call_args[0][2] == '9876543210' | ||
| assert mock_send.call_args[1] == {'messaging_type': 'template'} |
There was a problem hiding this comment.
The test test_send_template_message is correctly updated to async. However, the send method is patched without specifying the awaited attribute. This should be updated to reflect the asynchronous nature of the method being tested.
- with patch("kairon.chat.handlers.channels.clients.whatsapp.on_premise.WhatsappOnPremise.send",
+ with patch("kairon.chat.handlers.channels.clients.whatsapp.on_premise.WhatsappOnPremise.send.awaited",Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| @pytest.mark.asyncio | |
| async def test_send_template_message(self, whatsapp_on_premise): | |
| namespace = "test_namespace" | |
| name = "test_template_name" | |
| to_phone_number = "9876543210" | |
| with patch("kairon.chat.handlers.channels.clients.whatsapp.on_premise.WhatsappOnPremise.send", | |
| autospec=True) as mock_send: | |
| mock_send.return_value = { | |
| "contacts": [{"input": "+55123456789", "status": "valid", "wa_id": "55123456789"}]} | |
| response = whatsapp_on_premise.send_template_message(namespace=namespace, name=name, | |
| response = await whatsapp_on_premise.send_template_message(namespace=namespace, name=name, | |
| to_phone_number=to_phone_number) | |
| assert response == {"contacts": [{"input": "+55123456789", "status": "valid", "wa_id": "55123456789"}]} | |
| assert mock_send.call_args[0][1] == {'language': {'code': 'en', 'policy': 'deterministic'}, | |
| 'name': 'test_template_name', 'namespace': 'test_namespace'} | |
| assert mock_send.call_args[0][2] == '9876543210' | |
| assert mock_send.call_args[1] == {'messaging_type': 'template'} | |
| @pytest.mark.asyncio | |
| async def test_send_template_message(self, whatsapp_on_premise): | |
| namespace = "test_namespace" | |
| name = "test_template_name" | |
| to_phone_number = "9876543210" | |
| with patch("kairon.chat.handlers.channels.clients.whatsapp.on_premise.WhatsappOnPremise.send.awaited", | |
| autospec=True) as mock_send: | |
| mock_send.return_value = { | |
| "contacts": [{"input": "+55123456789", "status": "valid", "wa_id": "55123456789"}]} | |
| response = await whatsapp_on_premise.send_template_message(namespace=namespace, name=name, | |
| to_phone_number=to_phone_number) | |
| assert response == {"contacts": [{"input": "+55123456789", "status": "valid", "wa_id": "55123456789"}]} | |
| assert mock_send.call_args[0][1] == {'language': {'code': 'en', 'policy': 'deterministic'}, | |
| 'name': 'test_template_name', 'namespace': 'test_namespace'} | |
| assert mock_send.call_args[0][2] == '9876543210' | |
| assert mock_send.call_args[1] == {'messaging_type': 'template'} |
| event = MessageBroadcastEvent(bot, user) | ||
| event.validate() | ||
| event_id = event.enqueue(EventRequestType.trigger_async.value, config=config) | ||
| event.execute(event_id) | ||
| result = await event.execute(event_id) |
There was a problem hiding this comment.
The test test_execute_message_broadcast_with_pyscript_timeout should verify the result of the event execution to ensure it matches the expected outcome.
| def __init__(self, page_access_token, **kwargs): | ||
| """ | ||
| @required: | ||
| page_access_token | ||
| @optional: | ||
| session | ||
| api_version | ||
| app_secret | ||
| """ | ||
|
|
||
| self.page_access_token = page_access_token | ||
| self.session = AioRestClient(False) | ||
| self.api_version = kwargs.get('api_version', DEFAULT_API_VERSION) | ||
| self.graph_url = 'https://graph.facebook.com/v{api_version}'.format(api_version=self.api_version) | ||
| self.app_secret = kwargs.get('app_secret') |
There was a problem hiding this comment.
Initialization of AioRestClient and usage of api_version in the formatted URL.
- self.session = AioRestClient(False)
+ self.session = AioRestClient()Consider removing the False argument if it's not required for AioRestClient initialization. Also, ensure that api_version is always a valid version string to avoid runtime errors in URL formatting.
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def __init__(self, page_access_token, **kwargs): | |
| """ | |
| @required: | |
| page_access_token | |
| @optional: | |
| session | |
| api_version | |
| app_secret | |
| """ | |
| self.page_access_token = page_access_token | |
| self.session = AioRestClient(False) | |
| self.api_version = kwargs.get('api_version', DEFAULT_API_VERSION) | |
| self.graph_url = 'https://graph.facebook.com/v{api_version}'.format(api_version=self.api_version) | |
| self.app_secret = kwargs.get('app_secret') | |
| def __init__(self, page_access_token, **kwargs): | |
| """ | |
| @required: | |
| page_access_token | |
| @optional: | |
| session | |
| api_version | |
| app_secret | |
| """ | |
| self.page_access_token = page_access_token | |
| self.session = AioRestClient() | |
| self.api_version = kwargs.get('api_version', DEFAULT_API_VERSION) | |
| self.graph_url = 'https://graph.facebook.com/v{api_version}'.format(api_version=self.api_version) | |
| self.app_secret = kwargs.get('app_secret') |
| @@ -42,6 +42,7 @@ def __init__( | |||
| conversation: Dict[Text, Any], | |||
| bot: Text, | |||
| service_url: Text, | |||
| session: AioRestClient(False), | |||
There was a problem hiding this comment.
💡 NOTE
Codebase verification is a beta feature.
The output from the script shows the instantiation of MSTeamBot but does not include the session parameter in the constructor call. This suggests that the instantiation of MSTeamBot has not been updated to include the new session parameter as required by the changes in the __init__ method.
Based on this information, the following issue has been identified:
- The instantiation of
MSTeamBotin themsteams.pyfile does not include the newsessionparameter, which is now required after the changes made to the__init__method of theMSTeamBotclass.
Analysis chain
Addition of session parameter in the __init__ method of MSTeamBot class.
Ensure that the session parameter is documented and that all instantiations of MSTeamBot are updated to pass this new parameter.
It seems there was an issue with the previous script due to an unrecognized file type specifier. Let's correct the script by removing the file type specifier and rerun the search for instances of MSTeamBot instantiation.
The output indicates that there are instances of MSTeamBot being instantiated in the msteams.py file. However, the output does not show the parameters being passed to these instantiations, which is necessary to verify if the new session parameter is included.
To ensure that the session parameter is being correctly passed to all instances of MSTeamBot, we need to examine the full instantiation code. Let's generate a new script to view the lines of code where MSTeamBot is instantiated.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all instantiations of MSTeamBot include the new session parameter.
rg --type python 'MSTeamBot\('
Length of output: 63
Script:
#!/bin/bash
# Verify that all instantiations of MSTeamBot include the new session parameter.
rg 'MSTeamBot\('
Length of output: 243
Script:
#!/bin/bash
# Extract the lines of code around the instantiation of MSTeamBot to check for the session parameter.
rg -C 3 'MSTeamBot\(' kairon/chat/handlers/channels/msteams.py
Length of output: 662
| token_response = await self.make_request("POST", uri, request_body=payload) | ||
|
|
||
| if token_response.ok: | ||
| token_data = token_response.json() | ||
| access_token = token_data["access_token"] | ||
| token_expiration = token_data["expires_in"] | ||
| if token_response.status_code == 200: | ||
| access_token = token_response["access_token"] | ||
| token_expiration = token_response["expires_in"] | ||
|
|
||
| delta = datetime.timedelta(seconds=int(token_expiration)) | ||
| MSTeamBot.token_expiration_date = datetime.datetime.now() + delta |
There was a problem hiding this comment.
Handling of token response status and access token extraction in _get_headers.
Ensure that the response status and token extraction are handled correctly. The status code check should be against HTTPStatus.OK for clarity.
- if token_response.status_code == 200:
+ if token_response.status_code == HTTPStatus.OK:Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| token_response = await self.make_request("POST", uri, request_body=payload) | |
| if token_response.ok: | |
| token_data = token_response.json() | |
| access_token = token_data["access_token"] | |
| token_expiration = token_data["expires_in"] | |
| if token_response.status_code == 200: | |
| access_token = token_response["access_token"] | |
| token_expiration = token_response["expires_in"] | |
| delta = datetime.timedelta(seconds=int(token_expiration)) | |
| MSTeamBot.token_expiration_date = datetime.datetime.now() + delta | |
| token_response = await self.make_request("POST", uri, request_body=payload) | |
| if token_response.status_code == HTTPStatus.OK: | |
| access_token = token_response["access_token"] | |
| token_expiration = token_response["expires_in"] | |
| delta = datetime.timedelta(seconds=int(token_expiration)) | |
| MSTeamBot.token_expiration_date = datetime.datetime.now() + delta |
| if not send_response != 200: | ||
| logger.error( | ||
| "Error trying to send botframework messge. Response: %s", | ||
| send_response.text, | ||
| ) | ||
| raise Exception(f"Exception while responding to MSTeams:: {send_response.text} and status::{send_response.status_code}") |
There was a problem hiding this comment.
Error handling and logging improvements in the send method.
The condition if not send_response != 200: seems to be incorrect. It should be checking for a status code that indicates failure.
- if not send_response != 200:
+ if send_response.status_code != HTTPStatus.OK:Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if not send_response != 200: | |
| logger.error( | |
| "Error trying to send botframework messge. Response: %s", | |
| send_response.text, | |
| ) | |
| raise Exception(f"Exception while responding to MSTeams:: {send_response.text} and status::{send_response.status_code}") | |
| if send_response.status_code != HTTPStatus.OK: | |
| logger.error( | |
| "Error trying to send botframework messge. Response: %s", | |
| send_response.text, | |
| ) | |
| raise Exception(f"Exception while responding to MSTeams:: {send_response.text} and status::{send_response.status_code}") |
… instead of requests. 2. Fixed unit and integration test cases for the same.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tests/integration_test/chat_service_test.py (10 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/integration_test/chat_service_test.py
Summary by CodeRabbit
New Features
MessengerClientOutputclass for enhanced interaction with the Facebook Messenger API.Improvements
Bug Fixes
Tests
Documentation