Botbuilder-dotnet: ReplaceDialog is not replacing correctly

[xieofxie]

Github issues should be used for bugs and feature requests. Use Stack Overflow for general "how-to" questions.

Version

What package version of the SDK are you using.

Current master

Describe the bug

Give a clear and concise description of what the bug is.

After ReplaceDialog, the actions followed are still working.

To Reproduce

Steps to reproduce the behavior:

1. See my branch for the example https://github.com/xieofxie/botbuilder-dotnet/tree/hualxie/replace_bug/tests/Microsoft.Bot.Builder.TestBot.Json/Samples/ReplaceBug
2. Start Testbot.Json and run 26.replace

Expected behavior

Give a clear and concise description of what you expected to happen.

'replace main' should not be sent as it is after ReplaceDialog:

Screenshots

If applicable, add screenshots to help explain your problem.

Additional context

Add any other context about the problem here.

I tried to modify ReplaceDialog as RepeatDialog, but dialog id may not be found in parent.
I need this for the following scenario:
A begins B, B replaces C, and A received result from C.

Thanks

[bug]

[vishwacsena]

@carlosscastro @tomlm believes this was fixed in R9. can you confirm?

[xieofxie]

I just checked it on 275cb070dbd5c328b8a932c864dbd0a0c1d92991 of master and it still exists.

[cleemullins]

@carlosscastro Per @tomlm & @vishwacsena we believe this to be a P0. Carlos, can you provide an ETA for a fix? This is thought to be related to the cycle-detection code.

[carlosscastro]

Thanks @cleemullins - I can work onthis next week, approximately starting on 7/16.

[cwhitten]

@vishwacsena has this fixed in a separate pull request

[carlosscastro]

Looking at the PR, cool to see. However it seems to break a test. I'll sync up with Vishwac. If more work is required, I have time to help this week.

[carlosscastro]

Also note that this doesn't seem cycle detection related, since the bug reproes with cycle detection disabled, and there is no StackOverflowException.

[vishwacsena]

@carlosscastro Here's a crisp repro

Expected

Bot:Say 'start' to get started
User: where
Bot: outer dialog..
User: start
Bot: Starting child dialog
Bot: Say 'replace' to get started
User: where
Bot: root dialog..
User: replace
Bot: Replacing this dialog with a child
Bot: This dialog (newDialog) will end after this message
Bot: child dialog has ended and returned back
User: where
Bot: outer dialog..

```C#
var outderDialog = new AdaptiveDialog("outer")
{
AutoEndDialog = false,
Generator = new TemplateEngineLanguageGenerator(),
Recognizer = new RegexRecognizer()
{
Intents = new List()
{
new IntentPattern()
{
Intent = "start",
Pattern = "start"
},
new IntentPattern()
{
Intent = "where",
Pattern = "where"
}
}
},
Triggers = new List()
{
new OnBeginDialog()
{
Actions = new List

()
{
new SendActivity("Say 'start' to get started")
}
},

    // joke is always available if it is an interruption.
    new OnIntent()
    {
        Intent = "start",
        Actions = new List<Dialog>()
        {
            new SendActivity("Starting child dialog"),
            new BeginDialog()
            {
                Dialog = "root"
            },
            new SendActivity("child dialog has ended and returned back")
        }
    },
    new OnIntent()
    {
        Intent = "where",
        Actions = new List<Dialog>()
        {
            new SendActivity("outer dialog..")
        }
    }
}

};

var rootDialog = new AdaptiveDialog("root")
{
AutoEndDialog = false,
Generator = new TemplateEngineLanguageGenerator(),
Recognizer = new RegexRecognizer()
{
Intents = new List()
{
new IntentPattern()
{
Intent = "replace",
Pattern = "replace"
},
new IntentPattern()
{
Intent = "where",
Pattern = "where"
}
}
},
Triggers = new List()
{
new OnBeginDialog()
{
Actions = new List

()
{
new SendActivity("Say 'replace' to get started")
}
},

    // joke is always available if it is an interruption.
    new OnIntent()
    {
        Intent = "replace",
        Actions = new List<Dialog>()
        {
            new SendActivity("Replacing this dialog with a child"),
            new ReplaceDialog()
            {
                Dialog = "newDialog"
            },
            new SendActivity("You should not see these actions since this dialog has been replaced!")
        }
    },
    new OnIntent()
    {
        Intent = "where",
        Actions = new List<Dialog>()
        {
            new SendActivity("root dialog..")
        }
    }
}

};

var newDialog = new AdaptiveDialog("newDialog")
{
Triggers = new List()
{
new OnBeginDialog()
{
Actions = new List

()
{
new SendActivity("This dialog (newDialog) will end after this message")
}
}
}
};
```

[carlosscastro]

Thanks Vishwac! We synced up offline on this. I'll pick this one up Monday 7/27.

[carlosscastro]

Update: Actively working on this. The issue is pretty deep and challenges the fundamental decisions on how replace dialog should be implemented.

[carlosscastro]

Relabeled to P1 after discussing with Vishwac.

[carlosscastro]

We got to the bottom of this. The current ReplaceDialog action implementation needs to call dc.Parent.ReplaceDialog, since the one that needs to be replaced is the first dialog container found in the parent chain of the current active dialog.

However tihs fix does not work when replacing the root dialog. A workaround for that would be to hang all dialogs off of the DialogManager, however we need to define whether that is the right thing to do. Another approach would be to say that replacing the root dialog is not allowed. We'll have a discussion next week about what the right thing to do is.

[cwhitten]

As I gather, ReplaceDialog _will_ work when used outside of thee root dialog. @carlosscastro can you confirm?

What was the outcome of your discussion on the right thing to do? In the meantime this should be called out as a known issue and the fix should be prioritized in R11. cc @cleemullins

[carlosscastro]

Update: We discussed that

• Replacing the root dialog will not be a possibility since the root dialog is not captured in declarative right now, but configured as entry point in applications such as composer.
• If the root dialog needs to be replaced in some special scenario, we can 1) promote Containerdialog to be a $kind, and allow to list its dialogs along with its initial dialog. Then the initial dialog could be replaced, however the root will be a container dialog which will not be replaced. or 2) users can just nest in order to have a root adaptive dialog that's fixed.

So as part of this bug, in R11 we'll make the parent replace and error out when trying to replace the root dialog. Additionally, we'll create a work item to track implementing 1) or a variation, or live with workaround 2) for some time depending on priority.

[carlosscastro]

ETA for PR coming for this early next week.

[carlosscastro]

Started working on this. Have the potential fix written, testing it now.