Add note to NEEDS_WORK notifications. (#4684)

* Add note to NEEDS_WORK notifications.

* Allow more time for creating a test feature

* Commented out make_golden

internals/notifier.py

@@ -99,16 +99,21 @@ def format_email_body(
     prop_name = escape(prop['prop_name'])  # Ensure to escape
     new_val = prop['new_val']
     old_val = prop['old_val']
+    note = prop.get('note')
 
     # Escaping values before passing to highlight_diff
     highlighted_old_val = highlight_diff(old_val, new_val, 'deletion')
     highlighted_new_val = highlight_diff(old_val, new_val, 'addition')
 
+    note_line = f'<b>Note:</b> {note}<br/>' if note else ''
+
     # Using f-strings for clear formatting
     formatted_changes += (
         f'<li><b>{prop_name}:</b><br/>'
-        f'<b>old:</b> {highlighted_old_val}<br/>'
-        f'<b>new:</b> {highlighted_new_val}<br/></li><br/>')
+        f'<b>Old:</b> {highlighted_old_val}<br/>'
+        f'<b>New:</b> {highlighted_new_val}<br/>'
+        f'{note_line}'
+        f'</li><br/>')
 
   if not formatted_changes:
     formatted_changes = '<li>None</li>'

internals/notifier_helpers.py

@@ -144,6 +144,10 @@ def notify_subscribers_of_vote_changes(fe: 'FeatureEntry', gate: Gate,
       'old_val': old_state_name,
       'new_val': state_name,
   }
+  if new_state == Vote.NEEDS_WORK:
+    changed_props['note'] = (
+        'Feature owners must press the "Re-request review" button '
+        'after requested changes have been completed.')
 
   params = {
     'changes': [changed_props],

internals/notifier_helpers_test.py

@@ -13,6 +13,7 @@
 # limitations under the License.
 from unittest import mock
 
+from api import converters
 from internals import notifier_helpers
 import testing_config  # Must be imported before the module under test.
 from internals.core_models import FeatureEntry, Stage, MilestoneSet
@@ -113,6 +114,28 @@ def test_vote_changes_activities__created(self, mock_task_helpers):
 
     mock_task_helpers.assert_called_once()
 
+  @mock.patch('framework.cloud_tasks_helpers.enqueue_task')
+  def test_vote_changes_activities__needs_work_note(self, mock_task_helpers):
+    """When we notify about NEEDS_WORK, it has a process note."""
+    notifier_helpers.notify_subscribers_of_vote_changes(
+        self.feature_1, self.gate_1, '[email protected]',
+        Vote.NEEDS_WORK, Vote.NA)
+
+    prop_change = {
+        'prop_name': 'API Owners review status http://127.0.0.1:7777/feature/2925?gate=123',
+        'old_val': 'na',
+        'new_val': 'needs_work',
+        'note': 'Feature owners must press the "Re-request review" button after requested changes have been completed.',
+        }
+    expected_params = {
+        'changes': [prop_change],
+        'is_update': True,
+        'triggering_user_email': '[email protected]',
+        'feature': converters.feature_entry_to_json_verbose(self.feature_1),
+        }
+    mock_task_helpers.assert_called_once_with(
+        '/tasks/email-subscribers', expected_params)
+
   @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(

internals/notifier_test.py

@@ -198,6 +198,21 @@ def test_format_email_body__update_with_changes(self):
     self.assertEqual(body_html,
       TESTDATA['test_format_email_body__update_with_changes.html'])
 
+  def test_format_email_body__update_with_changes_and_note(self):
+    """Some property changes can include a note."""
+    self.changes.append({
+        'prop_name': 'Enterprise review status URL',
+        'old_val': 'review_started',
+        'new_val': 'needs_work',
+        'note': 'You need to press a button',
+        })
+    with test_app.app_context():
+      body_html = notifier.format_email_body(
+          'update-feature-email.html', self.template_fe, self.changes)
+    # TESTDATA.make_golden(body_html, 'test_format_email_body__update_with_changes_and_note.html')
+    self.assertEqual(body_html,
+      TESTDATA['test_format_email_body__update_with_changes_and_note.html'])
+
   def test_accumulate_reasons(self):
     """We can accumulate lists of reasons why we sent a message to a user."""
     addr_reasons = collections.defaultdict(list)
@@ -1299,7 +1314,7 @@ def test_make_activation_failed_email(self):
       handler = notifier.OTActivationFailedHandler()
       stage_dict = converters.stage_to_json_dict(self.ot_stage)
       email_task = handler.build_email(stage_dict)
-      TESTDATA.make_golden(email_task['html'], 'test_make_activation_failed_email.html')
+      # TESTDATA.make_golden(email_task['html'], 'test_make_activation_failed_email.html')
       self.assertEqual(
         email_task['subject'],
         'Automated trial activation request failed for Example Trial')

internals/testdata/notifier_test/test_format_email_body__update_with_changes.html

@@ -27,7 +27,7 @@
 <section id="details" style="margin: 16px; padding: 16px; background: white; border: 2px solid #ccc; border-radius:8px;">
   <div><b>Updates made by [email protected]:</b></div>
   <ul>
-    <li><b>test_prop:</b><br/><b>old:</b> test <span style="background:#FDD">old</span> value<br/><b>new:</b> test <span style="background:#DFD">new</span> value<br/></li><br/>
+    <li><b>test_prop:</b><br/><b>Old:</b> test <span style="background:#FDD">old</span> value<br/><b>New:</b> test <span style="background:#DFD">new</span> value<br/></li><br/>
   </ul>
 </section>
 

internals/testdata/notifier_test/test_format_email_body__update_with_changes_and_note.html

@@ -0,0 +1,43 @@
+
+<div style="background: #eee; padding: 0 16px 16px">
+<div style="color: #666; padding: 8px 0 0 16px;">Local testing</div>
+
+<div style="padding: 8px; background: white; border-top: 4px solid #888; border-color: #01579b;">
+<section id="context" style="margin: 16px; padding: 16px; background: white; border: 2px solid #ccc; border-radius:8px;">
+  <table>
+    <tr>
+      <td rowspan=2>
+
+<div style="border: 4px solid #01579b; border-radius: 50%; width: 32px; height: 32px; margin-right: 8px;">
+  </div>
+
+</td>
+      <td><b>Updated</b> feature entry:</td>
+    </tr>
+    <tr>
+      <td><a style="font-size: 140%;"
+             href="http://127.0.0.1:7777/feature/123"
+             >feature template</a>
+      </td>
+    </tr>
+  </table>
+</section>
+
+
+<section id="details" style="margin: 16px; padding: 16px; background: white; border: 2px solid #ccc; border-radius:8px;">
+  <div><b>Updates made by [email protected]:</b></div>
+  <ul>
+    <li><b>test_prop:</b><br/><b>Old:</b> test <span style="background:#FDD">old</span> value<br/><b>New:</b> test <span style="background:#DFD">new</span> value<br/></li><br/><li><b>Enterprise review status URL:</b><br/><b>Old:</b> <span style="background:#FDD">review_started</span><br/><b>New:</b> <span style="background:#DFD">needs_work</span><br/><b>Note:</b> You need to press a button<br/></li><br/>
+  </ul>
+</section>
+
+
+<section id="next-steps" style="margin: 16px; padding: 16px; background: white; border: 2px solid #ccc; border-radius:8px;">
+  <div><b>Your next steps:</b></div>
+  <div style="margin: 16px;">
+    <a href="http://127.0.0.1:7777/feature/123" style="text-decoration: none; font-weight: bold; color: white; background: #01579b; border-radius: 8px; padding: 8px 16px; "
+     >View feature details</a></div>
+</section>
+
+</div>
+</div>

packages/playwright/tests/test_utils.js

@@ -360,11 +360,11 @@ export async function createNewFeature(page) {
   // Submit the form.
   const submitButton = page.locator('input[type="submit"]');
   await submitButton.click();
-  await delay(500);
+  await delay(1500);
 
   // Wait until we are on the Feature page.
   await page.waitForURL('**/feature/*');
-  await delay(500);
+  await delay(1500);
 }
 
 export async function gotoNewFeatureList(page) {