-
Notifications
You must be signed in to change notification settings - Fork 212
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
Implement Alexa CanFulfillIntentRequest #378
base: master
Are you sure you want to change the base?
Conversation
add support for canFulfillIntent in request, response and register handler
take head change
Verify that your skill handles a CanFulfillIntentRequest call with no userId present
add documentation
Hi @orlylev, thanks for your contribution! According to Alexa's documents it seems like the |
Hi
I think it should be separate from the regular intent handler because is is
relay not the business logic of implementing the intent.
On the other hand looks like it is a light logic for the intent and list of
slots values that can be common in many cases so I don't fill it should be
a separate handler per intent.
I intent to add basic common logic based on slots list that may work in
most cases.
Orly
…On Wed, Oct 24, 2018 at 9:51 AM Kobi Meirson ***@***.***> wrote:
Hi @orlylev <https://github.com/orlylev>, thanks for your contribution!
According to Alexa's documents
<https://developer.amazon.com/docs/custom-skills/understand-name-free-interaction-for-custom-skills.html>
it seems like the CanFulfillIntentRequest is called along with an intent
name and slot, hence a different function should be defined per each intent.
Your implementation defines a single "global function" which receives all
the CanFulfillIntentRequests from Alexa. Any reason you chose this
implementation? (or am I missing something?)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#378 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AQWC2j_SlISNfSNHTfEE5eyV5BO6vI1Uks5uoA39gaJpZM4XzNRa>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thanks so much for this!
I'm honestly having a hard time making sense of this, even sitting down with both the Amazon documentation and the full patch. I left a few comments along the lines of places where I think some tweaks to documentation might make things more clearer.
It also seems to me like there's a larger philosophical discussion to be had about where canFulfillIntent code should conceptually live.
Based on the included test files and @kobim's comment earlier, it seems to me like there's a single global function that handles these requests. Is that right? I think I agree with @kobim that having this defined per-intent is more in line with other design decisions this library has made.
I agree this is separate from the business logic for handling an intent, but I wonder if it should be, say, an additional parameter passed into app.intent() when defining a new intent that contains a validation function.
(I'd imagine that might eventually allow a configuration object rather than a function — since we have slot information, we could totally do validation at a framework level in cases where you don't care about custom logic. But definitely something for the future rather than now 😄)
@@ -183,6 +186,9 @@ export class request { | |||
|
|||
/** @deprecated */ | |||
session: (key: string) => any; | |||
|
|||
/** Returns the request CanFulfillIntent */ | |||
getCanFulfillIntent: () => object; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we strongly type this? You should be able to specify that it returns a canFulfillIntent
.
@@ -414,3 +426,10 @@ export interface PlaybackController { | |||
name: string; | |||
function: RequestHandler; | |||
} | |||
|
|||
export class canFulfillIntent { | |||
canFulfillIntent: object; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, this canFulfillIntent
sub-object is the Amazon type described here, yeah?
It'd be great if we could add that custom interface to alexa.d.ts and reference it here. I think making it clear this is the underlying Amazon response object will help clear up the confusion of why a canFulfillIntent
object itself has a field called canFulfillIntent
I updated the example (README.md) I hope it is more clear
The flow for canFulfillIntent is difference from real handling the logic of
implementation so I think it should be separate
The response is different and the needed logic is different
It can be break into handler per intent but I feel that global handler is
better in this case. It can be light common logic for many intents.
|
Add for handler deducted for CanFulfillIntentRequest
Implement default response for Can Fulfill for the intent and slots
Changes to enable working without user id (as needed by AWS)
See details in Amazon documentation:
https://developer.amazon.com/docs/custom-skills/implement-canfulfillintentrequest-for-name-free-interaction.html
https://developer.amazon.com/docs/custom-skills/understand-name-free-interaction-for-custom-skills.html
resolves #367