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

Extract slots with mapping conditions during form activation #11149

Conversation

ancalita
Copy link
Member

@ancalita ancalita commented May 26, 2022

Proposed changes:

  • Fixes the extraction of a slot with mapping conditions that should be filled with information from the user message that triggers the form.

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 May 26, 2022 16:16
@ancalita ancalita requested a review from a team as a code owner May 26, 2022 16:16
@ancalita ancalita requested review from losterloh, m-vdb and indam23 and removed request for a team May 26, 2022 16:16
Copy link
Collaborator

@m-vdb m-vdb left a comment

Choose a reason for hiding this comment

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

Nice turnaround @ancalita 🚀 I've left a few comments

rasa/core/actions/forms.py Show resolved Hide resolved
f"these events: {events_as_str}."
)

tracker.update_with_events(extraction_events, domain)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there any reason why this is called here and at the same time the function returns the events? As far as I understand, the processor will update the tracker with the events returned by this function

Copy link
Member Author

Choose a reason for hiding this comment

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

I have to update the tracker immediately in order for the tracker to update its tracker.slots attribute, which gets used in the for loop below (L610-612) to extract the prefilled slots. Because the tracker is temporary, it won't duplicate the events in the actual tracker that the processor updates.

rasa/core/actions/forms.py Show resolved Hide resolved
rasa/core/actions/forms.py Outdated Show resolved Hide resolved
tests/core/actions/test_forms.py Show resolved Hide resolved
rasa/core/actions/forms.py Show resolved Hide resolved
@ancalita ancalita requested a review from m-vdb May 27, 2022 09:36
rasa/core/actions/loops.py Outdated Show resolved Hide resolved
@ancalita ancalita requested a review from m-vdb May 27, 2022 11:23
@m-vdb
Copy link
Collaborator

m-vdb commented May 27, 2022

@ancalita given Arjaan response on the ticket, I think this should wait a review from Lukas and/or Melinda. I initially started giving a review in order to go faster but now we're waiting on customer feedback.

@joeri-jongen-kbc-be
Copy link

@ancalita given Arjaan response on the ticket, I think this should wait a review from Lukas and/or Melinda. I initially started giving a review in order to go faster but now we're waiting on customer feedback.

@m-vdb @ancalita
I'm working for the customer, we validated the fix and it seems to work 🥳

Copy link
Contributor

@losterloh losterloh left a comment

Choose a reason for hiding this comment

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

Very nice 🚀 Just left some small comments and will leave it up to you whether to tackle them or not.

rasa/core/actions/forms.py Outdated Show resolved Hide resolved
rasa/core/actions/forms.py Outdated Show resolved Hide resolved
rasa/core/actions/forms.py Outdated Show resolved Hide resolved
tests/core/actions/test_forms.py Outdated Show resolved Hide resolved
@m-vdb
Copy link
Collaborator

m-vdb commented Jun 1, 2022

BTW I'll defer to @losterloh review here and remove myself from the reviewers. I'll also remove Melinda as she's off this week

@m-vdb m-vdb removed request for indam23 and m-vdb June 1, 2022 12:20
…of-a-form-for-the-intent-that-starts-the-form
@ancalita ancalita enabled auto-merge (squash) June 1, 2022 12:46
@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2022

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

@ancalita ancalita merged commit 177d411 into 3.1.x Jun 1, 2022
@ancalita ancalita deleted the SB-101-slot-mapping-from-entity-in-context-of-a-form-for-the-intent-that-starts-the-form branch June 1, 2022 13:29
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