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

Catch premature dialog endings #3361

Merged
merged 5 commits into from
Mar 5, 2021

Conversation

mdrichardson
Copy link
Contributor

@mdrichardson mdrichardson commented Mar 5, 2021

Fixes #3355

Description

runDialog() works great for dialogs that take more than one turn to complete, but it fails otherwise. A single-step or immediately-cancelled dialog should be pretty rare, but this should fix things for them.

The general flow for the way things work currently:

  1. bot.js file calls runDialog() (usually in onTurn() or onMessage()
  2. runDialog sets result to continueDialog()
  3. If no dialogs have started (normal), the result will be DialogTurnStatus.Empty, so runDialog then calls beginDialog().
  4. Normally, the turn completes and when the user messages the bot to respond to a dialog prompt, runDialog() is called again, so continueDialog() is called again.
    • However, if--after first calling beginDialog()--the dialog is cancelled or ended before the turn ends, result remains DialogTurnStatus.Empty (instead of getting updated to Completed or Cancelled), so EoC is never sent back to the parent

Specific Changes

Allow result to be updated with the beginDialog() call.

Testing

Tested using the bots in the linked issue and it worked.

@coveralls
Copy link

coveralls commented Mar 5, 2021

Pull Request Test Coverage Report for Build 625821802

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.003%) to 84.887%

Files with Coverage Reduction New Missed Lines %
libraries/botbuilder-lg/src/templateExtensions.ts 1 84.48%
Totals Coverage Status
Change from base Build 625820512: -0.003%
Covered Lines: 18646
Relevant Lines: 20923

💛 - Coveralls

@mdrichardson mdrichardson marked this pull request as ready for review March 5, 2021 01:28
@mdrichardson mdrichardson requested review from a team as code owners March 5, 2021 01:28
@stevengum stevengum requested review from stevengum and removed request for Stevenic and luhan2017 March 5, 2021 19:39
Copy link
Member

@stevengum stevengum left a comment

Choose a reason for hiding this comment

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

Small changes and cleanup requested, otherwise it looks great! Thanks for the quick turnaround 👍🏼

Copy link
Member

@stevengum stevengum left a comment

Choose a reason for hiding this comment

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

🚀

@stevengum stevengum merged commit df0f316 into microsoft:main Mar 5, 2021
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.

1 step waterfall dialogs don't resolve back to parent dialogs
3 participants