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

check if ActionExecutionRejected should be thrown after custom action for slot validations was called #7035

Merged
merged 9 commits into from
Oct 28, 2020

Conversation

wochinge
Copy link
Contributor

@wochinge wochinge commented Oct 16, 2020

Proposed changes:

  • fix Send potential ActionExecutionRejected after form validation #6977
  • check for a required ActionExecutionRejection error after custom action for slot validations / extractions was called. This doesn't reject forms before users could intervene.
  • don't deactivate the form in case the user manually returned a ActionExecutionRejected event
  • raise InvalidDomain instead of ValueError for invalid slot mappings to avoid unnecessary Sentry errors
  • a custom action can reject its execution by returning a ActionExecutionRejected event and still apply other events (e.g. slots)
  • update related documentation

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)

@wochinge wochinge marked this pull request as ready for review October 16, 2020 15:04
changelog/6977.improvement.md Show resolved Hide resolved
rasa/core/actions/forms.py Show resolved Hide resolved
@wochinge wochinge requested a review from Ghostvv October 19, 2020 09:58
Copy link
Contributor

@Ghostvv Ghostvv left a comment

Choose a reason for hiding this comment

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

please update the docs as part of this PR, otherwise looks good to me 🚀

changelog/6977.improvement.md Show resolved Hide resolved
@wochinge
Copy link
Contributor Author

@ArjaanBuijk Could you please check the documentation?

@wochinge
Copy link
Contributor Author

@tczekajlo The security check for the secrets is currently failing. it's not required but I think we need to exclude mdx files from the check

@wochinge
Copy link
Contributor Author

@melindaloubser1 Requested review from you since @ArjaanBuijk doesn't seems to be busy

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.

I made some small phrasing changes directly since it won't allow me to request changes on deleted lines

@wochinge
Copy link
Contributor Author

Thank you so much for this blazing fast review and changes @melindaloubser1 🙌

@wochinge
Copy link
Contributor Author

@Ghostvv I'm gonna dismiss your changes requested because you said it's good to go apart from the docs and they were approved now.

@wochinge wochinge dismissed Ghostvv’s stale review October 28, 2020 08:12

docs are there and requested changes were worked in

@rasabot rasabot merged commit fa7567d into master Oct 28, 2020
@rasabot rasabot deleted the form-reject branch October 28, 2020 12:50
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.

Send potential ActionExecutionRejected after form validation
4 participants