Merge remote-tracking branch 'openstreetmap/pull/1401'

app/helpers/notifier_helper.rb

@@ -2,4 +2,24 @@ module NotifierHelper
   def fp(text)
     format_paragraph(text, 72, 0)
   end
+
+  def link_to_user(display_name)
+    link_to(
+      display_name,
+      user_url(display_name, :host => SERVER_URL),
+      :target => "_blank",
+      :style => "text-decoration: none; color: #222; font-weight: bold"
+    )
+  end
+
+  def message_body(&block)
+    render(
+      :partial => "message_body",
+      :locals => { :body => capture(&block) }
+    )
+  end
+
+  def style_message(html)
+    html.gsub /<p>/, '<p style="color: black; margin: 0.75em 0">'
+  end
 end

app/models/notifier.rb

@@ -3,6 +3,8 @@ class Notifier < ActionMailer::Base
           :return_path => EMAIL_RETURN_PATH,
           :auto_submitted => "auto-generated"
   helper :application
+  before_action :set_shared_template_vars
+  before_action :attach_project_logo
 
   def signup_confirm(user, token)
     with_recipient_locale user do
@@ -76,6 +78,9 @@ class Notifier < ActionMailer::Base
       @replyurl = url_for(:host => SERVER_URL,
                           :controller => "message", :action => "reply",
                           :message_id => message.id)
+      @author = @from_user
+
+      attach_user_avatar(message.sender)
 
       mail :from => from_address(message.sender.display_name, "m", message.id, message.digest),
            :to => message.recipient.email,
@@ -106,6 +111,9 @@ class Notifier < ActionMailer::Base
                           :action => "new",
                           :display_name => comment.user.display_name,
                           :title => "Re: #{comment.diary_entry.title}")
+      @author = @from_user
+
+      attach_user_avatar(comment.user)
 
       mail :from => from_address(comment.user.display_name, "c", comment.id, comment.digest, recipient.id),
            :to => recipient.email,
@@ -122,7 +130,9 @@ class Notifier < ActionMailer::Base
       @friendurl = url_for(:host => SERVER_URL,
                            :controller => "user", :action => "make_friend",
                            :display_name => @friend.befriender.display_name)
+      @author = @friend.befriender.display_name
 
+      attach_user_avatar(@friend.befriender)
       mail :to => friend.befriendee.email,
            :subject => I18n.t("notifier.friend_notification.subject", :user => friend.befriender.display_name)
     end
@@ -142,6 +152,9 @@ class Notifier < ActionMailer::Base
                      I18n.t("notifier.note_comment_notification.anonymous")
                    end
 
+      @author = @commenter
+      attach_user_avatar(comment.author)
+
       subject = if @owner
                   I18n.t("notifier.note_comment_notification.#{@event}.subject_own", :commenter => @commenter)
                 else
@@ -161,6 +174,7 @@ class Notifier < ActionMailer::Base
       @changeset_comment = comment.changeset.tags["comment"].presence
       @time = comment.created_at
       @changeset_author = comment.changeset.user.display_name
+      @author = @commenter
 
       subject = if @owner
                   I18n.t("notifier.changeset_comment_notification.commented.subject_own", :commenter => @commenter)
@@ -168,12 +182,35 @@ class Notifier < ActionMailer::Base
                   I18n.t("notifier.changeset_comment_notification.commented.subject_other", :commenter => @commenter)
                 end
 
+      attach_user_avatar(comment.author)
+
       mail :to => recipient.email, :subject => subject
     end
   end
 
   private
 
+  def set_shared_template_vars
+    @root_url = root_url(:host => SERVER_URL)
+  end
+
+  def attach_project_logo
+    attachments.inline["logo.png"] = File.read("#{Rails.root}/app/assets/images/osm_logo_30.png")
+  end
+
+  def attach_user_avatar(user)
+    attachments.inline["avatar.png"] = File.read(user_avatar_file_path(user))
+  end
+
+  def user_avatar_file_path(user)
+    image = user && user.image
+    if image && image.file?
+      return image.path(:small)
+    else
+      return "#{Rails.root}/app/assets/images/users/images/small.png"
+    end
+  end
+
   def with_recipient_locale(recipient)
     I18n.with_locale Locale.available.preferred(recipient.preferred_languages) do
       yield

