<p><b>@tomhughes</b> requested changes on this pull request.</p>
<p>Mostly looks fine but I've left a few comments and questions inline - mostly minor nits though.</p><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1401#pullrequestreview-16718626">app/models/notifier.rb</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p>I don't think this variable really needs such a long name. I'd suggest something like <code>@author</code> would be fine and more concise.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1401#pullrequestreview-16718626">app/models/notifier.rb</a>:</p>
<pre style='color:#555'>> mail :to => recipient.email, :subject => subject
end
end
private
+ def set_shared_template_vars
+ @root_url = root_url(:host => SERVER_URL)
+ attach_project_logo
</pre>
<p>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 <code>before_action</code> filter.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1401#pullrequestreview-16718626">app/views/notifier/_user_message_table.html.erb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,18 @@
+<table style="font-size: 15px; font-style: italic; margin: 15px; background-color: #eee; width: 520px">
</pre>
<p>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 <code>_message_body.html.erb</code> instead maybe?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1401#pullrequestreview-16718626">app/views/notifier/changeset_comment_notification.html.erb</a>:</p>
<pre style='color:#555'>> <% else %>
<%= t "notifier.changeset_comment_notification.commented.partial_changeset_without_comment" %>
<% end %>
</p>
-==
-<%= @comment.to_html %>
-==
+<%= render "notifier/user_message_table", :captured => capture { %>
</pre>
<p>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 <code>render :partial => "message_body"</code> here I think.</p>
<p>Also can we rename <code>:captured</code> to <code>:body</code> or something that better specifies what it is, and always use <code>do</code>/<code>end</code> for multiline blocks and not <code>{</code>/<code>}</code> please.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1401#pullrequestreview-16718626">app/views/notifier/changeset_comment_notification.html.erb</a>:</p>
<pre style='color:#555'>>
-<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")) %>
</pre>
<p>The <code><nobr></code> tag is <a href="https://developer.mozilla.org/en-US/docs/Web/HTML/Element/nobr">non-standard</a> so we should use CSS instead unless there's a very good reason.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1401#pullrequestreview-16718626">test/integration/user_creation_test.rb</a>:</p>
<pre style='color:#555'>> 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]
</pre>
<p>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.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1401#pullrequestreview-16718626">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABWnLcMxY9ED3IdVOKNaEbfmGrqXayYWks5rSlZ8gaJpZM4LYyok">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/ABWnLWa2KiWTneiMM1JLQAwImmsS4MHKks5rSlZ8gaJpZM4LYyok.gif" width="1" /></p>
<div itemscope itemtype="http://schema.org/EmailMessage">
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
<link itemprop="url" href="https://github.com/openstreetmap/openstreetmap-website/pull/1401#pullrequestreview-16718626"></link>
<meta itemprop="name" content="View Pull Request"></meta>
</div>
<meta itemprop="description" content="View this Pull Request on GitHub"></meta>
</div>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/openstreetmap/openstreetmap-website","title":"openstreetmap/openstreetmap-website","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/openstreetmap/openstreetmap-website"}},"updates":{"snippets":[{"icon":"PERSON","message":"@tomhughes requested changes on #1401"}],"action":{"name":"View Pull Request","url":"https://github.com/openstreetmap/openstreetmap-website/pull/1401#pullrequestreview-16718626"}}}</script>