diff --git a/internals/notifier.py b/internals/notifier.py index 180fdfe55886..e14ec30835f1 100644 --- a/internals/notifier.py +++ b/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'Note: {note}
' if note else '' + # Using f-strings for clear formatting formatted_changes += ( f'
  • {prop_name}:
    ' - f'old: {highlighted_old_val}
    ' - f'new: {highlighted_new_val}

    • ') + f'Old: {highlighted_old_val}
    ' + f'New: {highlighted_new_val}
    ' + f'{note_line}' + f'
    ') if not formatted_changes: formatted_changes = '
  • None
    • ' diff --git a/internals/notifier_helpers.py b/internals/notifier_helpers.py index 1e71759cb303..40397f676fad 100644 --- a/internals/notifier_helpers.py +++ b/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], diff --git a/internals/notifier_helpers_test.py b/internals/notifier_helpers_test.py index 9b93a52b0bee..9ce20bae8603 100644 --- a/internals/notifier_helpers_test.py +++ b/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, 'abc@example.com', + 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': 'abc@example.com', + '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( diff --git a/internals/notifier_test.py b/internals/notifier_test.py index 60f69378d532..955485ab1ec1 100644 --- a/internals/notifier_test.py +++ b/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') diff --git a/internals/testdata/notifier_test/test_format_email_body__update_with_changes.html b/internals/testdata/notifier_test/test_format_email_body__update_with_changes.html index 57a87e17d636..5e26e8f1a9e7 100644 --- a/internals/testdata/notifier_test/test_format_email_body__update_with_changes.html +++ b/internals/testdata/notifier_test/test_format_email_body__update_with_changes.html @@ -27,7 +27,7 @@
    Updates made by editor_template@example.com:
    diff --git a/internals/testdata/notifier_test/test_format_email_body__update_with_changes_and_note.html b/internals/testdata/notifier_test/test_format_email_body__update_with_changes_and_note.html new file mode 100644 index 000000000000..510c571af354 --- /dev/null +++ b/internals/testdata/notifier_test/test_format_email_body__update_with_changes_and_note.html @@ -0,0 +1,43 @@ + +
    +
    Local testing
    + +
    +
    + + + + + + + + +
    + +
    +
    + +    		Updated feature entry:
    feature template +
    +
    + + +
    +
    Updates made by editor_template@example.com:
    +
      +
    • test_prop:
      Old: test old value
      New: test new value

    • Enterprise review status URL:
      Old: review_started
      New: needs_work
      Note: You need to press a button

      • +
    +
    + + +
    +
    Your next steps:
    +
    + View feature details
    +
    + +
    +
    \ No newline at end of file diff --git a/packages/playwright/tests/test_utils.js b/packages/playwright/tests/test_utils.js index aaedf90640ab..0ef880d20be9 100644 --- a/packages/playwright/tests/test_utils.js +++ b/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) {