Botframework-solutions: "Role" property of conversation update activity set to null

[manish-95]

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

1. Initialize connection to Bot through WebChat using DirectLine.
2. Open a connection to the Bot.
3. Note in the bot back-end logic that turnContext.Activity.From.Role is set to null, and not "user".
4. Note the error.

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]

[stevkan]

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?

[compulim]

@stevkan this schema may give you some hints.

In short, activity.from.role is something in the spec, but Direct Line service didn't send it to us. So Web Chat to have figure it ourselves.

I think @manish-95 is talking about SDK was not having the activity.from.role. So this one should either fixed in Direct Line service, or patched in SDK.

@stevkan could you follow up with bot team to see who should pick up this one?

[manish-95]

@stevkan this schema may give you some hints.

In short, activity.from.role is something in the spec, but Direct Line service didn't send it to us. So Web Chat to have figure it ourselves.

I think @manish-95 is talking about SDK was not having the activity.from.role. So this one should either fixed in Direct Line service, or patched in SDK.

@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

[carlosscastro]

@EricDahlvang could you please look at this from an SDK perspective? We need some sdk guidance here. Thanks!

[EricDahlvang]

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

https://github.com/microsoft/botframework-solutions/blob/master/samples/csharp/assistants/enterprise-assistant/VirtualAssistantSample/Bots/DefaultActivityHandler.cs#L155

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";
                }
            }
        }

https://github.com/microsoft/botframework-solutions/blob/master/sdk/csharp/libraries/microsoft.bot.solutions/Proactive/ProactiveStateMiddleware.cs#L31

if (!string.IsNullOrEmpty(activity.From.Role) && activity.From.Role.Equals("user", StringComparison.InvariantCultureIgnoreCase))

Lastly, the spec states here:

Channel account role
The role field indicates whether entity behind the account is a user or bot. This field is intended for use in the Transcript format [16] to distinguish between activities sent by users and activities sent by bots. The value of the role field is a string.

A7511: Senders SHOULD NOT include this field. Receivers SHOULD ignore this field.

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.

[EricDahlvang]

Transferring to https://github.com/microsoft/botframework-solutions/

[peterinnesmsft]

@ryanlengel @lzc850612 could you take a look and do an initial pass of this issue?

[lzc850612]

@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.

[EricDahlvang]

@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.

[peterinnesmsft]

@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!

[manish-95]

@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.

[peterinnesmsft]

@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!

[manish-95]

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.

[github-actions[bot]]

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

[peterinnesmsft]

@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.

[mrivera-ms]

@peterinnesmsft can this be closed now?

[EricDahlvang]

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

[Batta32]

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.

[EricDahlvang]

@Batta32 have proactive skills been tested with 4.11? the code paths which utilize DefaultActivityHandler.cs and ProactiveStateMiddleware.cs ?

[Batta32]

Sorry @EricDahlvang, we meant that we successfully validated this using C# Bot-Solutions version 1.0.1-daily322 (aitemplates feed) which contains the PR #3633 and uses SDK R9.

Should we validate this using R11?

[hcyang]

@EricDahlvang, can you respond to @Batta32's comment? Thanks.

[EricDahlvang]

Sorry, I'm not sure. Looks like maybe this will not be an issue, since #3633 removed the check for role?

[Batta32]

@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 😊.

[Batta32]

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 role property set to null using Web Chat and DirectLine.

We think that this issue can be closed as the changes will be present in the new stable packages of Solutions.

[EricDahlvang]

Thanks @Batta32