Fix regression mailer preview path #1388
Conversation
This fixes the spec suite to expose issues with `ActionMailer` being configured incorrectly when previews are not available. This addresses several issues: - The incorrect configuration options are only exposed once `ActionMailer::Base` is loaded; this forces the class to load in the mailer initializer and at the end of our script - This adds specs specifically for the "development" environment which were missing before; it appeared that by not setting the environment it would default to "development" per the script, however, when the command shells out the `RAILS_ENV` is already set to "test" from rspec running. To be certain we cover the other possible edge cases we specifically add more tests for `ActionMailer` not being available, and for a custom configuration being set after `rspec-rails` is loaded. - Despite us asking Rails to turn off eager loading in our custom script, several files were still getting eager loaded; this explicitly clears the eager load paths to stop this - This fixes how the mailer script is shelled out in Ruby 1.8.7; now the environment variables are properly formatted and standard error is piped to standard out Exposes the regression issue in #1386
The `preview_path` feature of `ActionMailer` was added in Rails 4.1. Versions before that release are not able to handle that configuration. The regression issue occurred in part to the spec suite overlooking changes to the environment and how that affects the shelling out. Additionally, the previous changes noted that the configuration for a particular module always responds to a setting; even when it should not. However, while this logic was noted for `show_previews`, a few lines further down, that same logic was overlooked for `preview_path`. It appears the only available methods for detecting `ActionMailer`, and it's features, is tasting the top-level `config` in addition to checking the Rails version string.
|
|
||
| if defined?(ActionMailer) | ||
| # This will force the loading of ActionMailer settings | ||
| ActionMailer::Base.smtp_settings |
There was a problem hiding this comment.
Is the same on all versions of Rails?
Provide a more accurate description of the behavior we are avoiding and why the current behavior is used. [ci skip]
| else | ||
| Rails.application.configure do | ||
| config.action_mailer.default_url_options = { :host => ENV['DEFAULT_URL'] } | ||
| end |
There was a problem hiding this comment.
Just curious why this change is related to this PR? Shouldn't we always have been doing this?
There was a problem hiding this comment.
Yes, it should always have been there. Rails changed how they provided access to the configuration in 4.1. Incidentally the mailer specs / script never executed with a code path testing a default url option on Rails < 4.1. Which is also partially related to why this issue wasn't noticed sooner.
After fixing the mailer specs to run better coverage for Rails < 4.1, reproducing the issue, more things started failing as this code path was actually run. Thus, a fix is included here. The commit 9522842 has a long explanation of several related things which got fixed in the spec suite.
|
Thanks for getting on this promptly, @cupakromer. I'm happy to defer to @JonRowe's review since I don't really have the context for this. |
Fix regression mailer preview path
|
Looks all good to me |
|
👍 |
|
@cupakromer Are you planning to work up a 3-2-maintenance backport for this? |
|
@myronmarston done: #1389 Just waiting for the sanity check to go green. |
Back port pull request #1388 from rspec/fix-regression-mailer-preview-path
|
FYI Currently blocked on upgrading a Rails 4.0 app to use Ruby 2.2. This bug is preventing me from upgrading to rspec-rails 3.2, but issue #1273 (recommended solution: upgrade to 3.2) prevents the suite from running on rspec-rails 3.1 |
|
@chrisbloom7 -- I released rspec-rails 3.2.3 with a fix for this yesterday, so you should be able to upgrade now. |
|
@myronmarston Thanks, everything is green on Rails 4.0, rspec-rails 3.2.2, and Ruby 2.2.2 |
This fixes the regression where setting the
ActionMailerpreview path leaked into versions which does not support it (Rails before 4.1).The cause of the regression was a series of failures:
ActionMailer::Baseto load; thus the incorrect setting was not loaded exposing the issueENVwas already available, resulting in theRAILS_ENValready being set to "test". Thus the test was not explicitly testing the code as expected.config.action_mailer.show_previewsalways returningtrue, yet code slightly further down was relying onconfig.action_mailer.preview_pathbeing able to returnfalse(which it never will)This fixes each of these issues.
Based on minimal local testing it appears that
defined?(ActionMailer), while possibly loadingActionMailer, will not force the configuration to load. The configuration is only withActionMailer::Base.I'm weary 😩 about switching out the logic yet again regarding:
defined?(ActionMailer)vsconfig.respond_to?(:action_mailer). It's possible that on any further version the behavior of the config being loaded or the config changing to always respondtruecan occur. For now, it seems thatconfig.respond_to?(:action_mailer)is working and stable. Though I'm always open to further discussion.Resolve #1386
/cc @tagliala, @olance, @myronmarston