-
Notifications
You must be signed in to change notification settings - Fork 479
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
Teams ActionBasedMessagingExtensionFetchTask Scenario #2577
Teams ActionBasedMessagingExtensionFetchTask Scenario #2577
Conversation
…skAsync Update ActionBasedMessagingExtensionFetchTaskBot and Readme.md
Pull Request Test Coverage Report for Build 80150
💛 - Coveralls |
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1 |
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.
Overall it looks good, some issues in the README and concern over having the OnTeamsMessagingExtensionSubmitActionAsync()
in here as well.
26. Select the bot channel registration | ||
27. Go to Settings | ||
28. Select the "Teams" icon under "Add a featured channel | ||
29. Click Save |
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 whole thing looks really painful, I appreciate that you entered in all of the steps, but there has to be a better way to document this.
Perhaps a README in the /tests/Teams
folder; but this sort of information we're going to want to put in our Teams samples too.
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 copied the steps from a different Readme.
Maybe we can just link to here: https://docs.microsoft.com/en-us/microsoftteams/platform/concepts/bots/bots-create#create-a-bot-for-microsoft-teams (note: the steps in this doc don't require an Azure Subscription)
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.
Tien added a bunch of detail in the Readme for unfurling. I think for the moment its OK to have the duplication. But the most useful thing for me is where we talk about Teams.
protected override async Task<MessagingExtensionActionResponse> OnTeamsMessagingExtensionSubmitActionAsync(ITurnContext<IInvokeActivity> turnContext, MessagingExtensionAction action, CancellationToken cancellationToken) | ||
{ | ||
var reply = MessageFactory.Text("OnTeamsMessagingExtensionSubmitActionAsync MessagingExtensionAction: " + JsonConvert.SerializeObject(action)); | ||
await turnContext.SendActivityAsync(reply, cancellationToken); |
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.
Does this work if I use the action extension in a channel that the bot does not have access to?
Part of me thinks this scenario is fine, and another part of me thinks we should perhaps break out the OnTeamsMessagingExtensionSubmitActionAsync
into a separate Scenario.
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.
No, sending to a conversation in which the bot is not installed throws an Unauthorized.
Do you think we should either 1) remove these lines that send the message, or 2) add a try/catch and ignore the exception ?
I think having OnTeamsMessagingExtensionSubmitActionAsync in this Scenario helps differentiate it from the non-task module Messaging Extension Scenario But, I don't feel strongly about this. We should discuss which Scenarios we want to cover which functionality (there is a multitude of different flows possible, and the interactions vary depending on a variety of factors).
|
||
var adaptiveCard = GetCardFromSubmitExampleData(JsonConvert.DeserializeObject<SubmitExampleData>(action.Data.ToString())); | ||
|
||
return new MessagingExtensionActionResponse |
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 this behavior in a screenshot in the README? I don't think I saw it there...
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.
Step 4?
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1 |
26. Select the bot channel registration | ||
27. Go to Settings | ||
28. Select the "Teams" icon under "Add a featured channel | ||
29. Click Save |
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.
Tien added a bunch of detail in the Readme for unfurling. I think for the moment its OK to have the duplication. But the most useful thing for me is where we talk about Teams.
{ | ||
Record.Add(MethodBase.GetCurrentMethod().Name); | ||
return base.OnTeamsMessagingExtensionFetchTaskAsync(turnContext, cancellationToken); | ||
return base.OnTeamsMessagingExtensionFetchTaskAsync(turnContext, query, cancellationToken); |
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.
my bad... these tests could perhaps be a little deeper :-(
(they only check the dispatching not the values in the query)
Add MessagingExtensionQuery param to OnTeamsMessagingExtensionFetchTaskAsync
Update ActionBasedMessagingExtensionFetchTaskBot and Readme.md