<p><b>@tomhughes</b> requested changes on this pull request.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1401#pullrequestreview-14819141">app/models/notifier.rb</a>:</p>
<pre style='color:#555'>> mail :to => recipient.email, :subject => subject
end
end
private
+ def user_avatar_file_path(user)
+ image = user.image
+ if image.file?
+ return image.path.sub("/original/", "/small/")
+ else
+ return Rails.root.join("app", "assets", "images", "users", "images", "small.png")
+ end
+ end
+
</pre>
<p>I haven't got time to look it up right now, but this all looks horribly wrong to me. I'm sure the first branch of the if must have a better solution than that if you read up on paperclip and the second ought to be done with <code>image_path</code> or something.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1401#pullrequestreview-14819141">config/locales/en.yml</a>:</p>
<pre style='color:#555'>> @@ -1315,10 +1315,23 @@ en:
subject_own: "[OpenStreetMap] %{commenter} has commented on one of your changesets"
subject_other: "[OpenStreetMap] %{commenter} has commented on a changeset you are interested in"
your_changeset: "%{commenter} has left a comment on one of your changesets created at %{time}"
+ your_changeset_html: '<a href="%{commenter_url}" target="_blank" style="text-decoration: none; color: #222"><strong>%{commenter}</strong></a>
+ has left a comment on one of your changesets
+ created at %{time}'
</pre>
<p>This is a translation nightmare - aside from the fact that it duplicates the plain text version there's all sorts of stuff here that is technical detail that we don't really want to expose translators to if we can avoid it as they'll just manage to break it. Do we really need the strong tag? Without that we can just reuse the text version and put the HTML in the template. If we do want it can't we add it as part of the commenter expansion and achieve the same result?</p>
<p>Obviously the same applies to other translations.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1401#pullrequestreview-14819141">app/views/notifier/changeset_comment_notification.html.erb</a>:</p>
<pre style='color:#555'>> + </table>
+ </td>
+ </tr>
+ <tr>
+ <td style="text-align: center; font-size: 11px">
+ <p>
+ <%= t 'notifier.changeset_comment_notification.unsubscribe_html', :url => @changeset_url %>
+ </p>
+ <p style="margin-bottom: 10px">
+ <a href="<%= @root_url %>" target="_blank" style="color: #222">OpenStreetMap</a>
+ </p>
+ </td>
+ </tr>
+ </table>
+ </body>
+</html>
</pre>
<p>There's a lot of boiler plate here that makes this very hard to read, and if you're planning to expand this to other emails then I then think you need to look for a way to separate it out so that it's not duplicated all over the shop.</p>
<p>Is there something clever we can do here with a layout?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1401#pullrequestreview-14819141">app/models/notifier.rb</a>:</p>
<pre style='color:#555'>> @@ -168,12 +170,24 @@ def changeset_comment_notification(comment, recipient)
I18n.t("notifier.changeset_comment_notification.commented.subject_other", :commenter => @commenter)
end
+ attachments.inline["logo.png"] = File.read(Rails.root.join("app", "assets", "images", "osm_logo_30.png"))
</pre>
<p>Shouldn't thin use <code>image_path</code> or something? Explicitly building a path to an asset like that seems wrong.</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-14819141">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABWnLQ6g7bvLk44jfTN6k7vxI0Ot43dKks5rOEj8gaJpZM4LYyok">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/ABWnLdhAbKmtTParTT9y-fV8a_Y2-ccLks5rOEj8gaJpZM4LYyok.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-14819141"></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-14819141"}}}</script>