-
Notifications
You must be signed in to change notification settings - Fork 79
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
Updating new sample - msgext-northwind-inventory-csharp #143
Updating new sample - msgext-northwind-inventory-csharp #143
Conversation
…adeesh-MSFT/Copilot-for-M365-Samples into v-jegadeesh/northwind-csharp
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.
please fix the given comments
samples/msgext-northwind-inventory-csharp/MessageExtensions/DiscountedSearchCommand.cs
Outdated
Show resolved
Hide resolved
samples/msgext-northwind-inventory-csharp/MessageExtensions/productSearchCommand.cs
Outdated
Show resolved
Hide resolved
samples/msgext-northwind-inventory-csharp/AdaptiveCards/successCard.json
Outdated
Show resolved
Hide resolved
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.
looks correct, Approving!
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.
Thank you for your PR @Jegadeesh-MSFT, I've left a few comments, let's get these addressed and I'll give it another pass.
I noticed an error on the adaptive card when added to a message in Teams.
I wasn't able to test this in Copilot as the tenant I am using is having issues so that is an outstanding task for me to complete.
{ | ||
"$schema": "https://developer.microsoft.com/en-us/json-schemas/teams/v1.16/MicrosoftTeams.schema.json", | ||
"manifestVersion": "1.16", | ||
"version": "1.0.9", |
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.
Let's make this 1.0.0
to reflect that this is the first version of the sample.
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.
fixed
|
||
Version|Manifest version|Date|Author|Comments | ||
-------|--|--|----|-------- | ||
1.0|1.16|November 15, 2023 |Jegadeesh V <br/> Wajeed Shaikh|Initial release for Csharp labs |
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.
Let's update the table to make it up to date:
- update the version column should match the version in the manifest, ideally,
1.0.0
- update the date to the current date
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.
fixed
- [Azure subscription](https://portal.azure.com) | ||
- [Teams Toolkit for Visual Studio](https://learn.microsoft.com/en-us/microsoftteams/platform/toolkit/toolkit-v4/install-teams-toolkit-vs?pivots=visual-studio-v17-7) | ||
- You will need a Microsoft work or school account with [permissions to upload custom Teams applications](https://learn.microsoft.com/microsoftteams/platform/concepts/build-and-test/prepare-your-o365-tenant#enable-custom-teams-apps-and-turn-on-custom-app-uploading). The account will also need a Microsoft Copilot for Microsoft 365 license to use the extension in Copilot. | ||
- You will need to create [local Azure Storage](https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azurite?tabs=visual-studio%2Cblob-storage). |
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.
Let's update the link here to point to the relevant section in the page and remove the en-us
locale, https://learn.microsoft.com/azure/storage/common/storage-use-azurite?tabs=visual-studio%2Cblob-storage#running-azurite-from-an-aspnet-project
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.
fixed
- You will need a Microsoft work or school account with [permissions to upload custom Teams applications](https://learn.microsoft.com/microsoftteams/platform/concepts/build-and-test/prepare-your-o365-tenant#enable-custom-teams-apps-and-turn-on-custom-app-uploading). The account will also need a Microsoft Copilot for Microsoft 365 license to use the extension in Copilot. | ||
- You will need to create [local Azure Storage](https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azurite?tabs=visual-studio%2Cblob-storage). | ||
|
||
## Setup local Azure Storage |
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.
Visual Studio 2022 has Azure Storage emulation built in, we should look to use this instead of introducing a dependency on node and the Azurite npm package. See Use the Azurite emulator for local Azure Storage development: Running Azurite from an ASP.NET project
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.
Fixed
APP_NAME_SUFFIX=local | ||
|
||
# Generated during provision, you can also add your own variables. | ||
BOT_ID= |
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 environment variable is not given a value after running Prepare Teams App Dependencies process so can be removed.
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.
fixed
SECRET_STORAGE_ACCOUNT_CONNECTION_STRING=UseDevelopmentStorage=true | ||
~~~ | ||
|
||
### Ports occupied error |
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 isn't relevant to Visual Studio
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.
fixed
|
||
This happens if you shut down the debugger and then immediately start your app again. It takes a moment for your app to stop running and release its TCP ports. Just close the error and try again. | ||
|
||
### The app takes a long time to start after pressing F5 or the debug button |
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 isn't relevant to Visual Studio
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.
Fixed
MicrosoftAppPassword: ${{SECRET_AAD_APP_CLIENT_SECRET}} | ||
MicrosoftAppType: ${{MICROSOFT_APP_TYPE}} | ||
MicrosoftAppTenantId: ${{MICROSOFT_APP_TENANT_ID}} | ||
StorageConnectionString: "UseDevelopmentStorage=true" |
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 avoid hard coding values in the YML file. I would recommend creating an environment variable in the env.local.user
file to store this value.
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.
fixed
[Ll]og/ | ||
|
||
# Notification local store | ||
.notification.localstore.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.
We should include the following paths:
appsettings.json
.vs
__azurite*
__blobstorage__
__queuestorage__
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.
fixed
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.
fixed
@@ -0,0 +1,12 @@ | |||
{ |
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.
The test tool isn't used, so this file can be removed.
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.
fixed
@Jegadeesh-MSFT I've made this PR draft, please mark as ready for review you've completed the requested changes 👍 |
using Microsoft.Bot.Builder.Teams; | ||
using Microsoft.Bot.Schema.Teams; | ||
using Microsoft.Bot.Schema; | ||
using Newtonsoft.Json.Linq; |
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 unused and can be removed.
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.
Fixed
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.Extensions.Logging; | ||
using System; |
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.
These are unused and can be removed.
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.
Fixed
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.
Thank you for the updates @Jegadeesh-MSFT there are a few more comments to be addressed before we merge.
For some of the code changes, please keep in mind that the changes may also need to be reflected in the code tour markdown page.
var action = invokeValue.Action as AdaptiveCardInvokeAction; | ||
if (action == null) | ||
{ | ||
throw new InvalidOperationException("AdaptiveCardInvokeAction is null"); | ||
} | ||
|
||
var verb = action.Verb?.ToString(); | ||
|
||
switch (verb) | ||
{ | ||
case "ok": | ||
return await CardHandler.HandleTeamsCardActionUpdateStockAsync(turnContext, _configuration ,cancellationToken); | ||
|
||
case "restock": | ||
return await CardHandler.HandleTeamsCardActionRestockAsync(turnContext, _configuration, cancellationToken); | ||
|
||
case "cancel": | ||
return await CardHandler.HandleTeamsCardActionCancelRestockAsync(turnContext, _configuration, cancellationToken); | ||
|
||
default: | ||
return Utils.CreateActionErrorResponse((int)HttpStatusCode.OK, 0, $"ActionVerbNotSupported: {verb} is not a supported action verb."); | ||
} |
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 can remove the cast and simplify the switch statement to use an expression
var action = invokeValue.Action as AdaptiveCardInvokeAction; | |
if (action == null) | |
{ | |
throw new InvalidOperationException("AdaptiveCardInvokeAction is null"); | |
} | |
var verb = action.Verb?.ToString(); | |
switch (verb) | |
{ | |
case "ok": | |
return await CardHandler.HandleTeamsCardActionUpdateStockAsync(turnContext, _configuration ,cancellationToken); | |
case "restock": | |
return await CardHandler.HandleTeamsCardActionRestockAsync(turnContext, _configuration, cancellationToken); | |
case "cancel": | |
return await CardHandler.HandleTeamsCardActionCancelRestockAsync(turnContext, _configuration, cancellationToken); | |
default: | |
return Utils.CreateActionErrorResponse((int)HttpStatusCode.OK, 0, $"ActionVerbNotSupported: {verb} is not a supported action verb."); | |
} | |
var action = invokeValue.Action ?? throw new InvalidOperationException("AdaptiveCardInvokeAction is null"); | |
var verb = action.Verb?.ToString(); | |
return verb switch | |
{ | |
"ok" => await CardHandler.HandleTeamsCardActionUpdateStockAsync(turnContext, _configuration, cancellationToken), | |
"restock" => await CardHandler.HandleTeamsCardActionRestockAsync(turnContext, _configuration, cancellationToken), | |
"cancel" => await CardHandler.HandleTeamsCardActionCancelRestockAsync(turnContext, _configuration, cancellationToken), | |
_ => Utils.CreateActionErrorResponse((int)HttpStatusCode.OK, 0, $"ActionVerbNotSupported: {verb} is not a supported action verb."), | |
}; |
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.
Fixed
switch (query.CommandId) | ||
{ | ||
case ProductSearchCommand.CommandId: | ||
return await ProductSearchCommand.HandleTeamsMessagingExtensionQueryAsync(turnContext, query, _configuration, cancellationToken); | ||
|
||
case DiscountedSearchCommand.CommandId: | ||
return await DiscountedSearchCommand.HandleTeamsMessagingExtensionQueryAsync(turnContext, query, _configuration, cancellationToken); | ||
|
||
default: | ||
throw new InvalidOperationException("Unsupported command"); |
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 can simplify here by removing the switch statement and using an expression.
switch (query.CommandId) | |
{ | |
case ProductSearchCommand.CommandId: | |
return await ProductSearchCommand.HandleTeamsMessagingExtensionQueryAsync(turnContext, query, _configuration, cancellationToken); | |
case DiscountedSearchCommand.CommandId: | |
return await DiscountedSearchCommand.HandleTeamsMessagingExtensionQueryAsync(turnContext, query, _configuration, cancellationToken); | |
default: | |
throw new InvalidOperationException("Unsupported command"); | |
return query.CommandId switch | |
{ | |
ProductSearchCommand.CommandId => await ProductSearchCommand.HandleTeamsMessagingExtensionQueryAsync(turnContext, query, _configuration, cancellationToken), | |
DiscountedSearchCommand.CommandId => await DiscountedSearchCommand.HandleTeamsMessagingExtensionQueryAsync(turnContext, query, _configuration, cancellationToken), | |
_ => throw new InvalidOperationException("Unsupported command"), | |
}; |
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.
Fixed
public class SearchBot : TeamsActivityHandler | ||
{ | ||
private readonly ILogger<SearchBot> _logger; | ||
private readonly IConfiguration _configuration; | ||
|
||
public SearchBot(ILogger<SearchBot> logger, IConfiguration configuration) | ||
{ | ||
_logger = logger; | ||
_configuration = configuration; | ||
} |
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 can use a Primary constructor here
public class SearchBot : TeamsActivityHandler | |
{ | |
private readonly ILogger<SearchBot> _logger; | |
private readonly IConfiguration _configuration; | |
public SearchBot(ILogger<SearchBot> logger, IConfiguration configuration) | |
{ | |
_logger = logger; | |
_configuration = configuration; | |
} | |
public class SearchBot(ILogger<SearchBot> logger, IConfiguration configuration) : TeamsActivityHandler | |
{ | |
private readonly ILogger<SearchBot> _logger = logger; | |
private readonly IConfiguration _configuration = configuration; |
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.
Fixed
public class BotController : ControllerBase | ||
{ | ||
private readonly IBotFrameworkHttpAdapter Adapter; | ||
private readonly IBot Bot; | ||
|
||
public BotController(IBotFrameworkHttpAdapter adapter, IBot bot) | ||
{ | ||
Adapter = adapter; | ||
Bot = bot; | ||
} |
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 can use a primary constructor here
public class BotController : ControllerBase | |
{ | |
private readonly IBotFrameworkHttpAdapter Adapter; | |
private readonly IBot Bot; | |
public BotController(IBotFrameworkHttpAdapter adapter, IBot bot) | |
{ | |
Adapter = adapter; | |
Bot = bot; | |
} | |
public class BotController(IBotFrameworkHttpAdapter adapter, IBot bot) : ControllerBase | |
{ | |
private readonly IBotFrameworkHttpAdapter Adapter = adapter; | |
private readonly IBot Bot = bot; |
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.
Fixed
using System; | ||
using System.Collections.Generic; | ||
using System.IO; | ||
using System.Linq; | ||
using System.Text.Json; | ||
using System.Threading.Tasks; | ||
using Azure; | ||
using Azure.Data.Tables; | ||
using msgext_northwind_inventory_csharp.Models; |
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 the unnecessary usings
using System; | |
using System.Collections.Generic; | |
using System.IO; | |
using System.Linq; | |
using System.Text.Json; | |
using System.Threading.Tasks; | |
using Azure; | |
using Azure.Data.Tables; | |
using msgext_northwind_inventory_csharp.Models; | |
using System.Text.Json; | |
using Azure; | |
using Azure.Data.Tables; | |
using msgext_northwind_inventory_csharp.Models; |
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.
Fixed
using msgext_northwind_inventory_csharp; | ||
using msgext_northwind_inventory_csharp.Bots; | ||
using msgext_northwind_inventory_csharp.DbSetup; | ||
using Microsoft.Bot.Builder; | ||
using Microsoft.Bot.Builder.Integration.AspNet.Core; | ||
using Microsoft.Bot.Connector.Authentication; | ||
using Microsoft.Extensions.Configuration; | ||
using Microsoft.Bot.Builder.BotFramework; | ||
using Microsoft.Extensions.DependencyInjection; | ||
using Microsoft.Extensions.Hosting; | ||
using msgext_northwind_inventory_csharp.NorthwindDB; |
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 unnecessary usings
using msgext_northwind_inventory_csharp; | |
using msgext_northwind_inventory_csharp.Bots; | |
using msgext_northwind_inventory_csharp.DbSetup; | |
using Microsoft.Bot.Builder; | |
using Microsoft.Bot.Builder.Integration.AspNet.Core; | |
using Microsoft.Bot.Connector.Authentication; | |
using Microsoft.Extensions.Configuration; | |
using Microsoft.Bot.Builder.BotFramework; | |
using Microsoft.Extensions.DependencyInjection; | |
using Microsoft.Extensions.Hosting; | |
using msgext_northwind_inventory_csharp.NorthwindDB; | |
using msgext_northwind_inventory_csharp; | |
using msgext_northwind_inventory_csharp.Bots; | |
using msgext_northwind_inventory_csharp.DbSetup; | |
using Microsoft.Bot.Builder; | |
using Microsoft.Bot.Builder.Integration.AspNet.Core; | |
using Microsoft.Bot.Connector.Authentication; |
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.
fixed
- uses: script | ||
with: | ||
run: | ||
# echo "::set-teamsfx-env MICROSOFT_APP_TYPE=SingleTenant"; | ||
echo "::set-teamsfx-env MICROSOFT_APP_TYPE=MultiTenant"; | ||
echo "::set-teamsfx-env MICROSOFT_APP_TENANT_ID=${{AAD_APP_TENANT_ID}}"; |
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 step seems unnecessary, what is it used for?
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 part is a TTK auto generated code, It can be edited and directly we can add value "AAD_APP_TENANT_ID" and "MultiTenant" to the appsettings part in next line.
Due to naming conventions looked fine we left as it is. Please let me know if you want me to edit it.
> IMPORTANT - Microsoft Copilot for Microsoft 365 only works in the "New" Teams. Please don't miss this step! | ||
|
||
> If you restart your debugger after switching to "New" teams, you may get an error message after the debugger starts. This is a known problem; please just close the error dialog and continue testing. |
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.
Block quotes formatting should be surrounded by squared brackets, be prefixed with an exclamation mark and on it's own line for correct formatting to be applied.
> IMPORTANT - Microsoft Copilot for Microsoft 365 only works in the "New" Teams. Please don't miss this step! | |
> If you restart your debugger after switching to "New" teams, you may get an error message after the debugger starts. This is a known problem; please just close the error dialog and continue testing. | |
> [!IMPORTANT] | |
> Microsoft Copilot for Microsoft 365 only works in the "New" Teams. Please don't miss this step! | |
> If you restart your debugger after switching to "New" teams, you may get an error message after the debugger starts. This is a known problem; please just close the error dialog and continue testing. |
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.
fixed
|
||
|
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.
Let's remove this additional empty space
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.
fixed
using Newtonsoft.Json; | ||
using msgext_northwind_inventory_csharp.Handlers; | ||
|
||
namespace msgext_northwind_inventory_csharp.Bots |
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.
Let's update the namespace with a friendlier name
namespace msgext_northwind_inventory_csharp.Bots | |
namespace NorthwindInventory.Bots |
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.
fixed
Hi @garrytrinder , We have fixed all the PR comments. For the two items below, we need confirmation. 1 . #143 (comment) Thanks ! CC: @Wajeed-msft |
Thanks @Jegadeesh-MSFT, I'm getting the following error when following the instructions in
|
Hi @garrytrinder , The error seems to be caused by the missing SECRET_STORAGE_CONNECTION_STRING in the .env file. I will check and update it accordingly. Thanks ! |
Hi @garrytrinder , Fixed the issue and updated the exercises accordingly. Please verify from your end. Thanks ! |
Thanks for the updates @Jegadeesh-MSFT 👍 |
No description provided.