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

Cannot add Slot Resolutions in the IntentRequest #20

Closed
eemmzz opened this issue Jul 24, 2017 · 10 comments
Closed

Cannot add Slot Resolutions in the IntentRequest #20

eemmzz opened this issue Jul 24, 2017 · 10 comments

Comments

@eemmzz
Copy link
Contributor

eemmzz commented Jul 24, 2017

I'd like to be able to test synonym resolution logic in my Alexa skill but currently this framework doesn't allow any extension of the basic {name: "SlotName", value: "SlotValue"} structure.

I need to be able to add a resolutions object to a slot as documented here: https://developer.amazon.com/public/solutions/alexa/alexa-skills-kit/docs/entity-resolution-for-slot-types#intentrequest-changes

See below example JSON from the above link.

"resolutions": {
  "resolutionsPerAuthority": [
    {
      "authority": "amzn1.er-authority.echo-sdk.<skill_id>.MEDIA_TYPE",
      "status": {
        "code": "ER_SUCCESS_MATCH"
      },
      "values": [
        {
          "value": {
            "name": "song",
            "id": "SONG"
          }
        }
      ]
    }
  ]
}

Currently getIntentRequest does the following logic for slots:

if (!slots) {
  slots = {};
}
else {
  for (var key in slots) {
    slots[key] = {name: key, value: slots[key]};
  }
}

I would propose that instead we're allowed to inject the slots object as required. The Alexa SDKs will be changing quite frequently due to the constant shift in this new technology and frameworks like these will need to be able to keep up. So I would propose the following change:

intent": {"name": intentName, "slots": slots || {}}

And remove the if/else logic referenced above.

This allows users of the test framework to pass through whatever slot configurations they need. A user may pass through an invalid object but if they do their tests will fail and alert them to that.

I'm more than happy to raise a PR for this if the idea is accepted. We would really like to keep using this test framework.

@hoegertn
Copy link
Collaborator

hoegertn commented Jul 25, 2017

Hi,
this is on my agenda and I will try to implement a solution this week.

see #17

@eemmzz
Copy link
Contributor Author

eemmzz commented Jul 26, 2017

Ah I didn't spot that issue somehow. Need better glasses. Thank you for the quick reply 👍

@hoegertn
Copy link
Collaborator

hoegertn commented Aug 1, 2017

@eemmzz can you please test and review the feature I added to the master branch? This should provide the requested functionality.

@eemmzz
Copy link
Contributor Author

eemmzz commented Aug 2, 2017

@hoegertn thank you, I'll give it a go tomorrow

@eemmzz
Copy link
Contributor Author

eemmzz commented Aug 4, 2017

We had a look at the changes, and this has helped us for some initial testing but we had some extra feedback:

  • Cant add more than one resolution due to the template creating an array of size one
  • Cant do any tests around when status code is false since it's hardcoded as 'ER_SUCCESS_MATCH'
  • Slot Id is mandatory although in reality it's actually an optional value and can be null
  • In the future if amazon change the structure of the resolution object it may cause a delay in waiting for the test framework to be updated.

@hoegertn
Copy link
Collaborator

hoegertn commented Aug 7, 2017

Thanks for the feedback:

  • I will have a look at some ways to solve this
  • Will fix that
  • would it be ok to just provide null as an argument?
  • You can still set the resolutions object directly on the request in your test code.

@eemmzz
Copy link
Contributor Author

eemmzz commented Aug 7, 2017

Allowing us to set it as null would be fine. And ah ok regarding setting the object directly on the request. Would it be worth updating the examples to include slot resolutions?

Also any idea when this will be published to npm?

@hoegertn
Copy link
Collaborator

hoegertn commented Aug 7, 2017

Updating the examples sound good. Would you mind creating a PR for this?

I'm currently on holiday with poor wifi. Will release to npm when I am back on Wednesday.

@eemmzz
Copy link
Contributor Author

eemmzz commented Aug 8, 2017

Created the following PR #22

@hoegertn
Copy link
Collaborator

added the ER_SUCCESS_NO_MATCH case and the possibility to add multiple resolutions.

Published as version 1.1.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants