-
Notifications
You must be signed in to change notification settings - Fork 3
handle opted out contacts #9
handle opted out contacts #9
Conversation
go_http/send.py
Outdated
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.
Happy with us using this for now, but I think we need a better way of checking what kind of error happened than matching against the error message. Maybe we need a separate field for this, like type: 'opted_out'
or something? @rudigiesler @hodgestar @jerith thoughts?
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.
This is something we could do, but it would be quite a big change to how the API works. But I think it would be a useful thing to plan some time for.
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 there is no reason
the .get()
will return None
and x in None
with raise a TypeError
. Not sure it's a huge issue, but just mentioning it.
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 think it would be good to add some sort of error type to the API responses, but I agree with Rudi that that's probably best left for another PR. I don't think it should be a huge change to the API, so if we can fit it into this sprint, that would be cool, but it's not a priority for me.
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.
+1 on not doing this in this PR.
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 can change the line quickly to read response.get('reason', '')
, which should fix the None
issue, although I think the API always returns a reason for 400 errors, but I think it's good to make sure that the client doesn't break confusingly if there is no reason.
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.
.get(..., '')
sounds good.
Two minor comments, looks good. |
👍 |
👍 again! |
…contacts handle opted out contacts
If a contact is opted out, the new API will send a 400 error; we should catch this error and raise a ContactOptedOut error instead.