-
Notifications
You must be signed in to change notification settings - Fork 1
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
Handle acks and nacks from sent messages #5
Handle acks and nacks from sent messages #5
Conversation
…s' of github.com:praekelt/vumi-twilio-api into feature/issue-5-handle-acks-and-nacks-from-sent-messages
Ready for review. |
@@ -2,12 +2,21 @@ language: python | |||
python: | |||
- "2.6" | |||
- "2.7" | |||
services: | |||
- "riak" |
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.
We don't need riak, do we?
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.
I see we've added a MessageStore -- that seems like a crazy requirement but I'm going to keep reading.
default=None, static=True) | ||
status_callback_method = ConfigText( | ||
"The HTTP method to use when sending the callback status", | ||
default='POST', static=True) |
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.
If I'm understanding the Twilio documentation correctly, all of these should just be the equivalent of account fallback values because they can also be specified in the Call
request?
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.
Ah! These are the URLs for received calls?
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.
In that case I recommend we make the values not-static. That will allow us to sub-class this application and make the URLs message-dependent later on.
Left some comments. We definitely don't want to depend on the message store but its otherwise looking good! Sorry it took me so long to get to this. Have a good weekend! |
Ready for another review. |
self._data = { | ||
'error_type': error_type, | ||
'error_message': error_message, | ||
} |
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.
We should probably doing something like super(Error, self).__init__(error_type=error_type, error_message=error_message)
rather than setting _data
.
Left some very minor comments. Otherwise looks good. Woot on making the SessionID manager it's own class -- that worked out very nicely. |
👍 |
…acks-from-sent-messages Handle acks and nacks from sent messages
No description provided.