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

Mailer preview request model #20646

Closed

Conversation

@equivalent
Copy link
Contributor

commented Jun 20, 2015

Alternative to my other PR #20608 (please have a look first)

passing request model instead of params

feature (in compare to my other RP) gives developer the opportunity to extend Rails::MailersController and override the request_model => more protecting Preview class

# config/initializer/mailer_preview.rb

# upon rails/mailers/my_mailer/good_news?user_id:123&client_id:345
module OverrideRequestModel
  def self.prepended(base)
     base.before_action :authenticate! 
  end

  def request_model
     OpenStruct.new(user: current_user, client_id: params[:client_id])
  end

  def current_user
    User.find session[:current_user_id]
  end

  def authenticate!
     raise "not authenticated" unless current_user.admin?
  end
end

Rails::MailersController.prepend OverrideRequestModel

This way In MyMailerPreview I can

class MyMailerPreview < ActionMailer::Preview 

  def good_news
    do_some_stuff_with_user_object
    MyMailer.good_news(client)
  end

  private
     def do_some_stuff_with_user_object
        request_model.user.do_some_stuff
     end

     def client
        Client.find(request_model.client_id) 
     end
end

@equivalent equivalent force-pushed the equivalent:mailer-preview-request-model branch from 29732f8 to a5b0e83 Jun 22, 2015

@equivalent equivalent closed this Jun 22, 2015

@equivalent

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2015

re-triggering travis ci

@equivalent equivalent reopened this Jun 22, 2015

@tom-lord

This comment has been minimized.

Copy link

commented Jun 22, 2015

+1

1 similar comment
@zdfs

This comment has been minimized.

Copy link

commented Jun 25, 2015

+1

@jboonstra

This comment has been minimized.

Copy link

commented Jul 23, 2015