app/views/layouts/notifier.html.erb

@@ -0,0 +1,46 @@
+<html>
+  <head>
+    <meta charset="UTF-8"></meta>
+  </head>
+  <body style="padding: 0; margin: 0; font-size: 14px; font-family: 'Helvetica Neue', Arial, sans-serif; color: #222">
+    <table style="background-color: #eee; width: 100%">
+      <tr>
+        <td style="text-align: center">
+          <table style="width: 600px; color: #222; margin-left: auto; margin-right: auto">
+            <tr>
+              <td style="width: 30px; padding: 10px 10px 10px 0px">
+                <a href="<%= @root_url %>" target="_blank">
+                  <%= image_tag attachments["logo.png"].url, alt: "OpenStreetMap logo", title: "OpenStreetMap", height: "30", width: "30", border: "0" %>
+                </a>
+              </td>
+              <td style="padding: 10px 0px">
+                <a href="<%= @root_url %>" target="_blank" style="text-decoration: none; color: #000">
+                  <h1 style="font-size: 18px; font-weight: 600; margin: 0; text-align: left">OpenStreetMap</h1>
+                </a>
+              </td>
+            </tr>
+            <tr>
+              <td colspan="2">
+                <table style="background-color: #fff; color: #222; border: solid 1px #ccc; border-collapse: separate">
+                  <tr>
+                    <td style="text-align: left; padding: 0px 15px 5px 15px">
+                      <%= raw style_message(yield) %>
+                    </td>
+                  </tr>
+                </table>
+              </td>
+            </tr>
+          </table>
+        </td>
+      </tr>
+      <tr>
+        <td style="text-align: center; font-size: 11px">
+          <%= yield :footer %>
+          <p style="margin-bottom: 10px">
+            <a href="<%= @root_url %>" target="_blank" style="color: #222">OpenStreetMap</a>
+          </p>
+        </td>
+      </tr>
+    </table>
+  </body>
+</html>

app/views/notifier/_gpx_description.html.erb

@@ -1,14 +1,12 @@
-<%= t'notifier.gpx_notification.greeting' %>
-
 <%= t'notifier.gpx_notification.your_gpx_file' %>
-
-  <%= @trace_name %>
-
+<strong><%= @trace_name %></strong>
 <%= t'notifier.gpx_notification.with_description' %>
-
-  <%= @trace_description %>
+<em><%= @trace_description %></em>
 <% if @trace_tags.length>0 %>
-<%= t'notifier.gpx_notification.and_the_tags' %>
-<% @trace_tags.each do |tag| %>
-  <%= tag.tag.rstrip %><% end %><% else %>
-<%= t'notifier.gpx_notification.and_no_tags' %><% end %>
+  <%= t'notifier.gpx_notification.and_the_tags' %>
+  <em><% @trace_tags.each do |tag| %>
+    <%= tag.tag.rstrip %>
+  <% end %></em>
+<% else %>
+  <%= t'notifier.gpx_notification.and_no_tags' %>
+<% end %>

app/views/notifier/_message_body.html.erb

@@ -0,0 +1,21 @@
+<table style="font-size: 15px; font-style: italic; margin: 15px 0px; background-color: #eee; width: 520px">
+  <tr>
+    <td style="width: 50px; vertical-align: top; padding: 15px">
+      <%= link_to(
+          image_tag(
+            attachments["avatar.png"].url,
+            alt: @author,
+            title: @author,
+            width: 50,
+            height: 50,
+            border: 0
+          ),
+          user_url(@author, :host => SERVER_URL),
+          :target => "_blank"
+      ) %>
+    </td>
+    <td style="text-align: left; vertical-align: top; padding-right: 10px">
+      <%= body %>
+    </td>
+  </tr>
+</table>

