Skip to content
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

[Alexa] Add Alexa Adapter #95

Closed
wants to merge 1 commit into from

Conversation

iMicknl
Copy link
Contributor

@iMicknl iMicknl commented Aug 13, 2019

work in progress
PR for review and CI purposes. Mostly based on @garypretty his work in the dotnet repo.

@szul szul added this to In progress in Adapters Sep 26, 2019
@iMicknl
Copy link
Contributor Author

iMicknl commented Nov 17, 2019

@szul, this has been stale for a while.. I will work on it in the upcoming weeks and publish a first version.

Possibly I will settle for a 'just-voice' version which and work on the other Alexa features later on, if there is any demand.

@garypretty
Copy link
Member

This works. I am planning a refactoring of many of the features beyond voice anyway. So you should certainly wait for them before implementing. Cheers!

@iMicknl
Copy link
Contributor Author

iMicknl commented Nov 17, 2019

@garypretty currently the .NET adapter is sending the last activity only, where I think the first activity would make more sense. Is this something that will change during the refactor, or will it at least be configurable?

@szul
Copy link
Contributor

szul commented Nov 17, 2019

@iMicknl This works for me. I approach a lot of these as MVP packages to get some basic functionality out there. Then expand as time and contributions permit.

@iMicknl
Copy link
Contributor Author

iMicknl commented Jan 2, 2020

@garypretty some questions;

  • Currently the .NET adapter is sending the last activity only, where I think the first activity would make more sense. Is this something that will change during the refactor, or will it at least be configurable?
  • Why do you have the option ValidateIncomingAlexaRequests? For testing purposes only?
  • Would you like the same naming? Really long classnames like AlexaIntentRequestToMessageActivityMiddleware are not really common in Javascript.
  • Why is AlexaRequestToMessageEventActivitiesMiddleware necessary? Shouldn't those transformations be done in the adapter core?

@iMicknl iMicknl marked this pull request as ready for review January 2, 2020 22:39
@iMicknl iMicknl requested a review from szul as a code owner January 2, 2020 22:39
@iMicknl iMicknl self-assigned this Jan 2, 2020
@garypretty
Copy link
Member

garypretty commented Jan 3, 2020

@iMicknl Good questions 👍

Currently the .NET adapter is sending the last activity only, where I think the first activity would make more sense. Is this something that will change during the refactor, or will it at least be configurable?
Take a look at this PR, which is introducing some changes to the current Alexa preview. https://github.com/BotBuilderCommunity/botbuilder-community-dotnet/pull/182/files. This actually introduces the idea of concatenating the text from multiple activities (we need this for compat with the virtual assistant). I think we will ultimately either take the approach is that PR or, something I am toying with, is having be a choice between Use First Activity / Concatenate Activities / Use Last Activity. What are your thoughts?

Why do you have the option ValidateIncomingAlexaRequests? For testing purposes only?
Yes, this is to allow you to test without the Alexa service. Even doing something as sending sample JSON via postman to your bot for offline development (I built the first draft of this adapter on an 8 hour flight with no wifi, hence this setting being introduced right at the beginning :) )

Would you like the same naming? Really long classnames like AlexaIntentRequestToMessageActivityMiddleware are not really common in Javascript.
Personally, I would like to keep the same names. I understand that keeping naming (or other parity) parity between the two languages can bring up issues like this, but I do think it helps those who are building bots in both languages have a familiarity between the two implementations - it also helps us with maintainability as it is easier to spot when something changes on one side and needs to be mirrored on the other. ***FYI - AlexaIntentRequestToMessageActivityMiddleware will be kept in .NET for backwards compat reasons. The new default will be AlexaRequestToMessageEventActivitiesMiddleware, so I would consider not including the first at all in the JS / TS version.

Why is AlexaRequestToMessageEventActivitiesMiddleware necessary? Shouldn't those transformations be done in the adapter core?
The principle here is to have the adapter do a transformation into an activity as closely closely based on the incoming channel payload as possible. i.e. An incoming IntentRequest -> Activity of type "IntentRequest" or incoming Alexa LaunchRequest -> Activity type of "Launch Request". This means that someone familiar with Alexa or whom is only building a bot for that platform can work as close to the Alexa types as possible. We then provide the middleware to abstract further into well-understood activity types for the BF. e.g. in the new PR I referenced above (https://github.com/BotBuilderCommunity/botbuilder-community-dotnet/pull/182/files) we are updating the middleware to convert Alexa LaunchRequest into a ConversationUpdate activity. Most people will use this middleware and it is likely to be the default approach, but having it within middleware means that people can go closer to Alexa, or even replace the middleware and decide for themselves how they want the transform to happen. If we put it in the core adapter, we remove a significant amount of flexibility I think.

Does the make sense? Let me know if anything needs clarifying. Happy to discuss further.

@iMicknl iMicknl changed the title [WIP] Add Alexa adapter [Alexa] Initial version Jan 16, 2020
@iMicknl iMicknl changed the title [Alexa] Initial version [Alexa] Add Alexa Adapter Feb 11, 2020
@iMicknl
Copy link
Contributor Author

iMicknl commented Feb 11, 2020

Closing in favor of #157

@iMicknl iMicknl closed this Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Adapters
  
In progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants