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'.
Created attachment 406320 [details] Patch
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?
Oh, I see you said we want to add links?
(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.
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 "<" and "&" as "&" and other such things. Also not sure that using "<br>" is the best way to format HTML mail.
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 "<" and "&" as "&" 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?
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.
(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.
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('&', '&').replace('<', '<').replace('>', '>') 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.
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.
(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.
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.
Committed r265553: <https://trac.webkit.org/changeset/265553>
<rdar://problem/66919501>