-
Notifications
You must be signed in to change notification settings - Fork 906
Update sample from Bot Builder SDK to Teams PR-6 #1844
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
base: main
Are you sure you want to change the base?
Conversation
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.
use minimal APIs instead of controllers
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.
We are following standard Template created by TeamsToolkit. Controller has been used in all standard templates.
| /// <summary> | ||
| /// Removes bot mention from the message text. | ||
| /// </summary> | ||
| private string RemoveBotMention(string text) |
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 function should be in the SDK
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.
Removed the custom Function RemoveBotMention.
| } | ||
|
|
||
| // Loads and sends an adaptive card from the specified JSON file | ||
| private async Task SendAdaptiveCardAsync(IContext.Client client, string cardFileName, string userName = null) |
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.
use Cards API instead of JSON
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 code to use Cards API.
| } | ||
| else | ||
| { | ||
| await client.Send(CommandString); |
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.
we should pass the cancellationToken to all async functions
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.
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 file should be .gitignored, and add a launchSetting.example.json as a reference
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.
Added this file in .gitignore , also added launchSetting.example.json as a reference
| <LangVersion>latest</LangVersion> | ||
| <TargetFramework>net10.0</TargetFramework> | ||
| <ImplicitUsings>enable</ImplicitUsings> | ||
| <LangVersion>14.0</LangVersion> |
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.
any reason to use LangVersion?
We should enable nullable support
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.
| <PackageReference Include="Microsoft.AspNetCore.Mvc.NewtonsoftJson" Version="6.0.11" /> | ||
| <PackageReference Include="Microsoft.Bot.Builder.Integration.AspNet.Core" Version="4.18.1" /> | ||
| <PackageReference Include="Azure.Identity" Version="1.13.1" /> | ||
| <PackageReference Include="Microsoft.Teams.Api" Version="2.0.*" /> |
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.
reference only the top level package
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.
| <PackageReference Include="Microsoft.Teams.Plugins.AspNetCore" Version="2.0.*" /> | ||
| <PackageReference Include="Microsoft.Teams.Common" Version="2.0.*" /> | ||
| <PackageReference Include="AdaptiveCards.Templating" Version="1.6.0" /> | ||
| <PackageReference Include="Newtonsoft.Json" Version="13.0.3" /> |
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.
any reason to explicitly reference newtonsoft?
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.
Removed it.
| </ItemGroup> | ||
|
|
||
| <!-- Exclude old Bot Builder files from compilation --> | ||
| <ItemGroup> |
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.
remove these
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.
Removed.
|
|
||
| <!-- Exclude generated PolySharp files --> | ||
| <ItemGroup> | ||
| <Compile Remove="obj\**\PolySharp.SourceGenerators\**\*.g.cs" /> |
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.
how is this used? I'm not aware of the need to use PolySharp
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.
Removed.
| </ItemGroup> | ||
|
|
||
| <!-- Exclude local settings from publish --> | ||
| <ItemGroup> |
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.
remove, use defaults
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.
Removed.
| "Teams": { | ||
| "ClientId": "", | ||
| "ClientSecret": "", | ||
| "BotType": "", |
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.
what is BotType? I dot not recognize this setting
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.
Removed all BotType references.
| @@ -0,0 +1,19 @@ | |||
| <Project> | |||
| <PropertyGroup> | |||
| <PolySharpIncludeGeneratedTypes> | |||
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.
why is this here?
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.
Removed the file.
| var config = builder.Configuration.Get<ConfigOptions>(); | ||
|
|
||
| // Factory function to create access tokens using managed identity credentials | ||
| Func<string[], string?, Task<ITokenResponse>> createTokenFactory = async (string[] scopes, string? tenantId) => |
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.
once we merge MSAL support, we won't need this.
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.
Removed the fuction.
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 file is usually gitignored.. what's the reason to commit it?
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.
| > NOTE: When you create your app registration, you will create an App ID and App password - make sure you keep these for later. | ||
|
|
||
| 2. Run ngrok - point to port 3978 | ||
| 2. Run ngrok - point to port 5130 |
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.
do not lead with ngrok, we should prioritize devtunnels
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 the README to prioritize dev tunnels over ngrok.
| * Leave **Redirect URI** empty. | ||
| * Choose **Register**. | ||
| B) On the overview page, copy and save the **Application (client) ID, Directory (tenant) ID**. You'll need those later when updating your Teams application manifest and in the appsettings.json. | ||
| B) On the overview page, copy and save the **Application (client) ID, Directory (tenant) ID**. You'll need those later when updating your Teams application manifest and in the appsettings.Development.json. |
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.
in which case do we need the TenantID in the manifest?
appSettings.Development.json is just one way of providing values to IConfiguration. Instead of this file, I'd recommend launchSettings.json
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.
| - Modify the `/appsettings.Development.json` and fill in the following details: | ||
| - `ClientId` - Generated from Step 1 is the application app id | ||
| - `ClientSecret` - Generated from Step 1, also referred to as Client secret | ||
| - `BotType` - Set the bot type (e.g., "MultiTenant" or "SingleTenant") |
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.
we are not doing multitenant anymore
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.
Removed BotType entirely.
| <PackageReference Include="Azure.Identity" Version="1.13.1" /> | ||
| <PackageReference Include="Microsoft.Teams.Api" Version="2.0.*" /> | ||
| <PackageReference Include="Microsoft.Teams.Apps" Version="2.0.*" /> | ||
| <PackageReference Include="Microsoft.Teams.Plugins.AspNetCore" Version="2.0.*" /> |
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.
reference only the top level package
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 and only kept by default created packages and moved sample to different PR:
#1853
| <ItemGroup> | ||
| <Content Update="appsettings.json"> | ||
| <CopyToOutputDirectory>Always</CopyToOutputDirectory> | ||
| <Content Remove="appsettings.Development.json" /> |
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.
use defaults
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.
They are by default created.
| if (member.Id != activity.Recipient.Id) | ||
| { | ||
| await client.Send("Welcome to the suggested actions bot. This bot will introduce you to suggested actions. Please answer the question:"); | ||
| await SendSuggestedActionsCardAsync(client, log); |
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.
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.
| "commandName": "Project", | ||
| "dotnetRunMessages": true, | ||
| "applicationUrl": "https://localhost:7130;http://localhost:5130", | ||
| "applicationUrl": "http://localhost:5130", |
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.
we should standarize to 3978
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 to 3978 and moved Sample to different PR: #1854
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.
having one action per sample/language will lead to 202 actions. We should have only one, or one per language max.
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.
Deleted the file, we will update one single file.
This reverts commit 0030343.





No description provided.