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

Allow for customizing the name of the sender to put on notification emails #393

Merged
merged 1 commit into from
Nov 24, 2020

Conversation

hispanic
Copy link

This addresses issue #392 - "Provide support for customizing the name of the sender on notification emails".

The changes for this are minimal.

  • I added a new email.fromName property to the JSON config schema. The default value is "Staticman".
  • I added a reference to this new property in Notification.js

While in there, I corrected the doc for the email.fromAddress property.

The relevant unit test has been updated. I performed integration testing on Heroku.

The changes here overlap with changes made for PR #391. If that PR is merged-in before this one and that makes this merge burdensome, please LMK and I can re-create this PR. If/when this is merged-in, I'll be happy to submit a PR to update the documentation site.

Thank you for Staticman!

…mails, something other than "Staticman".

- Added email.fromName to the JSON config schema.
- Updated config schema doc to remove reference to non-existent notifications.fromAddress parameter in the site config.
- Updated unit test.
Copy link
Collaborator

@alexwaibel alexwaibel left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@alexwaibel alexwaibel merged commit 029e55c into eduardoboucas:dev Nov 24, 2020
@hispanic
Copy link
Author

hispanic commented Dec 1, 2020

Nice! However, given our other discussion regarding githubWebhookSecret and gitlabWebhookSecret as part of PR #388 (comment), I'm a little surprised that you aren't of the mind that fromName (and, for that matter, fromAddress) belongs in the YAML siteConfig, as well. Of course, the same could be said for the akismet and analytics properties. Thoughts? Regardless, happy to contribute.

@alexwaibel
Copy link
Collaborator

For the sake of this PR it's fine to add fromName to the config JSON for consistency since that's also where fromAddress is set.

I agree we should consider moving these into the site config YAML at some point though. I think there's already an issue open regarding this (at least for the akismet properties).

@hispanic hispanic deleted the notif-sender-name branch December 5, 2020 17:38
JonBoyleCoding pushed a commit to JonBoyleCoding/staticman that referenced this pull request Feb 6, 2023
…mails, something other than "Staticman". (eduardoboucas#393)

- Added email.fromName to the JSON config schema.
- Updated config schema doc to remove reference to non-existent notifications.fromAddress parameter in the site config.
- Updated unit test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants