Bug 215331 – [ews] send emails as html instead of plain-text

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Aakash Jain]

EWS emails are currently plain-text. We should change them to html. It would allow better formatting. e.g.: including clickable text like 'click here' or 'test history'.

[Aakash Jain]

Created attachment 406320 [details]
Patch

[Darin Adler]

Comment on attachment 406320 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406320&action=review

> Tools/ChangeLog:3
> +        [ews] send emails as html instead of plain-text

Why?

[Darin Adler]

Oh, I see you said we want to add links?

[Aakash Jain]

(In reply to Darin Adler from comment #3)
> Oh, I see you said we want to add links?
Yeah. Adding links for one particular use-case in Bug 215337.

[Darin Adler]

Comment on attachment 406320 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406320&action=review

> Tools/BuildSlaveSupport/ews-build/send_email.py:46
> +    text = text.encode('utf-8')
> +    text = text.replace('\n', '<br>')

This seems to be missing logic to encode "<" as "&lt;" and "&" as "&amp;" and other such things.

Also not sure that using "<br>" is the best way to format HTML mail.

[Jonathan Bedard]

Comment on attachment 406320 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406320&action=review

>> Tools/BuildSlaveSupport/ews-build/send_email.py:46
>> +    text = text.replace('\n', '<br>')
> 
> This seems to be missing logic to encode "<" as "&lt;" and "&" as "&amp;" and other such things.
> 
> Also not sure that using "<br>" is the best way to format HTML mail.

If we really want to worry about encoding all the html bits, maybe we should use something like jinja2?

[Aakash Jain]

Created attachment 406461 [details]
Updated patch

Added logic to encode '<', '>' and '&' in the build logs. Only the build logs contains arbitrary content. Rest of the email content is generated by our code, so wouldn't contains these characters.

Also formatted the logs with <code> tag. I found formatting with <code> little better than <pre> tag.

[Darin Adler]

(In reply to Aakash Jain from comment #7)
> Added logic to encode '<', '>' and '&' in the build logs. Only the build
> logs contains arbitrary content. Rest of the email content is generated by
> our code, so wouldn't contains these characters.

Why not do the encoding closer to where it’s used so we can’t get tripped up by things that happen to contain those characters? The encoding is needed when we decide to treat something as HTML. There’s no reason to program depending on "we know it doesn’t contain these characters" when it doesn’t make the script significantly simpler, more efficient, or more correct.

[Jonathan Bedard]

Comment on attachment 406461 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406461&action=review

> Tools/BuildSlaveSupport/ews-build/steps.py:1477
> +                logs = logs.replace('&', '&amp;').replace('<', '&lt;').replace('>', '&gt;')

Feel like there is more that we need to escape (although I can't think of what that would be at the moment)...not urgent, but it does seems like we should consider using a library of some sort here.

> Tools/BuildSlaveSupport/ews-build/steps.py:1500
> +                email_text += u'\n\nError lines:\n\n<code>{}</code>'.format(logs)

Ditto on the more we need to escape. Also, we might consider putting this escaping in a function so that when we do need to modify it in the future, we won't forget about it.

[Darin Adler]

The best place in the code to escape is when taking a variable containing plain text and inserting it into HTML. Seems unnecessary and unhelpful to do it elsewhere ahead of time.

[Aakash Jain]

(In reply to Darin Adler from comment #8)
> (In reply to Aakash Jain from comment #7)
> > Added logic to encode '<', '>' and '&' in the build logs. Only the build
> > logs contains arbitrary content. Rest of the email content is generated by
> > our code, so wouldn't contains these characters.
> 
> Why not do the encoding closer to where it’s used so we can’t get tripped up
> by things that happen to contain those characters? The encoding is needed
> when we decide to treat something as HTML. There’s no reason to program
> depending on "we know it doesn’t contain these characters" when it doesn’t
> make the script significantly simpler, more efficient, or more correct.
Actually my previous comment wasn't entirely correct. Rest of the email content (generated by our code) also contains these characters ('<','>'), which we don't want to replace. For e.g.: https://bugs.webkit.org/attachment.cgi?id=406329&action=review adds <a href="{}">test history</a>

Given that constraint, this was the closest I could keep it to the email part. It's encoded just before being added to email_text.

Other place I considered putting this was inside filter_logs_containing_error(). But maybe in future, we could consider displaying this filtered output in Buildbot UI or status-bubble hover-over message, where this encoding wouldn't be required.

[Aakash Jain]

Comment on attachment 406461 [details]
Updated patch

Marked as r? again since the patch has changed since last r+.

Red 'services' bubble is due to recently landed Bug 215401 which requires buildbot restart.

[Aakash Jain]

Committed r265553: <https://trac.webkit.org/changeset/265553>

[Radar WebKit Bug Importer]

<rdar://problem/66919501>