Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Place email template improvements #120

Merged
merged 8 commits into from
Apr 23, 2018
Merged

Place email template improvements #120

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5929470
refactor(place emails): support multiple BCC emails
goldpbear Apr 18, 2018
3478f66
refactor(place emails): send emails to BCC list even when no recipien…
goldpbear Apr 18, 2018
18db98f
refactor(place emails): remove check for email submitter field
goldpbear Apr 18, 2018
4425061
WIP: bcc emails
goldpbear Apr 18, 2018
ee3731b
WIP: bcc emails
goldpbear Apr 18, 2018
44e53d5
WIP: bcc emails
goldpbear Apr 18, 2018
d696aed
WIP: bcc emails
goldpbear Apr 21, 2018
2ed1923
refactor: address review feedback
goldpbear Apr 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/sa_api_v2/models/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@goldpbear goldpbear Apr 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

subject = models.CharField(max_length=512)
body_text = models.TextField()
body_html = models.TextField(blank=True, default=None)
Expand Down
12 changes: 6 additions & 6 deletions src/sa_api_v2/views/base_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1231,6 +1231,7 @@ def trigger_emails(self, email_templates, obj):
logger.info('[EMAIL] Starting email send')

from_email = email_template.from_email
bcc_email = email_template.bcc_email

logger.debug('[EMAIL] Got from email')

Expand All @@ -1241,9 +1242,7 @@ def trigger_emails(self, email_templates, obj):
recipient_email = self.request.DATA[email_field]
logger.debug('[EMAIL] recipient_email: ' + recipient_email)
except KeyError:
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 = ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can add a logger.debug here to clarify that recipient_email was not provided?


logger.debug('[EMAIL] Got to email')

Expand All @@ -1256,14 +1255,15 @@ 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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"or" -> "and"

# then no need to go further.
if not recipient_email and not bcc_email:
return

logger.debug('[EMAIL] Going ahead, recipient exists')

# Set optional values
bcc_list = [email_template.bcc_email]
bcc_list = email_template.bcc_email.split(',')

logger.debug('[EMAIL] Got bcc email')

Expand Down