Include message with emails #849

Merged
merged 6 commits into from Jan 28, 2016

Conversation

Projects
None yet
4 participants
@Calvinp
Contributor

Calvinp commented Jan 19, 2016

Now that Singularity has the capability to accept optional user messages for tasks, these messages can be included in any emails sent because of user actions with messages. This commit includes that ability.
It also includes the ability to set a variable, LOG_EMAILS_FOR_DEBUG - If this variable is set to true, all emails sent will be logged in the debug logs. This allows local testing of emails without an SMTP server.

Calvinp added some commits Jan 19, 2016

When the user modifies a request and adds an optional message, this m…
…essage will be sent in the header of any email that gets sent out becasue of that request.
@@ -287,6 +291,10 @@ private void prepareTaskMail(Optional<SingularityTask> task, SingularityTaskId t
final String body = Jade4J.render(taskTemplate, templateProperties);
+ if (LOG_EMAILS_FOR_DEBUG) {
+ LOG.debug("TheMail: " + body);

This comment has been minimized.

@tpetr

tpetr Jan 19, 2016

Member

I'd suggest logging this at the LOG.trace() level instead, so that people can more easily control whether or not the email should be logged

@tpetr

tpetr Jan 19, 2016

Member

I'd suggest logging this at the LOG.trace() level instead, so that people can more easily control whether or not the email should be logged

+ if (LOG_EMAILS_FOR_DEBUG) {
+ LOG.debug("TheMail: " + body);
+ }
+

This comment has been minimized.

@tpetr

tpetr Jan 19, 2016

Member

We should minimize the number of times the email body is logged -- i think just having this one here will suffice

@tpetr

tpetr Jan 19, 2016

Member

We should minimize the number of times the email body is logged -- i think just having this one here will suffice

@Calvinp

This comment has been minimized.

Show comment
Hide comment
@Calvinp

Calvinp Jan 20, 2016

Contributor

These two issues have been fixed and the branch pushed again.

Contributor

Calvinp commented Jan 20, 2016

These two issues have been fixed and the branch pushed again.

@Calvinp

This comment has been minimized.

Show comment
Hide comment
@Calvinp

Calvinp Jan 20, 2016

Contributor

Whoops, I lied before, forgot about making it trace level. Now they're both fixed.

Contributor

Calvinp commented Jan 20, 2016

Whoops, I lied before, forgot about making it trace level. Now they're both fixed.

@@ -62,6 +68,10 @@ public void stop() {
}
public void queueMail(final List<String> toList, final List<String> ccList, final String subject, final String body) {
+ if (LOG_EMAILS_FOR_DEBUG) {
+ LOG.trace("TheMail: " + body);

This comment has been minimized.

@ssalinas

ssalinas Jan 20, 2016

Member

We can do without the LOG_EMAILS_FOR_DEBUG. We can control the logging level for each class in the configuration for Singularity, so just putting it at trace should be enough.

Also for logging, you can so something more like this rather that using "string" + "string"

LOG.trace("Sending email: {}, to: {}, cc: {}", body, toList, ccList)
@ssalinas

ssalinas Jan 20, 2016

Member

We can do without the LOG_EMAILS_FOR_DEBUG. We can control the logging level for each class in the configuration for Singularity, so just putting it at trace should be enough.

Also for logging, you can so something more like this rather that using "string" + "string"

LOG.trace("Sending email: {}, to: {}, cc: {}", body, toList, ccList)

This comment has been minimized.

@jhaber

jhaber Jan 20, 2016

Member

For readability I've found it to be easier to put the recipients first and the body last so you're not scrolling through a wall of HTML looking for the recipient at the end

@jhaber

jhaber Jan 20, 2016

Member

For readability I've found it to be easier to put the recipients first and the body last so you're not scrolling through a wall of HTML looking for the recipient at the end

@Calvinp

This comment has been minimized.

Show comment
Hide comment
@Calvinp

Calvinp Jan 20, 2016

Contributor

Fixed

Contributor

Calvinp commented Jan 20, 2016

Fixed

@@ -30,6 +30,8 @@
import io.dropwizard.lifecycle.Managed;
+
+

This comment has been minimized.

@ssalinas

ssalinas Jan 20, 2016

Member

super nit-picky, but can we get rid of these extra new lines here and down by the LOG.trace

@ssalinas

ssalinas Jan 20, 2016

Member

super nit-picky, but can we get rid of these extra new lines here and down by the LOG.trace

@Calvinp

This comment has been minimized.

Show comment
Hide comment
@Calvinp

Calvinp Jan 20, 2016

Contributor

Not sure how those got there in the first place, but they're gone now.

Contributor

Calvinp commented Jan 20, 2016

Not sure how those got there in the first place, but they're gone now.

@tpetr tpetr modified the milestone: 0.4.9 Jan 27, 2016

ssalinas added a commit that referenced this pull request Jan 28, 2016

@ssalinas ssalinas merged commit 8f5fbf6 into master Jan 28, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details

@ssalinas ssalinas deleted the include_message_with_emails branch Jan 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment