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.

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

Fix time display emails #902

Merged
merged 6 commits into from Mar 10, 2016
Merged

Fix time display emails #902

merged 6 commits into from Mar 10, 2016

Conversation

@Calvinp
Copy link
Contributor

@Calvinp Calvinp commented Feb 17, 2016

In pause emails, when an expiration time was set:

  • The timestamp displayed in a confusing way (24h clock always UTC). Now the format and timezone are configurable, and it defaults to 12h clock (but still UTC).
  • A space was missing after the timestamp. This is fixed.
Calvinp added 2 commits Feb 16, 2016
…lt time format and time zone. Also the time zone is shown.
@@ -38,9 +38,13 @@

private final Optional<String> uiBaseUrl;
private final Optional<SMTPConfiguration> smtpConfiguration;
private String taskDatePattern;

This comment has been minimized.

@wsorenson

wsorenson Feb 17, 2016
Contributor

make these final

This comment has been minimized.

@Calvinp

Calvinp Feb 17, 2016
Author Contributor

Done

@@ -76,6 +77,8 @@
@Min(value = 1, message = "Must be positive and non-zero")
private int defaultBounceExpirationMinutes = 60;

private String defaultDatePattern = "MMM dd h:mm:ss a zzz";

This comment has been minimized.

@wsorenson

wsorenson Feb 17, 2016
Contributor

If these config properties are going to be used for mail only, they should indicate in the name.

This comment has been minimized.

@Calvinp

Calvinp Feb 17, 2016
Author Contributor

I'm not sure if they're going to be used for mail only or not - it's plausible that we would want to use them for something else. I don't know what that thing would be though.

This comment has been minimized.

@ssalinas

ssalinas Feb 17, 2016
Member

I think we should still make it more specific. At the moment, it is only used for mail, so let's make the name reflect that. Also it isn't really a default that we are falling back on, it is just a setting that will be used by everything in the mailer. So maybe just calling it mailerDatePattern ?


@Inject
public MailTemplateHelpers(SandboxManager sandboxManager, SingularityConfiguration singularityConfiguration) {
taskDatePattern = singularityConfiguration.getDefaultDatePattern();
timeZone = singularityConfiguration.getTimeZone();

This comment has been minimized.

@ssalinas

ssalinas Feb 17, 2016
Member

lets follow the format of the ones below with this.x = ...

@@ -185,6 +188,8 @@

private long startNewReconcileEverySeconds = TimeUnit.MINUTES.toSeconds(10);

private TimeZone timeZone = TimeZone.getTimeZone("UTC");

This comment has been minimized.

@ssalinas

ssalinas Feb 17, 2016
Member

Will this initialize correctly from a String? (i.e. reading it from a yaml field set to 'EST' or 'UTC') It very well might, but something we should probably test to be sure

This comment has been minimized.

@Calvinp

Calvinp Feb 17, 2016
Author Contributor

Yes, it is still able to start up.
I renamed the field to mailerTimeZone, and putting a top level line 'mailerTimeZone: UTC' into the yaml doesn't break the build.

@@ -76,6 +77,8 @@
@Min(value = 1, message = "Must be positive and non-zero")
private int defaultBounceExpirationMinutes = 60;

private String mailerDatePattern = "MMM dd h:mm:ss a zzz";

This comment has been minimized.

@tpetr

tpetr Feb 18, 2016
Member

we should move these mailer* config fields into SMTPConfiguration

@tpetr tpetr modified the milestone: 0.4.12 Feb 23, 2016

import org.apache.commons.lang3.text.WordUtils;
import org.apache.commons.lang3.time.DateFormatUtils;
import org.apache.commons.lang3.tuple.Pair;

This comment has been minimized.

@tpetr

tpetr Feb 26, 2016
Member

i dont think this import is necessary

@ssalinas ssalinas added the hs_stable label Mar 7, 2016
ssalinas added a commit that referenced this pull request Mar 10, 2016
Fix time display emails
@ssalinas ssalinas merged commit 5b8524c into master Mar 10, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@ssalinas ssalinas deleted the fix_time_display_emails branch Mar 10, 2016
@ssalinas ssalinas removed hs_qa labels Mar 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.