-
Notifications
You must be signed in to change notification settings - Fork 486
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
add installation brand name to subject of verify email message #3941 #3945
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes broken email branding issue for verification emails sent by the app upon user email change 👍 also consolidates another piece of mail into MailUtil.java
mailService.sendSystemEmail(toAddress, subject, messageBody); | ||
} | ||
} catch (Exception e) { | ||
logger.info("The root dataverse is not present. Don't send a notification to dataverseAdmin."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this warrant more than info? I'd at least put this at warn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh, as we discussed in person, anything higher than "fine" is shown. I think "info" is high enough.
Dataverse rootDataverse = dataverseService.findRootDataverse(); | ||
if (rootDataverse != null) { | ||
String rootDataverseName = rootDataverse.getName(); | ||
// FIXME: consider refactoring this into MailServiceBean.sendNotificationEmail. Then we might need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might need what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might need what?
Sorry, the developer died while writing that comment. 😉
I just finished off the comment in 5b75860. Thanks for noticing, @oscardssmith !
@@ -64,6 +64,8 @@ public static String getSubjectTextBasedOnNotification(UserNotification userNoti | |||
return BundleUtil.getStringFromBundle("notification.email.import.filesystem.subject", rootDvNameAsList); | |||
case CHECKSUMIMPORT: | |||
return BundleUtil.getStringFromBundle("notification.email.import.checksum.subject", rootDvNameAsList); | |||
case CONFIRMEMAIL: | |||
return BundleUtil.getStringFromBundle("notification.email.verifyEmail.subject", rootDvNameAsList); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I just mentioned to @sekmiller some day it might be nice to associate these bundle keys with the UserNotification object. Maybe you just call userNotification.getEmailSubject() or something. Out of scope for now but something to consider for the future.
Looks good. Passing to QA. As Phil noted there are some things worth thinking about when we do a more comprehensive rewrite of notifications. |
New Contributors
Welcome! New contributors should at least glance at CONTRIBUTING.md, especially the section on pull requests where we encourage you to reach out to other developers before you start coding. Also, please note that we measure code coverage and prefer you write unit tests. Pull requests can still be reviewed without tests or completion of the checklist outlined below. Thanks!
Related Issues
Pull Request Checklist