From 0b93518b37ac649d125fe0073dcd860777a9bc17 Mon Sep 17 00:00:00 2001 From: tracyboehrer Date: Wed, 6 May 2020 14:20:02 -0500 Subject: [PATCH] SkillDialog activity type handling updates --- .../botbuilder/core/bot_framework_adapter.py | 20 ++-- .../botbuilder/core/invoke_response.py | 8 ++ .../skills/begin_skill_dialog_options.py | 12 +-- .../botbuilder/dialogs/skills/skill_dialog.py | 85 ++++++++++------ .../dialogs/skills/skill_dialog_options.py | 2 + .../tests/test_skill_dialog.py | 98 ++++++++++++++++--- 6 files changed, 163 insertions(+), 62 deletions(-) diff --git a/libraries/botbuilder-core/botbuilder/core/bot_framework_adapter.py b/libraries/botbuilder-core/botbuilder/core/bot_framework_adapter.py index 24c84a89c..731e37b70 100644 --- a/libraries/botbuilder-core/botbuilder/core/bot_framework_adapter.py +++ b/libraries/botbuilder-core/botbuilder/core/bot_framework_adapter.py @@ -481,14 +481,8 @@ async def process_activity_with_identity( await self.run_pipeline(context, logic) - if activity.type == ActivityTypes.invoke: - invoke_response = context.turn_state.get( - BotFrameworkAdapter._INVOKE_RESPONSE_KEY # pylint: disable=protected-access - ) - if invoke_response is None: - return InvokeResponse(status=int(HTTPStatus.NOT_IMPLEMENTED)) - return invoke_response.value - + # Handle ExpectedReplies scenarios where the all the activities have been buffered and sent back at once + # in an invoke response. # Return the buffered activities in the response. In this case, the invoker # should deserialize accordingly: # activities = ExpectedReplies().deserialize(response.body).activities @@ -498,6 +492,16 @@ async def process_activity_with_identity( ).serialize() return InvokeResponse(status=int(HTTPStatus.OK), body=expected_replies) + # Handle Invoke scenarios, which deviate from the request/request model in that + # the Bot will return a specific body and return code. + if activity.type == ActivityTypes.invoke: + invoke_response = context.turn_state.get( + BotFrameworkAdapter._INVOKE_RESPONSE_KEY # pylint: disable=protected-access + ) + if invoke_response is None: + return InvokeResponse(status=int(HTTPStatus.NOT_IMPLEMENTED)) + return invoke_response.value + return None async def __generate_callerid(self, claims_identity: ClaimsIdentity) -> str: diff --git a/libraries/botbuilder-core/botbuilder/core/invoke_response.py b/libraries/botbuilder-core/botbuilder/core/invoke_response.py index 7d258559e..fa0b74577 100644 --- a/libraries/botbuilder-core/botbuilder/core/invoke_response.py +++ b/libraries/botbuilder-core/botbuilder/core/invoke_response.py @@ -24,3 +24,11 @@ def __init__(self, status: int = None, body: object = None): """ self.status = status self.body = body + + def is_successful_status_code(self) -> bool: + """ + Gets a value indicating whether the invoke response was successful. + :return: A value that indicates if the HTTP response was successful. true if status is in + the Successful range (200-299); otherwise false. + """ + return 200 <= self.status <= 299 diff --git a/libraries/botbuilder-dialogs/botbuilder/dialogs/skills/begin_skill_dialog_options.py b/libraries/botbuilder-dialogs/botbuilder/dialogs/skills/begin_skill_dialog_options.py index da2d39914..a9d21ca3f 100644 --- a/libraries/botbuilder-dialogs/botbuilder/dialogs/skills/begin_skill_dialog_options.py +++ b/libraries/botbuilder-dialogs/botbuilder/dialogs/skills/begin_skill_dialog_options.py @@ -5,19 +5,13 @@ class BeginSkillDialogOptions: - def __init__( - self, activity: Activity, connection_name: str = None - ): # pylint: disable=unused-argument + def __init__(self, activity: Activity): self.activity = activity - self.connection_name = connection_name @staticmethod def from_object(obj: object) -> "BeginSkillDialogOptions": if isinstance(obj, dict) and "activity" in obj: - return BeginSkillDialogOptions(obj["activity"], obj.get("connection_name")) + return BeginSkillDialogOptions(obj["activity"]) if hasattr(obj, "activity"): - return BeginSkillDialogOptions( - obj.activity, - obj.connection_name if hasattr(obj, "connection_name") else None, - ) + return BeginSkillDialogOptions(obj.activity) return None diff --git a/libraries/botbuilder-dialogs/botbuilder/dialogs/skills/skill_dialog.py b/libraries/botbuilder-dialogs/botbuilder/dialogs/skills/skill_dialog.py index f0ed30d38..b26fa6341 100644 --- a/libraries/botbuilder-dialogs/botbuilder/dialogs/skills/skill_dialog.py +++ b/libraries/botbuilder-dialogs/botbuilder/dialogs/skills/skill_dialog.py @@ -36,7 +36,6 @@ def __init__(self, dialog_options: SkillDialogOptions, dialog_id: str): self.dialog_options = dialog_options self._deliver_mode_state_key = "deliverymode" - self._sso_connection_name_key = "SkillDialog.SSOConnectionName" async def begin_dialog(self, dialog_context: DialogContext, options: object = None): """ @@ -44,7 +43,7 @@ async def begin_dialog(self, dialog_context: DialogContext, options: object = No :param dialog_context: The dialog context for the current turn of conversation. :param options: (Optional) additional argument(s) to pass to the dialog being started. """ - dialog_args = SkillDialog._validate_begin_dialog_args(options) + dialog_args = self._validate_begin_dialog_args(options) await dialog_context.context.send_trace_activity( f"{SkillDialog.__name__}.BeginDialogAsync()", @@ -61,24 +60,22 @@ async def begin_dialog(self, dialog_context: DialogContext, options: object = No is_incoming=True, ) + # Store delivery mode in dialog state for later use. dialog_context.active_dialog.state[ self._deliver_mode_state_key ] = dialog_args.activity.delivery_mode - dialog_context.active_dialog.state[ - self._sso_connection_name_key - ] = dialog_args.connection_name - # Send the activity to the skill. - eoc_activity = await self._send_to_skill( - dialog_context.context, skill_activity, dialog_args.connection_name - ) + eoc_activity = await self._send_to_skill(dialog_context.context, skill_activity) if eoc_activity: return await dialog_context.end_dialog(eoc_activity.value) return self.end_of_turn async def continue_dialog(self, dialog_context: DialogContext): + if not self._on_validate_activity(dialog_context.context.activity): + return self.end_of_turn + await dialog_context.context.send_trace_activity( f"{SkillDialog.__name__}.continue_dialog()", label=f"ActivityType: {dialog_context.context.activity.type}", @@ -98,17 +95,13 @@ async def continue_dialog(self, dialog_context: DialogContext): # Create deep clone of the original activity to avoid altering it before forwarding it. skill_activity = deepcopy(dialog_context.context.activity) + skill_activity.delivery_mode = dialog_context.active_dialog.state[ self._deliver_mode_state_key ] - connection_name = dialog_context.active_dialog.state[ - self._sso_connection_name_key - ] # Just forward to the remote skill - eoc_activity = await self._send_to_skill( - dialog_context.context, skill_activity, connection_name - ) + eoc_activity = await self._send_to_skill(dialog_context.context, skill_activity) if eoc_activity: return await dialog_context.end_dialog(eoc_activity.value) @@ -163,8 +156,7 @@ async def end_dialog( await super().end_dialog(context, instance, reason) - @staticmethod - def _validate_begin_dialog_args(options: object) -> BeginSkillDialogOptions: + def _validate_begin_dialog_args(self, options: object) -> BeginSkillDialogOptions: if not options: raise TypeError("options cannot be None.") @@ -182,26 +174,36 @@ def _validate_begin_dialog_args(options: object) -> BeginSkillDialogOptions: return dialog_args + def _on_validate_activity( + self, activity: Activity # pylint: disable=unused-argument + ) -> bool: + """ + Validates the activity sent during continue_dialog. + + Override this method to implement a custom validator for the activity being sent during continue_dialog. + This method can be used to ignore activities of a certain type if needed. + If this method returns false, the dialog will end the turn without processing the activity. + """ + return True + async def _send_to_skill( - self, context: TurnContext, activity: Activity, connection_name: str = None + self, context: TurnContext, activity: Activity ) -> Activity: - # Create a conversationId to interact with the skill and send the activity - conversation_id_factory_options = SkillConversationIdFactoryOptions( - from_bot_oauth_scope=context.turn_state.get(BotAdapter.BOT_OAUTH_SCOPE_KEY), - from_bot_id=self.dialog_options.bot_id, - activity=activity, - bot_framework_skill=self.dialog_options.skill, - ) - - skill_conversation_id = await self.dialog_options.conversation_id_factory.create_skill_conversation_id( - conversation_id_factory_options + 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) - skill_info = self.dialog_options.skill await self.dialog_options.conversation_state.save_changes(context, True) + skill_info = self.dialog_options.skill response = await self.dialog_options.skill_client.post_activity( self.dialog_options.bot_id, skill_info.app_id, @@ -229,7 +231,7 @@ async def _send_to_skill( # Capture the EndOfConversation activity if it was sent from skill eoc_activity = from_skill_activity elif await self._intercept_oauth_cards( - context, from_skill_activity, connection_name + context, from_skill_activity, self.dialog_options.connection_name ): # do nothing. Token exchange succeeded, so no oauthcard needs to be shown to the user pass @@ -239,6 +241,21 @@ async def _send_to_skill( return eoc_activity + async def _create_skill_conversation_id( + self, context: TurnContext, activity: Activity + ) -> str: + # Create a conversationId to interact with the skill and send the activity + conversation_id_factory_options = SkillConversationIdFactoryOptions( + from_bot_oauth_scope=context.turn_state.get(BotAdapter.BOT_OAUTH_SCOPE_KEY), + from_bot_id=self.dialog_options.bot_id, + activity=activity, + bot_framework_skill=self.dialog_options.skill, + ) + skill_conversation_id = await self.dialog_options.conversation_id_factory.create_skill_conversation_id( + conversation_id_factory_options + ) + return skill_conversation_id + async def _intercept_oauth_cards( self, context: TurnContext, activity: Activity, connection_name: str ): @@ -248,6 +265,8 @@ async def _intercept_oauth_cards( if not connection_name or not isinstance( context.adapter, ExtendedUserTokenProvider ): + # The adapter may choose not to support token exchange, in which case we fallback to + # showing an oauth card to the user. return False oauth_card_attachment = next( @@ -273,6 +292,8 @@ async def _intercept_oauth_cards( ) if result and result.token: + # If token above is null, then SSO has failed and hence we return false. + # If not, send an invoke to the skill with the token. return await self._send_token_exchange_invoke_to_skill( activity, oauth_card.token_exchange_resource.id, @@ -280,6 +301,8 @@ async def _intercept_oauth_cards( result.token, ) except: + # Failures in token exchange are not fatal. They simply mean that the user needs + # to be shown the OAuth card. return False return False @@ -310,4 +333,4 @@ async def _send_token_exchange_invoke_to_skill( ) # Check response status: true if success, false if failure - return response.status / 100 == 2 + return response.is_successful_status_code() diff --git a/libraries/botbuilder-dialogs/botbuilder/dialogs/skills/skill_dialog_options.py b/libraries/botbuilder-dialogs/botbuilder/dialogs/skills/skill_dialog_options.py index 53d56f72e..028490a40 100644 --- a/libraries/botbuilder-dialogs/botbuilder/dialogs/skills/skill_dialog_options.py +++ b/libraries/botbuilder-dialogs/botbuilder/dialogs/skills/skill_dialog_options.py @@ -18,6 +18,7 @@ def __init__( skill: BotFrameworkSkill = None, conversation_id_factory: ConversationIdFactoryBase = None, conversation_state: ConversationState = None, + connection_name: str = None, ): self.bot_id = bot_id self.skill_client = skill_client @@ -25,3 +26,4 @@ def __init__( self.skill = skill self.conversation_id_factory = conversation_id_factory self.conversation_state = conversation_state + self.connection_name = connection_name diff --git a/libraries/botbuilder-dialogs/tests/test_skill_dialog.py b/libraries/botbuilder-dialogs/tests/test_skill_dialog.py index 53b0a1d31..4b246189f 100644 --- a/libraries/botbuilder-dialogs/tests/test_skill_dialog.py +++ b/libraries/botbuilder-dialogs/tests/test_skill_dialog.py @@ -104,7 +104,13 @@ async def test_begin_dialog_options_validation(self): with self.assertRaises(TypeError): await client.send_activity("irrelevant") - async def test_begin_dialog_calls_skill(self): + async def test_begin_dialog_calls_skill_no_deliverymode(self): + return await self.begin_dialog_calls_skill(None) + + async def test_begin_dialog_calls_skill_expect_replies(self): + return await self.begin_dialog_calls_skill(DeliveryModes.expect_replies) + + async def begin_dialog_calls_skill(self, deliver_mode: str): activity_sent = None from_bot_id_sent = None to_bot_id_sent = None @@ -133,6 +139,67 @@ async def capture( sut = SkillDialog(dialog_options, "dialog_id") activity_to_send = MessageFactory.text(str(uuid.uuid4())) + activity_to_send.delivery_mode = deliver_mode + + client = DialogTestClient( + "test", + sut, + BeginSkillDialogOptions(activity=activity_to_send), + conversation_state=conversation_state, + ) + + # 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 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 + assert activity_to_send.text == activity_sent.text + assert DialogTurnStatus.Waiting == client.dialog_turn_result.status + + # Send a second message to continue the dialog + await client.send_activity(MessageFactory.text("Second message")) + + # Assert results for second turn + assert activity_sent.text == "Second message" + assert DialogTurnStatus.Waiting == client.dialog_turn_result.status + + # Send EndOfConversation to the dialog + await client.send_activity(Activity(type=ActivityTypes.end_of_conversation)) + + # Assert we are done. + assert DialogTurnStatus.Complete == client.dialog_turn_result.status + + async def test_should_handle_invoke_activities(self): + activity_sent = None + from_bot_id_sent = None + to_bot_id_sent = None + to_url_sent = None + + async def capture( + from_bot_id: str, + to_bot_id: str, + to_url: str, + service_url: str, # pylint: disable=unused-argument + conversation_id: str, # pylint: disable=unused-argument + activity: Activity, + ): + nonlocal from_bot_id_sent, to_bot_id_sent, to_url_sent, activity_sent + from_bot_id_sent = from_bot_id + to_bot_id_sent = to_bot_id + to_url_sent = to_url + activity_sent = activity + + mock_skill_client = self._create_mock_skill_client(capture) + + conversation_state = ConversationState(MemoryStorage()) + dialog_options = SkillDialogTests.create_skill_dialog_options( + conversation_state, mock_skill_client + ) + + sut = SkillDialog(dialog_options, "dialog_id") + activity_to_send = Activity(type=ActivityTypes.invoke, name=str(uuid.uuid4()),) client = DialogTestClient( "test", @@ -141,21 +208,27 @@ async def capture( conversation_state=conversation_state, ) + # 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 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 assert activity_to_send.text == activity_sent.text assert DialogTurnStatus.Waiting == client.dialog_turn_result.status + # Send a second message to continue the dialog await client.send_activity(MessageFactory.text("Second message")) + # Assert results for second turn assert activity_sent.text == "Second message" assert DialogTurnStatus.Waiting == client.dialog_turn_result.status + # Send EndOfConversation to the dialog await client.send_activity(Activity(type=ActivityTypes.end_of_conversation)) + # Assert we are done. assert DialogTurnStatus.Complete == client.dialog_turn_result.status async def test_cancel_dialog_sends_eoc(self): @@ -244,7 +317,7 @@ async def post_return(): conversation_state = ConversationState(MemoryStorage()) dialog_options = SkillDialogTests.create_skill_dialog_options( - conversation_state, mock_skill_client + conversation_state, mock_skill_client, connection_name ) sut = SkillDialog(dialog_options, dialog_id="dialog") activity_to_send = SkillDialogTests.create_send_activity() @@ -252,9 +325,7 @@ async def post_return(): client = DialogTestClient( "test", sut, - BeginSkillDialogOptions( - activity=activity_to_send, connection_name=connection_name, - ), + BeginSkillDialogOptions(activity=activity_to_send,), conversation_state=conversation_state, ) @@ -371,13 +442,11 @@ async def post_return(): conversation_state = ConversationState(MemoryStorage()) dialog_options = SkillDialogTests.create_skill_dialog_options( - conversation_state, mock_skill_client + conversation_state, mock_skill_client, connection_name ) sut = SkillDialog(dialog_options, dialog_id="dialog") activity_to_send = SkillDialogTests.create_send_activity() - initial_dialog_options = BeginSkillDialogOptions( - activity=activity_to_send, connection_name=connection_name, - ) + initial_dialog_options = BeginSkillDialogOptions(activity=activity_to_send,) client = DialogTestClient( "test", sut, initial_dialog_options, conversation_state=conversation_state, @@ -413,7 +482,7 @@ async def post_return(): conversation_state = ConversationState(MemoryStorage()) dialog_options = SkillDialogTests.create_skill_dialog_options( - conversation_state, mock_skill_client + conversation_state, mock_skill_client, connection_name ) sut = SkillDialog(dialog_options, dialog_id="dialog") activity_to_send = SkillDialogTests.create_send_activity() @@ -421,9 +490,7 @@ async def post_return(): client = DialogTestClient( "test", sut, - BeginSkillDialogOptions( - activity=activity_to_send, connection_name=connection_name, - ), + BeginSkillDialogOptions(activity=activity_to_send,), conversation_state=conversation_state, ) @@ -437,7 +504,9 @@ async def post_return(): @staticmethod def create_skill_dialog_options( - conversation_state: ConversationState, skill_client: BotFrameworkClient + conversation_state: ConversationState, + skill_client: BotFrameworkClient, + connection_name: str = None, ): return SkillDialogOptions( bot_id=str(uuid.uuid4()), @@ -449,6 +518,7 @@ def create_skill_dialog_options( app_id=str(uuid.uuid4()), skill_endpoint="http://testskill.contoso.com/api/messages", ), + connection_name=connection_name, ) @staticmethod