Treat list custom json facebook #9567

ErickGiffoni · 2021-09-07T20:58:35Z

Hello, I am continuing the work that was previously being done here -> #8332.
It solves #5657

Proposed changes:

use a Union for the json_message at send_custom_json, as suggested here Treat list custom json facebook #8332
delete some unused imports and invalid comments

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)

…lation

Co-authored-by: David <[email protected]>

* Add ability to point to a certificate in endpoints.yml (RasaHQ#9118) * Add cert file functionality to Endpoint * Apply suggestions from code review types Co-authored-by: Tobias Wochinger <[email protected]> * update changelog Co-authored-by: Tobias Wochinger <[email protected]> * Fix epoch override for TEDPolicy (RasaHQ#9182) * bump rasa-sdk dep and min compat version * fix bug add tests * add changelog * change test cases * remove test cases * prepared release of version 2.8.1 (RasaHQ#9183) * update rasa-sdk * prepared release of version 2.8.1 * update lock * Make UnexpecTEDIntentPolicy compatible with E2E data (RasaHQ#9203) * bump rasa-sdk dep and min compat version * add test and code * add changelog and refactor method * add test * more test cases * use applied_events * 2.8.x: Remove experimental feature warning for story validation and entity roles and groups (RasaHQ#9237) * I removed the experimental feature designation from the docs. * I removed the entity roles and groups experimental feature warning message from the code, and tested it using 'rasa train' on a minimal example. * I removed the unused import: rasa.shared.utils.common * Adding change log files for issues 8791 and 8024. * Rewording change logs to make it clear that the behaviour of the features remains unchanged. * prepared release of version 2.8.2 (RasaHQ#9262) * prepared release of version 2.8.2 * Updated the date of release. * install yarn dependency in cloned repository, in docs publication workflow * silence yarn warning * Fix typo in push_docs_to_branch.sh * Generated a new poetry lock file Co-authored-by: Joe Juzl <[email protected]> Co-authored-by: Tobias Wochinger <[email protected]> Co-authored-by: Daksh Varshneya <[email protected]> Co-authored-by: m-vdb <[email protected]>

Co-authored-by: David <[email protected]>

into macOS-compiler-issues

…wavymazy/rasa into treat-list-custom-json-facebook

ErickGiffoni · 2021-09-07T21:16:38Z

I just realised I accidentally pushed some files from another issue I was working on, sorry!
I'll make the proper adjustments later.

ErickGiffoni · 2021-09-08T22:28:09Z

Ok, ready for review.

ancalita

Thank you for continuing work on this issue 💯

In addition to the suggestions below, I'd like to request you to rebase to target 2.8.x since this is actually a bugfix and this could be released in a minor.

Probably the easiest would be to open a new PR with only the solution specific to this bugfix (and not the README diff that shouldn't be in this PR) to avoid potential git funkiness when rebasing. If you choose this route, you can close this PR afterwards.

ancalita · 2021-09-14T11:11:13Z

changelog/5657.improvement.md

@@ -0,0 +1 @@
+Add list handling in the `send_custom_json` method on `channels/facebook.py`.


I think this changelog entry should actually be a bugfix rather than improvement based on the original issue. It would also be great if you could give more details around the bug since it isn't very clear for me in which circumstances would the json_message be of type list rather than dict.

Ok. Done in a new branch rebase-treat-custom-json-fb.
I haven't pushed it yet, but soon I will and I'll also open the new PR.

ancalita · 2021-09-14T11:19:45Z

rasa/core/channels/facebook.py

    ) -> None:
        """Sends custom json data to the output."""
-
-        recipient_id = json_message.pop("sender", {}).pop("id", None) or recipient_id
+        if isinstance(json_message, dict) and "sender" in json_message.keys():


This also needs a check if json_message is list and assigning recipient_id to the extracted id from json_message.
Potentially also json_message needs to be updated to be of dict type when being given as an argument to self.messenger_client.send(), not entirely sure since Facebook code doesn't have type annotations.
I'd advise researching a bit how to test that this works with MessengerClient.

Done, except the part regarding json_message being updated to dict.
It seams to be ok passing a list to self.messenger_client.send().

rasa/core/channels/facebook.py

ancalita · 2021-09-14T11:22:30Z

tests/test_facebook.py

@@ -0,0 +1,91 @@
+import logging


Could you please shift this file into the correct subdirectory: tests/core/channels?

ancalita · 2021-09-14T11:24:56Z

tests/test_facebook.py

+            },
+        ]
+    }
+    json_with_id = {


This test should also check that send_custom_json works with json_message as list.
I would also suggest using pytest.mark.parametrize for different json inputs and expected values of recipient_id.

ErickGiffoni · 2021-09-14T14:11:24Z

@ancalita ok, I'm on it !

Co-authored-by: Anca Lita <[email protected]>

ErickGiffoni · 2021-09-21T20:11:15Z

Closing this PR. The new one is #9692, rebased to 2.8.x.

silvasara and others added 26 commits March 29, 2021 11:08

Add list handling in the send_custom_json method.

2cebc45

Fix lint problem

f69f4d2

adding tests and create a facebook file test

728dc91

Fixed issue (RasaHQ#5657) in MessengerBot.send_custom_json

d2e50c2

Merge branch 'main' into treat-list-custom-json-facebook

9f3bfcc

Merge branch 'main' into treat-list-custom-json-facebook

d8c8b66

add a support guide for macOS users regarding rasa development instal…

aa7b4eb

…lation

update README with a link to the new macOS support guide

3966ece

Merge branch 'main' into macOS-compiler-issues

a21deeb

Apply suggestions from code review

f007a30

Co-authored-by: David <[email protected]>

Merge branch 'main' into macOS-compiler-issues

fcf7893

Merge branch 'main' into macOS-compiler-issues

9ae8c35

Merge branch 'main' into macOS-compiler-issues

a87ab3d

Merge branch 'main' into macOS-compiler-issues

773e821

Merge branch 'main' into macOS-compiler-issues

0006416

Merge branch 'main' into macOS-compiler-issues

317dc7b

Apply suggestions from code review

0b7485f

Co-authored-by: David <[email protected]>

Merge branch 'macOS-compiler-issues' of https://github.com/FGA-GCES/rasa

cef6c77

into macOS-compiler-issues

Merge branch 'main' into macOS-compiler-issues

4b6e650

Merge branch 'main' of git://github.com/RasaHQ/rasa into HEAD

1dcfd33

Merge commit '1dcfd33e71b' into macOS-compiler-issues

0d05a9d

Merge branch 'macOS-compiler-issues' of https://github.com/FGA-GCES/rasa

4993ea1

into macOS-compiler-issues

Merge branch 'treat-list-custom-json-facebook' of https://github.com/…

d1a03d1

…wavymazy/rasa into treat-list-custom-json-facebook

remove the unused imports and comments

ee0ddb2

uses a Union for the json_message, as suggested here RasaHQ#8332

49174dd

ErickGiffoni requested a review from a team as a code owner September 7, 2021 20:58

ErickGiffoni requested review from ancalita and removed request for a team September 7, 2021 20:58

ErickGiffoni mentioned this pull request Sep 7, 2021

Sprint 3 FGA-GCES/rasa#120

Closed

ErickGiffoni mentioned this pull request Sep 7, 2021

Treat list custom json facebook #8332

Closed

3 tasks

ErickGiffoni added 5 commits September 8, 2021 18:34

removing irrelevant file

8a642d2

updates README with branch main

831e3a3

updates file test_unexpected_intent_policy with branch main

841e6e7

updates file test_ted_policy with branch main

f8b3a28

Merge branch 'main' into treat-list-custom-json-facebook

0214197

Merge branch 'main' into treat-list-custom-json-facebook

895a068

ancalita linked an issue Sep 14, 2021 that may be closed by this pull request

Type error in facebook.py #5657

Closed

ancalita requested changes Sep 14, 2021

View reviewed changes

ErickGiffoni mentioned this pull request Sep 14, 2021

Sprint 4 - 2021/1 FGA-GCES/rasa#130

Closed

pass recipient_id as return value when pop fails

a11a6b2

Co-authored-by: Anca Lita <[email protected]>

ErickGiffoni mentioned this pull request Sep 21, 2021

Treat list in custom json Facebook : rebasing to 2.8.x #9692

Merged

4 tasks

ErickGiffoni closed this Sep 21, 2021

SamuelNoB deleted the treat-list-custom-json-facebook branch January 27, 2022 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat list custom json facebook #9567

Treat list custom json facebook #9567

ErickGiffoni commented Sep 7, 2021 •

edited

Loading

ErickGiffoni commented Sep 7, 2021 •

edited

Loading

ErickGiffoni commented Sep 8, 2021

ancalita left a comment

ancalita Sep 14, 2021

ErickGiffoni Sep 21, 2021

ancalita Sep 14, 2021

ErickGiffoni Sep 21, 2021

ancalita Sep 14, 2021

ErickGiffoni Sep 21, 2021

ancalita Sep 14, 2021

ErickGiffoni Sep 21, 2021

ErickGiffoni commented Sep 14, 2021

ErickGiffoni commented Sep 21, 2021

		@@ -0,0 +1 @@
		Add list handling in the `send_custom_json` method on `channels/facebook.py`.

Treat list custom json facebook #9567

Treat list custom json facebook #9567

Conversation

ErickGiffoni commented Sep 7, 2021 • edited Loading

ErickGiffoni commented Sep 7, 2021 • edited Loading

ErickGiffoni commented Sep 8, 2021

ancalita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErickGiffoni commented Sep 14, 2021

ErickGiffoni commented Sep 21, 2021

ErickGiffoni commented Sep 7, 2021 •

edited

Loading

ErickGiffoni commented Sep 7, 2021 •

edited

Loading