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

handle asset_host #30

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 4 additions & 9 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -256,18 +256,13 @@ end

## Known issues ##

Roadie will not be able to find your stylesheets if you have an `asset_host` configured and will ignore those lines when inlining.

A workaround for this is to not use `asset_host` in your mailers:
If you need to set an `asset_host` in your mailer, it must be done before `Roadie::Rails::Automatic` is `include`d. For example:

```ruby
config.action_controller.asset_host = # ...
config.action_mailer.asset_host = nil

# or

class MyMailer < ActionMailer::Base
self.asset_host = nil
self.asset_host = 'http://mail-cdn.mysite.com'

include Roadie::Rails::Automatic
end
```

Expand Down
3 changes: 3 additions & 0 deletions lib/roadie/rails.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ module Rails
end
end

require "active_support/concern"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer not to use ActiveSupport::Concern, but I cannot find any rational reason not to (we're inside the Rails adapter, after all!) so I'll approve it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any bad side effects for using ActiveSupport::Concern?
I am curious (since I use in my gems as well)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just that standard Ruby handles this pretty easily itself without the extra sugar (which feels like adding sugar to your soda IMO, maybe it's a bit sweeter but I don't see the point right here).


Strawman time

It's a bit like having an ActiveSupport library that overrides the new method on classes like this:

module NewWithOptionalBlock
  def new(*)
    instance = super
    if block_given?
      yield instance
    end
    instance
  end
end

I just don't see the point in using an external library for this pattern when just adding it straight to your class would do. Just accept a block in initialize and be done with it.


But again, I cannot say a good reason not to do it here as we're in Rails, and the argument that it's so easy to implement without Rails means that if Rails ever removes it, it's very easy to remove again. It's a double-edged sword that way. :-)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks for your explanation.


require "roadie"

require "roadie/rails/version"
Expand All @@ -12,6 +14,7 @@ module Rails
require "roadie/rails/mail_inliner"

require "roadie/rails/asset_pipeline_provider"
require "roadie/rails/css_localizer"

require "roadie/rails/mailer"

Expand Down
6 changes: 6 additions & 0 deletions lib/roadie/rails/automatic.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
module Roadie
module Rails
module Automatic
extend ActiveSupport::Concern

included do
include CssLocalizer
end

def mail(*args, &block)
super.tap do |email|
email.extend InlineOnDelivery
Expand Down
36 changes: 36 additions & 0 deletions lib/roadie/rails/css_localizer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
module Roadie
module Rails
module CssLocalizer
extend ActiveSupport::Concern

included do
old_asset_host = self.asset_host
if old_asset_host
# don't include an asset_host for CSS files, so that they are picked up by Roadie
self.asset_host = Proc.new { |source, request|
uri = URI::parse(source)
if uri.path.end_with?('.css')
nil
else
# logic copied from Rails
# http://git.io/vhYx7Q
if old_asset_host.respond_to?(:call)
# the original asset_host was a Proc
arity = old_asset_host.respond_to?(:arity) ? old_asset_host.arity : old_asset_host.method(:call).arity
args = [source]
args << request if request && (arity > 1 || arity < 0)
old_asset_host.call(*args)
elsif old_asset_host =~ /%d/
# the original asset_host was a string value that needs the filename checksum added
old_asset_host % (Zlib.crc32(source) % 4)
else
# the original asset_host was a simple string value
old_asset_host
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be a way to re-use the AssetUrlHelper mixin rather than copying-and-pasting, but I didn't want to spend too much time on it before getting initial feedback.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this part needs improvement. I might be able to take a stab at it if you don't have any ideas.

My initial thought is to move all of this into a new class (so the replacement proc is as simple as possible, offloading everything to this new class) and have it include the helper. It would also make it easier to unit test.

end
}
end
end
end
end
end
6 changes: 6 additions & 0 deletions lib/roadie/rails/mailer.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
module Roadie
module Rails
module Mailer
extend ActiveSupport::Concern

included do
include CssLocalizer
end

def roadie_mail(options = {}, &block)
email = mail(options, &block)
MailInliner.new(email, roadie_options).execute
Expand Down
23 changes: 23 additions & 0 deletions spec/integration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,29 @@ def parse_html_in_email(mail)
email.deliver
end

it "sets the proper host for images with automatic mailer" do
email = app.read_delivered_email(:asset_email)

html = email.html_part.body.decoded
expect(html).to include '<!DOCTYPE'
expect(html).to include '<head'
expect(html).to_not include '<link'

document = parse_html_in_email(email)
expect(document).to have_selector('body img')

if app.using_asset_pipeline?
expected_image_url = 'http://asset.host.com/assets/rails.png'
else
expected_image_url = 'http://asset.host.com/images/rails.png'
end
expect(document).to have_selector("img[src=\"#{expected_image_url}\"]")

# If we deliver mails we can catch weird problems with headers being invalid
email.delivery_method :test
email.deliver
end

it "inlines no styles when roadie_options are nil" do
email = app.read_delivered_email(:disabled_email)

Expand Down
2 changes: 2 additions & 0 deletions spec/lib/roadie/rails/automatic_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ module Roadie
module Rails
describe Automatic do
base_mailer = Class.new do
cattr_accessor :asset_host

def initialize(email = nil)
@email = email
end
Expand Down
39 changes: 39 additions & 0 deletions spec/lib/roadie/rails/css_localizer_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
require 'spec_helper'

module Roadie
module Rails
describe CssLocalizer do
base_mailer = Class.new do
cattr_accessor :asset_host
end

describe "#asset_host" do
it "passes all arguments to the underlying simple accessor" do
string_host_mailer = Class.new(base_mailer) do
self.asset_host = 'http://original.com'

include CssLocalizer
end

asset_host = string_host_mailer.asset_host
expect(asset_host).to be_a(Proc)
expect(asset_host.call('foo.png')).to eq('http://original.com')
end

it "passes all arguments to the underlying proc accessor" do
proc_host_mailer = Class.new(base_mailer) do
self.asset_host = Proc.new { |source, request|
'http://original.com'
}

include CssLocalizer
end

asset_host = proc_host_mailer.asset_host
expect(asset_host).to be_a(Proc)
expect(asset_host.call('foo.png')).to eq('http://original.com')
end
end
end
end
end
2 changes: 2 additions & 0 deletions spec/lib/roadie/rails/mailer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ module Roadie
module Rails
describe Mailer do
some_mailer = Class.new do
cattr_accessor :asset_host

include Mailer

def initialize(email = nil)
Expand Down
6 changes: 6 additions & 0 deletions spec/railsapps/shared/all/app/mailers/auto_mailer.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
class AutoMailer < ActionMailer::Base
self.asset_host = 'http://asset.host.com'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't think of a good way to do integration tests for a string and a proc for each type of mixin... open to ideas.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's okay to have it being different types in the different mailers. As long as both paths have been crossed and we have some unit tests I think we're good.


include Roadie::Rails::Automatic

default from: 'john@example.com'
Expand All @@ -7,6 +9,10 @@ def normal_email
generate_email
end

def asset_email
mail(to: 'example@example.org', subject: "Notification for you")
end

def disabled_email
generate_email
end
Expand Down
2 changes: 2 additions & 0 deletions spec/railsapps/shared/all/app/mailers/mailer.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
class Mailer < ActionMailer::Base
self.asset_host = 'http://asset.host.com'

include Roadie::Rails::Mailer

default from: 'john@example.com'
Expand Down
11 changes: 11 additions & 0 deletions spec/railsapps/shared/all/app/views/mailer/asset_email.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<!DOCTYPE html>
<html>
<head>
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<%= stylesheet_link_tag "email" %>
</head>
<body>
<h1>Normal email</h1>
<%= image_tag "rails.png" %>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Asset email