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

RegistrationForm notification_sender_address settings #4992

Closed
vasantvohra opened this issue Jul 6, 2021 · 3 comments · Fixed by #4996
Closed

RegistrationForm notification_sender_address settings #4992

vasantvohra opened this issue Jul 6, 2021 · 3 comments · Fixed by #4996
Labels

Comments

@vasantvohra
Copy link
Contributor

Describe the bug
The notification_sender_address field description is ambiguous in regards to the RegistrationForm.sender_address.

To Reproduce
Steps to reproduce the behavior:

  1. Create a new event, enter the contact email in settings.
  2. Create a new registration form, keeping the notification sender address empty.
  3. Do some actions which trigger an automated notification email.
  4. The email received has a from address from the event's contact email not the config.no_reply email.

Screenshots

Event settings contact email

image

Registration form settings notification_sender_address

image

Email notification

image

Additional context
We fixed it in the UN plugin by just overriding the property and returning self.notification_sender_address, if it is None, the make_email automatically takes config.no_reply.
The UN doesn't want to use the contact emails for the automated notification emails.

@vasantvohra vasantvohra added the bug label Jul 6, 2021
@vasantvohra
Copy link
Contributor Author

@ThiefMaster Let me know, how you would like this to be resolved? I can open PR for this one too.
Thanks

@ThiefMaster
Copy link
Member

I see 3 possible solutions here:

  • dynamically set the description of the field to use the contact address
  • never use the contact address implicitly, ie always use noreply if there's no emailaddress set
  • add a new event-level setting to set the default sender address for all outgoing emails within that event

I think the second one is the best "easy" fix, since depending on the setup the indico server may not even be authorized to send emails using the contact address...

The third option would be a bit more complex but be explicit, and the default (not set) would be identical to option 2...

@vasantvohra
Copy link
Contributor Author

vasantvohra commented Jul 6, 2021

Perfect, I'll go with option 2 like

@property
    def sender_address(self):
        return self.notification_sender_address

If someone wants to use the event's contact email, they can copy-paste in the registration form settings as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants