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

Dressed up notification mails #1401

Merged

Conversation

Projects
None yet
8 participants
@saintamh
Copy link

commented Jan 1, 2017

This is an attempt at dressing up a bit the notification emails that OSM.org sends out. Currently they look rather plain, and while we coder types may be used to that style, others might be surprised by it. My hope is that with more "conventional" or "modern" looking messages we might just improve response rates among casual mappers.

I've put together a basic implementation (see screenshots below). This pull request is still incomplete, but I'm putting it here to open a discussion regarding both design and implementation. I'm not a web designer, and I have only passing familiarity with Rails or with Ruby, so if you see anything that looks wrong, it probably is -- let me know.

Writing portable HTML/CSS for email clients is trickier than for browsers. This table is a handy reference. The current code is not pretty, but I've tried it in a few clients so far and it renders OK, though there's still GUI kinks to be ironed out. I'm attaching a few screenshots. You can see there's still bugs, e.g. the spacing above the message body is too thin on Yahoo.

Open questions

Shared template: So far I've only implemented changeset_comment_notification.html.erb. Since every mail, regardless of whether it's a notification for a message, for a changeset comment, or for a new friend, would have the same header, footer, and overall structure, I'd think it would make sense to have that overall structure in a single file. If we decide to tweak the CSS, we probably want to have to do it in one place only. What's the proper Rails way to reuse the template like that? EDIT 2017-01-12 - now implemented, as a partial layout

Attaching images: Currently two images are used: one for the logo and one for the user's avatar. The current code attaches them to the mail and displays them inline. This gets around some clients' protection against remote images (though some still require a click to display them, e.g. Yahoo). Attaching images increases the size of each message significantly, will this have an impact on the load that the servers can handle?

HTML in I18N strings: I've added an "*_html" companion to the relevant I18N strings. We need HTML tags within the text, though, and since you can't have <style> CSS blocks in emails, the styling goes right into the locale file, which feels just wrong. Is there a better way? EDIT - 2017-01-12 - now removed, existing I18N strings now left untouched

New I18N string: The current design would require one new translation. The English runs "To unsubscribe from updates to this changeset, visit and click 'Unsubscribe'". Is this a possibility? How long does it take to get new translations done?

TODO

  • All message templates, not just changeset comment notifications.
  • Create HTML copies of the relevant I18N strings for all languages. EDIT 2017-01-12 - No longer necessary
  • Test the rendering of the mails on several more platforms once the design is set.
  • Handle users without avatars, or who use Gravatar

Screenshots

gmail_chrome_linux--long--avatar

saintamh added some commits Dec 29, 2016

Cleaned up HTML
Removed some cargo-cult HTML attributes and CSS. Styling HTML for email clients is trickier than for browsers, I'm trying to keep the code as succinct but also as compatible as possible.
Tweaked cell padding
Tweaked cell padding to align the top of the avatar and of the message text a bit better.
Better loading of attached images
Got rid of that big ugly base64 blob which was only a temporary crutch anyway. Added a png file instead. Use File.read rather than File.open for a 66% reduction in verbosity.
Move localised text to locale file
Moved a string of English text that will require translation from the HTML template to the locale I18N file.
@pnorman

This comment has been minimized.

Copy link
Contributor

commented Jan 1, 2017

I've put together a basic implementation (see screenshots below)

How do these look in different email clients, not just webmail?

@pnorman

This comment has been minimized.

Copy link
Contributor

commented Jan 1, 2017

The main travis errors are from rubocop and are about the type of quotes

Fixed quotes
Replaced single quotes with double quotes, to comply with rubocop rules.
@saintamh

This comment has been minimized.

Copy link
Author

commented Jan 1, 2017

So far the only non-webmail client I've tried is Thunderbird, and it looked good. I've enlisted family members to try it out in Outlook on Windows (a few versions) and Mail.app on MacOS and iOS. I'm waiting for the design to stabilize a bit (assuming there will be changes) before sending them requests for screenshots.

I'm also planning tests in Hotmail, Yandex, Mail.ru, and IMP Horde, which are all web-based. Any suggestions for further platforms to test are much welcome.

I think I've fixed the single-quote vs double-quote issue.

return Rails.root.join("app", "assets", "images", "users", "images", "small.png")
end
end

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jan 2, 2017

Member

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.

This comment has been minimized.

Copy link
@saintamh

saintamh Jan 2, 2017

Author

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).

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jan 2, 2017

Member

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

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jan 2, 2017

Member

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

@@ -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 comment has been minimized.

Copy link
@tomhughes

tomhughes Jan 2, 2017

Member

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.

This comment has been minimized.

Copy link
@saintamh

saintamh Jan 3, 2017

Author

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

</tr>
</table>
</body>
</html>

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jan 2, 2017

Member

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

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jan 2, 2017

Member

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

This comment has been minimized.

Copy link
@saintamh

saintamh Jan 2, 2017

Author

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

@tomhughes

This comment has been minimized.

Copy link
Member

commented Jan 2, 2017

@pnorman web mail clients are oddly generally far more tricky than actual mail clients because they are generally very picky about what HTML they will render...