app/views/notifier/changeset_comment_notification.html.erb

@@ -1,20 +1,26 @@
-<p><%= t 'notifier.changeset_comment_notification.greeting' %></p>
-
 <p>
   <% if @owner %>
-    <%= t "notifier.changeset_comment_notification.commented.your_changeset", :commenter => @commenter, :time => @time %>
+    <%= raw t "notifier.changeset_comment_notification.commented.your_changeset", :commenter => link_to_user(@commenter), :time => @time %>
   <% else %>
-    <%= t "notifier.changeset_comment_notification.commented.commented_changeset", :commenter => @commenter, :time => @time, :changeset_author => @changeset_author %>
+    <%= raw t "notifier.changeset_comment_notification.commented.commented_changeset", :commenter => link_to_user(@commenter), :time => @time, :changeset_author => @changeset_author %>
   <% end %>
   <% if @changeset_comment %>
-    <%= t "notifier.changeset_comment_notification.commented.partial_changeset_with_comment", :changeset_comment => @changeset_comment %>
+    <%= raw t "notifier.changeset_comment_notification.commented.partial_changeset_with_comment", :changeset_comment => content_tag("em", @changeset_comment) %>
   <% else %>
     <%= t "notifier.changeset_comment_notification.commented.partial_changeset_without_comment" %>
   <% end %>
 </p>
 
-==
-<%= @comment.to_html %>
-==
+<%= message_body do %>
+  <%= @comment.to_html %>
+<% end %>
 
-<p><%= raw t 'notifier.changeset_comment_notification.details', :url => link_to(@changeset_url, @changeset_url) %></p>
+<p>
+  <%= raw t 'notifier.changeset_comment_notification.details', :url => link_to(@changeset_url, @changeset_url, :style => "white-space: nowrap") %>
+</p>
+
+<% content_for :footer do %>
+  <p>
+    <%= raw t 'notifier.changeset_comment_notification.unsubscribe', :url => link_to(@changeset_url, @changeset_url, :style => "color: #222; white-space: nowrap") %>
+  </p>
+<% end %>

app/views/notifier/changeset_comment_notification.text.erb

@@ -16,3 +16,5 @@
 ==
 
 <%= t 'notifier.changeset_comment_notification.details', :url => @changeset_url %>
+
+<%= t 'notifier.changeset_comment_notification.unsubscribe', :url => @changeset_url %>

app/views/notifier/diary_comment_notification.html.erb

@@ -1,9 +1,18 @@
-<p><%= t'notifier.diary_comment_notification.hi', :to_user => @to_user %></p>
+<p>
+  <%= t'notifier.diary_comment_notification.hi', :to_user => @to_user %>
+</p>
+<p>
+  <%= raw t'notifier.diary_comment_notification.header', :from_user => link_to_user(@from_user), :subject => content_tag("em", @title) %>
+</p>
 
-<p><%= raw t'notifier.diary_comment_notification.header', :from_user => link_to(@from_user, :host => SERVER_URL, :controller => :user, :action => :view, :display_name => @from_user), :subject => @title %></p>
+<%= message_body do %>
+  <%= @text.to_html %>
+<% end %>
 
-==
-<%= @text.to_html %>
-==
-
-<p><%= raw t'notifier.diary_comment_notification.footer', :readurl => link_to(@readurl, @readurl), :commenturl => link_to(@commenturl, @commenturl), :replyurl => link_to(@replyurl, @replyurl) %></p>
+<% content_for :footer do %>
+  <p><%= raw t'notifier.diary_comment_notification.footer',
+             :readurl => link_to(@readurl, @readurl) + tag(:br),
+             :commenturl => link_to(@commenturl, @commenturl) + tag(:br),
+             :replyurl => link_to(@replyurl, @replyurl)
+  %></p>
+<% end %>

