Mange / roadie-rails

Making HTML emails comfortable for the Rails rockstars
MIT License
363 stars 65 forks source link

Document.asset_providers overriden on Options#initialize #15

Closed opsidao closed 9 years ago

opsidao commented 10 years ago

Hi there!

Today I've come across what looks like a bug in the current version of roadie-rails (1.0.3), let me explain a bit. Whenever I try to inline a css link I'm hitting this exception:

     NoMethodError:
       undefined method `find_stylesheet' for nil:NilClass
     # ./vendor/bundle/gems/roadie-3.0.0/lib/roadie/provider_list.rb:41:in `block in find_stylesheet'
     # ./vendor/bundle/gems/roadie-3.0.0/lib/roadie/provider_list.rb:40:in `each'
     # ./vendor/bundle/gems/roadie-3.0.0/lib/roadie/provider_list.rb:40:in `find_stylesheet'
     # ./vendor/bundle/gems/roadie-3.0.0/lib/roadie/asset_provider.rb:8:in `find_stylesheet!'
     # ./vendor/bundle/gems/roadie-3.0.0/lib/roadie/asset_scanner.rb:84:in `read_link_element'
     # ./vendor/bundle/gems/roadie-3.0.0/lib/roadie/asset_scanner.rb:74:in `read_stylesheet'
     # ./vendor/bundle/gems/roadie-3.0.0/lib/roadie/asset_scanner.rb:42:in `block in extract_css'
     # ./vendor/bundle/gems/nokogiri-1.6.3.1/lib/nokogiri/xml/node_set.rb:237:in `block in each'
     # ./vendor/bundle/gems/nokogiri-1.6.3.1/lib/nokogiri/xml/node_set.rb:236:in `upto'
     # ./vendor/bundle/gems/nokogiri-1.6.3.1/lib/nokogiri/xml/node_set.rb:236:in `each'
     # ./vendor/bundle/gems/roadie-3.0.0/lib/roadie/asset_scanner.rb:41:in `map'
     # ./vendor/bundle/gems/roadie-3.0.0/lib/roadie/asset_scanner.rb:41:in `extract_css'
     # ./vendor/bundle/gems/roadie-3.0.0/lib/roadie/document.rb:84:in `inline'
     # ./vendor/bundle/gems/roadie-3.0.0/lib/roadie/document.rb:60:in `transform'
     # ./vendor/bundle/gems/roadie-rails-1.0.2/lib/roadie/rails/mail_inliner.rb:29:in `transform_html'
     # ./vendor/bundle/gems/roadie-rails-1.0.2/lib/roadie/rails/mail_inliner.rb:21:in `improve_body'
     # ./vendor/bundle/gems/roadie-rails-1.0.2/lib/roadie/rails/mail_inliner.rb:13:in `execute'
     # ./vendor/bundle/gems/roadie-rails-1.0.2/lib/roadie/rails/mailer.rb:6:in `roadie_mail'

When I put some debugging into it, it turned out that the @providers variables was set to [nil], which logically lead to the previous error when trying to call a method on it.

After some digging around I arrived to the following piece of code in the Options module (this is not in the backtrace, but it seems to be the source for this problem; it gets called from here):

document.asset_providers = asset_providers

Before this line, the document.asset_providers had the default ProviderList.wrap(FilesystemProvider.new), the problem is that the asset_providers method, does correctly check if providers is set to something and, if it isn't, doesn't set the value for self.asset_providers, so self.asset_providers is nil unless you pass it through the options hash to the initializer.

This means that the previous line simply overrides document.asset_providers with nil unconditionally, unless you passed an option.

My guess is that just doing something like:

document.asset_providers = asset_providers if asset_providers

should be enough, but I'm not sure if I'm missing something.... how do you see this? Did I explain myself?

Mange commented 10 years ago

Wow! Thanks for an excellent bug report. The hyperlinks were a real timesaver. You're awesome.

I'll make sure to fix this.

Mange commented 10 years ago

Actually, the "strange" thing here is that options.asset_providers shouldn't have been nil in the first place. It's set inside the Railtie and Options don't allow nil to be assigned to that attribute.

Mange commented 10 years ago

I've pushed a fix to master that should make this work again for you. A patch release will happen soon in case you don't want to switch to master.

opsidao commented 10 years ago

Much :heart:!

I'll give it a try in the next refactor :wink:, I already delivered the feature putting the stylesheets in the html head of the email and using roadie to do the inlining, time constrains dictate, but I'll give some feedback when I do have time to test it.

Thanks a lot, and keep up the good work!

Mange commented 10 years ago

Thanks. And sorry for not handling this earlier; I was on vacation with my family. :sunny:

tomasc commented 10 years ago

Running into similar issue, switched to master, seems the @pipeline variable is nil.

