Fix URL quoting of message-id to allow email threading. (#1664)

internals/notifier.py

@@ -20,6 +20,7 @@ import logging
 import datetime
 import json
 import os
+import urllib
 
 from framework import ramcache
 from google.cloud import ndb
@@ -289,6 +290,7 @@ def get_thread_id(feature, approval_field):
 
   thread_url = thread_url.split('#')[0]  # Chop off any anchor
   thread_url = thread_url.split('?')[0]  # Chop off any query string params
+  thread_url = urllib.parse.unquote(thread_url)  # Convert %40 to @.
 
   thread_id = None
   if thread_url.startswith(BLINK_DEV_ARCHIVE_URL_PREFIX):
@@ -305,7 +307,7 @@ def post_comment_to_mailing_list(
   to_addr = settings.REVIEW_COMMENT_MAILING_LIST
   from_user = author_addr.split('@')[0]
   approval_field = approval_defs.APPROVAL_FIELDS_BY_ID[approval_field_id]
-  subject = '%s: %s' % (approval_field.name, feature.name)
+  subject = 'Re: %s: %s' % (approval_field.name, feature.name)
   thread_id = get_thread_id(feature, approval_field)
   references = None
   if thread_id:

internals/notifier_test.py

@@ -351,21 +351,24 @@ class FeatureStarTest(testing_config.CustomTestCase):
 class FunctionsTest(testing_config.CustomTestCase):
 
   def setUp(self):
+    quoted_msg_id = 'xxx%3Dyyy%40mail.gmail.com'
+    impl_url = notifier.BLINK_DEV_ARCHIVE_URL_PREFIX + '123' + quoted_msg_id
+    expr_url = notifier.TEST_ARCHIVE_URL_PREFIX + '456' + quoted_msg_id
     self.feature_1 = models.Feature(
         name='feature one', summary='sum', category=1, visibility=1,
         standardization=1, web_dev_views=1, impl_status_chrome=1,
-        intent_to_implement_url=notifier.BLINK_DEV_ARCHIVE_URL_PREFIX + '123',
-        intent_to_experiment_url=notifier.TEST_ARCHIVE_URL_PREFIX + '456')
+        intent_to_implement_url=impl_url,
+        intent_to_experiment_url=expr_url)
     # Note: There is no need to put() it in the datastore.
 
   def test_get_thread_id__normal(self):
     """We can select the correct approval thread field of a feature."""
     self.assertEqual(
-        '123',
+        '123xxx=yyy@mail.gmail.com',
         notifier.get_thread_id(
             self.feature_1, approval_defs.PrototypeApproval))
     self.assertEqual(
-        '456',
+        '456xxx=yyy@mail.gmail.com',
         notifier.get_thread_id(
             self.feature_1, approval_defs.ExperimentApproval))
     self.assertEqual(
@@ -377,6 +380,6 @@ class FunctionsTest(testing_config.CustomTestCase):
     """We can select the correct approval thread field of a feature."""
     self.feature_1.intent_to_implement_url += '?param=val#anchor'
     self.assertEqual(
-        '123',
+        '123xxx=yyy@mail.gmail.com',
         notifier.get_thread_id(
             self.feature_1, approval_defs.PrototypeApproval))

py2/sendemail.py

@@ -80,13 +80,17 @@ def handle_outbound_mail_task():
   message.check_initialized()
 
   if references:
-    message.headers = {'References': references}
+    message.headers = {
+        'References': references,
+        'In-Reply-To': references,
+    }
 
   logging.info('Will send the following email:\n')
   logging.info('Sender: %s', message.sender)
   logging.info('To: %s', message.to)
   logging.info('Subject: %s', message.subject)
   logging.info('References: %s', references or '(not included)')
+  logging.info('In-Reply-To: %s', references or '(not included)')
   logging.info('Body:\n%s', message.html[:settings.MAX_LOG_LINE])
   if settings.SEND_EMAIL:
     message.send()

py2/sendemail_py2test.py

@@ -59,6 +59,7 @@ class OutboundEmailHandlerTest(unittest.TestCase):
     mock_message.send.assert_called_once_with()
     self.assertEqual({'message': 'Done'}, actual_response)
     self.assertEqual(self.refs, mock_message.headers['References'])
+    self.assertEqual(self.refs, mock_message.headers['In-Reply-To'])
 
   @mock.patch('settings.SEND_EMAIL', True)
   @mock.patch('google.appengine.api.mail.EmailMessage')