app/views/notifier/email_confirm.html.erb

@@ -4,4 +4,4 @@
 
 <p><%= t 'notifier.email_confirm_html.click_the_link' %></p>
 
-<p><a href="<%= @url %>"><%= @url %></a></p>
+<p><a href="<%= @url %>" style="white-space: nowrap"><%= @url %></a></p>

app/views/notifier/friend_notification.html.erb

@@ -1,7 +1,9 @@
 <p><%= t 'notifier.friend_notification.had_added_you', :user => @friend.befriender.display_name %></p>
 
-<p><%= raw t 'notifier.friend_notification.see_their_profile', :userurl => link_to(@viewurl, @viewurl) %></p>
+<%= message_body do %>
+  <p><%= raw t 'notifier.friend_notification.see_their_profile', :userurl => link_to(@viewurl, @viewurl) %></p>
 
-<% unless @friend.befriendee.is_friends_with?(@friend.befriender) -%>
-<p><%= raw t 'notifier.friend_notification.befriend_them', :befriendurl => link_to(@friendurl, @friendurl) %></p>
-<% end -%>
+  <% unless @friend.befriendee.is_friends_with?(@friend.befriender) -%>
+  <p><%= raw t 'notifier.friend_notification.befriend_them', :befriendurl => link_to(@friendurl, @friendurl) %></p>
+  <% end -%>
+<% end %>

app/views/notifier/gpx_failure.html.erb

@@ -1,9 +1,16 @@
-<%= render :partial => "gpx_description" %>
-<%= t'notifier.gpx_notification.failure.failed_to_import' %>
+<p><%= t'notifier.gpx_notification.greeting' %></p>
 
+<p>
+  <%= render :partial => "gpx_description" %>
+  <%= t'notifier.gpx_notification.failure.failed_to_import' %>
+</p>
+
+<blockquote>
   <%= @error %>
+</blockquote>
 
-<%= t'notifier.gpx_notification.failure.more_info_1' %>
-<%= t'notifier.gpx_notification.failure.more_info_2' %>
-
+<p>
+  <%= t'notifier.gpx_notification.failure.more_info_1' %>
+  <%= t'notifier.gpx_notification.failure.more_info_2' %>
   <%= t'notifier.gpx_notification.failure.import_failures_url' %>
+</p>

app/views/notifier/gpx_success.html.erb

@@ -1,2 +1,6 @@
-<%= render :partial => "gpx_description" %>
-<%= t'notifier.gpx_notification.success.loaded_successfully', :trace_points => @trace_points, :possible_points => @possible_points %>
+<p><%= t'notifier.gpx_notification.greeting' %></p>
+
+<p>
+  <%= render :partial => "gpx_description" %>
+  <%= t'notifier.gpx_notification.success.loaded_successfully', :trace_points => @trace_points, :possible_points => @possible_points %>
+</p>

app/views/notifier/message_notification.html.erb

@@ -1,9 +1,22 @@
-<p><%= t'notifier.message_notification.hi', :to_user => @to_user %></p>
+<p>
+  <%= t'notifier.message_notification.hi', :to_user => @to_user %>
+</p>
+<p>
+  <%= raw t'notifier.message_notification.header',
+          :from_user => link_to_user(@from_user),
+          :subject => content_tag("em", @title)
+  %>
+</p>
 
-<p><%= raw t'notifier.message_notification.header', :from_user => link_to(@from_user, :host => SERVER_URL, :controller => :user, :action => :view, :display_name => @from_user), :subject => @title %></p>
+<%= message_body do %>
+  <%= @text.to_html %>
+<% end %>
 
-==
-<%= @text.to_html %>
-==
-
-<p><%= t'notifier.message_notification.footer_html', :readurl => link_to(@readurl, @readurl), :replyurl => link_to(@replyurl, @replyurl) %></p>
+<% content_for :footer do %>
+  <p>
+    <%= t'notifier.message_notification.footer_html',
+          :readurl => link_to(@readurl, @readurl) + tag(:br),
+          :replyurl => link_to(@replyurl, @replyurl)
+    %>
+  </p>
+<% end %>

