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 userId parameter to signOutUser #797

Closed
wants to merge 10 commits into from

Conversation

Aliandi
Copy link
Contributor

@Aliandi Aliandi commented Feb 22, 2019

Parity with C#

Description

It was found that the TypeScript method signOutUser() differed from the C# implementation of signOutUserAsync(). The TypeScript version expects less parameters, not letting the user pass a userId.

TypeScript
public async signOutUser(context: TurnContext, connectionName: string): Promise<void>

C#
public virtual async Task SignOutUserAsync(ITurnContext turnContext, string connectionName = null, string userId = null, CancellationToken cancellationToken = default(CancellationToken))

Specific Changes

  • add userId optional parameter to the method
  • add handling for when userId is null

@coveralls
Copy link

coveralls commented Feb 22, 2019

Pull Request Test Coverage Report for Build #2052

  • 0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.1%) to 87.41%

Changes Missing Coverage Covered Lines Changed/Added Lines %
libraries/botbuilder/src/botFrameworkAdapter.ts 0 2 0.0%
Files with Coverage Reduction New Missed Lines %
libraries/botframework-config/src/botConfigurationBase.ts 2 85.59%
Totals Coverage Status
Change from base Build #2041: -0.1%
Covered Lines: 3004
Relevant Lines: 3307

💛 - Coveralls

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.

Is it practical to add a test around this?

if (!context.activity.from || !context.activity.from.id) {
throw new Error(`BotFrameworkAdapter.signOutUser(): missing from or from.id`);
}
!userId? userId = context.activity.from.id: userId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you do this using a less weird syntax? This makes my head hurt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@munozemilio
Copy link
Contributor

Hi @Aliandi,

Fork merges are disabled that's why the build is failing. Chris has grant you write permissions to the repo.
Could you please push your changes in a new branch from this repo?

Thanks!

@Aliandi
Copy link
Contributor Author

Aliandi commented Mar 26, 2019

Replaced by #837

@Aliandi Aliandi closed this Mar 26, 2019
@denscollo denscollo deleted the issue/signOutUser branch September 19, 2019 19:06
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.

5 participants