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

Dialogs.OAuthPrompt.GetUserTokenAsync bugfix #1243

Merged
merged 1 commit into from
Jan 14, 2019
Merged

Dialogs.OAuthPrompt.GetUserTokenAsync bugfix #1243

merged 1 commit into from
Jan 14, 2019

Conversation

mark-szabo
Copy link
Contributor

Fixing a ArgumentNullException bug which is triggered if I call the Dialogs.OAuthPrompt.GetUserTokenAsync after a non-text message (ex. button click), and context.Activity.Text is not set.

Fixing a ArgumentNullException bug which is triggered if I call the Dialogs.OAuthPrompt.GetUserTokenAsync after a non-text message (ex. button click).
@cleemullins cleemullins self-assigned this Jan 8, 2019
@cleemullins cleemullins added the bug Indicates an unexpected problem or an unintended behavior. label Jan 8, 2019
@cleemullins
Copy link
Contributor

@mark-szabo Thanks! This looks good to me, but a code change means there should be a new Unit Test validating the old & behavior. Are you able to add that unit test?

@johnataylor Can you confirm the behaivor in the JS SDK? We should have the same behavior here, and I'm not sure which behavior is correct. Certainly the bug that Mark fixed here looks like a clear-cut bug.

@mark-szabo
Copy link
Contributor Author

mark-szabo commented Jan 8, 2019

Hi @cleemullins sadly I don't have a dev env for botbuilder-dotnet, so it'd take time... If it's not fitting into your schedule, I'll do it as soon as I have time to set up a dev env and get familiar with your unit tests.

@cleemullins
Copy link
Contributor

Thanks @mark-szabo. The unit tests should be pretty straight forward. There is already a file that has tests for that area ("[filename]Tests.cs")

@mark-szabo
Copy link
Contributor Author

HI @cleemullins, I have started to write a unit test for this but I think that's sadly not possible for this specific method.

Here is the test I wrote:

[TestClass]
public class OAuthPromptTests
{
    [TestMethod]
    public async Task OAuthPromptShouldNotThrowWhenNonTextMessage()
    {
        var convoState = new ConversationState(new MemoryStorage());
        var dialogState = convoState.CreateProperty<DialogState>("dialogState");

        var adapter = new TestAdapter()
            .Use(new AutoSaveStateMiddleware(convoState));

        var dialogs = new DialogSet(dialogState);

        var prompt = new OAuthPrompt(
            "TestOAuthPrompt",
            new OAuthPromptSettings
            {
                ConnectionName = "adal",
                Text = "Click to sign in!",
                Title = "Sign in",
                Timeout = 300000, // User has 5 minutes to login (1000 * 60 * 5)
            });

        dialogs.Add(prompt);

        var activity = new Activity { Type = ActivityTypes.Message, Value = 2 };

        await new TestFlow(adapter, async (turnContext, cancellationToken) =>
        {
            var dc = await dialogs.CreateContextAsync(turnContext, cancellationToken);

            var tokenResponse = prompt.GetUserTokenAsync(dc.Context, cancellationToken);

            var results = await dc.ContinueDialogAsync(cancellationToken);

            if (results.Status == DialogTurnStatus.Waiting)
            {
                await turnContext.SendActivityAsync("Test complete.");
            }
        })
        .Send(activity)
        .AssertReply("Test complete.")
        .StartTestAsync();
    }
}

The issue is that the TestAdapter adapter is not a BotFrameworkAdapter which OAuthPrompt.GetUserToken() expects:
https://github.com/Microsoft/botbuilder-dotnet/blob/2f73c35c1a06705b811e108321fadacda059210e/libraries/Microsoft.Bot.Builder.Dialogs/Prompts/OAuthPrompt.cs#L187-L190

@cleemullins cleemullins merged commit 036e2ce into microsoft:master Jan 14, 2019
@cleemullins
Copy link
Contributor

Thanks. I've merged this in. This should show up on the daily build server tonight, and will ship as part of our 4.3 release in mid-feb.

@mark-szabo
Copy link
Contributor Author

Awesome, thanks @cleemullins! 😉

@mark-szabo mark-szabo deleted the patch-1 branch January 14, 2019 19:01
ShYuPe pushed a commit to ShYuPe/botbuilder-dotnet that referenced this pull request Aug 25, 2020
* Update python 3.7 and 3.6 versions

* Trying x numbering for latest version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or an unintended behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants