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

Contact page - customizable email to per tenant #1622

Merged
merged 12 commits into from
Jun 22, 2020
Merged

Conversation

orangewolf
Copy link
Member

@orangewolf orangewolf commented Apr 18, 2020

Fixes #1207 #517

Split settings of default contact page email "from" and email "to". There is a Hyrax.config.contact_email as well, but we really need both a "from" and a "to" email address and we want to be able to set both from ENV.

Override Hyrax default so that the from address isn't whatever is put in the form, as many MTAs do now allow non-specified from addresses. Instead include the contact page email in the reply-to and the subject.

Allow changing the "to" email address on a per tenant basis. Provides an admin form for a given tenant to set where they want contact emails to be sent to.

@samvera/hyrax-code-reviewers

Please note contribution for this PR comes from Hykuup.com and NYC's DORIS project

@no-reply
Copy link

are there changes from this we'd like to port back to Hyrax, especially to help avoid the copied-code situation?

i think maybe opening up ContactFormController to dependency injection for its models would be a good start? ContactFormController.model_class = MyCustomContactForm would at least let you inherit the existing model and extend it, instead of copying it in?

no-reply pushed a commit to samvera/hyrax that referenced this pull request Jun 17, 2020
since the `ContactForm` model doesn't use `ActiveRecord`, it can be easily
subclassed and customized. allowing the controller to use a configured/injected
model helps avoid the need for hard monkeypatches in downstream apps; see,
e.g. samvera/hyku#1622
@orangewolf
Copy link
Member Author

@no-reply yes, seems like a good call going forward. I'm tempted to get this merged and open a ticket for it as part of the broader upgrade task set.

@no-reply
Copy link

oh yeah. i definitely wouldn't delay this on upstream improvements. just wanting to set you up for a cleaner solution down the road.

no-reply pushed a commit to samvera/hyrax that referenced this pull request Jun 17, 2020
since the `ContactForm` model doesn't use `ActiveRecord`, it can be easily
subclassed and customized. allowing the controller to use a configured/injected
model helps avoid the need for hard monkeypatches in downstream apps; see,
e.g. samvera/hyku#1622
no-reply pushed a commit to samvera/hyrax that referenced this pull request Jun 17, 2020
since the `ContactForm` model doesn't use `ActiveRecord`, it can be easily
subclassed and customized. allowing the controller to use a configured/injected
model helps avoid the need for hard monkeypatches in downstream apps; see,
e.g. samvera/hyku#1622
@jeremyf
Copy link
Contributor

jeremyf commented Jun 22, 2020

Approved, but I don't want to merge as I won't have space to further help out.

@cjcolvar cjcolvar merged commit 11b5512 into master Jun 22, 2020
@cjcolvar cjcolvar deleted the contact_mailer branch June 22, 2020 19:23
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.

Allow tenant admins to set contact form email address
5 participants