@erictheise

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2017

Litmus is a commercial service that renders emails as they would appear in many desktop, mobile, & web clients. I do not at present have access to the service but perhaps someone who does would be sympathetic to partnering up with @saintamh to test these emails on their organization's dime.

@saintamh

This comment has been minimized.

Copy link
Author

commented Jan 2, 2017

@pnorman thanks for the valuable feedback!

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

I'll look into image_path.

can't we add it as part of the commenter expansion and achieve the same result?

Yes, that sounds like it would make sense. As I wrote above, I too am not comfortable with having HTML/CSS in the locale files. I'll move the HTML bits to Rubyland, which feels.. less wrong?

I'd rather keep the <strong> emphasis on the user name, though, I think it makes sense for that information to stand out. Unless others disagree?

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

Aha, I think "layouts" might be the Rails keyword I was missing for the template-template I was trying to describe above -- I'll look that up.

I'll try to implement the above in the coming days.

@saintamh

This comment has been minimized.

Copy link
Author

commented Jan 2, 2017

@erictheise Litmus looks like a very valuable tool. Perhaps we could start with more informal testing, and once we've got something we think is right, use a tool like Litmus to thoroughly check rendering.

Unless someone has an account, it looks like the cheapest we can get away with would be to buy one month of access, at $99.

saintamh added some commits Jan 2, 2017

Proper way to get path to small avatar
Rather than performing perilous string substitution on paths, just use the built-in way to select the small version of the avatar image file.
More compact image paths
Make the file paths to image assets more compact. I investigated using `image_path` but could only get it to return the path for a public URL, which is different and also includes the asset pipeline digest.
Remove HTML markup from locale file
Rather than including HTML markup in the locale file (which also has the inconvenience of requiring both a plain-text and an HTML duplicate of the same string), move the markup to the template. Also added a helper to reduce clutter in the template slightly.
:target => "_blank",
:style => "text-decoration: none; color: #222; font-weight: bold"
)
end
end

This comment has been minimized.

Copy link
@saintamh

saintamh Jan 3, 2017

Author

I added the link_to_user helper to declutter the template a bit, but then it means this Ruby file now contains some CSS instructions. What's the proper Rails way to do this?

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jan 3, 2017

Member

Is there something wrong with what you've done? I mean we would normally avoid inline CSS completely but for email that's not possible so I don't see what other option there is?

This comment has been minimized.

Copy link
@saintamh

saintamh Jan 3, 2017

Author

I suppose the alternative would be not to create a helper, and copy-paste its contents in the templates instead of the call to the helper. Inline the helper function, basically. But if you're happy enough with this, I'm happy too.

saintamh added some commits Jan 6, 2017

First steps towards using layouts
Since all HTML mails will presumably share the same overall layout, and the markup is fairly dense, it makes sense to use Rails layouts to share the structure across mail templates. This commit moves the core structure of the HTML mail to a layout. It will need to be further refined so that notifications that involve a text message sent by another user can share the avatar-and-message-text structure.
Moved message table HTMl to a partial template
Several notifications are about a message having been sent from one user to another via OSM. I've moved the code for the HTML table that holds the actual user message, with the avatar and the body text, to its own partial template, so that it can be reused. I've updated a second notifier message to the new template, diary_comment_notification.
Dressed up a few more messages.
email_confirm, message_notification and signup_confirm now use the new templates. This fixes some, but not all, of the currently broken tests.
Modify tests to search only text parts
The tests assumed that every part in the multipart emails that we send were either plain text or HTML. Now we have image attachments, against whose contents the tests were still trying to match regexes. The tests have been modified to only run regexes on the text parts of the mails.
@saintamh

This comment has been minimized.

Copy link
Author

commented Jan 26, 2017

@tomhughes yes, please do. Let me know in particular whether the apply_inline_css helper function I added in cf11913, which runs a regex substitution over a string of HTML, is an acceptable way of doing things.

Meanwhile I wrote a little web-based tool that will let testers send themselves a copy of the new mails, ahead of this being merged. I'm thinking of setting up a wiki page where people can upload screenshots of what the new mails look like in their client. And I'll write a diary entry asking for volunteers. That might give us the breadth of testing that we need, what do you reckon?

Set border=0 on that img
IE9 shows a big nasty blue border on linked images if you don't specify border=0
@tomhughes
Copy link
Member

left a comment

A few more review comments. There's also a live version running at http://tomh.apis.dev.openstreetmap.org/ if people want to test things.

)
end

def apply_inline_css(html)

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jan 28, 2017

Member

I think apply_inline_css is a bit too vague for the name - something like style_message would make it clearer what it does.

This comment has been minimized.

Copy link
@saintamh

saintamh Jan 28, 2017

Author

Makes sense -- done.


def email_text_parts(message)
text_parts = []
message.parts.each do |part|

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jan 28, 2017

Member

You could avoid initialising the array and then returning it by using each_with_object here. So something like:

message.parts.each_with_object([]) do |part,text_parts|

and then get rid of the first and last lines of the method.

This comment has been minimized.

Copy link
@saintamh

saintamh Jan 28, 2017

Author

Nice. Done.

