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

Adds optional reply_to email reporter config option #794

Merged
merged 7 commits into from
Feb 15, 2024

Conversation

trevorshannon
Copy link
Contributor

It can sometimes be useful to have a reply-to address different from the from address, especially when sending automated emails such as those generated by urlwatch. If email reports are being sent to a distribution list, a reply-to address of that list makes more sense than the (likely unmonitored) bot address.

This PR adds the reply_to config option for the email reporter. Note that the Reply-To header will now always be added to outgoing emails, but if the reply_to option is missing, that header will simply be populated with the same value as From

@Jamstah
Copy link
Contributor

Jamstah commented Feb 7, 2024

You should also update the CHANGELOG.md.

@trevorshannon
Copy link
Contributor Author

oops, thanks I always forget that.

Copy link
Owner

@thp thp left a comment

Choose a reason for hiding this comment

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

Thanks! Some feedback:

  • CHANGELOG.md could have reply_to in backticks (for formatting)
  • Should Reply-To only be set if it's non-empty and reply_to_email != from_email (because you do self.config.get('reply_to', self.config['from'])?
  • If you check non-empty, self.config.get('reply_to', self.config['from']) can become self.config.get('reply_to', '')
  • Add reply_to to the default e-mail config in lib/urlwatch/storage.py (empty string)

@trevorshannon
Copy link
Contributor Author

Thanks for the comments. I agree that adding the Reply-To header only if a user makes use of the reply_to param is safer because the currently released behavior will be completely unchanged when reply_to is unused.

I think that if the user enters in the same address for from and reply_to, that's still perfectly valid and we should add the Reply-To header to the email in that case (even though it should not make any difference to an email client's reply behavior). If you feel strongly that there should be an equality check between from and reply_to then I can add it.

I took care of the change log and the default email config.

Copy link
Owner

@thp thp left a comment

Choose a reason for hiding this comment

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

Very nice! And I agree that specifying a reply_to value should cause the header to be added, even if it's the same as the from value, good point.

@thp thp merged commit b9c6d88 into thp:master Feb 15, 2024
5 checks passed
@trevorshannon trevorshannon deleted the reply-to branch February 15, 2024 19:35
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.

3 participants