Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Dressed up notification mails #1401

Merged
merged 42 commits into from Feb 5, 2017
Merged

Dressed up notification mails #1401

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
e6d1958
intial commit, for 'en' locale only, for changeset_comment_notificati…
saintamh Dec 29, 2016
45e90cf
reverting erroneous deletion in previous commit
saintamh Dec 29, 2016
689769c
avatar inline attachment
saintamh Dec 29, 2016
1f0f7ba
avatar inline attachment; text alignment
saintamh Dec 29, 2016
94062a1
Cleaned up HTML
saintamh Dec 30, 2016
5f663b7
Tweaked cell padding
saintamh Dec 31, 2016
b5ef2be
Better loading of attached images
saintamh Jan 1, 2017
20bd72d
Move localised text to locale file
saintamh Jan 1, 2017
1d86379
Remove unused import
saintamh Jan 1, 2017
b5fd9f5
Use parameterised server root URL rather than hardcoded
saintamh Jan 1, 2017
20a74fc
Remove unused variable
saintamh Jan 1, 2017
7a4e13e
Fixed quotes
saintamh Jan 1, 2017
ab9aaf7
Proper way to get path to small avatar
saintamh Jan 2, 2017
beaa85e
More compact image paths
saintamh Jan 2, 2017
ac08130
Remove HTML markup from locale file
saintamh Jan 3, 2017
479a4bd
First steps towards using layouts
saintamh Jan 6, 2017
417176f
Moved message table HTMl to a partial template
saintamh Jan 8, 2017
3aa69ff
Dressed up a few more messages.
saintamh Jan 9, 2017
31778fd
Removed unbalanced tag HTML tag
saintamh Jan 11, 2017
0449024
Modify tests to search only text parts
saintamh Jan 11, 2017
e59d6c5
Rubocop finds 'return' statements redundant
saintamh Jan 11, 2017
39c3fa4
Added a greeting to the changeset comment message
saintamh Jan 12, 2017
22f7c1f
Dressed up friend_notification mail
saintamh Jan 12, 2017
6d1c10a
Dressed up gpx notification mails
saintamh Jan 12, 2017
235e77f
Dressed up lost password mail
saintamh Jan 12, 2017
7ef57b2
Dressed up note comment messages
saintamh Jan 12, 2017
75f96fc
Logo was being attached twice on changeset comment notifications
saintamh Jan 13, 2017
c942138
Simplified the partial layout
saintamh Jan 13, 2017
433b7c5
Remove that ugly style="margin: 0"
saintamh Jan 13, 2017
a0627ec
Revert "Added a greeting to the changeset comment message"
saintamh Jan 14, 2017
1e19e80
Renamed @user_message_author to @author as suggested
saintamh Jan 16, 2017
e5152c3
Move attach_project_logo to be its own before_action filter, as sugge…
saintamh Jan 16, 2017
e3e5ad4
Refactored the code to invoke the message body partial layout, as sug…
saintamh Jan 16, 2017
1285c9f
Convert those <nobr> tags to CSS
saintamh Jan 16, 2017
774e583
Rubocop says to use "hash rockets"
saintamh Jan 16, 2017
25d8078
Put the user avatar in a <p> block
saintamh Jan 23, 2017
cf11913
Apply inline styling to all <p> tags
saintamh Jan 25, 2017
1def88b
Left-align logo
saintamh Jan 25, 2017
f83b719
Set border=0 on that img
saintamh Jan 27, 2017
a371aad
Renamed `apply_inline_css` to `style_message` as suggested
saintamh Jan 28, 2017
74e28f8
Use each_with_object and make the code pithier
saintamh Jan 28, 2017
223a12c
That new message in the HTML version of the mail should also be added…
saintamh Jan 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Binary file added app/assets/images/osm_logo_30.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
14 changes: 14 additions & 0 deletions app/models/notifier.rb
View file
Open in desktop
Expand Up @@ -154,10 +154,12 @@ def note_comment_notification(comment, recipient)

def changeset_comment_notification(comment, recipient)
with_recipient_locale recipient do
@root_url = root_url(:host => SERVER_URL)
@changeset_url = changeset_url(comment.changeset, :host => SERVER_URL)
@comment = comment.body
@owner = recipient == comment.changeset.user
@commenter = comment.author.display_name
@commenter_url = user_url(comment.author.display_name, :host => SERVER_URL)
@changeset_comment = comment.changeset.tags["comment"].presence
@time = comment.created_at
@changeset_author = comment.changeset.user.display_name
Expand All @@ -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"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed it to File.read("#{Rails.root}/app/assets/images/osm_logo_30.png"), as discussed below.