NoMethodError: undefined method `[]' for nil:NilClass
.rvm/gems/ruby-2.1.2/bundler/gems/roadie-rails-78c580b1bc3c/lib/roadie/rails/asset_pipeline_provider.rb:13:in `find_stylesheet'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/provider_list.rb:41:in `block in find_stylesheet'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/provider_list.rb:40:in `each'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/provider_list.rb:40:in `find_stylesheet'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/asset_provider.rb:8:in `find_stylesheet!'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/asset_scanner.rb:84:in `read_link_element'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/asset_scanner.rb:74:in `read_stylesheet'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/asset_scanner.rb:42:in `block in extract_css'
.rvm/gems/ruby-2.1.2/gems/nokogiri-1.6.3.1/lib/nokogiri/xml/node_set.rb:237:in `block in each'
.rvm/gems/ruby-2.1.2/gems/nokogiri-1.6.3.1/lib/nokogiri/xml/node_set.rb:236:in `upto'
.rvm/gems/ruby-2.1.2/gems/nokogiri-1.6.3.1/lib/nokogiri/xml/node_set.rb:236:in `each'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/asset_scanner.rb:41:in `map'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/asset_scanner.rb:41:in `extract_css'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/document.rb:83:in `inline'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/document.rb:59:in `transform'
.rvm/gems/ruby-2.1.2/bundler/gems/roadie-rails-78c580b1bc3c/lib/roadie/rails/mail_inliner.rb:29:in `transform_html'
.rvm/gems/ruby-2.1.2/bundler/gems/roadie-rails-78c580b1bc3c/lib/roadie/rails/mail_inliner.rb:21:in `improve_body'
.rvm/gems/ruby-2.1.2/bundler/gems/roadie-rails-78c580b1bc3c/lib/roadie/rails/mail_inliner.rb:13:in `execute'
.rvm/gems/ruby-2.1.2/bundler/gems/roadie-rails-78c580b1bc3c/lib/roadie/rails/inline_on_delivery.rb:11:in `deliver'

Any ideas? I will try to investigate further. I have default rails engine setup with blank Dummy app which I run the tests against.

Mange commented 10 years ago

Aha, so roadie-rails is used in a Rails engine? That could explain it. I'll see how an engine is supposed to access assets.

For now, I'll make it not add the AssetPipelineProvider at all when it cannot access assets.

Mange commented 10 years ago

So I have not been able to find any clear documentation on how to access the asset pipeline from inside an engine, but I think the current workaround (I just pushed) should suffice for 90%+ of cases as most people with the asset pipeline probably precompiles the assets.

I think I'm going to regard this as good enough, if it works for you, and then wait for someone with the use-case "Engine + Asset pipeline + No precompilation" to report a bug before I try to make it work.

tomasc commented 10 years ago

Thanks Magnus! We are further, but still with an error:

