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

Adaptive dialogs bug fix #2201

Merged
merged 19 commits into from
Jun 5, 2020
Merged

Adaptive dialogs bug fix #2201

merged 19 commits into from
Jun 5, 2020

Conversation

chon219
Copy link
Member

@chon219 chon219 commented May 7, 2020

Fixes: #2210
Fixes: #2209

Description

We found some minor bugs in adaptive dialogs and we need fix them in this PR.

Specific Changes

  • Registered HttpRequest as well as related converters
  • Added missing onComputeId() for OAuthInput
  • Avoid possible null pointer exceptions in Inputs
  • Avoid possible null pointer exceptions in RegexRecognizer

@chon219 chon219 requested review from Stevenic and luhan2017 May 7, 2020 07:43
@chon219 chon219 self-assigned this May 7, 2020
@coveralls
Copy link

coveralls commented May 7, 2020

Pull Request Test Coverage Report for Build 136129

  • 42 of 43 (97.67%) changed or added relevant lines in 14 files are covered.
  • 9 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.5%) to 80.855%

Changes Missing Coverage Covered Lines Changed/Added Lines %
libraries/botbuilder-dialogs-adaptive/src/input/oauthInput.ts 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
libraries/botframework-streaming/src/webSocket/webSocketTransport.ts 2 80.65%
libraries/botframework-streaming/src/webSocket/webSocketServer.ts 7 66.67%
Totals Coverage Status
Change from base Build 136116: 0.5%
Covered Lines: 12790
Relevant Lines: 15104

💛 - Coveralls

@cleemullins cleemullins added R10 Release 10 - August 17th, 2020 and removed Adaptive labels May 7, 2020
@cleemullins
Copy link
Contributor

@chon219, This is an R10 fix, and with R10 we're starting a new policy. PRs need to have an issue. Can you please create an issue, tag & triage it appropiatly, and then tie it to this PR?

@cleemullins
Copy link
Contributor

WE'll be enabling the github enforcement of this soon, but we should start now.

@chon219
Copy link
Member Author

chon219 commented May 7, 2020

@chon219, This is an R10 fix, and with R10 we're starting a new policy. PRs need to have an issue. Can you please create an issue, tag & triage it appropiatly, and then tie it to this PR?

OK, I'll create related issues later. Thanks for reminding.

@Stevenic Stevenic requested a review from stevengum May 8, 2020 20:38
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.

Please add unit test coverage for these changes.

Copy link
Member

@stevengum stevengum left a comment

Choose a reason for hiding this comment

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

One change for the OAuthInput, but there are some other concerns I have regarding the use of Entity in the RegexRecognizer. (And add unit tests)

@stevengum stevengum dismissed cleemullins’s stale review June 5, 2020 18:45

Dismissing as unit tests were added to PR

@stevengum stevengum merged commit 1e4bb68 into master Jun 5, 2020
@stevengum stevengum deleted the zim/adaptive-bug-fix branch June 5, 2020 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R10 Release 10 - August 17th, 2020
Projects
None yet
Development

Successfully merging this pull request may close these issues.

onComputeId method not found in OAuthInput Microsoft.HttpRequest not registered
6 participants