Notify subscribers of a new comment (#2835)

* Notify subscribers of a new comment

* Add /tasks/email-comments notifier task

* Fix

---------

Co-authored-by: Kyle Ju <kyleju@chromium.org>

api/comments_api.py

@@ -17,8 +17,8 @@ from typing import Any
 
 from framework import basehandlers
 from framework import permissions
-from internals.review_models import Activity, Amendment
-from internals import notifier
+from internals.review_models import Activity, Amendment, Gate
+from internals import notifier, notifier_helpers
 
 
 def amendment_to_json_dict(amendment: Amendment) -> dict[str, Any]:
@@ -91,6 +91,15 @@ class CommentsAPI(basehandlers.APIHandler):
           author=user.email(), content=comment_content)
       comment_activity.put()
 
+    # Notify subscribers of new comments when user posts a comment
+    # via the gate column.
+    if gate_id:
+      gate = Gate.get_by_id(gate_id)
+      if not gate:
+        self.abort(404, msg='Gate not found; notifications abort.')
+      notifier_helpers.notify_subscribers_of_new_comments(
+          feature, gate, user.email(), comment_content)
+
     # We can only be certain which intent thread we want to post to with
     # a relevant gate ID in order to get the intent_thread_url field from
     # the corresponding Stage entity.

api/comments_api_test.py

@@ -254,8 +254,9 @@ class CommentsAPITest(testing_config.CustomTestCase):
     self.assertIsNotNone(activity)
     self.assertIsNone(activity.deleted_by)
 
+  @mock.patch('internals.notifier_helpers.notify_subscribers_of_new_comments')
   @mock.patch('internals.approval_defs.get_approvers')
-  def test_post__comment_only(self, mock_get_approvers):
+  def test_post__comment_only(self, mock_get_approvers, mock_notifier):
     """Handler adds a comment only, does not require approval permission."""
     mock_get_approvers.return_value = []
     testing_config.sign_in('user2@chromium.org', 123567890)

dispatch.yaml

@@ -13,5 +13,8 @@ dispatch:
 - url: "*/tasks/email-reviewers"
   service: notifier
 
+- url: "*/tasks/email-comments"
+  service: notifier
+
 - url: "*/*"
   service: default

internals/notifier.py

