Support multiple preview paths for mailers #31595

fatkodima · 2017-12-29T22:32:11Z

It should be great to see mailer previews from engines. This PR allows to do this.

Originated from a bit outdated #16836.
Notable changes compared to that PR:

config.action_mailer.preview_path deprecating
Engine initializer to automatically add mailer preview paths to the lookup path for mailer previews for the application

Thanks in advance.

rails-bot · 2017-12-29T22:32:13Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kamipo (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

georgeclaghorn

❤️

georgeclaghorn · 2017-12-30T01:01:53Z

actionmailer/lib/action_mailer/preview.rb

+        Please use preview_paths instead.
+      MSG
+      self.class.preview_paths.first
+    end


This instance method is currently private, so I think it can be removed without deprecation.

Looks like instance reader is public

rails/actionmailer/lib/action_mailer/preview.rb

Line 14 in bce675e

mattr_accessor :preview_path, instance_writer: false

You‘re right, sorry. 😅

georgeclaghorn · 2017-12-30T01:07:10Z

railties/test/application/mailer_previews_test.rb

+      assert_match '<li><a href="/rails/mailers/notifier/foo">foo</a></li>', last_response.body
+      assert_match '<h3><a href="/rails/mailers/confirm">Confirm</a></h3>', last_response.body
+      assert_match '<li><a href="/rails/mailers/confirm/foo">foo</a></li>', last_response.body
+    end


This test can be combined with the existing one for loading previews from custom preview paths above.

fatkodima · 2018-01-15T23:44:27Z

@georgeclaghorn, thanks for review. Fixed your notes, squashed and rebased.
Anything else?

MadLittleMods · 2018-03-22T04:08:30Z

Friendly ping to revive 👋

We have a use case for this to support some GitLab Enterprise Edition specific email previews which are namespaced under the ee/ directory.

config.action_mailer.preview_paths = [
  'spec/mailers/previews',
  'ee/spec/mailers/previews'
]

Here is the current config, https://gitlab.com/gitlab-org/gitlab-ce/blob/b17457a7f7fb390c829ff5feed7e4635bf2b4521/config/environments/development.rb#L42

rails-bot · 2019-12-18T07:52:34Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

fatkodima · 2019-12-25T09:36:34Z

@georgeclaghorn Wdyt about moving this forward?

simi · 2019-12-25T13:47:01Z

I was just looking for the implementation details. Since it pass preview_path to Dir[], isn't is possible to pass multiple values in matching literal `{a,b}?

ActionMailer::Base.preview_path = "{/etc,/tmp}"

Anyway it is just non-intuitive workaround. Introducing action_mailer.preview_paths seems more intuitive.

simi · 2019-12-25T13:47:43Z

actionmailer/lib/action_mailer/preview.rb

@@ -25,7 +25,31 @@ module Previews #:nodoc:
      mattr_accessor :preview_interceptors, instance_writer: false, default: [ActionMailer::InlinePreviewInterceptor]
    end

+    def preview_path
+      ActiveSupport::Deprecation.warn(<<-MSG.squish)
+        Using preview_path option is deprecated and will be removed in Rails 6.0.


⌛ Rails 6.1 now

fatkodima · 2020-05-20T23:16:10Z

@eugeneius please, review 🙏

actionmailer/lib/action_mailer/railtie.rb

guides/source/action_mailer_basics.md

railties/test/application/mailer_previews_test.rb

eugeneius · 2020-05-26T12:17:15Z

Engine initializer to automatically add mailer preview paths to the lookup path for mailer previews for the application

Did this part not work out? It sounds useful, since that's the main motivating use case for supporting multiple preview paths.

kaspth · 2020-05-26T12:18:48Z

Yeah that, and engines auto include controllers/routes etc in the main app. So this would fit right with that.

fatkodima · 2020-05-26T16:48:34Z

@eugeneius thanks! Updated.

Re:

Engine initializer to automatically add mailer preview paths to the lookup path for mailer previews for the application

I initially implemented that. Then something went wrong and I decided to remove this and allow for engines to manually add preview paths. And now everything went right ( 😄 )and I added back engine initializer (and tests for it).

kennyadsl · 2021-01-22T09:51:11Z

This would be super useful for Rails Engines that add mailer previews in the host application as we do in Solidus. We worked around the issue with this PR but would be great to have support for that in Rails directly. Any chance to move this forward?

p8 · 2021-07-06T18:39:01Z

@fatkodima could you fix the merge conflicts and update the deprecations from 6.2 to 7.1? 🙇

fatkodima · 2021-07-06T20:04:22Z

@p8 Rebased and changed 6.2 to 7.1.

p8 · 2021-07-06T20:05:23Z

Thanks @fatkodima !

fxn · 2023-08-25T07:32:46Z

I believe this needs a revision, it introduces a regression.

Since config.paths is cached as soon as accessed, config.autoload_paths and friends are going to have whatever paths["test/mailers/previews"] had at the time the object was memoized. Therefore, concatenating to paths["test/mailers/previews"] as it is done by the initializer does not necessarily add them to the autoload paths. If users configured config.autoload_paths, for example, mailer previews won't be found.

I would consider removing the newly introduced paths["test/mailers/previews"] and having the initializer push things to the autoload paths simply by concatenating to config.autoload_paths by hand, which is a good interface between railties and engines or applications. Please, see #49032, which needed a workaround for this.

fxn · 2023-08-25T09:59:11Z

Let me illustrate the regression.

In Rails 7.0, if you have a directory lib/mailer_previews and this in config/application.rb:

config.autoload_paths
config.action_mailer.preview_path = "#{Rails.root}/lib/mailer_previews"

then

% bin/rails r 'pp ActiveSupport::Dependencies.autoload_paths.grep(/lib/)'
["/Users/fxn/tmp/del_mailer_previews/lib/mailer_previews"]

With this patch, lib/mailer_previews is no longer in the autoload paths.

Besides being a regression, conceptually this does not look good. It wouldn't be right even if the feature was new.

(EDIT: In main the directory appears due to the workaround introduced in #49032, but that should be reverted, or updated after a revision is in place.)

fatkodima · 2023-08-25T10:34:19Z

Please, feel free to provide a PR, if you will, since you better know how to implement it properly. 🙏

fxn · 2023-08-25T11:12:30Z

@fatkodima let me understand your patch better. The initializer

initializer "action_mailer.set_autoload_paths", before: :set_autoload_paths do |app|
  options = app.config.action_mailer
  app.config.paths["test/mailers/previews"].concat(options.preview_paths)
end

adds preview paths from the application/global configuration point app.config.action_mailer.preview_paths. In what sense does this patch allow to configure these per engine too?

fatkodima · 2023-08-25T12:04:20Z

These changes https://github.com/rails/rails/pull/31595/files#diff-c72ea90e9c6b7c60285006ea4c34548ae9257dffcda0c1b2f41fffd75a14363b are expected to do that.

fxn · 2023-08-25T12:24:04Z

So engines do not have configuration API for it, right? (I don't consider config.paths["..."] to be public API).

fatkodima · 2023-08-25T13:00:55Z

Right.

jon-sully · 2023-10-18T20:25:09Z

For those coming here because their Previews no longer got autoloaded after upgrading to Rails 7.1 and that use RSpec (so your previews are under /spec... not /test...), RSpec isn't updated to inject the preview_paths correctly quite yet:

rspec/rspec-rails#2703

In the meantime,

  config.action_mailer.preview_paths << "#{Rails.root}/spec/mailers/previews"

rails-bot assigned kamipo Dec 29, 2017

georgeclaghorn suggested changes Dec 30, 2017

View reviewed changes

fatkodima force-pushed the mailer-preview_paths branch 2 times, most recently from 833d4c9 to 6db2c09 Compare January 9, 2018 12:09

fatkodima force-pushed the mailer-preview_paths branch from 6db2c09 to d2332af Compare January 15, 2018 23:07

fatkodima force-pushed the mailer-preview_paths branch from d2332af to a74b349 Compare January 20, 2018 00:26

gmcgibbon added actionmailer railties docs labels Sep 30, 2018

rails-bot bot added the stale label Dec 18, 2019

rails-bot bot closed this Dec 25, 2019

simi reviewed Dec 25, 2019

View reviewed changes

fatkodima mentioned this pull request May 19, 2020

Unify raise_on_missing_translations for views and controllers #31571

Merged

eugeneius reopened this May 20, 2020

rails-bot bot removed the stale label May 20, 2020

fatkodima force-pushed the mailer-preview_paths branch 2 times, most recently from 511ffc1 to c3da463 Compare May 20, 2020 23:03

eugeneius reviewed May 26, 2020

View reviewed changes

fatkodima force-pushed the mailer-preview_paths branch 2 times, most recently from d8f34be to 8f8c78c Compare May 26, 2020 16:29

georgeclaghorn reopened this Nov 24, 2020

rails-bot bot removed the stale label Nov 24, 2020

georgeclaghorn added the pinned label Nov 24, 2020

Base automatically changed from master to main January 14, 2021 17:00

aldesantis mentioned this pull request Jan 22, 2021

Fix ActionMailer preview loading solidusio/solidus#3901

Merged

5 tasks

p8 mentioned this pull request Jul 6, 2021

Allow set a Array of Path on Preview Mailers Path #42710

Closed

fatkodima force-pushed the mailer-preview_paths branch from 8fa0a8c to 4671550 Compare July 6, 2021 19:51

rafaelfranca added the ready PRs ready to merge label Jul 6, 2021

zzak linked an issue Jul 9, 2021 that may be closed by this pull request

Allow preview_path accept a array, not only a string #42701

Closed

EduardoGHdez mentioned this pull request Feb 28, 2022

Mailers preview don't work with Engines decidim/decidim#8934

Closed

fatkodima mentioned this pull request Mar 27, 2022

Add new Rails/MailerPreviews cop rubocop/rubocop-rails#686

Open

fatkodima force-pushed the mailer-preview_paths branch from 4671550 to fd7e999 Compare June 30, 2022 11:22

Support multiple preview paths for mailers

64ad045

fatkodima force-pushed the mailer-preview_paths branch 2 times, most recently from fb22c79 to 64ad045 Compare June 30, 2022 12:10

rafaelfranca merged commit 95066e5 into rails:main Aug 9, 2022

fatkodima deleted the mailer-preview_paths branch August 25, 2023 13:01

alexevanczuk mentioned this pull request Oct 21, 2023

Fix deprecated use of config.action_mailer.preview_path = in favor of ...preview_paths << to support Rails 7.1 upgrade rspec/rspec-rails#2705

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple preview paths for mailers #31595

Support multiple preview paths for mailers #31595

fatkodima commented Dec 29, 2017

rails-bot commented Dec 29, 2017

georgeclaghorn left a comment

georgeclaghorn Dec 30, 2017

fatkodima Jan 3, 2018

georgeclaghorn Jan 3, 2018

georgeclaghorn Dec 30, 2017

fatkodima commented Jan 15, 2018

MadLittleMods commented Mar 22, 2018

rails-bot bot commented Dec 18, 2019

fatkodima commented Dec 25, 2019

simi commented Dec 25, 2019

simi Dec 25, 2019

fatkodima commented May 20, 2020

eugeneius commented May 26, 2020

kaspth commented May 26, 2020

fatkodima commented May 26, 2020

kennyadsl commented Jan 22, 2021

p8 commented Jul 6, 2021

fatkodima commented Jul 6, 2021

p8 commented Jul 6, 2021

fxn commented Aug 25, 2023 •

edited

fxn commented Aug 25, 2023 •

edited

fatkodima commented Aug 25, 2023

fxn commented Aug 25, 2023

fatkodima commented Aug 25, 2023

fxn commented Aug 25, 2023

fatkodima commented Aug 25, 2023

jon-sully commented Oct 18, 2023

Support multiple preview paths for mailers #31595

Support multiple preview paths for mailers #31595

Conversation

fatkodima commented Dec 29, 2017

rails-bot commented Dec 29, 2017

georgeclaghorn left a comment

Choose a reason for hiding this comment

georgeclaghorn Dec 30, 2017

Choose a reason for hiding this comment

fatkodima Jan 3, 2018

Choose a reason for hiding this comment

georgeclaghorn Jan 3, 2018

Choose a reason for hiding this comment

georgeclaghorn Dec 30, 2017

Choose a reason for hiding this comment

fatkodima commented Jan 15, 2018

MadLittleMods commented Mar 22, 2018

rails-bot bot commented Dec 18, 2019

fatkodima commented Dec 25, 2019

simi commented Dec 25, 2019

simi Dec 25, 2019

Choose a reason for hiding this comment

fatkodima commented May 20, 2020

eugeneius commented May 26, 2020

kaspth commented May 26, 2020

fatkodima commented May 26, 2020

kennyadsl commented Jan 22, 2021

p8 commented Jul 6, 2021

fatkodima commented Jul 6, 2021

p8 commented Jul 6, 2021

fxn commented Aug 25, 2023 • edited

fxn commented Aug 25, 2023 • edited

fatkodima commented Aug 25, 2023

fxn commented Aug 25, 2023

fatkodima commented Aug 25, 2023

fxn commented Aug 25, 2023

fatkodima commented Aug 25, 2023

jon-sully commented Oct 18, 2023

fxn commented Aug 25, 2023 •

edited

fxn commented Aug 25, 2023 •

edited