Mail sdk changes #1
Conversation
This commit adds the remaining error handling tests for the SMTP mail sending functionality. - Adds a test to ensure `MissingRecipientsError` is raised when no recipients are provided for an admin email. - Adds a test to ensure `InvalidSenderError` is raised for SMTP authentication errors. - Adds a test to ensure a generic `Error` is raised for SMTP connection errors and missing `SMTP_HOST` configuration. - Corrects the exception type raised in the `send()` method to use the base `Error` class for transient SMTP errors.
There was a problem hiding this comment.
I am not going to be fussy about this change in the SDK since I see the quality of the current code is pretty weird with in that we have an uber-file called mail.py of 1955 and all the logic is dumped there. So with that in mind, it is fine to have this current of branching logic for Python
In Java, the code is more modular and I would encourage using a proper DP like Factory or Strategy pattern there (or if there is something cleaner/better). Just leaving this non-blocking comment here posterity for Java and if you want to use it as input(up to your judgment)
YashBanka
left a comment
YashBanka
left a comment
There was a problem hiding this comment.
My review is focused on the Python implementation, as I don't have the full context on the migration plan. Looks good to merge from my perspective.
This change prefixes all environment variables related to the external SMTP mail service with `APPENGINE_` to improve namespacing and prevent potential conflicts in the deployment environment. The following variables have been renamed: - USE_SMTP_MAIL_SERVICE -> APPENGINE_USE_SMTP_MAIL_SERVICE - SMTP_HOST -> APPENGINE_SMTP_HOST - SMTP_PORT -> APPENGINE_SMTP_PORT - SMTP_USER -> APPENGINE_SMTP_USER - SMTP_PASSWORD -> APPENGINE_SMTP_PASSWORD - SMTP_USE_TLS -> APPENGINE_SMTP_USE_TLS - ADMIN_EMAIL_RECIPIENTS -> APPENGINE_ADMIN_EMAIL_RECIPIENTS The changes have been applied to both the SDK implementation in `mail.py` and the corresponding unit tests in `mail_unittest.py
Fixes #<issue_number_goes_here>