Locale Fixes #3739
Locale Fixes #3739
Conversation
| await ContinueConversationAsync(claimsIdentity, reference, audience, null, callback, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| public async Task ContinueConversationAsync(ClaimsIdentity claimsIdentity, ConversationReference reference, string audience, string locale, BotCallbackHandler callback, CancellationToken cancellationToken) | ||
| { |
There was a problem hiding this comment.
I don't think this is right way to do this. I think ConversationReference should capture the locale.
There was a problem hiding this comment.
Reviewed with the development team. Needs review by @dandriscoll.
There was a problem hiding this comment.
I don't know, within this part of the stack, what the protocol implications of putting the locale here are. If we set locale within the conversationRefererence, does it end up on the wire somewhere?
(Setting locale within conversationReference for storage reasons is the right approach, assuming no externalities introduced by setting it here.)
There was a problem hiding this comment.
Oh, interesting point. Yes ConversationReference can be serialized into an activity, Activity in X can have a conversation reference to another conversation Y. ConversationReference is the information you need to be able to switch to Y if you need to. The pattern on this is that we need the original locale of the conversation Y to continue that conversation, so it seems like it should be in there, even if it gets serialized to wire.
@dandriscoll does that seem right to you?
In reply to: 409127783 [](ancestors = 409127783)
There was a problem hiding this comment.
OK, I talked with Dan and we agreed it is appropriate to modify ConversationReference.
Changes would be:
- Modify ConversationReference in schema assembly to have locale
- Modify CreateConversationReference to capture the locale.
Then all people get the proper behavior without having to change their code.
Rest of the APIS then are unchanged, except to use conversationReference.locale in appropriate place.
|
Blocked waiting for agreement. Email thread ongoing. |
tomlm
added
Draft PR
and removed
blocked
This reverts commit b4e30cb.
|
@tomlm @Stevenic @dandriscoll Ready for review. There were several places that call |
Fixes: #3645
Once ported, this will also address this issue
When porting, I will also use the ported issues to address this issue, since these PRs touch a couple of the same files.
Changes
Testing
I added to one existing test to check for locale with ContinueConversation. There wasn't really a good way to test for locale in the EndOfConversation events/activities.
Ports
I've got most of the work done for JS and Python as well, but am holding off on submitting PRs for those until this one is approved.