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.

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

add create conversation to CloudAdapter #5562

Merged
merged 2 commits into from Jun 15, 2021
Merged

add create conversation to CloudAdapter #5562

merged 2 commits into from Jun 15, 2021

Conversation

johnataylor
Copy link
Member

Fixes #5488

Also updates the abstract class to avoid casts in the application code.

@johnataylor johnataylor requested a review from a team as a code owner April 29, 2021 19:41
@johnataylor johnataylor marked this pull request as draft April 29, 2021 19:41
@coveralls
Copy link
Collaborator

coveralls commented Apr 29, 2021

Pull Request Test Coverage Report for Build 251768

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 14 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.06%) to 78.908%

Files with Coverage Reduction New Missed Lines %
/libraries/Microsoft.Bot.Builder/BotAdapter.cs 5 71.43%
/libraries/Microsoft.Bot.Builder/CloudAdapterBase.cs 9 72.86%
Totals Coverage Status
Change from base Build 251299: 0.06%
Covered Lines: 18189
Relevant Lines: 23051

💛 - Coveralls

@stevengum stevengum self-requested a review May 12, 2021 20:43
/// specified users, the ID of the activity's <see cref="IActivity.Conversation"/>
/// will contain the ID of the new conversation.</para>
/// </remarks>
public virtual Task CreateConversationAsync(string botAppId, string channelId, string serviceUrl, ConversationParameters conversationParameters, BotCallbackHandler callback, CancellationToken cancellationToken)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific reason we didn't have this method before? Or any of the CreateConversationAsync implementations in BotFrameworkAdapter?

Will this support Skills making 1:1 conversations on behalf of the host bot (e.g. in Teams)?

@johnataylor @EricDahlvang

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it was just an omission. This improves the signature slightly. Its probably fair to say we simply don't have enough examples to make strong claims around how general any of this is. It improves the typing (removes the casts) by adding it to the abstract class and makes things more consistent. This works for Teams.

@tracyboehrer
Copy link
Member

Is this an R14 goal? I only ask because at least for the Teams conversation sample, this would be required to get around my hack (which we could never release). I can't say whether the Composer Teams samples are on the slate for R14, so it may not matter.

@johnataylor johnataylor marked this pull request as ready for review June 11, 2021 23:08
@johnataylor
Copy link
Member Author

Is this an R14 goal? I only ask because at least for the Teams conversation sample, this would be required to get around my hack (which we could never release). I can't say whether the Composer Teams samples are on the slate for R14, so it may not matter.

@johnataylor johnataylor reopened this Jun 12, 2021
@johnataylor
Copy link
Member Author

@tracyboehrer yes absolutely R14. Precisely because the gap you identified! Thanks!

Copy link
Member

@EricDahlvang EricDahlvang left a comment

Choose a reason for hiding this comment

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

This works for Teams. However, will this work from a skill without also exposing 'audience'? (as pointed out by @stevengum )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CreateConversationAsync to CloudAdapter implementation
6 participants