@@ -161,6 +161,25 @@ def apply_subscription_rules(
   return results
 
 
+def add_core_receivers(fe: FeatureEntry, addr_reasons: dict[str, list[str]]):
+  accumulate_reasons(
+    addr_reasons, fe.owner_emails,
+    'You are listed as an owner of this feature'
+  )
+  accumulate_reasons(
+    addr_reasons, fe.editor_emails,
+    'You are listed as an editor of this feature'
+  )
+  accumulate_reasons(
+    addr_reasons, fe.cc_emails,
+    'You are CC\'d on this feature'
+  )
+  accumulate_reasons(
+    addr_reasons, fe.devrel_emails,
+    'You are a devrel contact for this feature.'
+  )
+
+
 def make_feature_changes_email(fe: FeatureEntry, is_update: bool=False,
     changes: Optional[list]=None):
   """Return a list of task dicts to notify users of feature changes."""
@@ -182,22 +201,7 @@ def make_feature_changes_email(fe: FeatureEntry, is_update: bool=False,
 
   addr_reasons: dict[str, list[str]] = collections.defaultdict(list)
 
-  accumulate_reasons(
-    addr_reasons, fe.owner_emails,
-    'You are listed as an owner of this feature'
-  )
-  accumulate_reasons(
-    addr_reasons, fe.editor_emails,
-    'You are listed as an editor of this feature'
-  )
-  accumulate_reasons(
-    addr_reasons, fe.cc_emails,
-    'You are CC\'d on this feature'
-  )
-  accumulate_reasons(
-    addr_reasons, fe.devrel_emails,
-    'You are a devrel contact for this feature.'
-  )
+  add_core_receivers(fe, addr_reasons)
   accumulate_reasons(
     addr_reasons, watcher_emails,
     'You are watching all feature changes'
@@ -253,6 +257,30 @@ def make_review_requests_email(fe: FeatureEntry, gate_type: int, changes: Option
   return all_tasks
 
 
+def make_new_comments_email(fe: FeatureEntry, gate_type: int, changes: Optional[list]=None):
+  """Return a list of task dicts to notify of new comments."""
+  if changes is None:
+    changes = []
+  fe_stages = stage_helpers.get_feature_stages(fe.key.integer_id())
+  email_html = format_email_body(True, fe, fe_stages, changes)
+
+  subject = 'New comments for feature: %s' % fe.name
+  triggering_user_email = fe.updater_email
+
+  addr_reasons: dict[str, list[str]] = collections.defaultdict(list)
+  add_core_receivers(fe, addr_reasons)
+
+  # Add gate reviewers.
+  approvers = approval_defs.get_approvers(gate_type)
+  reasons = 'You are the reviewer for this gate'
+  accumulate_reasons(addr_reasons, approvers, reasons)
+
+  all_tasks = [convert_reasons_to_task(
+                   addr, reasons, email_html, subject, triggering_user_email)
+               for addr, reasons in sorted(addr_reasons.items())]
+  return all_tasks
+
+
 class FeatureStar(ndb.Model):
   """A FeatureStar represent one user's interest in one feature."""
   email = ndb.StringProperty(required=True)
@@ -430,6 +458,29 @@ class FeatureReviewHandler(basehandlers.FlaskHandler):
     return {'message': 'Done'}
 
 
+class FeatureCommentHandler(basehandlers.FlaskHandler):
+  """This task handles feature comment notifications by making email tasks."""
+
+  IS_INTERNAL_HANDLER = True
+
+  def process_post_data(self, **kwargs):
+    self.require_task_header()
+
+    feature = self.get_param('feature')
+    gate_type = self.get_param('gate_type')
+    changes = self.get_param('changes', required=False) or []
+
+    logging.info('Starting to notify of comments for feature %s',
+                 repr(feature)[:settings.MAX_LOG_LINE])
+
+    fe = FeatureEntry.get_by_id(feature['id'])
+    if fe:
+      email_tasks = make_new_comments_email(fe, gate_type, changes)
+      send_emails(email_tasks)
+
+    return {'message': 'Done'}
+
+
 BLINK_DEV_ARCHIVE_URL_PREFIX = (
     'https://groups.google.com/a/chromium.org/d/msgid/blink-dev/')
 TEST_ARCHIVE_URL_PREFIX = (

internals/notifier_helpers.py

@@ -134,3 +134,24 @@ def notify_subscribers_of_vote_changes(fe: 'FeatureEntry', gate: Gate,
 
   # Create task to email subscribers.
   cloud_tasks_helpers.enqueue_task('/tasks/email-subscribers', params)
+
+
+def notify_subscribers_of_new_comments(fe: 'FeatureEntry', gate: Gate,
+    email: str, comment: str) -> None:
+  """Notify subscribers of a new comment."""
+  gate_id = gate.key.integer_id()
+  gate_url = 'https://chromestatus.com/feature/%s?gate=%s' % (
+      fe.key.integer_id(), gate_id)
+  changed_props = {
+      'prop_name': '%s posted a new comment in %s' % (email, gate_url),
+      'old_val': 'na',
+      'new_val': comment,
+  }
+
+  params = {
+    'changes': [changed_props],
+    'gate_type': gate.gate_type,
+    'feature': converters.feature_entry_to_json_verbose(fe)
+  }
+
+  cloud_tasks_helpers.enqueue_task('/tasks/email-comments', params)

internals/notifier_helpers_test.py

@@ -85,7 +85,7 @@ class ActivityTest(testing_config.CustomTestCase):
     changed_fields = [('editor_emails', None, [])]
     notifier_helpers.notify_subscribers_and_save_amendments(
         self.feature_1, changed_fields)
-    
+
     activities = Activity.get_activities(self.feature_1.key.integer_id())
     # No activity entity created.
     self.assertTrue(len(activities) == 0)
@@ -105,3 +105,10 @@ class ActivityTest(testing_config.CustomTestCase):
     self.assertEqual(activities[0].content, expected_content)
 
     mock_task_helpers.assert_called_once()
+
+  @mock.patch('framework.cloud_tasks_helpers.enqueue_task')
+  def test_notify_subscribers_of_new_comments(self, mock_task_helpers):
+    notifier_helpers.notify_subscribers_of_new_comments(
+        self.feature_1, self.gate_1, 'abc@example.com', 'fake comments')
+
+    mock_task_helpers.assert_called_once()

internals/notifier_test.py

@@ -418,6 +418,66 @@ class EmailFormattingTest(testing_config.CustomTestCase):
         True, self.fe_1, self.fe_1_stages, self.changes)
     mock_get_approvers.assert_called_once_with(1)
 
+  @mock.patch('internals.notifier.format_email_body')
+  @mock.patch('internals.approval_defs.get_approvers')
+  def test_make_new_comments_email(self, mock_get_approvers, mock_f_e_b):
+    """We send email to approvers for a review request."""
+    mock_f_e_b.return_value = 'mock body html'
+    mock_get_approvers.return_value = ['approver1@example.com']
+
+    actual_tasks = notifier.make_new_comments_email(
+        self.fe_1, 1, self.changes)
+    self.assertEqual(6, len(actual_tasks))
+    (review_task_1, feature_cc_task, devrel_task,
+     feature_editor_task, feature_owner_task, feature_editor_task_2) = actual_tasks
+
+    self.assertEqual('New comments for feature: feature one', review_task_1['subject'])
+    self.assertIn('mock body html', review_task_1['html'])
+    self.assertIn('<li>You are the reviewer for this gate</li>',
+      review_task_1['html'])
+    self.assertEqual('approver1@example.com', review_task_1['to'])
+
+    # Notification to feature owner.
+    self.assertEqual('feature_owner@example.com', feature_owner_task['to'])
+    self.assertEqual('New comments for feature: feature one',
+      feature_owner_task['subject'])
+    self.assertIn('mock body html', feature_owner_task['html'])
+    self.assertIn('<li>You are listed as an owner of this feature</li>',
+      feature_owner_task['html'])
+
+    # Notification to feature editor.
+    self.assertEqual('New comments for feature: feature one',
+      feature_editor_task['subject'])
+    self.assertIn('mock body html', feature_editor_task['html'])
+    self.assertIn('<li>You are listed as an editor of this feature</li>',
+      feature_editor_task['html'])
+    self.assertEqual('feature_editor@example.com', feature_editor_task['to'])
+
+    # Notification to devrel to feature changes.
+    self.assertEqual('New comments for feature: feature one', devrel_task['subject'])
+    self.assertIn('mock body html', devrel_task['html'])
+    self.assertIn('<li>You are a devrel contact for this feature.</li>',
+      devrel_task['html'])
+    self.assertEqual('devrel1@gmail.com', devrel_task['to'])
+
+    # Notification to user CC'd on feature changes.
+    self.assertEqual('New comments for feature: feature one',
+      feature_cc_task['subject'])
+    self.assertIn('mock body html', feature_cc_task['html'])
+    self.assertIn('<li>You are CC\'d on this feature</li>',
+      feature_cc_task['html'])
+    self.assertEqual('cc@example.com', feature_cc_task['to'])
+
+    self.assertEqual('New comments for feature: feature one', feature_editor_task_2['subject'])
+    self.assertIn('mock body html', feature_editor_task_2['html'])
+    self.assertIn('<li>You are listed as an editor of this feature</li>',
+      feature_editor_task_2['html'])
+    self.assertEqual('owner_1@example.com', feature_editor_task_2['to'])
+
+    mock_f_e_b.assert_called_once_with(
+        True, self.fe_1, self.fe_1_stages, self.changes)
+    mock_get_approvers.assert_called_once_with(1)
+
   @mock.patch('internals.notifier.format_email_body')
   def test_make_feature_changes_email__starrer(self, mock_f_e_b):
     """We send email to users who starred the feature."""

main.py

@@ -240,6 +240,7 @@ internals_routes: list[Route] = [
   Route('/tasks/email-subscribers', notifier.FeatureChangeHandler),
   Route('/tasks/detect-intent', detect_intent.IntentEmailHandler),
   Route('/tasks/email-reviewers', notifier.FeatureReviewHandler),
+  Route('/tasks/email-comments', notifier.FeatureCommentHandler),
 
   Route('/admin/schema_migration_gate_status',
       schema_migration.EvaluateGateStatus),

notifier.staging.yaml

@@ -14,6 +14,10 @@ handlers:
   script: auto
   # Header checks prevent raw access to this handler.  Tasks have headers.
 
+- url: /tasks/email-comments
+  script: auto
+  # Header checks prevent raw access to this handler.  Tasks have headers.
+
 app_engine_apis: true
 
 # Set up VPC Access Connector for Redis.

notifier.yaml

@@ -14,6 +14,10 @@ handlers:
   script: auto
   # Header checks prevent raw access to this handler.  Tasks have headers.
 
+- url: /tasks/email-comments
+  script: auto
+  # Header checks prevent raw access to this handler.  Tasks have headers.
+
 app_engine_apis: true
 
 # Set up VPC Access Connector for Redis in prod.