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

Add configuration to enable mail previews #15970

Conversation

@lengarvey
Copy link
Contributor

lengarvey commented Jun 29, 2014

Adds config.action_mailer.preview_enabled

This allows mail previewing to be enabled easily in non-development
environments such as staging. The default is set to true for development
so no changes should be required to existing Rails applications.

The mail preview path can still be configured using the existing
config.action_mailer.preview_path configuration option.

Adding this prevents devs from having to do stuff like:

  config.action_mailer.preview_path ||= defined?(Rails.root) ? "#{Rails.root}/test/mailers/previews" : nil

  routes.append do
    get '/rails/mailers'         => "rails/mailers#index"
    get '/rails/mailers/*path'   => "rails/mailers#preview"
  end

in order to enable mail previewing on staging.

@chancancode
chancancode reviewed Jun 29, 2014
View changes
actionmailer/CHANGELOG.md Outdated
This config option can be used to enable the mail preview in environments
other than development (such as staging).

Defaults to `true` in staging and false elsewhere.

This comment has been minimized.

Copy link
@chancancode

chancancode Jun 29, 2014

Member

s/staging/development/?

@chancancode
chancancode reviewed Jun 29, 2014
View changes
actionmailer/CHANGELOG.md Outdated
@@ -1,3 +1,12 @@
* Add `config.mail_preview_enabled` configuration option.

This comment has been minimized.

Copy link
@chancancode

chancancode Jun 29, 2014

Member

config.action_mailer.enable_preview seems like a more natural name for this

This comment has been minimized.

Copy link
@lengarvey

lengarvey Jun 29, 2014

Author Contributor

I initially started with this but then this requires railties/lib/rails/application/finisher.rb be aware of the config.action_mailer. Which isn't the case all the time (at least it isn't in the test suite).

@chancancode
chancancode reviewed Jun 29, 2014
View changes
railties/CHANGELOG.md Outdated

Defaults to `true` in staging and false elsewhere.

*Leonard Garvey*

This comment has been minimized.

Copy link
@chancancode

chancancode Jun 29, 2014

Member

Not sure if we need this here, @rafaelfranca ?

@chancancode
chancancode reviewed Jun 29, 2014
View changes
railties/lib/rails/railtie/configuration.rb Outdated
@@ -48,6 +48,10 @@ def app_generators
@@app_generators
end

def mail_preview_enabled
@@options[:mail_preview_enabled] ||= Rails.env.development?
end

This comment has been minimized.

Copy link
@chancancode

chancancode Jun 29, 2014

Member

This should probably be in the Action Mailer's config rather than in the global one

This comment has been minimized.

Copy link
@chancancode

chancancode Jun 29, 2014

Member

If we are making this configurable, it seems more natural to add this to the generated development.rb and default this to false. However I'm unsure how to migrate that at this point

This comment has been minimized.

Copy link
@lengarvey

lengarvey Jun 29, 2014

Author Contributor

As soon as I pushed this last night I realised that I wanted to redo this without adding this extra stuff here. In terms of migration path it seems easier to just default this to true for development environments and false for others.

This comment has been minimized.

Copy link
@chancancode

chancancode Jun 29, 2014

Member

(re: this config being global, sorry I missed your explanation in the description. we should still figure out how to make this work without being global though. I don't know how to do this off the top of my head, but @pixeltrix might have ideas)

@chancancode chancancode added this to the 4.2.0 milestone Jun 29, 2014
@chancancode
Copy link
Member

chancancode commented Jun 29, 2014

@pixeltrix does the overall feature seems fine to you?

@lengarvey
Copy link
Contributor Author

lengarvey commented Jun 30, 2014

I reworked this so there aren't any more changes inside railties/lib except to remove the route appending from the finisher. This also lets me place the configuration item in config.action_mailer instead of adding a global configuration item.

@rafaelfranca
Copy link
Member

rafaelfranca commented Jun 30, 2014

We need to update the configuration guides to add this new config.

@lengarvey
Copy link
Contributor Author

lengarvey commented Jul 1, 2014

I've updated the configuration guides to include both this new config and the existing config.action_mailer.preview_path config which didn't seem to be covered.

@guilleiguaran
Copy link
Member

guilleiguaran commented Jul 1, 2014

Please squash your commits on a single one

Adds `config.action_mailer.preview_enabled`

This allows mail previewing to be enabled easily in non-development
environments such as staging. The default is set to true for development
so no changes should be required to existing Rails applications.

The mail preview path can still be configured using the existing
`config.action_mailer.preview_path` configuration option.

Adding this avoids devs from having to do stuff like:
https://gist.github.com/lengarvey/fa2c9bd6cdbeba96526a

Update actionmailer/CHANGELOG with new configuration.
Update configuring guide with new configuratation.
Add `config.action_mailer.preview_path` to configuring guide.
@rafaelfranca rafaelfranca merged commit 84ed7b8 into rails:master Jul 1, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
rafaelfranca added a commit that referenced this pull request Jul 1, 2014
…_mail_preview

Add configuration to enable mail previews
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.