Serve TwiML from our own app by zachmargolis · Pull Request #761 · 18F/identity-idp

zachmargolis · 2016-11-22T19:01:30Z

Why:
So we can add "press 1 to repeat" behavior

(This is currently blocked on some ~~Twilio integration SSL certificate issues~~basic auth environment variable configuration)

--

update! unblocked!

jessieay

Some general comments from me -- looking good! 🚃

jessieay · 2016-11-22T19:18:33Z

app/controllers/voice_controller.rb

since this is a constant, what about naming it something clearer rather than requiring comment? eg: NUMBER_OF_TIMES_USER_CAN_REPEAT_CODE

jessieay · 2016-11-22T19:19:16Z

app/services/basic_auth_url.rb

do we need this comment?

I'll remove it

jessieay · 2016-11-22T19:19:52Z

app/views/voice/otp.xml.slim

woah! I have never seen an .xml.slim file before! 😎

Yeah I'm open to .xml.erb-ing this too? I figured since it's a dealbreaker to have malformed XML for the Twilio API, I'd make it harder to mess up? But I find ERBs easier to read sometimes

jessieay · 2016-11-22T19:20:28Z

config/environments/production.rb

does this mean we remove protocol from L 19 now?

Unsure, this is for the router, and that's for the mailer...maybe the router encompasses the mailer?

jessieay · 2016-11-22T19:21:06Z

spec/controllers/voice_controller_spec.rb

not sure I understand this: too low for what?

So we want to limit the # of times the call can be looped, so I track this state via a URL parameter. When the the repeat_count tracks the value and counts down, so maybe I just should have specified "when the repeat_count gets to 1" -- will fix

jessieay · 2016-11-22T19:22:30Z

app/views/voice/otp.xml.slim

is there a way to provide multiple language options with twilio?

I need to move this to be i18ned just like the message.

Twilio supports speaking in multiple languages, but we'd have to script the choice behavior, or get a language preference from the user ahead of time and just send that: https://support.twilio.com/hc/en-us/articles/223132827-What-languages-can-the-Say-verb-speak-

jessieay · 2016-11-22T19:23:55Z

spec/jobs/voice_otp_sender_job_spec.rb

we are still sending the same message, shouldn't we still check for it in our specs?

So now we're sending a URL to our own servers, and the response of that URL contains the message. The URL no longer contains the entire message anymore, so I figured I'd check for the most important part of the URL, which is the code parameter?

gotcha! makes sense.

jessieay · 2016-11-22T19:25:16Z

spec/services/basic_auth_url_spec.rb

would it make sense to interpolate the ENV vals in this string to make it more obvious what is going on here? Or would that obfuscate more than clarify?

I think it would obfuscate? I'll leave as-is for now?

jessieay · 2016-11-22T19:26:55Z

app/controllers/voice_controller.rb

nice! If were me I would probably just make this line its own private method, but up to you.

zachmargolis · 2016-12-06T01:02:00Z

@jessieay I believe this is unblocked now (going to deploy once more to test it) -- can you give it one last review?

jessieay

💯 -- obv we should manually test, but code looks good to me!

jessieay · 2016-12-06T01:04:58Z

app/jobs/voice_otp_sender_job.rb

    )
  end
-
-  def twimlet_url(code) # rubocop:disable Metrics/MethodLength


👏 glad to be done with this crazy method

**Why**: So we can add "press 1 to repeat" behavior for a limited number of retries

**Why**: Follow more rails-y naming conventions

zachmargolis · 2016-12-06T22:42:19Z

Confirmed once more on dev, merging!

zachmargolis added the status - blocked label Nov 22, 2016

zachmargolis force-pushed the margolis-twilio-repeat branch from de875a2 to d171ad1 Compare November 22, 2016 19:15

jessieay reviewed Nov 22, 2016

View reviewed changes

zachmargolis force-pushed the margolis-twilio-repeat branch from fb647d4 to 474c512 Compare November 22, 2016 19:41

zachmargolis mentioned this pull request Nov 23, 2016

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

Merged

zachmargolis self-assigned this Nov 23, 2016

zachmargolis force-pushed the margolis-twilio-repeat branch from 474c512 to f7152d6 Compare November 23, 2016 21:21

zachmargolis mentioned this pull request Nov 25, 2016

Add "human detection" to voice OTP phone call #768

Closed

zachmargolis force-pushed the margolis-twilio-repeat branch 2 times, most recently from a45dd52 to 5696ddf Compare November 28, 2016 15:57

zachmargolis force-pushed the margolis-twilio-repeat branch from 5696ddf to d4d8d24 Compare December 5, 2016 18:06

zachmargolis force-pushed the margolis-twilio-repeat branch from d4d8d24 to fc6b432 Compare December 6, 2016 01:02

jessieay approved these changes Dec 6, 2016

View reviewed changes

zachmargolis force-pushed the margolis-twilio-repeat branch from fed066d to c995f38 Compare December 6, 2016 21:53

Serve TwiML from our own app

4eb67bc

**Why**: So we can add "press 1 to repeat" behavior for a limited number of retries

zachmargolis force-pushed the margolis-twilio-repeat branch from c995f38 to 613afe9 Compare December 6, 2016 21:54

Rename VoiceController to Voice::OtpController

613afe9

**Why**: Follow more rails-y naming conventions

zachmargolis merged commit a45b2e2 into master Dec 6, 2016

zachmargolis deleted the margolis-twilio-repeat branch December 6, 2016 22:42

Conversation

zachmargolis commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jessieay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis commented Dec 6, 2016

Uh oh!

jessieay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zachmargolis commented Nov 22, 2016 •

edited

Loading

zachmargolis commented Dec 6, 2016 •

edited

Loading