[openstreetmap/openstreetmap-website] Dressed up notification mails (#1401)

[Tom Hughes]

tomhughes requested changes on this pull request.

Mostly looks fine but I've left a few comments and questions inline - mostly minor nits though.

> @@ -76,6 +77,9 @@ def message_notification(message)
       @replyurl = url_for(:host => SERVER_URL,
                           :controller => "message", :action => "reply",
                           :message_id => message.id)
+      @user_message_author = @from_user

I don't think this variable really needs such a long name. I'd suggest something like `@author` would be fine and more concise.

>        mail :to => recipient.email, :subject => subject
     end
   end
 
   private
 
+  def set_shared_template_vars
+    @root_url = root_url(:host => SERVER_URL)
+    attach_project_logo

This isn't really setting a shared variable so I don't think it really belongs here. I'd suggest just adding it as a separate `before_action` filter.

> @@ -0,0 +1,18 @@
+<table style="font-size: 15px; font-style: italic; margin: 15px; background-color: #eee; width: 520px">

I don't really like the name of this file, especially the encoding of an implementation detail (the fact that it's a table) in the name. Can we just use `_message_body.html.erb` instead maybe?

>    <% else %>
     <%= t "notifier.changeset_comment_notification.commented.partial_changeset_without_comment" %>
   <% end %>
 </p>
 
-==
-<%= @comment.to_html %>
-==
+<%= render "notifier/user_message_table", :captured => capture { %>

You don't need to specify the controller if it's the same, and we normally explicitly specify that it's a partial so use `render :partial => "message_body"` here I think.

Also can we rename `:captured` to `:body` or something that better specifies what it is, and always use `do`/`end` for multiline blocks and not `{`/`}` please.

>  
-<p><%= raw t 'notifier.changeset_comment_notification.details', :url => link_to(@changeset_url, @changeset_url) %></p>
+<% content_for :footer do %>
+  <p>
+    <%= raw t 'notifier.changeset_comment_notification.unsubscribe', :url => content_tag("nobr", link_to(@changeset_url, @changeset_url, :style => "color: #222")) %>

The `<nobr>` tag is [non-standard](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/nobr) so we should use CSS instead unless there's a very good reason.

>        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]

What was the reason for changing all these to only check the text parts? The consequence is that we're no longer validating that the HTML part includes the required links which I'd like to avoid if possible.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/1401#pullrequestreview-16718626
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20170115/40d2b84b/attachment.html>