Fix locale determination in confirm/choice prompts/inputs #2534

v-kydela · 2020-07-14T23:23:34Z

Description

While investigating the customer-reported issue, I discovered some related issues that I decided to fix at the same time. While C# and Python were unaffected by the original issue the customer reported, they may need to be updated for parity with the rest of my changes here.

Specific Changes

mapToNearestLanguage now correctly treats all falsey values as falsey and doesn't try to call toLowerCase on null, etc.
Since mapToNearestLanguage no longer throws an exception when passed a null culture code, ChoicePrompt.determineCulture no longer falls back to English to avoid passing a null. This avoids redundantly using English as a fallback twice, and it now works in a way that's consistent with ConfirmPrompt.determineCulture.
ChoicePrompt.determineCulture now uses hasOwnProperty to be consistent with the other three cases.
Confirm prompts, confirm inputs, and choice inputs now use PromptCultureModels.English.locale instead of "en-us" to avoid using a string literal and to be consistent with choice prompts.
Choice inputs and confirm inputs now have centralized determineCulture functions.

What I did not change

@cleemullins made a good point that we might want to throw an error instead of defaulting to English. I didn't like that idea at first because it seemed like it would create the same sense of surprise that this customer wanted to avoid, but now I'm considering that it might be better to throw an error after all. I'm biased as an English speaker, and for non-English speakers it might be very surprising to suddenly see English words and grammar in an otherwise-non-English bot, and it might be hard for a developer to figure out why that's happening. An error message could contain everything the dev needs to know to fix it. I left that alone for now since it seems outside of the scope of this fix, but it may warrant further discussion.
One of the four locales in the fallback chain for choice prompts and inputs is the locale in the recognizerOptions property. It's only used as a potential locale when recognizing input and not when rendering the prompt, which makes sense given the name of the property. But for simplicity and consistency and completeness, we might consider changing it so that it's used in both cases.

coveralls · 2020-07-14T23:34:41Z

Pull Request Test Coverage Report for Build 170401

20 of 22 (90.91%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.005%) to 83.346%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
libraries/botbuilder-dialogs-adaptive/src/input/choiceInput.ts	8	9	88.89%
libraries/botbuilder-dialogs-adaptive/src/input/confirmInput.ts	7	8	87.5%

Totals
Change from base Build 170396:	0.005%
Covered Lines:	15590
Relevant Lines:	17788

💛 - Coveralls

stevengum

Please see comments

libraries/botbuilder-dialogs/src/prompts/confirmPrompt.ts

libraries/botbuilder-dialogs/src/prompts/promptCultureModels.ts

stevengum · 2020-07-21T19:14:37Z

libraries/botbuilder-dialogs/src/prompts/choicePrompt.ts

