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.

Sign up for GitHub

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

Jump to bottom

[ATO-1685] Add SlotSet step in story with active_loop:null mapping condition #12927

Merged
merged 4 commits into from Oct 20, 2023
Merged

[ATO-1685] Add SlotSet step in story with active_loop:null mapping condition #12927

merged 4 commits into from Oct 20, 2023

Conversation

Tawakalt
Copy link
Contributor

@Tawakalt Tawakalt commented Oct 19, 2023 •
edited by jira bot

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)

@Tawakalt Tawakalt force-pushed the ATO-1685-add-SlotSet-step-in-story-with-null-active-loop-mc branch from 3479a31 to ec92c75 Compare October 19, 2023 17:14
@Tawakalt Tawakalt marked this pull request as ready for review October 20, 2023 07:31
@Tawakalt Tawakalt requested a review from a team as a code owner October 20, 2023 07:31
@Tawakalt Tawakalt requested review from ancalita and removed request for a team October 20, 2023 07:31
ancalita
ancalita reviewed Oct 20, 2023
View reviewed changes
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

LGTM 🙌🏻 Could you also please look into adding an integration test - e.g. running the validate stories functionality on a story that has an intent with entities + domain with the same from_entity slot with active_loop as null in the mapping?

@Tawakalt Tawakalt force-pushed the ATO-1685-add-SlotSet-step-in-story-with-null-active-loop-mc branch from b9d6c9a to 398b1f6 Compare October 20, 2023 14:30
@Tawakalt Tawakalt force-pushed the ATO-1685-add-SlotSet-step-in-story-with-null-active-loop-mc branch from 398b1f6 to f667920 Compare October 20, 2023 14:40
ancalita
ancalita reviewed Oct 20, 2023
View reviewed changes
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

Requested a clarification on the domain file, I also think the story is missing from the data subdir.

data/test/test_integration/domain_with_null_active_loop.yml Outdated Show resolved Hide resolved
tests/integration_tests/core/test_cli_response.py Show resolved Hide resolved
tests/integration_tests/core/test_cli_response.py Outdated Show resolved Hide resolved
@Tawakalt Tawakalt force-pushed the ATO-1685-add-SlotSet-step-in-story-with-null-active-loop-mc branch 3 times, most recently from e3758d0 to 1c1ee95 Compare October 20, 2023 15:59
@Tawakalt Tawakalt force-pushed the ATO-1685-add-SlotSet-step-in-story-with-null-active-loop-mc branch from 1c1ee95 to a35ba8e Compare October 20, 2023 16:08
@github-actions
Copy link
Contributor

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

ancalita
ancalita approved these changes Oct 20, 2023
View reviewed changes
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

Nice work 💯 Thanks for adding the integration test 🎉

@Tawakalt
Copy link
Contributor Author

Nice work 💯 Thanks for adding the integration test 🎉

Thanks for the speedy review 🚀

@Tawakalt Tawakalt merged commit fa00c49 into 3.6.x Oct 20, 2023
99 checks passed
@Tawakalt Tawakalt deleted the ATO-1685-add-SlotSet-step-in-story-with-null-active-loop-mc branch October 20, 2023 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ancalita ancalita ancalita approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Tawakalt @ancalita