Merge pull request #1326 from microsoft/trboehre/skillconversationid

Refactored SkillDialog to call ConversationFacotry.CreateConversationId only once

Author: tracyboehrer

libraries/botbuilder-dialogs/botbuilder/dialogs/skills/skill_dialog.py

@@ -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,
@@ -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:
@@ -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)
 
@@ -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)
 
@@ -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
@@ -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)
 
@@ -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)

libraries/botbuilder-dialogs/tests/test_skill_dialog.py

@@ -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,
@@ -40,7 +41,6 @@
     BeginSkillDialogOptions,
     DialogTurnStatus,
 )
-from botframework.connector.token_api.models import TokenExchangeResource
 
 
 class SimpleConversationIdFactory(ConversationIdFactoryBase):
@@ -148,10 +148,13 @@ async def capture(
             conversation_state=conversation_state,
         )
 
+        assert len(dialog_options.conversation_id_factory.conversation_refs) == 0
+
         # 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
@@ -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