+        const optLocale: string = opt && opt.locale ? opt.locale : null;
+        let culture: string = PromptCultureModels.mapToNearestLanguage(activity.locale || optLocale || this.defaultLocale);
+        if (!culture || !this.choiceDefaults.hasOwnProperty(culture)) {


Your changes make sense (thank you for removing the default PromptCultureModels.English.locale value).

However, this function in the larger context doesn't make sense to me. Could you walk me through it?

In ChoicePrompt.onRecognize determineCulture is called and overrides any pre-existing values on this.recognizerOptions. Aren't there concurrency issues if either this.choiceDefaults or this.recognizerOptions changes?

(this.choiceDefaults is potentially a pointer to a public object and this.recognizerOptions is a public property)

In ChoicePrompt.onPrompt the locale is obtained without it always being used.

If this.choiceOptions || this.choiceDefaults[PromptCultureModels.English.locale] resolve to undefined, this is fine because ChoiceFactory provides default values instead?

It seems like there are some redundancies going on, so any insight (and test coverage) would be helpful at clearing things up

I think determineCulture is just supposed to determine the culture by accounting for potential missing or malformed values and resorting to fallbacks.

this.choiceDefaults is private and doesn't change. Sure the object is public like you said, but it would be pretty strange to modify the object passed as choice defaults, and I don't think anything bad would happen even if it does get modified. Like with this.recognizerOptions, the worst that could happen is that different settings get used. It's certainly strange that the options locale gets modified in onRecognize but if the property was undefined then the new object that's created doesn't get assigned to the property. I guess if the same dialog instance was used by multiple users with different locales then their locales could get passed from one to another, and I guess that's why the options locale is third in the fallback chain.

I can see that in onPrompt the locale is obtained without it always being used, and I suspect the author chose clarity over efficiency since determineCulture isn't a slow function to run.

Yes, it looks like ChoiceFactory provides default values, but that seems a little outside the scope of this PR. The scope has already creeped beyond the customer's issue so I had thought it was more likely that you'd think it was too much than too little. While I was happy to expand the scope, I had still thought this PR should just be about determining the culture. Do you want to also include code about making sure the choice options work properly?

libraries/botbuilder-dialogs-adaptive/src/input/choiceInput.ts

stevengum · 2020-07-21T19:28:44Z

libraries/botbuilder-dialogs-adaptive/src/input/choiceInput.ts

@@ -136,4 +132,16 @@ export class ChoiceInput extends InputDialog {
        return `ChoiceInput[${ this.prompt && this.prompt.toString() }]`;
    }

+    private determineCulture(dc: DialogContext, opt: FindChoicesOptions = null): string {


Shouldn't this mirror the C# implementation of GetCulture()

I discuss in the description that I've made changes that, if accepted, may require parity pull requests in other repos. In this case, the C# implementation is flawed because it does not include the FindChoicesOptions as a potential fallback. Would you like me to remove everything that doesn't match C# and save those changes for later, or is it okay to make these fixes now?

carlosscastro · 2020-07-30T18:50:43Z

Thanks Kyle! I'm doing some similar work in c# with a slightly different approach. Can we not merge this PR until mid next week when I have the C# version? I want to make sure we do the same in both languages.

You mentioned that you proposed language changes for other languages. I'll keep those in mind when I do the C# work.

v-kydela · 2020-09-11T18:05:56Z

@carlosscastro
@stevengum

Can either of you comment on what's needed to move forward with this? I don't have permissions for the functional tests check that's failing and I don't know if that's relevant to this PR. I notice that it's not listed as a required check.

joshgummersall

Once we get these changes addressed I think we can land this PR.

libraries/botbuilder-dialogs-adaptive/src/input/confirmInput.ts

libraries/botbuilder-dialogs-adaptive/src/input/choiceInput.ts

…l expressions

joshgummersall · 2020-10-22T21:06:36Z

@v-kydela couple merge conflicts to resolve as well.

# Conflicts: # libraries/botbuilder-dialogs-adaptive/src/input/choiceInput.ts # libraries/botbuilder-dialogs-adaptive/src/input/confirmInput.ts # libraries/botbuilder-dialogs/src/prompts/choicePrompt.ts

Fix locale determination in confirm/choice prompts/inputs

499790d

v-kydela requested review from chon219, cleemullins, Danieladu and mdrichardson July 14, 2020 23:23

v-kydela requested review from a team as code owners July 14, 2020 23:23

v-kydela requested a review from luhan2017 July 14, 2020 23:23

munozemilio requested a review from stevengum July 20, 2020 18:09

stevengum reviewed Jul 21, 2020

View reviewed changes

libraries/botbuilder-dialogs-adaptive/src/input/choiceInput.ts Outdated Show resolved Hide resolved

stevengum reviewed Jul 21, 2020

View reviewed changes

Add mapToNearestLanguage test

fea70d3

v-kydela requested a review from stevengum July 23, 2020 22:18

stevengum added the R11 label Jul 28, 2020

stevengum assigned johnataylor and stevengum Jul 28, 2020

carlosscastro added 2 commits August 17, 2020 22:13

Merge branch 'master' into kyled/inputLocale

809924c

Merge branch 'master' into kyled/inputLocale

44046f7

carlosscastro requested a review from a team as a code owner September 1, 2020 18:48

gabog added this to the R11 milestone Sep 2, 2020

gabog removed the R11 label Sep 2, 2020

v-kydela mentioned this pull request Sep 11, 2020

BUG: The given key ‘de’ was not present in the dictionary microsoft/BotFramework-Composer#4094

Closed

8 tasks

Danieladu assigned johnataylor and stevengum and unassigned johnataylor and stevengum Sep 13, 2020

joshgummersall changed the base branch from master to main September 29, 2020 16:30

cleemullins removed this from the R11 milestone Oct 21, 2020

cleemullins unassigned johnataylor Oct 21, 2020

joshgummersall suggested changes Oct 21, 2020

View reviewed changes

Fix optional parameters, inferred variable types, and "or not" logica…

144e2bd

…l expressions

stevengum approved these changes Oct 22, 2020

View reviewed changes

Merge branch 'main' into kyled/inputLocale

0e92c81

# Conflicts: # libraries/botbuilder-dialogs-adaptive/src/input/choiceInput.ts # libraries/botbuilder-dialogs-adaptive/src/input/confirmInput.ts # libraries/botbuilder-dialogs/src/prompts/choicePrompt.ts

v-kydela requested a review from joshgummersall October 22, 2020 22:25

Merge branch 'main' into kyled/inputLocale

5be7bdc

joshgummersall approved these changes Oct 22, 2020

View reviewed changes

Merge branch 'main' into kyled/inputLocale

ae32f16

joshgummersall merged commit 4dd832e into main Oct 22, 2020

joshgummersall deleted the kyled/inputLocale branch October 22, 2020 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix locale determination in confirm/choice prompts/inputs #2534

Fix locale determination in confirm/choice prompts/inputs #2534

v-kydela commented Jul 14, 2020 •

edited

Loading

coveralls commented Jul 14, 2020 •

edited

Loading

stevengum left a comment

stevengum Jul 21, 2020

v-kydela Jul 21, 2020

stevengum Jul 21, 2020

v-kydela Jul 21, 2020 •

edited

Loading

carlosscastro commented Jul 30, 2020 •

edited

Loading

v-kydela commented Sep 11, 2020 •

edited

Loading

joshgummersall left a comment

joshgummersall commented Oct 22, 2020

Fix locale determination in confirm/choice prompts/inputs #2534

Fix locale determination in confirm/choice prompts/inputs #2534

Conversation

v-kydela commented Jul 14, 2020 • edited Loading

Description

Specific Changes

What I did not change

coveralls commented Jul 14, 2020 • edited Loading

Pull Request Test Coverage Report for Build 170401

💛 - Coveralls

stevengum left a comment

Choose a reason for hiding this comment

stevengum Jul 21, 2020

Choose a reason for hiding this comment

v-kydela Jul 21, 2020

Choose a reason for hiding this comment

stevengum Jul 21, 2020

Choose a reason for hiding this comment

v-kydela Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

carlosscastro commented Jul 30, 2020 • edited Loading

v-kydela commented Sep 11, 2020 • edited Loading

joshgummersall left a comment

Choose a reason for hiding this comment

joshgummersall commented Oct 22, 2020

v-kydela commented Jul 14, 2020 •

edited

Loading

coveralls commented Jul 14, 2020 •

edited

Loading

v-kydela Jul 21, 2020 •

edited

Loading

carlosscastro commented Jul 30, 2020 •

edited

Loading

v-kydela commented Sep 11, 2020 •

edited

Loading