-
Notifications
You must be signed in to change notification settings - Fork 283
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
fix: first activity from bot to user has fake replyToId value #3779
Conversation
function getAppropriateReplyToId(source: Partial<Activity>): string | undefined { | ||
if ( | ||
source.type !== ActivityTypes.ConversationUpdate || | ||
(source.channelId !== Channels.Directline && source.channelId !== Channels.Webchat) | ||
) { | ||
return source.id; | ||
} | ||
|
||
return undefined; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I debated about what to do with this function, which is also located in ActivityEx
. Since TurnContext
and ActivityEx
are in two separate libraries, I copy/pasted the function to keep exposed surface area lower. I wanted a function just to avoid the harder-to-read ternary (used in the .NET PR). So, this isn't DRY, but is DRY-er than copy/pasting the ternary around, I think.
I think this could be removed from here, entirely, though. In .NET, TurnContext doesn't have a getConversationReference
and instead relies on the one in ActivityEx
. I think it might be worth removing here as "Find all References" only returns it being used in 6 different places. However, this is core enough to the SDK and a little beyond the scope of this PR, so I was more hesitant to mess with it.
runtypes@6.3.0, runtypes@~6.3.0: | ||
runtypes@~6.3.0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This is due to accidentally forgetting to commit in previously-merged PR.
Pull Request Test Coverage Report for Build 951343060
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair points regarding the duplicated function. For now, I'd say it's fine.
Fixes #3732
Description
When the activity is the first one of the conversation and it's sent from bot to user, the ReplyToId was assigned to the bogus ConversationUpdate activity's id. The fix is to not assign the bogus ReplyToId if previous activity is the ConversationUpdate activity
Specific Changes
Testing
Tests are added