Skip to content

Implement "press 1 to repeat" voice OTP behavior via twimlets#767

Merged
zachmargolis merged 1 commit intomasterfrom
margolis-twimlet-menu
Nov 23, 2016
Merged

Implement "press 1 to repeat" voice OTP behavior via twimlets#767
zachmargolis merged 1 commit intomasterfrom
margolis-twimlet-menu

Conversation

@zachmargolis
Copy link
Contributor

Why:
Because this is easier to implement than having Twilio
call back in to us for the time being


This is an alternative to #761

Copy link
Contributor

@jessieay jessieay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is certainly simpler than #761...but it seems like using TwiML is a more standard approach. Which do you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the max? if so, maybe assign to a var to make test clearer? If not, why pick 4?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's the max, but I'll turn it into a while to make it clearar

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line is kinda hard to read -- can we break it up somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll split it into a few lines

@zachmargolis
Copy link
Contributor Author

zachmargolis commented Nov 23, 2016

@jessieay at the end of the day, this is building and serving TwiML for us, so I don't really mind. I think ideally we should move towards serving our own TwiML as it gives us more control. However while we're blocked on SSL issues, this gives us menu functionality in the meantime. ¯\_(ツ)_/¯

Copy link
Contributor Author

@zachmargolis zachmargolis Nov 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I inline the repeat = message_repeat(code) part to shorten the method and get under the line length limit, reek gets mad about repeat statements, this seemed like a decent compromise to me

Copy link
Contributor

@jessieay jessieay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Sounds like this code may go away soon anyway, so inline rubocop comments here don't bother me too much.

**Why**:
Because this is easier to implement than having Twilio
call back in to us for the time being
@zachmargolis zachmargolis merged commit bdfc288 into master Nov 23, 2016
@jessieay jessieay deleted the margolis-twimlet-menu branch December 20, 2016 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants