-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for SMS conversations #7
base: master
Are you sure you want to change the base?
Changes from all commits
cccf6fd
4c46443
d46e777
89c8b6b
7b81007
b1d56b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,19 @@ | |
app = Flask(__name__) | ||
|
||
from twilio import twiml | ||
from twilio.rest import TwilioRestClient | ||
|
||
import boto3 | ||
import botocore | ||
import os | ||
from datetime import datetime, timedelta | ||
from raven.contrib.flask import Sentry | ||
sentry = Sentry(app) | ||
|
||
s3 = boto3.resource('s3') | ||
bucket = s3.Bucket(os.environ['data_bucket']) | ||
twilio_client = TwilioRestClient(os.environ['twilio_sid'], | ||
os.environ['twilio_token']) | ||
|
||
# Where we're storing all our audio files. | ||
url_base = "https://s3-us-west-2.amazonaws.com/true-commitment/" | ||
|
@@ -68,6 +81,14 @@ | |
_original | ||
] | ||
|
||
messages = [ | ||
"We're no strangers to love.", | ||
"You know the rules, and so do I.", | ||
"A full commitment's what I'm thinking of.", | ||
"You wouldn't get this from any other guy.", | ||
"Call me?", | ||
] | ||
|
||
# Menu generation. I'd love to put this in its own function to be clean and | ||
# tidy, but if I put that at the end Python gets grumpy and I'm not sure how to | ||
# forward-declare. I could put it into a separate file and include that, but | ||
|
@@ -105,7 +126,7 @@ def play_tune(tune): | |
gather = response.gather(numDigits=1, timeout=10) | ||
gather.play(tune['url']) | ||
gather.say(menu) | ||
|
||
# Our goodbye triggers after gather times out. | ||
response.say(goodbye) | ||
|
||
|
@@ -153,5 +174,55 @@ def original(): | |
|
||
return str(play_tune(tune)) | ||
|
||
@app.route("/sms", methods=["POST"]) | ||
def sms(): | ||
"""Adds an incoming SMS to a queue, to reply to later.""" | ||
bucket.put_object( | ||
Key='queue/{to}/{from_}'.format(to=request.form['To'], | ||
from_=request.form['From']), | ||
Body='', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this lose the message they sent us? It would be visible in the Twilio logs, but in terms of maximum amusement for line operators, being able to keep the conversation in a more accessible form would be amazing. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or am I correct that these queued messages get deleted later (when we process them), so storing the SMS received in them is not exactly useful. :) Would it make more sense to have a separate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's exactly how it works! (I meant to add a PR comment summarising how everything works, but ran out of time.) |
||
) | ||
return "Hello world!" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could be wrong here, but my understanding is that Twilio requires a valid TwiML response in order to consider a message processed. If that return is going back to Twilio (which I think it is), then this will make Twilio unhappy. We should be able to get away with just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Twilio didn't generate an error message when I tested it, contrary to their docs. (Perhaps because I'm sending a response with whatever Flask's default Content-Type is (text/plain maybe?) rather than the TwiML type, it's not expecting to be able to do anything with it?) In any case, I agree that what you're suggesting is nicer, whether it's strictly required or not. |
||
|
||
def send_sms(): | ||
"""Empties the queue of incoming SMSes, replying to each one.""" | ||
for queue_entry in bucket.objects.filter(Prefix='queue/'): | ||
_, our_number, their_number = queue_entry.key.split("/") | ||
state_key = "state/{}/{}".format(our_number, their_number) | ||
|
||
# Find out which message we sent last, if any | ||
# Error handling taken from https://stackoverflow.com/a/33843019 | ||
try: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code climate is point out that this function is a bit complex, and this seems perfect for factoring out to a separate function which returns the next message number (or raises on error). I'm guessing that if we do throw an exception, we should log that but still try to process the other messages in our queue, so a single bad message doesn't knock out the entire SMS queue. (This isn't a blocker for this PR, but indicates future work that may be required.) I have no idea what the best way is to log an error from Lambda. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, no issues on separating out into a separate function. I'll do both that and what you're suggesting with error catching as well. For logging errors from Lambda, I'd use Sentry; it's a really well-thought-out tool with a generous free tier, and the whole thing is open source too! (We run the open-source version on-prem at work.) |
||
state_obj = s3.Object( | ||
os.environ['data_bucket'], | ||
state_key, | ||
) | ||
state_obj.load() | ||
except botocore.exceptions.ClientError as e: | ||
if e.response['Error']['Code'] == "404": | ||
last_msg = -1 | ||
else: | ||
raise | ||
else: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Python n00b 'woah' moment here... You can have a Am I correct the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep! You can also attach an |
||
last_msg = int(state_obj.get()['Body'].read().decode('utf8')) | ||
|
||
# Send the next one | ||
next_msg = last_msg + 1 | ||
if next_msg < len(messages): | ||
twilio_client.messages.create( | ||
to=their_number, | ||
from_=our_number, | ||
body=messages[next_msg], | ||
) | ||
|
||
# Record which message we just sent in S3 | ||
bucket.put_object( | ||
Key=state_key, | ||
Body=str(next_msg), | ||
Expires=datetime.now() + timedelta(days=7), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I correct this expires our messages after a week, so if the same person were to text again after a week has passed the conversation would start again from the beginning? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct. I wasn't sure what the threshold should be, but I felt like there should be some cut-off. |
||
) | ||
|
||
queue_entry.delete() | ||
|
||
if __name__ == "__main__": | ||
app.run() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
{ | ||
"dev": { | ||
"app_function": "rickroll.app", | ||
// Put a random S3 bucket name that doesn't already exist here. Zappa | ||
// will create it and put your Lambda code in it when you run | ||
// 'zappa init'. | ||
"s3_bucket": "insert-bucket-name-here", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One of my least favourite things about JSON is that it doesn't allow comments... 😭 While Zappa may be relaxed enough to allow them, the QA/Testing part of my soul stays awake at night when I know we're technically breaking a format standard. I know it's awful, but can I ask for something like: "s3_bucket": "An S3 bucket that doesn't already exist. Zappa will create it and use it for storage when you run `zappa init`" Comments are waaay nicer, and if we were using YAML or HJSON I'd be all over them. But every linter (including github's changes view) and my psyche is going to worry every time it sees a |
||
"aws_region": "us-east-1", | ||
"environment_variables": { | ||
// This should be a bucket WITHOUT public read access. Phone numbers | ||
// will get stored in here. | ||
"data_bucket": "rick-astley-data", | ||
// These should be set to an IAM user with access to your S3 bucket | ||
"aws_access_key_id": "INSERT_ACCESS_KEY_HERE", | ||
"aws_secret_access_key": "insert-secret-access-key-here", | ||
"twilio_sid": "insert-twilio-sid-here", | ||
"twilio_token": "insert-twilio-token-here" | ||
}, | ||
"events": [{ | ||
"function": "rickroll.send_sms", | ||
"expression": "rate(4 minutes)" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oooh, this is cool! So every N minutes (n=4 in this case) we'll clear the SMS queue of messages? (So on average, we'd see a reply every two minutes?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep! This seemed like the simplest way to have a delay before replying without actually making the Lambda function spin for a few minutes (and rack up charges in the process). I thought about randomly skipping 50% of the queue every time |
||
}] | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation! You are amazing! ❤️