<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 => link_to(@changeset_url, @changeset_url, :style => "color: #222; white-space: nowrap") %>

This comment has been minimized.

Copy link
@tomhughes

tomhughes Jan 28, 2017

Member

If you're going to add completely new information to the message then it should really be added to the text version as well.

This comment has been minimized.

Copy link
@saintamh

saintamh Jan 28, 2017

Author

Facepalm. Done.

@saintamh

This comment has been minimized.

Copy link
Author

commented Jan 28, 2017

Thanks, @tomhughes, I've implemented those changes. Thanks also for deploying to your test instance. I'll point volunteer testers to it, if that's OK?

@tomhughes

This comment has been minimized.

Copy link
Member

commented Jan 29, 2017

@saintamh Sure - that's what it's there for

@tomhughes tomhughes merged commit 223a12c into openstreetmap:master Feb 5, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 79.757%
Details
@saintamh

This comment has been minimized.

Copy link
Author

commented Feb 5, 2017

Thanks for merging @tomhughes ! There's a couple minor display bugs that have cropped up in Mirosoft mail clients (see https://wiki.openstreetmap.org/wiki/Dressed-up_Notification_Mails) and I'm working on a fix. Should I continue pushing to the same branch, or should I open a new PR for those?

Also I never tested what happens for users who use Gravatar. Best case scenario is the avatar won't display, but worst case is the mailer code will error out and the message won't be sent. I'll check that next.

@tomhughes

This comment has been minimized.

Copy link
Member

commented Feb 5, 2017

Open a new PR please @saintamh.

@HolgerJeromin

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2017

Should I wait with testing more clients?

@tomhughes

This comment has been minimized.

Copy link
Member

commented Feb 6, 2017

Don't see why - feel free to do whatever testing you think is good!

@trigpoint

This comment has been minimized.

Copy link

commented Feb 7, 2017

I am finding the messages very difficult to read in my phones native email client. When viewed overall the test is way to small. One zoomed in making the text readable only a small part of the message is viewable meaning a lot of screen dragging is needed.

@saintamh

This comment has been minimized.

Copy link
Author

commented Feb 7, 2017

@trigpoint that sounds worrying. Do other HTML mails usually work well?

What phone are you using, what version?

@trigpoint

This comment has been minimized.

Copy link

commented Feb 8, 2017

I am using a Jolla, running Sailfish OS.
2.0.5.6 Haapajoki

It does not normally have a problem with html e-mail. The problem here is that the content has been forced into a small area, that uses only a small proportion of the screen rendering fonts too small to be read on a 4.5 inch screen.

On my desktop the same e-mails appear as a box in the centre of the window, It looks weid to say the least.

@Zverik

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2017

Reopened note sends a notification with an empty box. Is it expected?

2017-02-08 19 58 59

@saintamh

This comment has been minimized.

Copy link
Author

commented Feb 8, 2017

Definitely not expected, I'll look into that. Thanks a lot for reporting it.

@saintamh

This comment has been minimized.

Copy link
Author

commented Feb 10, 2017

@trigpoint I've made tweaks to the HTML/CSS code that should help when viewing on narrow screens. They're not live yet, but if you'd like, you can send yourself a copy of the latest code using the tool at http://52.56.34.44:8080/ and see whether the situation has improved on your mobile.

@trigpoint

This comment has been minimized.

Copy link

commented Feb 11, 2017

@saintamh Those look a lot better. They are readable on my phone without zoom. I like it, thank you.
screenshot-17-02-11-01-33-00
screenshot-17-02-11-01-34-26

@trigpoint

This comment has been minimized.

Copy link

commented Feb 11, 2017

screenshot-17-02-11-01-38-04

@HolgerJeromin

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2017

I added Android K9 mail program and SquirrelMail webmailer with desktop chrome. All working nice

@neiljp

This comment has been minimized.

Copy link

commented Feb 12, 2017

I'm not sure at first glance how to take a screenshot on my android phone, but chrome+gmail on it has the note text going off beyond the right side and needs to be scrolled; is there any way to just have it fit the width? It shows ok when the phone is tilted, but that orientation isn't guaranteed, and it's a little frustrating to need to scroll/twist when it was working fine before!

@saintamh

This comment has been minimized.

Copy link
Author

commented Feb 12, 2017

Hi Neil,

Could you try sending yourself a mail using the tool at http://52.56.34.44:8080/ and tell me whether the problem persists? I've got a few fixes that are just about to be published, and they should address overflowing on narrow screens -- though I've not tested it on Android. Could you let me know whether it works?

Thanks!

@neiljp

This comment has been minimized.

Copy link

commented Feb 12, 2017

Hi @saintamh,

I've just used the tool and that seems better overall, but the link does stretch the page wide slightly, and looking at the layout I wonder whether it would be good to have the text flow not just to the right, but also below the 'person' image, if possible?

@HolgerJeromin

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2017

On my phone pushing the power and volume down button takes a Screenshot

@saintamh

This comment has been minimized.

Copy link
Author

commented Feb 12, 2017

@neiljp That's a good idea. It's true there's a lot of wasted space under the avatar. I'll look into that, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.