215331 2020-08-10 12:45:01 -0700 [ews] send emails as html instead of plain-text 2020-08-12 14:04:51 -0700 1 1 1 Unclassified WebKit Tools / Tests Other Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 215220 215337 215425 1 aakash_jain aakash_jain aakash_jain ap darin dewei_zhu jbedard ryanhaddad webkit-bug-importer oldest_to_newest 1679161 0 aakash_jain 2020-08-10 12:45:01 -0700 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'. 1679162 1 406320 aakash_jain 2020-08-10 12:49:26 -0700 Created attachment 406320 Patch 1679183 2 406320 darin 2020-08-10 13:35:09 -0700 Comment on attachment 406320 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? 1679184 3 darin 2020-08-10 13:35:43 -0700 Oh, I see you said we want to add links? 1679205 4 aakash_jain 2020-08-10 14:24:50 -0700 (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. 1679206 5 406320 darin 2020-08-10 14:30:34 -0700 Comment on attachment 406320 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. 1679425 6 406320 jbedard 2020-08-11 09:24:07 -0700 Comment on attachment 406320 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? 1679747 7 406461 aakash_jain 2020-08-12 09:48:07 -0700 Created attachment 406461 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. 1679749 8 darin 2020-08-12 09:52:15 -0700 (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. 1679750 9 406461 jbedard 2020-08-12 09:54:26 -0700 Comment on attachment 406461 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. 1679751 10 darin 2020-08-12 10:00:15 -0700 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. 1679755 11 aakash_jain 2020-08-12 10:08:38 -0700 (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. 1679760 12 406461 aakash_jain 2020-08-12 10:37:41 -0700 Comment on attachment 406461 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. 1679764 13 aakash_jain 2020-08-12 10:55:30 -0700 Committed r265553: <https://trac.webkit.org/changeset/265553> 1679765 14 webkit-bug-importer 2020-08-12 10:56:20 -0700 <rdar://problem/66919501> 406320 2020-08-10 12:49:26 -0700 2020-08-12 09:48:07 -0700 Patch bug-215331-20200810154925.patch text/plain 1729 aakash_jain SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDI2NTQ0MykKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDEzIEBACisyMDIwLTA4LTEwICBBYWthc2ggSmFpbiAgPGFha2FzaF9qYWluQGFwcGxlLmNv bT4KKworICAgICAgICBbZXdzXSBzZW5kIGVtYWlscyBhcyBodG1sIGluc3RlYWQgb2YgcGxhaW4t dGV4dAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjE1 MzMxCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBC dWlsZFNsYXZlU3VwcG9ydC9ld3MtYnVpbGQvc2VuZF9lbWFpbC5weToKKyAgICAgICAgKHNlbmRf ZW1haWwpOgorCiAyMDIwLTA4LTEwICBBYWthc2ggSmFpbiAgPGFha2FzaF9qYWluQGFwcGxlLmNv bT4KIAogICAgICAgICBbZXdzXSBIYW5kbGUgbG9ncyB3aXRoIHVuaWNvZGUgY2hhcmFjdGVycyBp biBld3MgZW1haWxzCkluZGV4OiBUb29scy9CdWlsZFNsYXZlU3VwcG9ydC9ld3MtYnVpbGQvc2Vu ZF9lbWFpbC5weQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9CdWlsZFNsYXZlU3VwcG9ydC9ld3MtYnVp bGQvc2VuZF9lbWFpbC5weQkocmV2aXNpb24gMjY1NDQzKQorKysgVG9vbHMvQnVpbGRTbGF2ZVN1 cHBvcnQvZXdzLWJ1aWxkL3NlbmRfZW1haWwucHkJKHdvcmtpbmcgY29weSkKQEAgLTIzLDYgKzIz LDggQEAKIGltcG9ydCBvcwogaW1wb3J0IHNtdHBsaWIKIAorZnJvbSBlbWFpbC5taW1lLnRleHQg aW1wb3J0IE1JTUVUZXh0CisKIGlzX3Rlc3RfbW9kZV9lbmFibGVkID0gb3MuZ2V0ZW52KCdCVUlM REJPVF9QUk9EVUNUSU9OJykgaXMgTm9uZQogCiBCT1RfV0FUQ0hFUlNfRU1BSUxTID0gWyd3ZWJr aXQtZXdzLWJvdC13YXRjaGVyc0Bncm91cC5hcHBsZS5jb20nXQpAQCAtNDAsMTAgKzQyLDE2IEBA IGRlZiBzZW5kX2VtYWlsKHRvX2VtYWlscywgc3ViamVjdCwgdGV4dCkKICAgICAgICAgcHJpbnQo J1NraXBwaW5nIGVtYWlsIHNpbmNlIG5vIHN1YmplY3Qgb3IgdGV4dCBpcyBzcGVjaWZpZWQnKQog ICAgICAgICByZXR1cm4KIAotICAgIGVtYWlsX2RhdGEgPSAiIiJGcm9tOiB7fVxuVG86IHt9XG5T dWJqZWN0OiB7fVxuXG57fSIiIi5mb3JtYXQoRlJPTV9FTUFJTCwgJywgJy5qb2luKHRvX2VtYWls cyksIHN1YmplY3QsIHRleHQuZW5jb2RlKCd1dGYtOCcpKQorICAgIHRleHQgPSB0ZXh0LmVuY29k ZSgndXRmLTgnKQorICAgIHRleHQgPSB0ZXh0LnJlcGxhY2UoJ1xuJywgJzxicj4nKQorCisgICAg bXNnID0gTUlNRVRleHQodGV4dCwgJ2h0bWwnKQorICAgIG1zZ1snRnJvbSddID0gRlJPTV9FTUFJ TAorICAgIG1zZ1snVG8nXSA9ICcsICcuam9pbih0b19lbWFpbHMpCisgICAgbXNnWydTdWJqZWN0 J10gPSBzdWJqZWN0CiAKICAgICBzZXJ2ZXIgPSBzbXRwbGliLlNNVFAoU0VSVkVSKQotICAgIHNl cnZlci5zZW5kbWFpbChGUk9NX0VNQUlMLCB0b19lbWFpbHMsIGVtYWlsX2RhdGEpCisgICAgc2Vy dmVyLnNlbmRtYWlsKEZST01fRU1BSUwsIHRvX2VtYWlscywgbXNnLmFzX3N0cmluZygpKQogICAg IHNlcnZlci5xdWl0KCkKIAogCg== 406461 2020-08-12 09:48:07 -0700 2020-08-12 10:49:24 -0700 Updated patch html_email.patch text/plain 3832 aakash_jain SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDI2NTU1MSkKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE2IEBACisyMDIwLTA4LTEyICBBYWthc2ggSmFpbiAgPGFha2FzaF9qYWluQGFwcGxlLmNv bT4KKworICAgICAgICBbZXdzXSBzZW5kIGVtYWlscyBhcyBodG1sIGluc3RlYWQgb2YgcGxhaW4t dGV4dAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjE1 MzMxCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgRGFyaW4gQWRsZXIuCisKKyAgICAgICAgKiBCdWls ZFNsYXZlU3VwcG9ydC9ld3MtYnVpbGQvc2VuZF9lbWFpbC5weToKKyAgICAgICAgKHNlbmRfZW1h aWwpOiBTZW5kIGVtYWlsIGFzIGh0bWwgaW5zdGVhZCBvZiBwbGFpbi10ZXh0LgorICAgICAgICAq IEJ1aWxkU2xhdmVTdXBwb3J0L2V3cy1idWlsZC9zdGVwcy5weToKKyAgICAgICAgKEFuYWx5emVD b21waWxlV2ViS2l0UmVzdWx0cy5zZW5kX2VtYWlsX2Zvcl9uZXdfYnVpbGRfZmFpbHVyZSk6IEVu Y29kZSBmZXcgY2hhcmFjdGVycyBsaWtlIDwsPiwmIGZvciBodG1sLiBBbHNvIGZvcm1hdCB0aGUg bG9ncyBpbiA8Y29kZT4gYmxvY2suCisgICAgICAgIChBbmFseXplQ29tcGlsZVdlYktpdFJlc3Vs dHMuc2VuZF9lbWFpbF9mb3JfcHJlZXhpc3RpbmdfYnVpbGRfZmFpbHVyZSk6IERpdHRvLgorCiAy MDIwLTA4LTEyICBKb25hdGhhbiBCZWRhcmQgIDxqYmVkYXJkQGFwcGxlLmNvbT4KIAogICAgICAg ICBbcmVzdWx0c2RicHldIE1vdmUgdG8gVG9vbHMvU2NyaXB0cy9saWJyYXJpZXMKSW5kZXg6IFRv b2xzL0J1aWxkU2xhdmVTdXBwb3J0L2V3cy1idWlsZC9zZW5kX2VtYWlsLnB5Cj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFRvb2xzL0J1aWxkU2xhdmVTdXBwb3J0L2V3cy1idWlsZC9zZW5kX2VtYWlsLnB5CShyZXZp c2lvbiAyNjU1NTEpCisrKyBUb29scy9CdWlsZFNsYXZlU3VwcG9ydC9ld3MtYnVpbGQvc2VuZF9l bWFpbC5weQkod29ya2luZyBjb3B5KQpAQCAtMjMsNiArMjMsOCBAQAogaW1wb3J0IG9zCiBpbXBv cnQgc210cGxpYgogCitmcm9tIGVtYWlsLm1pbWUudGV4dCBpbXBvcnQgTUlNRVRleHQKKwogaXNf dGVzdF9tb2RlX2VuYWJsZWQgPSBvcy5nZXRlbnYoJ0JVSUxEQk9UX1BST0RVQ1RJT04nKSBpcyBO b25lCiAKIEJPVF9XQVRDSEVSU19FTUFJTFMgPSBbJ3dlYmtpdC1ld3MtYm90LXdhdGNoZXJzQGdy b3VwLmFwcGxlLmNvbSddCkBAIC00MCwxMCArNDIsMTYgQEAgZGVmIHNlbmRfZW1haWwodG9fZW1h aWxzLCBzdWJqZWN0LCB0ZXh0KQogICAgICAgICBwcmludCgnU2tpcHBpbmcgZW1haWwgc2luY2Ug bm8gc3ViamVjdCBvciB0ZXh0IGlzIHNwZWNpZmllZCcpCiAgICAgICAgIHJldHVybgogCi0gICAg ZW1haWxfZGF0YSA9ICIiIkZyb206IHt9XG5Ubzoge31cblN1YmplY3Q6IHt9XG5cbnt9IiIiLmZv cm1hdChGUk9NX0VNQUlMLCAnLCAnLmpvaW4odG9fZW1haWxzKSwgc3ViamVjdCwgdGV4dC5lbmNv ZGUoJ3V0Zi04JykpCisgICAgdGV4dCA9IHRleHQuZW5jb2RlKCd1dGYtOCcpCisgICAgdGV4dCA9 IHRleHQucmVwbGFjZSgnXG4nLCAnPGJyPicpCisKKyAgICBtc2cgPSBNSU1FVGV4dCh0ZXh0LCAn aHRtbCcpCisgICAgbXNnWydGcm9tJ10gPSBGUk9NX0VNQUlMCisgICAgbXNnWydUbyddID0gJywg Jy5qb2luKHRvX2VtYWlscykKKyAgICBtc2dbJ1N1YmplY3QnXSA9IHN1YmplY3QKIAogICAgIHNl cnZlciA9IHNtdHBsaWIuU01UUChTRVJWRVIpCi0gICAgc2VydmVyLnNlbmRtYWlsKEZST01fRU1B SUwsIHRvX2VtYWlscywgZW1haWxfZGF0YSkKKyAgICBzZXJ2ZXIuc2VuZG1haWwoRlJPTV9FTUFJ TCwgdG9fZW1haWxzLCBtc2cuYXNfc3RyaW5nKCkpCiAgICAgc2VydmVyLnF1aXQoKQogCiAKSW5k ZXg6IFRvb2xzL0J1aWxkU2xhdmVTdXBwb3J0L2V3cy1idWlsZC9zdGVwcy5weQo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBUb29scy9CdWlsZFNsYXZlU3VwcG9ydC9ld3MtYnVpbGQvc3RlcHMucHkJKHJldmlzaW9u IDI2NTU1MSkKKysrIFRvb2xzL0J1aWxkU2xhdmVTdXBwb3J0L2V3cy1idWlsZC9zdGVwcy5weQko d29ya2luZyBjb3B5KQpAQCAtMTQ3NCw3ICsxNDc0LDggQEAgY2xhc3MgQW5hbHl6ZUNvbXBpbGVX ZWJLaXRSZXN1bHRzKGJ1aWxkcwogICAgICAgICAgICAgZW1haWxfdGV4dCA9ICdFV1MgaGFzIGRl dGVjdGVkIGJ1aWxkIGZhaWx1cmUgb24ge30gd2hpbGUgdGVzdGluZyBQYXRjaCB7fS4nLmZvcm1h dChidWlsZGVyX25hbWUsIHBhdGNoX2lkKQogICAgICAgICAgICAgZW1haWxfdGV4dCArPSAnXG5c bkZ1bGwgZGV0YWlscyBhcmUgYXZhaWxhYmxlIGF0OiB7fVxuXG5QYXRjaCBhdXRob3I6IHt9Jy5m b3JtYXQoYnVpbGRfdXJsLCBwYXRjaF9hdXRob3IpCiAgICAgICAgICAgICBpZiBsb2dzOgotICAg ICAgICAgICAgICAgIGVtYWlsX3RleHQgKz0gdSdcblxuRXJyb3IgbGluZXM6XG5cbnt9Jy5mb3Jt YXQobG9ncykKKyAgICAgICAgICAgICAgICBsb2dzID0gbG9ncy5yZXBsYWNlKCcmJywgJyZhbXA7 JykucmVwbGFjZSgnPCcsICcmbHQ7JykucmVwbGFjZSgnPicsICcmZ3Q7JykKKyAgICAgICAgICAg ICAgICBlbWFpbF90ZXh0ICs9IHUnXG5cbkVycm9yIGxpbmVzOlxuXG48Y29kZT57fTwvY29kZT4n LmZvcm1hdChsb2dzKQogICAgICAgICAgICAgZW1haWxfdGV4dCArPSAnXG5cblRvIHVuc3Vic2Ny aWJsZSBmcm9tIHRoZXNlIG5vdGlmaWNhdGlvbnMgb3IgdG8gcHJvdmlkZSBhbnkgZmVlZGJhY2sg cGxlYXNlIGVtYWlsIGFha2FzaF9qYWluQGFwcGxlLmNvbScKICAgICAgICAgICAgIHNlbmRfZW1h aWwoW3BhdGNoX2F1dGhvcl0sIGVtYWlsX3N1YmplY3QsIGVtYWlsX3RleHQpCiAgICAgICAgIGV4 Y2VwdCBFeGNlcHRpb24gYXMgZToKQEAgLTE0OTUsNyArMTQ5Niw4IEBAIGNsYXNzIEFuYWx5emVD b21waWxlV2ViS2l0UmVzdWx0cyhidWlsZHMKICAgICAgICAgICAgIGVtYWlsX3N1YmplY3QgPSAn QnVpbGQgZmFpbHVyZSBvbiB0cnVuayBvbiB7fScuZm9ybWF0KGJ1aWxkZXJfbmFtZSkKICAgICAg ICAgICAgIGVtYWlsX3RleHQgPSAnRmFpbGVkIHRvIGJ1aWxkIFdlYktpdCB3aXRob3V0IHBhdGNo IGluIHt9XG5cbkJ1aWxkZXI6IHt9XG5cbldvcmtlcjoge30nLmZvcm1hdChidWlsZF91cmwsIGJ1 aWxkZXJfbmFtZSwgd29ya2VyX25hbWUpCiAgICAgICAgICAgICBpZiBsb2dzOgotICAgICAgICAg ICAgICAgIGVtYWlsX3RleHQgKz0gdSdcblxuRXJyb3IgbGluZXM6XG5cbnt9Jy5mb3JtYXQobG9n cykKKyAgICAgICAgICAgICAgICBsb2dzID0gbG9ncy5yZXBsYWNlKCcmJywgJyZhbXA7JykucmVw bGFjZSgnPCcsICcmbHQ7JykucmVwbGFjZSgnPicsICcmZ3Q7JykKKyAgICAgICAgICAgICAgICBl bWFpbF90ZXh0ICs9IHUnXG5cbkVycm9yIGxpbmVzOlxuXG48Y29kZT57fTwvY29kZT4nLmZvcm1h dChsb2dzKQogICAgICAgICAgICAgc2VuZF9lbWFpbF90b19ib3Rfd2F0Y2hlcnMoZW1haWxfc3Vi amVjdCwgZW1haWxfdGV4dCkKICAgICAgICAgZXhjZXB0IEV4Y2VwdGlvbiBhcyBlOgogICAgICAg ICAgICAgcHJpbnQoJ0Vycm9yIGluIHNlbmRpbmcgZW1haWwgZm9yIGJ1aWxkIGZhaWx1cmU6IHt9 Jy5mb3JtYXQoZSkpCg==