Skip to content

Refactored SkillDialog to call ConversationFacotry.CreateConversationId only once #1326

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

Merged
merged 1 commit into from
Aug 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from copy import deepcopy
from typing import List

from botframework.connector.token_api.models import TokenExchangeRequest
from botbuilder.schema import (
Activity,
ActivityTypes,
Expand All @@ -22,13 +23,16 @@
DialogReason,
DialogInstance,
)
from botframework.connector.token_api.models import TokenExchangeRequest

from .begin_skill_dialog_options import BeginSkillDialogOptions
from .skill_dialog_options import SkillDialogOptions


class SkillDialog(Dialog):
SKILLCONVERSATIONIDSTATEKEY = (
"Microsoft.Bot.Builder.Dialogs.SkillDialog.SkillConversationId"
)

def __init__(self, dialog_options: SkillDialogOptions, dialog_id: str):
super().__init__(dialog_id)
if not dialog_options:
Expand Down Expand Up @@ -65,8 +69,18 @@ async def begin_dialog(self, dialog_context: DialogContext, options: object = No
self._deliver_mode_state_key
] = dialog_args.activity.delivery_mode

# Create the conversationId and store it in the dialog context state so we can use it later
skill_conversation_id = await self._create_skill_conversation_id(
dialog_context.context, dialog_context.context.activity
)
dialog_context.active_dialog.state[
SkillDialog.SKILLCONVERSATIONIDSTATEKEY
] = skill_conversation_id

# Send the activity to the skill.
eoc_activity = await self._send_to_skill(dialog_context.context, skill_activity)
eoc_activity = await self._send_to_skill(
dialog_context.context, skill_activity, skill_conversation_id
)
if eoc_activity:
return await dialog_context.end_dialog(eoc_activity.value)

Expand Down Expand Up @@ -101,7 +115,12 @@ async def continue_dialog(self, dialog_context: DialogContext):
]

# Just forward to the remote skill
eoc_activity = await self._send_to_skill(dialog_context.context, skill_activity)
skill_conversation_id = dialog_context.active_dialog.state[
SkillDialog.SKILLCONVERSATIONIDSTATEKEY
]
eoc_activity = await self._send_to_skill(
dialog_context.context, skill_activity, skill_conversation_id
)
if eoc_activity:
return await dialog_context.end_dialog(eoc_activity.value)

Expand All @@ -123,7 +142,8 @@ async def reprompt_dialog( # pylint: disable=unused-argument
)

# connection Name is not applicable for a RePrompt, as we don't expect as OAuthCard in response.
await self._send_to_skill(context, reprompt_event)
skill_conversation_id = instance.state[SkillDialog.SKILLCONVERSATIONIDSTATEKEY]
await self._send_to_skill(context, reprompt_event, skill_conversation_id)

async def resume_dialog( # pylint: disable=unused-argument
self, dialog_context: "DialogContext", reason: DialogReason, result: object
Expand Down Expand Up @@ -152,7 +172,10 @@ async def end_dialog(
activity.additional_properties = context.activity.additional_properties

# connection Name is not applicable for an EndDialog, as we don't expect as OAuthCard in response.
await self._send_to_skill(context, activity)
skill_conversation_id = instance.state[
SkillDialog.SKILLCONVERSATIONIDSTATEKEY
]
await self._send_to_skill(context, activity, skill_conversation_id)

await super().end_dialog(context, instance, reason)

Expand Down Expand Up @@ -187,18 +210,14 @@ def _on_validate_activity(
return True

async def _send_to_skill(
self, context: TurnContext, activity: Activity
self, context: TurnContext, activity: Activity, skill_conversation_id: str
) -> Activity:
if activity.type == ActivityTypes.invoke:
# Force ExpectReplies for invoke activities so we can get the replies right away and send
# them back to the channel if needed. This makes sure that the dialog will receive the Invoke
# response from the skill and any other activities sent, including EoC.
activity.delivery_mode = DeliveryModes.expect_replies

skill_conversation_id = await self._create_skill_conversation_id(
context, activity
)

# Always save state before forwarding
# (the dialog stack won't get updated with the skillDialog and things won't work if you don't)
await self.dialog_options.conversation_state.save_changes(context, True)
Expand Down
6 changes: 5 additions & 1 deletion libraries/botbuilder-dialogs/tests/test_skill_dialog.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from unittest.mock import Mock

import aiounittest
from botframework.connector.token_api.models import TokenExchangeResource
from botbuilder.core import (
ConversationState,
MemoryStorage,
Expand Down Expand Up @@ -40,7 +41,6 @@
BeginSkillDialogOptions,
DialogTurnStatus,
)
from botframework.connector.token_api.models import TokenExchangeResource


class SimpleConversationIdFactory(ConversationIdFactoryBase):
Expand Down Expand Up @@ -148,10 +148,13 @@ async def capture(
conversation_state=conversation_state,
)

assert len(dialog_options.conversation_id_factory.conversation_refs) == 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is nitpicking but this could be changed to
assert not dialog_options.conversation_id_factory.conversation_refs


# Send something to the dialog to start it
await client.send_activity(MessageFactory.text("irrelevant"))

# Assert results and data sent to the SkillClient for fist turn
assert len(dialog_options.conversation_id_factory.conversation_refs) == 1
assert dialog_options.bot_id == from_bot_id_sent
assert dialog_options.skill.app_id == to_bot_id_sent
assert dialog_options.skill.skill_endpoint == to_url_sent
Expand All @@ -162,6 +165,7 @@ async def capture(
await client.send_activity(MessageFactory.text("Second message"))

# Assert results for second turn
assert len(dialog_options.conversation_id_factory.conversation_refs) == 1
assert activity_sent.text == "Second message"
assert DialogTurnStatus.Waiting == client.dialog_turn_result.status

Expand Down