app/views/notifier/note_comment_notification.html.erb

@@ -1,13 +1,13 @@
 <p><%= t 'notifier.note_comment_notification.greeting' %></p>
 
 <% if @owner %>
-<p><%= t "notifier.note_comment_notification.#{@event}.your_note", :commenter => @commenter, :place => @place %></p>
+  <p><%= raw t "notifier.note_comment_notification.#{@event}.your_note", :commenter => link_to_user(@commenter), :place => @place %></p>
 <% else %>
-<p><%= t "notifier.note_comment_notification.#{@event}.commented_note", :commenter => @commenter, :place => @place %></p>
+  <p><%= raw t "notifier.note_comment_notification.#{@event}.commented_note", :commenter => link_to_user(@commenter), :place => @place %></p>
 <% end %>
 
-==
-<%= @comment.to_html %>
-==
+<%= message_body do %>
+  <%= @comment.to_html %>
+<% end %>
 
 <p><%= raw t 'notifier.note_comment_notification.details', :url => link_to(@noteurl, @noteurl) %></p>

app/views/notifier/signup_confirm.html.erb

@@ -4,6 +4,6 @@
 
 <p><%= t("notifier.signup_confirm.confirm") %></p>
 
-<p><%= link_to @url, @url %></p>
+<p><%= link_to @url, @url, :style => "white-space: nowrap" %></p>
 
 <p><%= t("notifier.signup_confirm.welcome") %></p>

config/locales/en.yml

@@ -1310,6 +1310,7 @@ en:
         commented_note: "%{commenter} has reactivated a map note you have commented on. The note is near %{place}."
       details: "More details about the note can be found at %{url}."
     changeset_comment_notification:
+      hi: "Hi %{to_user},"
       greeting: "Hi,"
       commented:
         subject_own: "[OpenStreetMap] %{commenter} has commented on one of your changesets"
@@ -1319,6 +1320,7 @@ en:
         partial_changeset_with_comment: "with comment '%{changeset_comment}'"
         partial_changeset_without_comment: "without comment"
       details: "More details about the changeset can be found at %{url}."
+      unsubscribe: 'To unsubscribe from updates to this changeset, visit %{url} and click "Unsubscribe".'
   message:
     inbox:
       title: "Inbox"

test/integration/user_creation_test.rb

@@ -135,10 +135,10 @@ class UserCreationTest < ActionDispatch::IntegrationTest
     assert_equal register_email.to[0], new_email
     # Check that the confirm account url is correct
     confirm_regex = Regexp.new("/user/redirect_tester/confirm\\?confirm_string=([a-zA-Z0-9]*)")
-    register_email.parts.each do |part|
+    email_text_parts(register_email).each do |part|
       assert_match confirm_regex, part.body.to_s
     end
-    confirm_string = register_email.parts[0].body.match(confirm_regex)[1]
+    confirm_string = email_text_parts(register_email)[0].body.match(confirm_regex)[1]
 
     # Check the page
     assert_response :success
@@ -248,10 +248,10 @@ class UserCreationTest < ActionDispatch::IntegrationTest
     assert_equal register_email.to[0], new_email
     # Check that the confirm account url is correct
     confirm_regex = Regexp.new("/user/redirect_tester_openid/confirm\\?confirm_string=([a-zA-Z0-9]*)")
-    register_email.parts.each do |part|
+    email_text_parts(register_email).each do |part|
       assert_match confirm_regex, part.body.to_s
     end
-    confirm_string = register_email.parts[0].body.match(confirm_regex)[1]
+    confirm_string = email_text_parts(register_email)[0].body.match(confirm_regex)[1]
 
     # Check the page
     assert_response :success
@@ -365,10 +365,10 @@ class UserCreationTest < ActionDispatch::IntegrationTest
     assert_equal register_email.to[0], new_email
     # Check that the confirm account url is correct
     confirm_regex = Regexp.new("/user/redirect_tester_google/confirm\\?confirm_string=([a-zA-Z0-9]*)")
-    register_email.parts.each do |part|
+    email_text_parts(register_email).each do |part|
       assert_match confirm_regex, part.body.to_s
     end
-    confirm_string = register_email.parts[0].body.match(confirm_regex)[1]
+    confirm_string = email_text_parts(register_email)[0].body.match(confirm_regex)[1]
 
     # Check the page
     assert_response :success
@@ -478,10 +478,10 @@ class UserCreationTest < ActionDispatch::IntegrationTest
     assert_equal register_email.to[0], new_email
     # Check that the confirm account url is correct
     confirm_regex = Regexp.new("/user/redirect_tester_facebook/confirm\\?confirm_string=([a-zA-Z0-9]*)")
-    register_email.parts.each do |part|
+    email_text_parts(register_email).each do |part|
       assert_match confirm_regex, part.body.to_s
     end
-    confirm_string = register_email.parts[0].body.match(confirm_regex)[1]
+    confirm_string = email_text_parts(register_email)[0].body.match(confirm_regex)[1]
 
     # Check the page
     assert_response :success
@@ -591,10 +591,10 @@ class UserCreationTest < ActionDispatch::IntegrationTest
     assert_equal register_email.to[0], new_email
     # Check that the confirm account url is correct
     confirm_regex = Regexp.new("/user/redirect_tester_windowslive/confirm\\?confirm_string=([a-zA-Z0-9]*)")
-    register_email.parts.each do |part|
+    email_text_parts(register_email).each do |part|
       assert_match confirm_regex, part.body.to_s
     end
-    confirm_string = register_email.parts[0].body.match(confirm_regex)[1]
+    confirm_string = email_text_parts(register_email)[0].body.match(confirm_regex)[1]
 
     # Check the page
     assert_response :success
@@ -704,10 +704,10 @@ class UserCreationTest < ActionDispatch::IntegrationTest
     assert_equal register_email.to[0], new_email
     # Check that the confirm account url is correct
     confirm_regex = Regexp.new("/user/redirect_tester_github/confirm\\?confirm_string=([a-zA-Z0-9]*)")
-    register_email.parts.each do |part|
+    email_text_parts(register_email).each do |part|
       assert_match confirm_regex, part.body.to_s
     end
-    confirm_string = register_email.parts[0].body.match(confirm_regex)[1]
+    confirm_string = email_text_parts(register_email)[0].body.match(confirm_regex)[1]
 
     # Check the page
     assert_response :success
@@ -817,10 +817,10 @@ class UserCreationTest < ActionDispatch::IntegrationTest
     assert_equal register_email.to[0], new_email
     # Check that the confirm account url is correct
     confirm_regex = Regexp.new("/user/redirect_tester_wikipedia/confirm\\?confirm_string=([a-zA-Z0-9]*)")
-    register_email.parts.each do |part|
+    email_text_parts(register_email).each do |part|
       assert_match confirm_regex, part.body.to_s
     end
-    confirm_string = register_email.parts[0].body.match(confirm_regex)[1]
+    confirm_string = email_text_parts(register_email)[0].body.match(confirm_regex)[1]
 
     # Check the page
     assert_response :success

test/test_helper.rb

@@ -183,5 +183,15 @@ module ActiveSupport
       stub_request(:get, "http://api.hostip.info/country.php?ip=0.0.0.0")
       stub_request(:get, "http://api.hostip.info/country.php?ip=127.0.0.1")
     end
+
+    def email_text_parts(message)
+      message.parts.each_with_object([]) do |part, text_parts|
+        if part.content_type.start_with?("text/")
+          text_parts.push(part)
+        elsif part.multipart?
+          text_parts.concat(email_text_parts(part))
+        end
+      end
+    end
   end
 end