-
Notifications
You must be signed in to change notification settings - Fork 486
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
Support Bot new invoke type: config #6632
Conversation
Pull Request Test Coverage Report for Build 352211
💛 - Coveralls |
7215218
to
89942cf
Compare
} | ||
|
||
/// <summary> | ||
/// Override this in a derived class to provide logic for when a config is fetched. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: should be "config is submitted".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in the next iteration
namespace Microsoft.Bot.Schema.Teams | ||
{ | ||
/// <summary> | ||
/// Specifies bot config authe, including type and suggestedActions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: "auth"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in the next iteration
/// <summary> | ||
/// Specifies Invoke response base including response type. | ||
/// </summary> | ||
public partial class InvokeResponseBase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to name it as InvokeResponseBase, will we have a class of InvokeResponse? I saw we have both classes for task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will align with our class design in the reference repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can sync up with you offline for this.
89942cf
to
c93d725
Compare
await ((IBot)bot).OnTurnAsync(turnContext); | ||
|
||
// Assert | ||
//Assert.Equal(2, bot.Record.Count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is commented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in the next iteration
await ((IBot)bot).OnTurnAsync(turnContext); | ||
|
||
// Assert | ||
//Assert.Equal(2, bot.Record.Count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is commented out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in the next iteration
b76b871
to
77cb79f
Compare
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll |
The PR will NOT be merged before the Bot config front-end changes rollout to R3. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #6634
Description
Support new bot invoke type: config.
Specific Changes
Testing
E2E manual tests against local changes for various config requests and responses.
Unit Tests.