Add support for finishOnKey in Gather blocks #4

Merged
merged 7 commits into from Mar 13, 2013

Projects

None yet

2 participants

@jongilbraith
Contributor

If a finish key is specified it will take everything up to that key and ignore the rest, as if Twilio had moved on and stopped listening to presses.

If the key isn't pressed at all, an exception is raised.

@JMongol
Owner
JMongol commented Mar 12, 2013

Cool - will take a more detailed look at this later. A quick question for you about raising the exception on a missing finish key though. If I set finishOnKey="#" and press "12345" instead of "12345#", doesn't Twilio eventually accept "12345" after some timeout period? If that's true, and this is technically valid behavior, is raising an exception the best choice? The test is probably better written if it uses the finish key, but the app would otherwise behave the same, finish key or not.

I can see arguing for it both ways, and am curious as to your perspective on this.

@jongilbraith
Contributor

That's a good point, Jack, I think that's right so would be inclined to go with accepting it without the finish key.

I'll double check the docs and amend the behaviour accordingly.

@JMongol
Owner
JMongol commented Mar 12, 2013

A couple of alternate approaches to consider:

  • Some type of flag on the object that indicates whether the keys were stripped or not. Might be useful to someone trying to verify this in a test, but I don't personally have a use for it.
  • Spew a warning to the console if the finishKey is omitted, as it's arguably a test issue.

Let me know what you come up with.

@jongilbraith
Contributor

Just (force) pushed a change to remove the exception but still test the behaviour using a finish key and timeout.

I guess this does lead us to a bunch more questions, such as the fact that there's also actually a default finish key too. I'll have a bit more of a think about it.

@JMongol
Owner
JMongol commented Mar 12, 2013

OK. I think the rest of the changelist is fine, so if you want to pull out the exception piece and we can rethink this (and add something in later), I think that would be fine too. There's value in having the finishKey recognized by the toolkit I think. Plus, it should be a non-breaking change, so adding it won't affect a bunch of folks.

@jongilbraith
Contributor

Cool. I'm almost done for the day now so I'll look into adding the default finishOnKey behaviour tomorrow.

In the meantime though it's probably still at least an improvement in that right now we do have a small bug in that if you do enter a finish key it doesn't get stripped out of params[:Digits]. I hadn't noticed it until now as it was just being passed into a find call and being coerced into an integer.

Jon Gilbraith added some commits Mar 12, 2013
Jon Gilbraith Add CallScope#gather_finish_on_key.
For accessing the finishOnKey option on a gather scope.
9d4a8bf
Jon Gilbraith Add a private method of CallScope#formatted_digits.
Takes an option of :finish_on_key in order to strip that key from the digits before returning it.

Also encapsulates existing logic to check for a nil value.
6d56cbf
Jon Gilbraith Use #formatted_digits in #request_for_twiml!.
Can now take finish_on_key into account.

Also no longer need to handle where options[:digits] is nil as dealt is with elsewhere.
84b2c5e
Jon Gilbraith Finally, update CallScope#press to use the finish_on_key value. 4cde6e8
Jon Gilbraith Add a spec to cover a gather with finishOnKey specified. daa3bb7
Jon Gilbraith Tighten up button press specs.
Match against "You entered 98765." rather than "You entered 98765".

Adding the trailing character compensates for the fact that has_say? checks only for partial matches, so we wouldn't detect when there were trailing digits returned.

E.g. "You entered 98765432", for which has_say? would right now return true.
90dced5
Jon Gilbraith Implement a default finish key of hash. 4dfc114
@jongilbraith
Contributor

Right, force pushed again, here's where we stand:

  • This now implements finishOnKey in that there is one by default (hash), you can specify your own, and that if not entered, the digits will still be accepted.
  • Fixed is a bug with the fact that if a finish key is pressed, it wasn't being stripped from the digits
@JMongol JMongol merged commit bd0f479 into JMongol:master Mar 13, 2013
@JMongol
Owner
JMongol commented Mar 13, 2013

Looks good - thanks for the changes. Will push the gem update out shortly.

@jongilbraith
Contributor

Np, cheers Jack.

@JMongol
Owner
JMongol commented Mar 13, 2013

FYI I fixed a minor bug in formatted_digits - the digits param should be a string, but in some existing tests it's a number, so I forced it to a string before calling split. Otherwise tests barf.

V 3.1 of the gem is available now on rubygems as well.

@jongilbraith
Contributor

Ah, good call, didn't consider that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment