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.

Sign up for GitHub

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

Jump to bottom

Add middleware to address transitional location of tenantId field #1754

Merged
merged 8 commits into from
Apr 23, 2019
Merged

Add middleware to address transitional location of tenantId field #1754

merged 8 commits into from
Apr 23, 2019

Conversation

benbrown
Copy link
Contributor

This is the C# version of this Javascript PR

Description

This change handles the in-transition location of the tenant ID field for MS Teams.

Specific Changes

Load a middleware in the constructor for BotFrameworkAdapter that will look for channelData.tenant.id and copy the value to conversation.tenantId for use in createConversation.

Testing

  • Connect a bot to MS Teams, then use adapter.createConversation to create a DM with a user.

@gabog gabog self-assigned this Apr 22, 2019
@gabog gabog added the 4.4 April 15, 2019 Release label Apr 22, 2019
@axelsrz axelsrz mentioned this pull request Apr 22, 2019
Closed
@gabog
Added TenantIdWorkaroundForTeamsMiddleware to BotFrameworkAdapter to …
8a3ce58 
…workaround issue with MSTeams not sending the TenantId in Acivity.Conversation.TenantId.

Replaced ArgumentNullException thrown by NullReferenceException (this is more apriopiate for the type of validatation that is being done).

Fixed types and stylecop violations on BotFrameworkAdapter.

Added unit tests for MSTeams workaround.
@gabog gabog requested a review from cleemullins April 23, 2019 03:38
@gabog gabog marked this pull request as ready for review April 23, 2019 03:44
@cleemullins
Copy link
Contributor 

No issues found in Microsoft.Bot.Builder.dll
No issues found in Microsoft.Bot.Builder.AI.Luis.dll
No issues found in Microsoft.Bot.Builder.AI.QnA.dll
No issues found in Microsoft.Bot.Builder.ApplicationInsights.dll
No issues found in Microsoft.Bot.Builder.Azure.dll
No issues found in Microsoft.Bot.Builder.Dialogs.dll
No issues found in Microsoft.Bot.Builder.TemplateManager.dll
No issues found in Microsoft.Bot.Configuration.dll
No issues found in Microsoft.Bot.Connector.dll
No issues found in Microsoft.Bot.Schema.dll

@gabog
Copy link
Contributor

gabog commented Apr 23, 2019

@benbrown, I updated the middleware to also check for ChannelId = MSTeams and also added some unit tests for it.
@cleemullins, I noticed some issues with the adapter code that I fixed too (please take a look if you have time). These changes include:

  • naming conventons
  • typos in documentation and variables
  • changed some validations to throw NullReferenceException instead of ArgumentNullExceptions (IMHO, ArgumentNullException was not the right exception to throw and we were using the argName parameter to pass an error message)

benbrown
benbrown commented Apr 23, 2019
View reviewed changes
Copy link
Contributor Author

@benbrown benbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks right to me

@cleemullins
Copy link
Contributor 

No issues found in Microsoft.Bot.Builder.dll
No issues found in Microsoft.Bot.Builder.AI.Luis.dll
No issues found in Microsoft.Bot.Builder.AI.QnA.dll
No issues found in Microsoft.Bot.Builder.ApplicationInsights.dll
No issues found in Microsoft.Bot.Builder.Azure.dll
No issues found in Microsoft.Bot.Builder.Dialogs.dll
No issues found in Microsoft.Bot.Builder.TemplateManager.dll
No issues found in Microsoft.Bot.Configuration.dll
No issues found in Microsoft.Bot.Connector.dll
No issues found in Microsoft.Bot.Schema.dll

cleemullins
cleemullins suggested changes Apr 23, 2019
View reviewed changes
Copy link
Contributor

@cleemullins cleemullins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕐

@cleemullins
Copy link
Contributor 

No issues found in Microsoft.Bot.Builder.dll
No issues found in Microsoft.Bot.Builder.AI.Luis.dll
No issues found in Microsoft.Bot.Builder.AI.QnA.dll
No issues found in Microsoft.Bot.Builder.ApplicationInsights.dll
No issues found in Microsoft.Bot.Builder.Azure.dll
No issues found in Microsoft.Bot.Builder.Dialogs.dll
No issues found in Microsoft.Bot.Builder.TemplateManager.dll
No issues found in Microsoft.Bot.Configuration.dll
No issues found in Microsoft.Bot.Connector.dll
No issues found in Microsoft.Bot.Schema.dll

@cleemullins cleemullins merged commit 9f90f33 into master Apr 23, 2019
@cleemullins cleemullins deleted the benbrown/884 branch April 23, 2019 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabog gabog gabog left review comments

@cleemullins cleemullins cleemullins approved these changes

Assignees

@gabog gabog

Labels
4.4 April 15, 2019 Release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@benbrown @cleemullins @gabog