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

[Tom Hughes]

tomhughes requested changes on this pull request.



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

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 `image_path` or something.

> @@ -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}'

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?

Obviously the same applies to other translations.

> +          </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>

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.

Is there something clever we can do here with a layout?

> @@ -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"))

Shouldn't thin use `image_path` or something? Explicitly building a path to an asset like that seems wrong.

-- 
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-14819141
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20170101/d108d4d7/attachment-0001.html>