Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix telegram message update #8719

Merged
merged 1 commit into from
May 27, 2021
Merged

Fix telegram message update #8719

merged 1 commit into from
May 27, 2021

Conversation

IzidoroBaltazar
Copy link
Contributor

@IzidoroBaltazar IzidoroBaltazar commented May 19, 2021

Proposed changes:

  • add handling of edited message on the Telegram channel
  • if the edited message is unhandled it is causing crash and all communication on the channel is stopped

When receiving edited message telegram channel connector crashes:

Exception occurred while handling uri: 'http://***/webhooks/telegram/webhook'
Traceback (most recent call last):
  File "/home/martin/anaconda3/envs/rasa-2.4/lib/python3.7/site-packages/sanic/app.py", line 976, in handle_request
    response = await response
  File "/home/martin/anaconda3/envs/rasa-2.4/lib/python3.7/site-packages/rasa/core/channels/telegram.py", line 218, in message
    if self._is_user_message(msg):
  File "/home/martin/anaconda3/envs/rasa-2.4/lib/python3.7/site-packages/rasa/core/channels/telegram.py", line 177, in _is_user_message
    return message.text is not None
AttributeError: 'NoneType' object has no attribute 'text'
Exception occurred while handling uri: 'http://***/webhooks/telegram/webhook'
Traceback (most recent call last):
  File "/home/martin/anaconda3/envs/rasa-2.4/lib/python3.7/site-packages/sanic/app.py", line 976, in handle_request
    response = await response
  File "/home/martin/anaconda3/envs/rasa-2.4/lib/python3.7/site-packages/rasa/core/channels/telegram.py", line 218, in message
    if self._is_user_message(msg):
  File "/home/martin/anaconda3/envs/rasa-2.4/lib/python3.7/site-packages/rasa/core/channels/telegram.py", line 177, in _is_user_message
    return message.text is not None
AttributeError: 'NoneType' object has no attribute 'text'
Exception occurred while handling uri: 'http://***/webhooks/telegram/webhook'
Traceback (most recent call last):
  File "/home/martin/anaconda3/envs/rasa-2.4/lib/python3.7/site-packages/sanic/app.py", line 976, in handle_request
    response = await response
  File "/home/martin/anaconda3/envs/rasa-2.4/lib/python3.7/site-packages/rasa/core/channels/telegram.py", line 218, in message
    if self._is_user_message(msg):
  File "/home/martin/anaconda3/envs/rasa-2.4/lib/python3.7/site-packages/rasa/core/channels/telegram.py", line 177, in _is_user_message
    return message.text is not None
AttributeError: 'NoneType' object has no attribute 'text'

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@sara-tagger sara-tagger requested a review from kedz May 19, 2021 12:00
@sara-tagger
Copy link
Collaborator

Thanks for submitting a pull request 🚀 @kedz will take a look at it as soon as possible ✨

@ancalita ancalita self-requested a review May 20, 2021 10:00
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

Hi @IzidoroBaltazar thank you for your contribution 🚀 I'd like to ask you to:

  • add more context in the proposed changes (details of the error causing bot to crash) or link to GH issue if it exists
  • add a test for this added functionality: you can either create a new file in tests.core.channels. as test_telegram.py or you can add test in module tests.core.test_channels.py

@kedz kedz removed their request for review May 20, 2021 18:32
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

I'm wondering if it's worth adding an extra assert in the test to check that the message received by calling GET to the same route is the same as the edited_message which was posted?

@IzidoroBaltazar
Copy link
Contributor Author

I'm wondering if it's worth adding an extra assert in the test to check that the message received by calling GET to the same route is the same as the edited_message which was posted?

I don't understand this requirement. Have a look at the webhook endpoint. GET is allowed but it's not implemented. Do you want to change behavior and return 200/success on GET calls? Currently on any GET call you will get 500.

@telegram_webhook.route("/webhook", methods=["GET", "POST"])        
async def message(request: Request) ->; Any:
    if request.method == "POST":

^ there is only POST branch in the if statement nothing else.

@ancalita
Copy link
Member

I'm wondering if it's worth adding an extra assert in the test to check that the message received by calling GET to the same route is the same as the edited_message which was posted?

I don't understand this requirement. Have a look at the webhook endpoint. GET is allowed but it's not implemented. Do you want to change behavior and return 200/success on GET calls? Currently on any GET call you will get 500.

@telegram_webhook.route("/webhook", methods=["GET", "POST"])        
async def message(request: Request) ->; Any:
    if request.method == "POST":

^ there is only POST branch in the if statement nothing else.

If the request.method is GET, then this line will execute. I will double-check with the team if this should be changed.

@ancalita
Copy link
Member

If the request.method is GET, then this line will execute. I will double-check with the team if this should be changed.

@IzidoroBaltazar I've checked with the team and the way GET request.method is handled should be improved. However this is beyond the scope of this PR, so I'll submit a separate issue for this improvement which will go through prioritisation and planning. Nothing needs to be action from your end - I'll re-run the CI checks since I see one job failed bacause of a flakey test.

@ancalita
Copy link
Member

Hi @IzidoroBaltazar could you please check if you have to update your branch with main branch updates? Otherwise, I cannot merge your branch into the target branch.

@IzidoroBaltazar
Copy link
Contributor Author

Hello @ancalita can you check now? I have updated my branch.

@ancalita
Copy link
Member

Hello @ancalita can you check now? I have updated my branch.

you might either have to push a Merge main into <your_branch> commit or update via the GH interface.

@alwx
Copy link
Contributor

alwx commented May 26, 2021

Hey @IzidoroBaltazar!
Now your PR contains too many changes that have nothing to do with the PR itself — it might be because your main is out of sync or because you rebased to a different branch.

Could you please check it and make sure your PR contains only the required changes? You might need to sync your fork using the instructions from here: https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@IzidoroBaltazar
Copy link
Contributor Author

Is it correct now? I'm bit confused with github upstream system at the moment.

@ancalita ancalita merged commit 6694304 into RasaHQ:main May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants