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: Automatic notifications to consider notification sender address #4996

Merged
merged 1 commit into from Jul 7, 2021

Conversation

vasantvohra
Copy link
Contributor

closes #4992

  • Let me know if the commit message needs to be changed.
  • If there's a need to add changes.rst
  • As part of this minor PR,
  • I upgrade my local to v3 🚀

image

@ThiefMaster
Copy link
Member

Maybe removing this logic from build_default_email_template (abstract email notifications) would be a good idea as well?

@vasantvohra
Copy link
Contributor Author

I see reply-to address there, so should I replace

event.contact_emails[0] if event.contact_emails else ''

with

config.NO_REPLY_EMAIL

?

@ThiefMaster
Copy link
Member

ThiefMaster commented Jul 7, 2021

no, just set it to '' all the time; that should result in indico not setting a reply-to address at all (noreply is already used in From)

@ThiefMaster
Copy link
Member

ThiefMaster commented Jul 7, 2021

A changelog entry would be useful for this (bugfixes section i guess, that's the most suitable one IMO):

- No longer silently fall back to the first event contact email address when sending
  registration emails where no explicit sender address has been configured (:issue:`4992`,
  :pr:`4996`, thanks :user:`vasantvohra`)

for the abstract notification templates I don't think it's needed since it only affects newly created ones

CHANGES.rst Outdated Show resolved Hide resolved
@ThiefMaster ThiefMaster enabled auto-merge (squash) July 7, 2021 13:23
@ThiefMaster ThiefMaster merged commit 0e57ff0 into indico:master Jul 7, 2021
@vasantvohra vasantvohra deleted the fix/sender_address branch July 7, 2021 14:11
vasantvohra added a commit to UNOG-Indico/indico-core that referenced this pull request Jul 7, 2021
This wasn't documented anywhere and rather confusing behavior.
vasantvohra added a commit to UNOG-Indico/indico-core that referenced this pull request Jul 9, 2021
This wasn't documented anywhere and rather confusing behavior.
vasantvohra added a commit to UNOG-Indico/indico-core that referenced this pull request Jul 9, 2021
This wasn't documented anywhere and rather confusing behavior.
OmeGak pushed a commit to UNOG-Indico/indico-core that referenced this pull request Jul 12, 2021
This wasn't documented anywhere and rather confusing behavior.
OmeGak pushed a commit to UNOG-Indico/indico-core that referenced this pull request Jul 13, 2021
This wasn't documented anywhere and rather confusing behavior.
vasantvohra added a commit to UNOG-Indico/indico-core that referenced this pull request Jul 13, 2021
This wasn't documented anywhere and rather confusing behavior.
pferreir pushed a commit to pferreir/indico that referenced this pull request Oct 12, 2021
This wasn't documented anywhere and rather confusing behavior.
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.

RegistrationForm notification_sender_address settings
2 participants