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 ValidationAction dispatching of BotUttered messages #11394

Merged
merged 6 commits into from
Aug 4, 2022

Conversation

ancalita
Copy link
Member

@ancalita ancalita commented Jul 28, 2022

Proposed changes:

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)

@ancalita ancalita marked this pull request as ready for review July 28, 2022 09:27
@ancalita ancalita requested a review from a team as a code owner July 28, 2022 09:27
@ancalita ancalita requested review from sanchariGr, indam23 and losterloh and removed request for a team July 28, 2022 09:27
@@ -180,6 +180,8 @@ async def run_action_extract_slots(
f"Default action '{ACTION_EXTRACT_SLOTS}' was executed, "
f"resulting in {len(extraction_events)} events: {events_as_str}"
)
await self.execute_side_effects(extraction_events, tracker, output_channel)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that the other side effects are scheduling / cancelling reminders - do you know if the behaviour for either of those now changes?
Is it possible to include unit tests for the expected behaviour of those two, once we know what it is?

Copy link
Member Author

@ancalita ancalita Aug 1, 2022

Choose a reason for hiding this comment

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

Good question, I should only apply the first side-effect (sending bot messages), since the other events are disallowed during validation (only SlotSet and BotUttered are allowed event types to be returned during slot validation, see here).

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, that makes sense.
One thing I am confused by, why do users want to return BotUttered events instead of returning responses directly? Or is this different for slot validation than for other custom actions?

Copy link
Member Author

Choose a reason for hiding this comment

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

why do users want to return BotUttered events instead of returning responses directly?

You mean why wouldn't they use dispatcher.utter_message instead of returning the BotUttered event? Not sure 😄 It seems both approaches are valid, so we wanted to enable that.

tests/core/test_processor.py Outdated Show resolved Hide resolved
@ancalita ancalita requested a review from indam23 August 1, 2022 12:52
changelog/11394.bugfix.md Outdated Show resolved Hide resolved
Copy link
Contributor

@indam23 indam23 left a comment

Choose a reason for hiding this comment

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

just a small suggestion for the changelog since it's only the messages that are being executed and not other side effects

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022

🚀 A preview of the docs have been deployed at the following URL: https://11394--rasahq-docs-rasa-v2.netlify.app/docs/rasa

@ancalita ancalita merged commit 273d9b1 into 3.2.x Aug 4, 2022
@ancalita ancalita deleted the ATO-245/bugfix-validation-action branch August 4, 2022 21:03
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.

2 participants