From d51dcd1705bb098560bf73405cc4967dee0e7e66 Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 26 Jul 2013 17:27:46 +1000 Subject: [PATCH] improve email styling, include extra respond link --- app/mailers/user_notifications.rb | 11 ++- app/views/email/_post.html.erb | 24 +++---- app/views/email/notification.html.erb | 15 +++- lib/email/message_builder.rb | 11 ++- lib/email/styles.rb | 93 ++++++++++++++++--------- spec/components/email/styles_spec.rb | 9 ++- spec/mailers/user_notifications_spec.rb | 18 +++++ 7 files changed, 120 insertions(+), 61 deletions(-) diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index c0a054118..2cd272651 100644 --- a/app/mailers/user_notifications.rb +++ b/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 diff --git a/app/views/email/_post.html.erb b/app/views/email/_post.html.erb index 0bd0ca941..7293eb1e3 100644 --- a/app/views/email/_post.html.erb +++ b/app/views/email/_post.html.erb @@ -1,22 +1,16 @@ - +
- - + + +
- + + - - - - - - - -
- <%= post.user.username %> - <%= l post.created_at, format: :short_no_year %>
- <%= correct_top_margin post.cooked.html_safe, "5px" %> + 		+ <%= post.user.username %>
+ <%= l post.created_at, format: :short_no_year %>
<%= post.cooked.html_safe %>
diff --git a/app/views/email/notification.html.erb b/app/views/email/notification.html.erb index b69048976..9064d8877 100644 --- a/app/views/email/notification.html.erb +++ b/app/views/email/notification.html.erb @@ -1,8 +1,11 @@ +
<%= render :partial => 'email/post', :locals => {:post => post} %> <% if context_posts.present? %> -
-

<%= t "user_notifications.previous_discussion" %>

+
+ %{respond_instructions} +
+

<%= t "user_notifications.previous_discussion" %>

<% context_posts.each do |p| %> <%= render :partial => 'email/post', :locals => {:post => p} %> @@ -10,3 +13,11 @@ <% end %>
+ +
+%{respond_instructions} +
+
+%{unsubscribe_link} +
+
diff --git a/lib/email/message_builder.rb b/lib/email/message_builder.rb index f71cba477..9e4a5b9f6 100644 --- a/lib/email/message_builder.rb +++ b/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 << "
".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 diff --git a/lib/email/styles.rb b/lib/email/styles.rb index abcca0da1..c7d930774 100644 --- a/lib/email/styles.rb +++ b/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\]/, "
") - result.gsub!(/\[\/email-indent\]/, "
") - result + strip_classes_and_ids + @fragment.to_html.tap do |result| + result.gsub!(/\[email-indent\]/, "
") + result.gsub!(/\[\/email-indent\]/, "
") + 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 \ No newline at end of file +end diff --git a/spec/components/email/styles_spec.rb b/spec/components/email/styles_spec.rb index b1b7d8c75..e845c5862 100644 --- a/spec/components/email/styles_spec.rb +++ b/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("") - 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("
") + expect(frag.to_html).to eq("
") + end + end context "html template formatter" do diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb index 69be03406..34596e55f 100644 --- a/spec/mailers/user_notifications_spec.rb +++ b/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)