attachments.inline["avatar.png"] = File.read(user_avatar_file_path(comment.author))

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

Copy link
Member

@tomhughes tomhughes Jan 2, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

...so having now looked it up image.path(:small) is the correct way to do the first one and image_path("users/images/small.png") for the second - both should give a root relative path to the file which you can just join to Rails.root.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image.path(:small) works, thanks.

However image_path("users/images/small.png") returns "/assets/users/images/small-19cb2e87cebaee3dbe77d1b540f68136dae6f8e63ec055907d19f639773101e2.png", which is the path part of a public URL. But I want something that I can File.read.

Would you find the above code less objectionable if I just concatenated it all into one string using slashes, e.g. "#{Rails.root}/app/assets/images/users/images/small.png" ? That seems more in line with what other parts of the code already do (for config files, though, not for asset files).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well in production you can just concat that with Rails.public_path and it should read fine though I'll grant you that might not work in development...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect your single string version is probably the best option. It shouldn't matter that we're avoiding the asset pipeline give it's all happening server side...

def with_recipient_locale(recipient)
I18n.with_locale Locale.available.preferred(recipient.preferred_languages) do
yield
Expand Down
93 changes: 73 additions & 20 deletions app/views/notifier/changeset_comment_notification.html.erb
View file
Open in desktop
@@ -1,20 +1,73 @@
<p><%= t 'notifier.changeset_comment_notification.greeting' %></p>

<p>
<% if @owner %>
<%= t "notifier.changeset_comment_notification.commented.your_changeset", :commenter => @commenter, :time => @time %>
<% else %>
<%= t "notifier.changeset_comment_notification.commented.commented_changeset", :commenter => @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 %>
<% else %>
<%= t "notifier.changeset_comment_notification.commented.partial_changeset_without_comment" %>
<% end %>
</p>

==
<%= @comment.to_html %>
==

<p><%= raw t 'notifier.changeset_comment_notification.details', :url => link_to(@changeset_url, @changeset_url) %></p>
<html>
<head>
<title></title>
<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">
<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: 15px 15px 5px 15px">
<p style="margin: 0">
<% if @owner %>
<%= t "notifier.changeset_comment_notification.commented.your_changeset_html", :commenter => @commenter, :commenter_url => @commenter_url, :time => @time %>
<% else %>
<%= t "notifier.changeset_comment_notification.commented.commented_changeset_html", :commenter => @commenter, :commenter_url => @commenter_url, :time => @time, :changeset_author => @changeset_author %>
<% end %>
<% if @changeset_comment %>
<%= t "notifier.changeset_comment_notification.commented.partial_changeset_with_comment_html", :changeset_comment => @changeset_comment %>
<% else %>
<%= t "notifier.changeset_comment_notification.commented.partial_changeset_without_comment" %>
<% end %>
</p>
<table style="font-size: 15px; font-style: italic; margin: 15px; background-color: #eee; width: 520px">
<tr>
<td style="width: 50px; vertical-align: top; padding: 15px">
<a href="<%= @commenter_url %>" target="_blank"><%= image_tag attachments["avatar.png"].url, alt: @commenter %></a>
</td>
<td style="text-align: left; vertical-align: top; padding-right: 10px">
<%= @comment.to_html %>
</td>
</tr>
</table>
<p>
<%= raw t 'notifier.changeset_comment_notification.details_html', :url => link_to(@changeset_url, @changeset_url) %>
</p>
</td>
</tr>
</table>
</td>
</tr>
</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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

13 changes: 13 additions & 0 deletions config/locales/en.yml
View file
Open in desktop
Expand Up @@ -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}'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now moved the HTML markup to the template. The *_html duplicated strings are no longer necessary and have been removed.

commented_changeset: "%{commenter} has left a comment on a map changeset you are watching created by %{changeset_author} at %{time}"
commented_changeset_html: '<a href="%{commenter_url}" target="_blank" style="text-decoration: none; color: #222"><strong>%{commenter}</strong></a>
has left a comment on a map changeset you are watching created by
%{changeset_author}
at %{time}'
partial_changeset_with_comment: "with comment '%{changeset_comment}'"
partial_changeset_with_comment_html: 'with comment <em>%{changeset_comment}</em>'
partial_changeset_without_comment: "without comment"
details: "More details about the changeset can be found at %{url}."
details_html: 'More details about the changeset can be found at
<nobr>%{url}</nobr>.'
unsubscribe_html: 'To unsubscribe from updates to this changeset,
visit <nobr><a style="color: #222" href="%{url}">%{url}</a></nobr>
and click "Unsubscribe".'
message:
inbox:
title: "Inbox"
Expand Down