-
Notifications
You must be signed in to change notification settings - Fork 528
"Role" property of conversation update activity set to null #3614
Comments
Hi @manish-95, can you post code for how you are generating a token in Web Chat as well as any relevant details (for example, is Enhanced Authentication enabled in the Direct Line channel)? And, what API do you call to request the token? If you call an API of your own, can you also include that code? |
@stevkan this schema may give you some hints. In short, I think @manish-95 is talking about SDK was not having the @stevkan could you follow up with bot team to see who should pick up this one? |
@stevkan @compulim I can add a bit more information here which might help. The ProactiveState middleware in the latest version of the BotFramework Solutions, looks for activity.from.role, which is null, hence it fails to update the Proactive State. In previous versions, it was looking in the activity.from.properties, which had a key value pair for role |
@EricDahlvang could you please look at this from an SDK perspective? We need some sdk guidance here. Thanks! |
As William mentioned, 'role' is not sent by Direct Line. Looking through the botframework-solutions repository, it appears this 'role' field has been adopted for a different purpose than originally intended. And, there is currently an inconsistency between DefaultActivityHandler and ProactiveStateMiddleware private void EnsureActivity(Activity activity)
{
if (activity != null)
{
if (activity.From != null)
{
activity.From.Name = "User";
activity.From.Properties["role"] = "user";
}
if (activity.Recipient != null)
{
activity.Recipient.Id = "1";
activity.Recipient.Name = "Bot";
activity.Recipient.Properties["role"] = "bot";
}
}
} if (!string.IsNullOrEmpty(activity.From.Role) && activity.From.Role.Equals("user", StringComparison.InvariantCultureIgnoreCase)) Lastly, the spec states here:
Based on this, it seems the VA bot should adopt a different method for proactive messaging state or maybe set activity.from.role in EnsureActivity, or use activity.From.Properties["role"] in the proactive middleware. |
Transferring to https://github.com/microsoft/botframework-solutions/ |
@ryanlengel @lzc850612 could you take a look and do an initial pass of this issue? |
@EricDahlvang the reason we were using activity.from.role is because in the middleware we need to only store the conversation reference of the activity that's coming from user to bot. if we don't use activity.from.role, what other way we could use to achieve this validation? Thanks. |
@lzc850612 I'm not sure. Is ProactiveStateMiddleware.OnTurn ever executed with an activity which is NOT from a user? If so, maybe a custom property? Either way, ProactiveStateMiddleware should be consistent with whatever is done in DefaultActivityHandler ... this inconsistency is the cause of the issue right now. |
@manish-95, @lzc850612 has checked in a PR related to this which we believe should help address this issue. We will work on actively pushing new releases in the next week or two. In the meantime, can you please take a look and work with him to see if this helps resolve your issue? Thanks! |
@peterinnesmsft @lzc850612 Sure, I'll definitely do that. For now , I have added a dirty fix for the same, by adding the Role property for each activity, but I can quickly copy the contents from this PR locally and verify. |
@manish-95 any luck on verifying this? We are tentatively looking to do some initial validation of a release candidate containing this fix next week, but any heads up if you've caught obvious issues would be greatly appreciated if you have them! |
Hi @peterinnesmsft , my apologies for the extremely late update on this, however, I have taken a copy of the file with the new changes, and it seems to work fine, for all the cases that I am using. |
This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days |
@manish-95 thank you for confirming! @Batta32 as I expect this will be resolved by our updates to the SDK package references, I'll assign this to you for further tracking. Once we have pushed out our own updates with the updated SDK references and validated, let's go ahead and close this out. |
@peterinnesmsft can this be closed now? |
Has this been tested against the latest sdk? The sdk is now utilizing the activity.recipient.role property. When a ConversationReference for a continue call, is created, this role will go with it. I'm guessing there is will now be conflicts with how DefaultActivityHandler.cs and ProactiveStateMiddleware.cs are using .Role |
Hi @mrivera-ms and @EricDahlvang , we already validated that this is correctly fixed with SDK R11 and we created the PR #3707 in order to use the latest SDK version. As soon as the PR #3707 is merged and new packages are published with this new version, this will be automatically solved. |
@Batta32 have proactive skills been tested with 4.11? the code paths which utilize DefaultActivityHandler.cs and ProactiveStateMiddleware.cs ? |
Sorry @EricDahlvang, we meant that we successfully validated this using C# Bot-Solutions version 1.0.1-daily322 ( Should we validate this using R11? |
@EricDahlvang, can you respond to @Batta32's comment? Thanks. |
Sorry, I'm not sure. Looks like maybe this will not be an issue, since #3633 removed the check for role? |
@EricDahlvang - PR #3633 removed the check for role. Just in case, we will do a validation of R11 with the changes implemented in PR #3633. As soon as we have any update, we will let you know the result 😊. |
Hi @EricDahlvang, sorry for the delay. We validated the fix of PR #3633 which removes the check for role using R11 and we confirmed that it's working having the We think that this issue can be closed as the changes will be present in the new stable packages of Solutions. |
Thanks @Batta32 |
Screenshots
Version
WebChat 4.9.1 via CDN
Describe the bug
When the conversationUpdate activity is sent by WebChat, the Role property of the activity is set to null. As a result, the ProactiveState does not get updated with new Conversation details since it looks for the role being equal to "user". This gets updated only when a skill is invoked/message is sent. As a result, if there are any proactive notifications, they are sent to the wrong conversation.
Steps to reproduce
Expected behavior
WebChat should pass the relevant role as "user", so that the Proactive State functionalities are not broken.
Additional context
WebChat implemented in Angular application.
[Bug]
The text was updated successfully, but these errors were encountered: