improve email styling, include extra respond link

app/mailers/user_notifications.rb

@@ -107,14 +107,12 @@ class UserNotifications < ActionMailer::Base
   end
 
   def notification_email(user, opts)
-    @notification = opts[:notification]
-    return unless @notification.present?
 
-    @post = opts[:post]
-    return unless @post.present?
+    return unless @notification = opts[:notification]
+    return unless @post = opts[:post]
 
     username = @notification.data_hash[:display_username]
-    notification_type = Notification.types[opts[:notification].notification_type].to_s
+    notification_type = Notification.types[@notification.notification_type].to_s
 
     context = ""
     context_posts = Post.where(topic_id: @post.topic_id)
@@ -147,7 +145,8 @@ class UserNotifications < ActionMailer::Base
       add_unsubscribe_link: true,
       allow_reply_by_email: opts[:allow_reply_by_email],
       template: "user_notifications.user_#{notification_type}",
-      html_override: html
+      html_override: html,
+      style: :notification
     }
 
     # If we have a display name, change the from address

app/views/email/_post.html.erb

@@ -1,22 +1,16 @@
-<table cellspacing="0" cellpadding="0" border="0" style='margin-bottom:20px;max-width:761px'>
+<table class='post-wrapper'>
   <tbody>
     <tr>
-      <td style='padding-right:10px;vertical-align:top;'>
-        <img width="45" height="45" src="<%= post.user.small_avatar_url%>" class="avatar" title="<%= post.user.username%>">
+      <td class='user-avatar'>
+        <img src="<%= post.user.small_avatar_url%>" title="<%= post.user.username%>">
       </td>
-      <td style="width: 100%">
-        <table cellspacing="0" cellpadding="0" border="0">
-          <tbody>
-            <tr>
-              <td style="font-size:13px;font-family:'lucida grande',tahoma,verdana,arial,sans-serif;padding-right:10px;">
-                <a style="color:#3b5998;text-decoration:none;font-weight:bold" href="<%=Discourse.base_url%>/users/<%= post.user.username_lower%>" target="_blank"><%= post.user.username %></a>
-              </td>
-              <td style="text-align:right;color:#999999;padding-right:5px;font-family:'lucida grande',tahoma,verdana,arial,sans-serif;font-size:11px"> <%= l post.created_at, format: :short_no_year %> </td>
-            </tr>
-          </tbody>
-        </table>
-        <%= correct_top_margin post.cooked.html_safe, "5px" %>
+      <td>
+        <a class="username" href="<%=Discourse.base_url%>/users/<%= post.user.username_lower%>" target="_blank"><%= post.user.username %></a><br>
+        <span class='date'><%= l post.created_at, format: :short_no_year %></span>
       </td>
     </tr>
+    <tr>
+      <td class='body'><%= post.cooked.html_safe %></td>
+    </tr>
   </tbody>
 </table>

app/views/email/notification.html.erb

@@ -1,8 +1,11 @@
+<div id='main'>
 <%= render :partial => 'email/post', :locals => {:post => post} %>
 
 <% if context_posts.present? %>
-  <hr>
-  <h4 style='font-size: 17px; color: #444;'><%= t "user_notifications.previous_discussion" %></h4>
+  <div class='footer'>
+    %{respond_instructions}
+  </div>
+  <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
 
   <% context_posts.each do |p| %>
   <%= render :partial => 'email/post', :locals => {:post => p} %>
@@ -10,3 +13,11 @@
 <% end %>
 
 <hr>
+
+<div class='footer'>
+%{respond_instructions}
+</div>
+<div class='footer'>
+%{unsubscribe_link}
+</div>
+</div>

lib/email/message_builder.rb

@@ -44,18 +44,23 @@ module Email
     def html_part
       return unless html_override = @opts[:html_override]
       if @opts[:add_unsubscribe_link]
-        html_override  << "<br>".html_safe
 
         if response_instructions = @template_args[:respond_instructions]
-          html_override << PrettyText.cook(response_instructions).html_safe
+          respond_instructions = PrettyText.cook(response_instructions).html_safe
+          html_override.gsub!("%{respond_instructions}", respond_instructions)
         end
 
-        html_override << PrettyText.cook(I18n.t('unsubscribe_link', template_args)).html_safe
+        unsubscribe_link = PrettyText.cook(I18n.t('unsubscribe_link', template_args)).html_safe
+        html_override.gsub!("%{unsubscribe_link}",unsubscribe_link)
       end
 
       styled = Email::Styles.new(html_override)
       styled.format_basic
 
+      if style = @opts[:style]
+        styled.send "format_#{style}"
+      end
+
       Mail::Part.new do
         content_type 'text/html; charset=UTF-8'
         body styled.to_html

lib/email/styles.rb

@@ -14,7 +14,8 @@ module Email
       @fragment.css('img').each do |img|
 
         if img['src'] =~ /\/assets\/emoji\//
-          img['style'] = "width: 20px; height: 20px;"
+          img['width'] = 20
+          img['height'] = 20
         else
           img['style'] = "max-width: 694px;"
         end
@@ -24,52 +25,78 @@ module Email
         end
       end
 
-      @fragment.css('div.post-indent').each do |div|
-        div['style'] = 'margin-left: 15px; margin-top: 20px; max-width: 694px;'
-      end
-
+      style('div.post-indent',' margin-left: 15px; margin-top: 20px; max-width: 694px;')
     end
 
