fix(api): 1392 send email to each recipient as separate requests#1595
fix(api): 1392 send email to each recipient as separate requests#1595JazzarKarim wants to merge 7 commits intobcgov:mainfrom
Conversation
JazzarKarim
force-pushed
the
1392-send-email-seperate
branch
from
413602d to
b3baba6
Compare
| last_failure_status = None | ||
| for recipient in recipients_list: | ||
| single_email = {**email, "recipients": recipient} | ||
| resp = requests.post( |
There was a problem hiding this comment.
This api call could results in errors that could lead to 5xx errors. is that intended?
There was a problem hiding this comment.
Actually Jimmy, good point. I've changed this to a return an HTTP 400 (Bad Request) if sending to particular recipient fails. I think that would be more accurate and makes more sense.
I've also added some logging to indicate which recipient it was.
There was a problem hiding this comment.
i am not sure about 400 as that would imply user error. I wouldnt do that unless we are absolutely sure that the error results from bad email (which would be a user error). what do you think?
There was a problem hiding this comment.
I was thinking that the error does indeed stem from a user error right? Since the email was poorly formatted (input was something wrong), like @ba$.com which is invalid. The sending of email fails in this case. That's why I was thinking in this a 4xx makes sense? Thoughts?
There was a problem hiding this comment.
The try catch logic will catch any excpetion and throw 4xx. I am not sure if thats the right behaviour
There was a problem hiding this comment.
OK, good point, thanks. Fixed.
Now, if the error is from the recipient's email being badly formatted, it's a 400 error.
Otherwise, it's a 502 error (Bad Gateway).
This should make the error message not to be misleading.
| interaction.customer_id = customer_id | ||
| interaction.user_id = user_id | ||
| interaction.notify_reference = notify_id | ||
| interaction.notify_reference = notify_json.get("ids") or notify_id |
There was a problem hiding this comment.
sanity check: we are modifying the shape of the notify_reference here. is this clear of any downstream side effects?
There was a problem hiding this comment.
I changed this to make it backwards compatible as a single notify ID again. That should be much safer. Thanks Jimmy.
|
2 participants
Issue:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the BC Registry and Digital Services BSD 3-Clause License