refactor and rename TeamsInfo #2648
Conversation
johnataylor
requested review from
035 and
stevengum
and removed request for
035
Pull Request Test Coverage Report for Build 81615
💛 - Coveralls |
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1 |
| private ITeamsConnectorClient _teamsConnectorClient; | ||
| private TeamsChannelData _teamsChannelData; | ||
|
|
||
| public TeamsInfo(string conversationId, ConnectorClient connectorClient, TeamsChannelData teamsChannelData) |
There was a problem hiding this comment.
this might not be right - probably better to pass in teamId rather than TeamChannelData...
| TeamsInfo = new TeamsInfo( | ||
| turnContext.Activity?.Conversation?.Id, | ||
| (ConnectorClient)turnContext.TurnState.Get<IConnectorClient>(), | ||
| turnContext.Activity.GetChannelData<TeamsChannelData>()); |
There was a problem hiding this comment.
better to say...
turnContext.Activity.GetChannelData()?.Team?.Id
i think we only need teamId
| @@ -79,10 +34,10 @@ public override async Task OnTurnAsync(ITurnContext turnContext, CancellationTok | |||
| throw new ArgumentException($"{nameof(turnContext)}.Activity must have non-null Type."); | |||
| } | |||
|
|
|||
| if (turnContext.Activity.ChannelId == Channels.Msteams) | |||
There was a problem hiding this comment.
Should we be creating this object if the ChannelId is not Msteams?
There was a problem hiding this comment.
the real dependency on having a teamId so I softened the check
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1 |
| { | ||
| _connectorClient = connectorClient; | ||
| _teamsConnectorClient = new TeamsConnectorClient(connectorClient.BaseUri, connectorClient.Credentials, connectorClient.HttpClient); | ||
| } |
There was a problem hiding this comment.
no this can be null - if it is then the methods throw later
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1 |
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1 |
There was a problem hiding this comment.
This PR looks good, but I noticed some weirdness so you need to revisit TeamsOperationsExtensions.
| var channelList = await GetTeamsConnectorClient(turnContext).Teams.FetchChannelListAsync(teamId, cancellationToken).ConfigureAwait(false); | ||
| return channelList.Conversations; |
There was a problem hiding this comment.
Seems silly that it returns a class with one property... I supposed the ConversationList class could be considered extensible.
That said, I think you need to rename the extension method.
There was a problem hiding this comment.
This is the Teams generated code from the swagger. The naming is slightly confusing but its pretty buried here.
|
It's optional on revisiting the extension method mentioned in my review, but if Teams decides to add more data to the ConversationList schema in the swagger file, that method name won't make sense. (It makes partial sense right now) |
No description provided.