+    def format_notification
+      style('.previous-discussion', 'font-size: 17px; color: #444;')
+      style('.date', "text-align:right;color:#999999;padding-right:5px;font-family:'lucida grande',tahoma,verdana,arial,sans-serif;font-size:11px")
+      style('.username', "font-size:13px;font-family:'lucida grande',tahoma,verdana,arial,sans-serif;color:#3b5998;text-decoration:none;font-weight:bold")
+      style('.post-wrapper', "margin-bottom:25px;max-width:761px")
+      style('.user-avatar', 'vertical-align:top;width:55px;')
+      style('.user-avatar img', nil, width: '45', height: '45')
+      style('hr', 'background-color: #ddd; height: 1px; border: 1px;')
+      # we can do this but it does not look right
+      # style('#main', 'font-family:"Helvetica Neue", Helvetica, Arial, sans-serif')
+      style('td.body', 'padding-top:10px;', colspan: "2")
+      correct_footer_style
+      reset_tables
+    end
+
+
     def format_html
 
-      @fragment.css('h3').each do |h3|
-        h3['style'] = 'margin: 15px 0 20px 0; border-bottom: 1px solid #ddd;'
-      end
-
-      @fragment.css('hr').each do |hr|
-        hr['style'] = 'background-color: #ddd; height: 1px; border: 1px;'
-      end
-
-
-      @fragment.css('a').each do |a|
-        a['style'] = 'text-decoration: none; font-weight: bold; font-size: 15px; color: #006699;'
-      end
-
-      @fragment.css('ul').each do |ul|
-        ul['style'] = 'margin: 0 0 0 10px; padding: 0 0 0 20px;'
-      end
-
-      @fragment.css('li').each do |li|
-        li['style'] = 'padding-bottom: 10px'
-      end
+      style('h3', 'margin: 15px 0 20px 0; border-bottom: 1px solid #ddd;')
+      style('hr', 'background-color: #ddd; height: 1px; border: 1px;')
+      style('a',' text-decoration: none; font-weight: bold; font-size: 15px; color: #006699;')
+      style('ul', 'margin: 0 0 0 10px; padding: 0 0 0 20px;')
+      style('li', 'padding-bottom: 10px')
+      style('div.digest-post', 'margin-left: 15px; margin-top: 20px; max-width: 694px;')
 
       @fragment.css('pre').each do |pre|
         pre.replace(pre.text)
       end
 
-      @fragment.css('div.digest-post').each do |div|
-        div['style'] = 'margin-left: 15px; margin-top: 20px; max-width: 694px;'
-      end
 
     end
 
     def to_html
-      result = @fragment.to_html
-      result.gsub!(/\[email-indent\]/, "<div style='margin-left: 15px'>")
-      result.gsub!(/\[\/email-indent\]/, "</div>")
-      result
+      strip_classes_and_ids
+      @fragment.to_html.tap do |result|
+        result.gsub!(/\[email-indent\]/, "<div style='margin-left: 15px'>")
+        result.gsub!(/\[\/email-indent\]/, "</div>")
+      end
     end
 
+    private
 
+    def correct_footer_style
+      @fragment.css('.footer').each do |element|
+        element['style'] = "font-size:13px;font-family:'lucida grande',tahoma,verdana,arial,sans-serif;color:#444;"
+        element.css('a').each do |inner|
+          inner['style'] = "color:#444;"
+        end
+      end
+    end
+
+    def strip_classes_and_ids
+      @fragment.css('*').each do |element|
+        element.delete('class')
+        element.delete('id')
+      end
+    end
+
+    def reset_tables
+      style('table',nil, cellspacing: '0', cellpadding: '0', border: '0')
+    end
+
+    def style(selector, style, attribs = {})
+      @fragment.css(selector).each do |element|
+        element['style'] = style if style
+        attribs.each do |k,v|
+          element[k] = v
+        end
+      end
+    end
   end
-end
+end

spec/components/email/styles_spec.rb

@@ -31,8 +31,8 @@ describe Email::Styles do
 
     it "adds a width and height to images with an emoji path" do
       frag = basic_fragment("<img src='/assets/emoji/fish.png'>")
-      expect(frag.at("img")["style"]).to match("width:")
-      expect(frag.at("img")["style"]).to match("height:")
+      expect(frag.at("img")["width"]).to eq("20")
+      expect(frag.at("img")["height"]).to eq("20")
     end
 
     it "converts relative paths to absolute paths" do
@@ -40,6 +40,11 @@ describe Email::Styles do
       expect(frag.at("img")["src"]).to eq("#{Discourse.base_url}/some-image.png")
     end
 
+    it "strips classes and ids" do
+      frag = basic_fragment("<div class='foo' id='bar'><div class='foo' id='bar'></div></div>")
+      expect(frag.to_html).to eq("<div><div></div></div>")
+    end
+
   end
 
   context "html template formatter" do

spec/mailers/user_notifications_spec.rb

@@ -41,6 +41,24 @@ describe UserNotifications do
     end
   end
 
+  describe '.user_replied' do
+    let!(:post) { Fabricate(:post) }
+    let!(:response) { Fabricate(:post, topic: post.topic)}
+    let!(:user) { Fabricate(:user) }
+    let!(:notification) { Fabricate(:notification, user: user) }
+
+    it 'generates a correct email' do
+      mail = UserNotifications.user_replied(response.user, post: response, notification: notification)
+
+      # 2 respond to links cause we have 1 context post
+      mail.html_part.to_s.scan(/To respond/).count.should == 2
+
+      # 1 unsubscribe
+      mail.html_part.to_s.scan(/To unsubscribe/).count.should == 1
+
+    end
+  end
+
 
   def expects_build_with(condition)
     UserNotifications.any_instance.expects(:build_email).with(user.email, condition)