Roadie::CssNotFound: Could not find stylesheet "/stylesheets/application.css"
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/asset_provider.rb:8:in `find_stylesheet!'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/asset_scanner.rb:84:in `read_link_element'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/asset_scanner.rb:74:in `read_stylesheet'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/asset_scanner.rb:42:in `block in extract_css'
.rvm/gems/ruby-2.1.2/gems/nokogiri-1.6.3.1/lib/nokogiri/xml/node_set.rb:237:in `block in each'
.rvm/gems/ruby-2.1.2/gems/nokogiri-1.6.3.1/lib/nokogiri/xml/node_set.rb:236:in `upto'
.rvm/gems/ruby-2.1.2/gems/nokogiri-1.6.3.1/lib/nokogiri/xml/node_set.rb:236:in `each'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/asset_scanner.rb:41:in `map'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/asset_scanner.rb:41:in `extract_css'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/document.rb:83:in `inline'
.rvm/gems/ruby-2.1.2/gems/roadie-3.0.0/lib/roadie/document.rb:59:in `transform'
.rvm/gems/ruby-2.1.2/bundler/gems/roadie-rails-544d3063a030/lib/roadie/rails/mail_inliner.rb:29:in `transform_html'
.rvm/gems/ruby-2.1.2/bundler/gems/roadie-rails-544d3063a030/lib/roadie/rails/mail_inliner.rb:21:in `improve_body'
.rvm/gems/ruby-2.1.2/bundler/gems/roadie-rails-544d3063a030/lib/roadie/rails/mail_inliner.rb:13:in `execute'
.rvm/gems/ruby-2.1.2/bundler/gems/roadie-rails-544d3063a030/lib/roadie/rails/inline_on_delivery.rb:11:in `deliver'

I will try to investigate further and get back to you.

Mange commented 10 years ago

It might be because of the slash in the front. I should extend this error message a bit to explain more.

tomasc commented 10 years ago

Could be. The error message though does not go any further than to the call of mailer's deliver method in my tests …

Mange commented 10 years ago

That error, does it happen in development or production? I think development for Rails engines might be broken a bit since the asset pipeline isn't used anymore.

tomasc commented 10 years ago

I get the error when running tests (mailer tests). I did not try to plug my engine to real Rails app yet. If you point me to a direction as to what to try / where to look I can try to help, would be great to get this working.

Mange commented 10 years ago

Okay, so the problem is this:

By default, roadie-rails will look for your file in public, then ask the asset pipeline for the asset with the given name. A rails engine does not have access to the asset pipeline, as far as I can tell. The asset is also not under public (I bet), so it cannot find it.

If you have no problems during development and production, a workaround might be to use another provider while running tests. See below.

If this blows up in other environments too, something needs to be done in the gem. I'm unsure what since I don't have an example engine to work on. If you could figure out, from the console, how to get a hold of the asset pipeline that would be swell. In a normal Rails app you get access to it by Rails.application.assets, but at least one part in this chain seems to be nil inside the engine.

Here's how you can work around the problem in the tests:

# Alternative 1: Band-aid (when using Automatic)
email = MyMailer.stuff
email.roadie_options.asset_providers << Roadie::NullProvider.new # to ignore assets not found
# email.roadie_options.asset_providers << Roadie::FilesystemProvider.new(Rails.root.join("app", "assets")) # to look in `app/assets` (no Sass)
email.deliver

# Alternative 2: Change your default config
# application.rb
config.roadie.asset_providers << Roadie::NullProvider.new # if Rails.env.test?
# config.roadie.asset_providers << Roadie::FilesystemProvider.new(config.root.join("app", "assets")) # if Rails.env.test?
<% # Alternative 3: Don't inline stylesheets in tests %>
<% # view.html.erb %>
<%= stylesheet_link_tag "application", (Rails.env.test? ? {data: {"roadie-immutable" => true}} : {}) %>

All of these are horrible, I know, but at least they should give you some room to breathe in so you can ship whatever you are working on until we figure this out.


Alternatively, I could try to take a stab at this when I get the time (and I cannot promise this will be too soon), but in that case I need to know more about your use-case. Are you using Engines as micro services in a larger ecosystem or are you creating a general "plug-in" engine to be used by anyone (open-source or not)? Did you add roadie-rails to your engine or was it dragged in by a container application? Are you running the tests of the engine, or are Rails "exporting" engine tests to the container? (I don't know if this question makes sense)

Would it be possible for me to get a hold of a small engine that has this problem, when asked? One that I can experiment on? (I could generate my own engine if not, it's just that I need some time to research engines in that case.)


Lastly, there's the workaround of not using roadie-rails at this moment and instead focus on vanilla roadie. The code of roadie-rails is pretty tamed when the whole Roadie::Options bit is stripped. It should be relatively easy for you to plug it in yourself. This will take longer than just to implement a workaround, but I still feel that I need to be explicit about it. roadie-rails is pretty simple.

tomasc commented 9 years ago

OK, I finally seem to have solved it.

class InlineCSSInterceptor
  def self.delivering_email message
    roadie_options = ::Rails.application.config.roadie
    roadie_options.asset_providers = [ Roadie::Rails::AssetPipelineProvider.new(Rails.application.assets) ]
    Roadie::Rails::MailInliner.new(message, roadie_options).execute
  end
end

ActionMailer::Base.register_interceptor(InlineCSSInterceptor)
Mange commented 9 years ago

AssetPipelineProvider should be added automatically when Rails.application.assets is present. As I said above, from my experimentation Rails.application.assets is nil inside engines, which is why it is never added.

Your code imply that it's present at a later time in the application lifecycle. That's interesting...


Your current solution isn't very good for production, however. Accessing an asset through the asset pipeline ignores precompiled assets and instead does live compilation each time an email is sent, which is slow and wasteful. I really don't recommend it.

Your solution also runs the risk of running Roadie twice if the mailers include either Roadie module. I recommend that you override the roadie_options method instead:

class MyMailer < ActionMailer::Base
private
  def roadie_options
    super.merge(asset_providers: [Roadie::Rails::AssetPipelineProvider.new(Rails.application.assets)])
  end

  # Append AssetPipelineProvider instead of replacing; this makes Roadie search the filesystem first, *then* the asset pipeline
  # def roadie_options
  #   super.combine(asset_providers: [Roadie::Rails::AssetPipelineProvider.new(Rails.application.assets)])
  # end
end
tomasc commented 9 years ago

Thanks for the feedback.

I will look into the situation with precompiled assets in production in more detail.

By the way, why is not Roadie in Rails implemented via standard ActionMailer interceptors (see #35)?

Mange commented 9 years ago

Because you cannot selectively activate it in specific mailers. Interceptors are global.

tors 5 feb 2015 08:53 Tomas Celizna notifications@github.com skrev:

Thanks for the feedback.

I will look into the situation with precompiled assets in production in more detail.

By the way, why is not Roadie in Rails implemented via standard ActionMailer interceptors (see #35 https://github.com/Mange/roadie-rails/issues/35)?

— Reply to this email directly or view it on GitHub https://github.com/Mange/roadie-rails/issues/15#issuecomment-73007472.

Mange commented 9 years ago

I'll close this while I wait for more feedback.

ayb commented 8 years ago

I ran into a similar issue today where my test suite was failing in a Rails 5 application because Roadie could not find a stylesheet.

For anyone that may find this -

I tried adding config.roadie.asset_providers << Roadie::NullProvider.new to my /environments/test.rb file but got the error undefined method "<<" for nil:NilClass (NoMethodError)

I guess Roadie has not been initialized at that point.

I was able to resolve the error by adding the following to my /config/initializers/assets.rb file:

Rails.application.config.roadie.asset_providers << Roadie::NullProvider.new if Rails.env.test?