search

mattstauffer / pulledover

Pulled Over App
http://pulledover.us/
MIT License
56 stars 16 forks source link

Add code to handle Twilio error `21610` (customer has replied with `STOP`) #26

Open mattstauffer opened 8 years ago

mattstauffer commented 8 years ago 
21610 - SMS cannot be sent to the 'To' number because the customer has replied with STOP
RDelorier commented 8 years ago

How should the exception be handled?

RDelorier commented 8 years ago

Started working on this here https://github.com/RDelorier/pulledover/tree/handle-twilio-error-21610

Merged those jobs and got it down to 3 spots that the blacklisted exception can happen, see the latest commit to find those 3 spots.

mattstauffer commented 8 years ago

@RDelorier I like some of the directions you're going there--merging and catching exceptions--and don't like some others--interfaces that we don't need, tons of docBlocks, etc.

However, it's not really up to you to read my brain on that :D In general, you're making some really great progress over there. I'll happily give a lot more feedback once you PR.

In general.. I think if someone STOPs, we need to keep that number, but we need to add a "deactivated" flag to it. Then we need to show that flag in the UI, and MAYBE we need to notify their friend? If the owner's the one that deactivated their own number.. I don't even know.

Additionally, if it says STOP we probably should prohibit that number from being added again.. but i don't know if I care to do all that work. Because my bet is, if someone adds their number later, it'll try to send them the verification text message, and then it'll error out, and our code will handle that.

Side note: If you're not in your fork, we'll want to handle this error on EVERY text we send, not just on verification--they might STOP and then later be sent a legit "your friend was pulled over" text, and we need to handle this exception on that too.

Thanks for your work on this @RDelorier; as always, do what you can/will and pass whatever is left, if anything to me. Thanks!!

RDelorier commented 8 years ago

@mattstauffer thanks for the feedback. The interface I'll go ahead and take out, I was thinking it would be helpful when merging those jobs but ended up not using it.

Just added the following stuff

I think the error messages on create are going to work not work on production since they are queued... I'm guessing you will want to just do some notifications instead.

I'm going to hand over the reins to you because there are some exec decisions to make on how to handle the exception. It also looks like you're going to want in incoming text route so that you can un-flag an number that removes themselves from the blacklist by responding "START".

Do you want me to pr what I have or do you just want to branch off mine.

Links for quick reference Twilio FAQ - Handling "STOP"

mattstauffer commented 8 years ago

@RDelorier Why don't you PR it to the handle-customer-STOP branch and I'll work from there? Thanks man!

https://github.com/mattstauffer/pulledover/tree/handle-customer-STOP

RDelorier commented 8 years ago

@mattstauffer I'll put it together now, I've never written a pr for an work in progress so here goes nothing!

in regards to episode 77, there is a reason I chose this particular project its just not as noble as your own

RDelorier commented 8 years ago

@mattstauffer Can you fast-forward that branch so the pr is smaller. I can rebase if you prefer but there will be some conflicts to deal with later.

mattstauffer commented 8 years ago

@RDelorier uhhh so I JUST got an email about fast-forwarding a branch.. i have no idea whether we resolved this or not. No idea what happened here. Do you still want me to do something?

RDelorier commented 8 years ago

@mattstauffer Ha I think your notifications are on the fritz. Can you fast forward handle-customer-STOP so I can switch #30 to it. There are still a few things that need to be done before it goes to master.

mattstauffer commented 8 years ago

@RDelorier yep. definitely on the fritz. Just got this.

By fast-forward it, do you mean rebase it onto master so it has the latest commits? Sorry, I'm not familiar with using the term that way. Thanks for your patience!

RDelorier commented 8 years ago

Ha yea rebase onto master, I've been pretty swamped with our latest deployment so patience is all I have 😉 On Jun 3, 2016 3:33 PM, "Matt Stauffer" notifications@github.com wrote:

@RDelorier https://github.com/RDelorier yep. definitely on the fritz. Just got this.

By fast-forward it, do you mean rebase it onto master so it has the latest commits? Sorry, I'm not familiar with using the term that way. Thanks for your patience!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mattstauffer/pulledover/issues/26#issuecomment-223672967, or mute the thread https://github.com/notifications/unsubscribe/ACMHe0HX7fjooONciU5o1SpqWPQlZuCbks5qIIGHgaJpZM4Hvk5h .

mattstauffer commented 8 years ago

@RDelorier done!

RDelorier commented 8 years ago

W00t thanks On Jun 3, 2016 3:45 PM, "Matt Stauffer" notifications@github.com wrote:

@RDelorier https://github.com/RDelorier done!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mattstauffer/pulledover/issues/26#issuecomment-223675900, or mute the thread https://github.com/notifications/unsubscribe/ACMHe4VcfTUP3BUcxnmNBxDtXExQWBGeks5qIIQ9gaJpZM4Hvk5h .