This would be handy for me when developing email with various iterations (as I'm doing right now). I believe the alternatives are to create a separate previewer method for each possible variant, or to continually tweak the parameters in the source code (and remember not to commit them).

@equivalent

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2015

I've manage to create a production mail preview for every individual user / status change / language with this (people in my company can edit mail content via a WYSIWYG editor and the interpolation for it and after that they want to review before they are sent)

I definitely see why original developer didn't introduced this from beginning (trust your app code and tests + this suppose to be just for development purpose) but by allowing passing params or request model you can use this to do lot of cool production stuff too.

@hungryzi

This comment has been minimized.

Copy link

commented Aug 17, 2015

+1

@pixeltrix

This comment has been minimized.

Copy link
Member

commented Aug 17, 2015

@equivalent you shouldn't use this in production - that's not intended use case for this feature. You can call your mailers without a deliver-* method to get a mail object and process it however you need it to be done.

I'm still not convinced by this or #20608 - are there enough variants of emails to warrant this? You can just create multiple preview methods if you need to show different variants and this is more discoverable than manually tweaking params.

Can anyone suggest a use case that's a little more detailed?

@equivalent

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2015

... you shouldn't use this in production ...

yep you shouldn't. The way how the original intention was proposed, this was just development tool. Yet you pass authentication object(Pundit) and params to it and "Yes we can" :) . I'm planing to write an article on full explanation how this is possible as son as this is actually merged. If you want I can write a scrap article in next few days and you will see that it's no security problem at all

I'm not trying to make this a built in "Rails production ready" solution for everyone. Just enabling more flexibility for those who know what they are doing.

I'm still not convinced by this or #20608 - are there enough variants of emails to warrant this?

we have 6 clients, each has 12 tempates and some have multiple languages (betwen 1 - 3 languages) so around 50 combination ?. I'm working on another code change that will add 6 more templates so 25 more combinations. The problem is that we do content interpolation (like you have "20 days left, or logo url)

Paranoid content managers gave me a task that they want to see every possible email (even if theoretically it's the same thing) .

I see your point of scepticism on whether this feature is needed, but in cases where you have dynamic emails templates and you need support all possible previews, they are just way too much work for developer to generate every preview on every content change.

@equivalent

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2015

look the main point is that originally previews allows to do previews only on specific object you pass in the code and it's awesome feature.

But if you want to construct the object with controller params there is no way to do that. Even if we ignore the production bit I was talking about, I actually read people complaining they wanted to do this kind of stuff in development and they couldn't.

rafaelfranca and others added some commits Sep 7, 2015

Merge pull request #21534 from Vratislav/clarify-custom-config-guide
[Rails Guides] Clarify custom code configuration [ci skip]
Memoized reflections accessor
Its value never change since associations are defined at class load time
so there is no need to build the hash everytime the method is called.

Before this change:

    Calculating -------------------------------------
             reflections   804.000  i/100ms
    -------------------------------------------------
             reflections      8.213k (±26.2%) i/s -     36.180k

After this change:

    Calculating -------------------------------------
             reflections    24.548k i/100ms
    -------------------------------------------------
             reflections      1.591M (±25.7%) i/s -      7.364M

Benchmark script:

    require 'bundler/setup'

    require 'active_record'
    require 'benchmark/ips'

    ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
    ActiveRecord::Migration.verbose = false

    ActiveRecord::Schema.define do
      100.times do |i|
        create_table "users#{i}", force: true
      end

      create_table :cars, force: true do |t|
        100.times do |i|
          t.references "users#{i}"
        end
      end
    end

    class Car < ActiveRecord::Base
      100.times do |i|
        belongs_to "users#{i}".to_sym
      end
    end

    Benchmark.ips do |x|
      x.report('reflections') { Car.reflections }
    end
Merge pull request #21502 from bernerdschaefer/bs-polymorphic-url_for…
…-dups-arguments

`url_for` does not modify polymorphic options
@jrochkind

This comment has been minimized.

Copy link
Contributor

commented on 2dfff4d Sep 9, 2015

The reason this is causing problems for current users is because rails new something --database=mysql generates a local Gemfile with an unconstrained gem 'mysql2'. But then mysql2_adapter.rb, as above, was loading the mysql2 gem with gem 'mysql2', '~> 0.3.13', which means only 0.3.x greater than 0.3.13.

Now that mysql2 0.4.0 is out, for a bundle install/update on the Gemfile from a generated Rails app, bundler will install mysql2 0.4.0, because of the unconstrained line in a newly generated Gemfile. But mysql2_adapter will refuse to use it, giving a somewhat confusing (although not wrong) error message.

You've updated the spec in mysql2_adapter above to gem 'mysql2', '>= 0.3.13', '< 0.5' -- but the basic problem remains lying in wait for whenever a mysql2 0.5.0 is released. No?

I think the same problem will occur when mysql 0.5.0 is released in the future, which it surely will be at some point -- rails new something --database=mysql will add an unconstrained mysql to the app Gemfile, which will allow 0.5.0, but mysql2_adapter will refuse to use it. So this is not a great fix.

I think the right solution is that the spec generated by rails new --database=mysql needs to match the spec in the gem line in mysql2_adapter. Either they both need to be unconstrained (like the generated Gemfile is now), or they both need to be constrained, and matching each other (like the gem line in mysql2_adapter is now).

Am I wrong?

This comment has been minimized.

Copy link
Member Author

replied Sep 9, 2015

You're absolutely right. No optional version requirements in RubyGems! Otherwise we'd put the constraint on the activerecord gem and this'd just work.

Please do take a look at adding gem requirements to the generator! It supports gem names only currently, but should definitely allow for more. See gem_for_database in Railties: https://github.com/rails/rails/blob/master/railties/lib/rails/generators/app_base.rb#L163-L167. Provide version requirements also and it'll Just Work 😁

This comment has been minimized.

Copy link
Member

replied Sep 9, 2015

We only enforce a minimum version for pg; maybe we should do likewise for mysql2?

This comment has been minimized.

Copy link
Member Author

replied Sep 9, 2015

Tough when a point release actually breaks stuff, though. We could go ~> 0.4 as a middle ground, cover up to 1.0.

This comment has been minimized.

Copy link
Contributor

replied Sep 9, 2015

In this case, the point release did NOT break stuff, apparently. According to this fix, mysql2 0.4.0 works fine -- it just wasn't allowed with the 'optional dependency version specification' in the mysql2_adapter.

I think changing the entire way 'optional dependencies' work in AR is probably out of scope here -- apparently AR doesn't want to express explicit dependencies on underlying database gems for each of it's possible databases, or this would indeed just be in the gemspec.

So I have no idea what the right answer is, except that one or the other, remove the upper bound from the 'optional dependency' in mysql2_adapter, OR change the generator to generate the same version specification. I guess I'd lean towards the first, because a generated version spec in your Gemfile is going to be something a lot of people never change, and get locked into old versions without realizing it.

Also out of scope here, probably, is discussing why the heck the mysql2 gem is still on a pre-1.0 version number -- when it's widely used in production -- instead of being on post-1.0 and committing to semver. If it did that, it would at least make things less confusing.

This comment has been minimized.

Copy link
Member Author

replied Sep 9, 2015

  1. Both AR and apps need to limit mysql2 to their known-interoperable versions.
  2. AR can't depend on mysql2, so AR needs an optional dep on mysql2 with . The only possibility is runtime gem activation.
  3. Apps does directly depend on mysql2, so it can state the dep and requirements in Gemfile.

I'd conclude that the best route is to write version deps to Gemfile when apps are generated. Thereafter, app authors are in charge of upgrades, as with any other library.

Please do investigate 😁 See https://github.com/rails/rails/blob/master/railties/lib/rails/generators/app_base.rb#L163-L167 — add current version requirements to the list, test, and you're done.

This comment has been minimized.

Copy link
Contributor

replied Sep 9, 2015

What will motivate or instruct future committers making changes to either of these two places, to make sure the two places in code that have a version spec always match?

Especially since mysql2 does not use semver so far as we know, and we have no reason in particular to think 0.5.0 will be backwards incompatible with 0.4.0 (0.4.0 was apparently not backwards incompat with 0.3.0, so we have some reason to think 0.5.0 will be similar...)

...why not just remove the upwards bound on the optional dependency in mysql2_adapter?

There aren't any great solutions here. Anyway, sorry I can not commit to making the PR to copy-paste the version spec to the generator too.

This comment has been minimized.

Copy link
Member Author

replied Sep 9, 2015

For anyone else interested in this little morsel, you can start here. Take a look at how the app generator decides which gem to use for which db: https://github.com/rails/rails/blob/master/railties/lib/rails/generators/app_base.rb#L163-L167

This comment has been minimized.

Copy link

replied Sep 14, 2015

I hope you guys realize that every n00b trying to use Rails with MySQL right now is flailing around in complete confusion over this. I know it's not really Rails fault that the mysql2 gem updated it's version, but when I searched for this error, the first 3 Google entries were all completely confused beginners, who don't understand why their "just generated" Rails apps don't work.

amatsuda and others added some commits Sep 10, 2015

Explicitly require AR/attribute where using it
autoloading this could possibly cause some weird race condition
when calling an AR::Attribute's singleton method on a threaded server.
Fix displaying mailer previews on non local requests.
When config `action_mailer.show_previews` is set, previews are displayed
regardless of local request check.

(cherry picked from commit 472358d)
When used by ActionMailer, ActionView should automatically use the co…
…rrect MIME type just as it does when used by ActionDispatch #11157

(cherry picked from commit efadc69)
Merge pull request #21676 from amitsuroliya/correct_result
Corrected numeric conversions output [ci skip]
fix application_controller require_dependency path generated by the s…
…caffold generator

This is follow up to #6643.
In #6643, the controller generator only had been fixed, in this commit to fix the scaffold generator.

(cherry picked from commit 2e81469)

@equivalent equivalent closed this Sep 21, 2015

@equivalent equivalent deleted the equivalent:mailer-preview-request-model branch Sep 21, 2015

@equivalent

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2015

I'm sorry everyone I've manage to screw up this PR when re-basing latest branch Upstream :(

here is the new PR #21702 for the same thing

@equivalent

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2015

meanwhile I've created patch-gem that is achieving this functionality if anyone is interested https://github.com/equivalent/mailer_preview_request_model (I'm using it in one application)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.