-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Add list handling in the send_custom_json method on facebook.py #7247
Conversation
Thanks for submitting a pull request 🚀 @wochinge will take a look at it as soon as possible ✨ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @silvasara
Thanks for your contribution!
Can you please add tests for this?
…st-custom-json-facebook
@wochinge I don't know if I can do the tests, but I'll try. Where would I put this test? The only test I found for this file was this: rasa/tests/core/test_channels.py Lines 146 to 165 in 0ccfa20
|
@silvasara Yep, it would go into |
Hello, I checked the missing tests for the correction, can I implement and send the pr? @wochinge |
@jppgomes Sure! It would be a pleasure! |
@@ -1,10 +1,10 @@ | |||
repos: | |||
- repo: https://github.com/ambv/black | |||
rev: 19.10b0 | |||
rev: 20.8b1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please change the items in this file back? These hooks should work as they are once you do poetry install
in the cloned repository
assert routes_list["fb_webhook.webhook"].startswith("/webhooks/facebook/webhook") | ||
|
||
|
||
def test_facebook_send_custon_json_list(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't test send_custom_json
- we need to call the actual code in this test (same in the one below)
|
||
recipient_id = json_message.pop("sender", {}).pop("id", None) or recipient_id | ||
if isinstance(json_message, list): | ||
if isinstance(json_message.pop(), list): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pop
gets and removes stuff from the list - do we really want that?
Hi guys, I'm the author of the post for this issue. I had some time today so I though might as well update this PR. Context: I have retested the issue with Facebook to understand where it comes from. Example of a custom Json that would cause issues:
Propose solution: I think the best way to handle this is by only looking for the sender ID in the custom template only if that template is of type Dict. Otherwise we could use the ID that is given as a function paramter. |
that would be awesome if you could wrap this up @wavymazy .
Would do you mean by this? |
Hi @wochinge, Unfortunately it seems I can't make a direct change to this PR unless given access. With regards to your question, the ID I'm refering to is the
|
@wavymazy How about forking the fork / cherry-picking the commits and then you create a new PR? |
Ok @wochinge. I can try this. |
Awesome! I'll close this one then. |
Proposed changes:
send_custom_json
method onchannels/facebook.py
.Status (please check what you already did):
black
(please check Readme for instructions)