Updates DialogManager to work with skills #3709
Conversation
…bot requests). Added unit tests. Added end to end Parent->Skill sample with DialogManager for testing.
gabog
requested review from
tomlm,
Stevenic,
carlosscastro,
cwhitten and
clearab
| var snapshotDc = dc; | ||
| while (snapshotDc.Child != null) | ||
| var snapshot = GetActiveDialogContext(dc).State.GetMemorySnapshot(); | ||
| var traceActivity = (Activity)Activity.CreateTraceActivity("BotState", "https://www.botframework.com/schemas/botState", snapshot, traceLabel); |
There was a problem hiding this comment.
Are we sending bot state traces somewhere else in the sdk? If yes it might be worth to unify the code and define constants for the strings. If not, are there other places where we'd want to reuse this? Basically suggest to spend a few minutes thinking of potential reuse of this and if there is potential, expose it in a way that can be consumed by other parts of the SDK rather than requiring a refactor to do so. If this seems very specific to DialogManager to you, then a private method, as-is would be ok and no action required.
There was a problem hiding this comment.
We are using traces in other parts parts of the framework but I am just preserving the traces that were there before the refactoring, can we address the standarization of traces this as an R10 task? (I wouldn't want to go and refactor code that is not relevant to DialogManager at this point).
libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/DialogManager.cs
Outdated
Show resolved
Hide resolved
| if (turnResult.Status == DialogTurnStatus.Complete || turnResult.Status == DialogTurnStatus.Cancelled) | ||
| { | ||
| var endMessageText = $"Dialog {_rootDialogId} has **completed**. Sending EndOfConversation."; | ||
| await turnContext.TraceActivityAsync($"{typeof(Dialog).Name}.RunAsync()", label: $"{endMessageText}", value: turnResult.Result, cancellationToken: cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
What if they've already sent an EndOfConversation? I would setup a listner to detect that they sent an EOC and only send one yourself if they haven't
There was a problem hiding this comment.
Do you have any samples on how to setup a listener? I am not familiar with that.
There was a problem hiding this comment.
Hi @Stevenic, I've been thinking about this, in what cases a dev would send an EoC by hand? Wouldn't that be a bug in their code?
Fixes #3657
This PR includes the following changes:
I also refactored DialogManager OnTurnAsync in a couple of private methods to split the Skill/Parent processing logic and reduce duplication.