search

OneBusAway / onebusaway-alexa

An Java-based app to communicate with Amazon Alexa for devices such as the Amazon Echo
Other
52 stars 18 forks source link

Fix #53 - Disambiguate Stop Codes #83

Closed vikram31291 closed 8 years ago

vikram31291 commented 8 years ago

Please make sure these boxes are checked before submitting your pull request - thanks!

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.05%) to 54.651% when pulling 687e2a61d31d7e4abaa4143809bf4f2c83b7f761 on vikram31291:vikram/multipleStops into 3d057bc6b3ce6ab78456672a8981d311d5cad00d on OneBusAway:master.

barbeau commented 8 years ago

Thanks @vikram31291! This looks pretty good. A few smaller comments inline.

My Echo power supply got fried and I'm waiting for a replacement, so I'm attempting to review this PR with Alex Dev Console Testing tab and the Echo Sim browser testing tool (which doesn't always seem to work well). With these tools, I'm having intermittent issues getting it to recognize Yes/No responses. For example, using EchoSim, if I set my region to "Tampa" and stop to "3105", it says that it found more than one stop, and asks if my stop is X. However, if I say "Yes" or "No" it sometimes says:

I could not locate a OneBusAway region near No, the city you gave...

In the Alexa Dev Console Testing tab, if I use lower case "yes" or "no" it works, but if I use upper case "Yes" or "No" it gives me the above region response.

Have you seen the same problem? Or does it seem to work consitently for you on an actual Echo?

I'm not sure if this is an artifact of the limited testing tools I have, or if it's an issue on the actual Echo as well.

Also, we had previously talked about adding the agency name on the end based on the routes serving that stop. So, for example:

"We found %d stops associated with the stop number. Did you mean the %s stop served by %s?"

Reasoning is that most duplicate stop codes arise from one stop code per agency, and the user will likely know which agency they want, but may not recognize the stop names.

Would you still be able to add this?

barbeau commented 8 years ago

ICLA manually confirmed :+1:

vikram31291 commented 8 years ago

@barbeau I fixed the little issues and pushed that out.

I will add the agency name in a separate change in the coming weeks.

As for the yes/no issue, I will try on my echo tomorrow; currently don't have access to it. I was using the dev tools to verify.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.01%) to 54.615% when pulling 5e7988add2bb6cf4314727870e439dcac3e67cfb on vikram31291:vikram/multipleStops into 5bf1d3d33b5a65998170dbe3c61262a11b120113 on OneBusAway:master.

barbeau commented 8 years ago

@vikram31291 I tested this using the Roger app and Yes/No recognition seems to work fine. Any objections to going ahead and merging this? I know we had talked about adding agency name but I'd like to go ahead and get this merged now, and we can add agency name in new PR.

vikram31291 commented 8 years ago

@barbeau Sounds good to me!

barbeau commented 8 years ago

Thanks @vikram31291! I appreciate you pulling this together. Sorry it took so long to review.

barbeau commented 8 years ago

I opened a new issue for adding agency name at https://github.com/OneBusAway/onebusaway-alexa/issues/85.

greenwoodcm commented 7 years ago

I have the same issue that barbeau had. When the app reads me the different stops and I say "yes" I get

"I could not locate a OneBusAway region near No, the city you gave..."

barbeau commented 7 years ago

@greenwoodcm bummer, I had hoped this was fixed. Does it always give the region response, or does it sometimes work correctly for you and set the stop? And are you seeing this on an Echo?

greenwoodcm commented 7 years ago

I have never gotten it to work, always the same. I can reproduce on both an echo and dot.

barbeau commented 7 years ago

@greenwoodcm Just opened new issue here - https://github.com/OneBusAway/onebusaway-alexa/issues/87.