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

Extract mail configuration into a gem #4377

Closed
wants to merge 2 commits into from

Conversation

jhawthorn
Copy link
Contributor

I feel that much like paperclip settings (#4139), mail settings should be configured only in initializers using the normal rails methods. (using config.action_mailer.smtp_settings)

Here I've extracted the functionality into a gem
https://github.com/jhawthorn/spree_mail_settings

Also will partially fix #3833

@radar
Copy link
Contributor

radar commented Mar 3, 2014

I'm 👍 on this. Less code for us all to maintain.

@jhawthorn
Copy link
Contributor Author

🚢 Finished up the gem and added a changelog entry. This should be good to go.

@GeekOnCoffee
Copy link
Contributor

Love it, thanks for the work @jhawthorn!

Couple of potential thoughts:

  • Should this get forked into the Spree organization?
  • Can we put really harsh deprecation errors into place for this? My personal opinion is we should stop an app from starting if any of the removed configs are set, giving them good instructions on how to set it properly and clear up the error... There was a ton of confusion on the S3 settings that I'd like to avoid repeating

@FineLineAutomation
Copy link

+1 on the harsh deprecation errors.

@jhawthorn
Copy link
Contributor Author

Forking to spree (or possibly the to-be-formed spree-community) sounds like a good idea.

I can't think of a decent way for it to give deprecation warnings for people who had previously set this in the admin but not an initializer. My hope is that stating the change clearly in the release notes and migration guide will help. This was not done for the S3 settings change in spree 2.1

@JDutil
Copy link
Member

JDutil commented Mar 4, 2014

I think going to https://github.com/spree-community would be fine, which has been established. @jhawthorn your now added to it as well if you would like to get it setup there before this is merged.

@huoxito
Copy link
Member

huoxito commented Mar 4, 2014

Showing 22 changed files with 13 additions and 525 deletions.

Awesome!

@jhawthorn
Copy link
Contributor Author

:shipit: I believe this is good to go. Repo has been moved to spree-contrib/spree_mail_settings

@GeekOnCoffee GeekOnCoffee mentioned this pull request Mar 10, 2014
@radar
Copy link
Contributor

radar commented Mar 20, 2014

@jhawthorn Apologies on the delay. I've added this to my master branch now.

@radar radar closed this in bc3dee0 Mar 20, 2014
medius pushed a commit to godaddy/spree that referenced this pull request Oct 14, 2015
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.

Spree::Config should not be used during initialization
6 participants