Place email template improvements #120
Conversation
goldpbear
force-pushed
the
goldpbear/place-email
branch
from
dc18d0b
to
18db98f
Compare
goldpbear
force-pushed
the
goldpbear/place-email
branch
from
dd947c4
to
44e53d5
Compare
There was a problem hiding this comment.
Just a couple suggestions:
- Make sure to run
manage.py makemigrationsto ensure that no migrations are needed - Unfortunately our continuous testing setup is broken, so make sure to run the tests locally and ensure that they are all passing
- Use an array for our
bcc_emailsfield (see comment)
src/sa_api_v2/models/core.py
Outdated
| @@ -211,7 +211,7 @@ class PlaceEmailTemplate (TimeStampedModel): | |||
| event = models.CharField(max_length=128, choices=EVENT_CHOICES, default='add') | |||
| recipient_email_field = models.CharField(max_length=128) | |||
| from_email = models.EmailField() | |||
| bcc_email = models.EmailField(blank=True, default=None) | |||
| bcc_email = models.CharField(max_length=512, blank=True, default=None) | |||
There was a problem hiding this comment.
As discussed, can we make this an array of models.EmailField, and rename the field to bcc_emails? It would be a bit cleaner and allow us to retain email validation.
Note that we will have to migrate the email in the existing bcc_email field, if present, and add it to be the first item in the array. We can add this to the migration itself, or we can do this manually, if it is simpler.
There was a problem hiding this comment.
My guess is it will be simpler to do this manually, since I think we only have one email template in use at the moment.
There was a problem hiding this comment.
I might be missing something, but from what I can tell ArrayField is a Django 1.8+ feature: https://docs.djangoproject.com/en/2.0/ref/contrib/postgres/fields/#arrayfield.
Is there another way to have arrays of fields? Maybe a simpler approach for the time being would be to just hard-code a fixed number of BCC EmailFields, like 5 or so, and we'd just be limited to having 5 BCC emails on our templates.
There was a problem hiding this comment.
Maybe a simpler approach for the time being would be to just hard-code a fixed number of BCC EmailFields, like 5 or so, and we'd just be limited to having 5 BCC emails on our templates.
I think that's a reasonable idea :-)
goldpbear
force-pushed
the
goldpbear/place-email
branch
from
9899fc3
to
d696aed
Compare
src/sa_api_v2/views/base_views.py
Outdated
| @@ -1256,17 +1260,13 @@ def trigger_emails(self, email_templates, obj): | |||
|
|
|||
| logger.debug('[EMAIL] Going ahead, no errors') | |||
|
|
|||
| # If the user didn't provide an email address, then no need to go further. | |||
| if not recipient_email: | |||
| # If the user didn't provide an email address, or no BCC emails are provided, | |||
src/sa_api_v2/views/base_views.py
Outdated
| if not recipient_email: | ||
| # If the user didn't provide an email address, or no BCC emails are provided, | ||
| # then no need to go further. | ||
| if not recipient_email and not bcc_list: |
There was a problem hiding this comment.
Can we move this check to above the if errors conditional, and add an error to the errors array? It would be more idiomatic with the way we are handling errors in this function instead of returning out of the function without any feedback.
src/sa_api_v2/views/base_views.py
Outdated
| errors.append("No '%s' field found on the place. " | ||
| "Be sure to configure the 'notifications.submitter_" | ||
| "email_field' property if necessary." % (email_field,)) | ||
| recipient_email = "" |
There was a problem hiding this comment.
Perhaps we can add a logger.debug here to clarify that recipient_email was not provided?
Addresses #117.
Fixes #121, #122.
Needs to be tested on the staging server before merging
UPDATE: seems to be working on the staging server as expected
Two tweaks to our place email template feature:
Note that the multi-email BCC list is just aCharField(so we lose email validation checks). There might be a better pattern for entering a list of emails, but I didn't turn up anything after a quick search. We may want to research more carefully but since this isn't user-facing it's not a huge deal I think.We now have a fixed number (5) of
EmailFields for BCC addresses.TODO: