From 60555f94ee68f8f809587281b117db50da7a09ff Mon Sep 17 00:00:00 2001 From: Kyle Ju Date: Thu, 14 Nov 2024 11:28:11 -0800 Subject: [PATCH] Notify feature owners for accuracy emails (#4528) * Send feature accuracy emails to owners only * nit --- internals/reminders.py | 28 ++++++-- internals/reminders_test.py | 135 ++++++++++++++++++++++++++++-------- 2 files changed, 129 insertions(+), 34 deletions(-) diff --git a/internals/reminders.py b/internals/reminders.py index 143aa6507cb7..c2ef6a0a3af7 100644 --- a/internals/reminders.py +++ b/internals/reminders.py @@ -32,6 +32,7 @@ from internals import slo from internals.core_enums import ( STAGE_TYPES_BY_FIELD_MAPPING) +from internals.user_models import UserPref import settings @@ -54,11 +55,16 @@ def get_current_milestone_info(anchor_channel: str): def choose_email_recipients( - feature: FeatureEntry, is_escalated: bool) -> list[str]: + feature: FeatureEntry, is_escalated: bool, is_accuracy_email: bool) -> list[str]: """Choose which recipients will receive the email notification.""" - # Only feature owners are notified for a non-escalated notification. - if not is_escalated: - return feature.owner_emails + + # Only feature owners are notified for accuracy or non-escalated notification emails, if not bounced. + if is_accuracy_email or not is_escalated: + user_prefs = UserPref.get_prefs_for_emails(feature.owner_emails) + receivers = list(set([up.email for up in user_prefs + if not up.bounced])) + if receivers: + return receivers # Escalated notification. Add extended recipients. ws_group_emails = [STAGING_EMAIL] @@ -77,7 +83,8 @@ def build_email_tasks( subject_format: str, body_template_path: str, current_milestone_info: dict, - escalation_check: Callable + escalation_check: Callable, + is_accuracy_email: Callable ) -> list[dict[str, Any]]: email_tasks: list[dict[str, Any]] = [] beta_date = datetime.fromisoformat(current_milestone_info['earliest_beta']) @@ -106,7 +113,7 @@ def build_email_tasks( subject = subject.replace(EMAIL_SUBJECT_PREFIX, 'Escalation request') else: subject = f'ESCALATED: {subject}' - recipients = choose_email_recipients(fe, is_escalated) + recipients = choose_email_recipients(fe, is_escalated, is_accuracy_email()) for recipient in recipients: email_tasks.append({ 'to': recipient, @@ -134,7 +141,7 @@ def get_template_data(self, **kwargs): email_tasks = build_email_tasks( features_to_notify, self.SUBJECT_FORMAT, self.EMAIL_TEMPLATE_PATH, current_milestone_info, - self.should_escalate_notification) + self.should_escalate_notification, self.is_accuracy_email) notifier.send_emails(email_tasks) recipients_str = '' @@ -215,6 +222,10 @@ def should_escalate_notification(self, feature: FeatureEntry) -> bool: """Determine if the notification should be escalated to more users.""" return False + # Subclasses should override if needed. + def is_accuracy_email(self) -> bool: + return False + # Subclasses should override if processing is needed after notifications sent. def changes_after_sending_notifications( self, features_notified: list[tuple[FeatureEntry, int]]) -> None: @@ -261,6 +272,9 @@ def should_escalate_notification(self, feature: FeatureEntry) -> bool: """Escalate notification if 2 previous emails have had no response.""" return feature.outstanding_notifications >= 2 + def is_accuracy_email(self) -> bool: + return True + def changes_after_sending_notifications( self, notified_features: list[tuple[FeatureEntry, int]]) -> None: """Updates the count of any outstanding notifications.""" diff --git a/internals/reminders_test.py b/internals/reminders_test.py index f2408ed51c85..58f6896f31ac 100644 --- a/internals/reminders_test.py +++ b/internals/reminders_test.py @@ -23,6 +23,7 @@ from internals.core_models import FeatureEntry, MilestoneSet, Stage from internals.review_models import Gate, Vote from internals import reminders +from internals.user_models import UserPref from google.cloud import ndb # type: ignore @@ -110,27 +111,56 @@ def setUp(self): self.feature_template.put() + self.owner_user_pref = UserPref( + email='feature_owner@example.com', + notify_as_starrer=False) + self.owner_user_pref.put() + owner_user_pref_1 = UserPref( + email='owner_1@example.com', + notify_as_starrer=False) + owner_user_pref_1.put() + owner_user_pref_2 = UserPref( + email='owner_2@example.com', + notify_as_starrer=False) + owner_user_pref_2.put() + self.maxDiff = None def tearDown(self) -> None: - kinds: list[ndb.Model] = [FeatureEntry, Stage] + kinds: list[ndb.Model] = [FeatureEntry, Stage, UserPref] for kind in kinds: for entity in kind.query(): entity.key.delete() def test_choose_email_recipients__normal(self): - """Normal reminders go to feature participants.""" + """Normal reminders go to feature owners.""" actual = reminders.choose_email_recipients( - self.feature_template, False) + self.feature_template, False, False) expected = ['feature_owner@example.com', ] self.assertEqual(set(actual), set(expected)) + def test_choose_email_recipients__owners_bounced(self): + """Normal reminders go to feature participants when owners' emails + are bounced.""" + self.owner_user_pref.bounced = True + self.owner_user_pref.put() + + actual = reminders.choose_email_recipients( + self.feature_template, False, False) + expected = ['creator@example.com', + 'feature_editor@example.com', + 'feature_owner@example.com', + 'mentor@example.com', + 'jrobbins-test@googlegroups.com', + ] + self.assertEqual(set(actual), set(expected)) + @mock.patch('settings.PROD', True) def test_choose_email_recipients__escalated(self): """Escalated reminders go to feature participants and lists.""" actual = reminders.choose_email_recipients( - self.feature_template, True) + self.feature_template, True, False) expected = ['creator@example.com', 'feature_owner@example.com', 'feature_editor@example.com', @@ -140,15 +170,50 @@ def test_choose_email_recipients__escalated(self): ] self.assertEqual(set(actual), set(expected)) + def test_choose_email_recipients__normal_accuracy_email(self): + """Normal accuracy emails go to feature owners.""" + actual = reminders.choose_email_recipients( + self.feature_template, False, True) + expected = ['feature_owner@example.com', + ] + self.assertEqual(set(actual), set(expected)) + + def test_choose_email_recipients__normal_accuracy_email_when_owners_bounced(self): + """Normal accuracy emails go to feature participants when owners' emails + are bounced.""" + self.owner_user_pref.bounced = True + self.owner_user_pref.put() + + actual = reminders.choose_email_recipients( + self.feature_template, False, True) + expected = ['creator@example.com', + 'feature_editor@example.com', + 'feature_owner@example.com', + 'mentor@example.com', + 'jrobbins-test@googlegroups.com', + ] + self.assertEqual(set(actual), set(expected)) + + @mock.patch('settings.PROD', True) + def test_choose_email_recipients_escalated_accuracy_email(self): + """Escalated accuracy emails go to feature owners.""" + actual = reminders.choose_email_recipients( + self.feature_template, True, True) + expected = ['feature_owner@example.com', + ] + self.assertEqual(set(actual), set(expected)) + def test_build_email_tasks_feature_accuracy(self): with test_app.app_context(): handler = reminders.FeatureAccuracyHandler() actual = reminders.build_email_tasks( - [(self.feature_template, 100)], - 'Action requested - Verify %s', - handler.EMAIL_TEMPLATE_PATH, - self.current_milestone_info, - handler.should_escalate_notification) + [(self.feature_template, 100)], + 'Action requested - Verify %s', + handler.EMAIL_TEMPLATE_PATH, + self.current_milestone_info, + handler.should_escalate_notification, + handler.is_accuracy_email, + ) self.assertEqual(1, len(actual)) task = actual[0] @@ -163,11 +228,13 @@ def test_build_email_tasks_feature_accuracy__enterprise(self): with test_app.app_context(): handler = reminders.FeatureAccuracyHandler() actual = reminders.build_email_tasks( - [(self.feature_template, 110)], - 'Action requested - Verify %s', - handler.EMAIL_TEMPLATE_PATH, - self.current_milestone_info, - handler.should_escalate_notification) + [(self.feature_template, 110)], + 'Action requested - Verify %s', + handler.EMAIL_TEMPLATE_PATH, + self.current_milestone_info, + handler.should_escalate_notification, + handler.is_accuracy_email, + ) self.assertEqual(1, len(actual)) task = actual[0] @@ -186,13 +253,15 @@ def test_build_email_tasks_feature_accuracy__escalated(self): with test_app.app_context(): handler = reminders.FeatureAccuracyHandler() actual = reminders.build_email_tasks( - [(self.feature_template, 100)], - 'Action requested - Verify %s', - handler.EMAIL_TEMPLATE_PATH, - self.current_milestone_info, - handler.should_escalate_notification) + [(self.feature_template, 100)], + 'Action requested - Verify %s', + handler.EMAIL_TEMPLATE_PATH, + self.current_milestone_info, + handler.should_escalate_notification, + handler.is_accuracy_email, + ) - self.assertEqual(5, len(actual)) + self.assertEqual(1, len(actual)) task = actual[0] self.assertEqual( 'Escalation request - Verify feature one', task['subject']) @@ -205,9 +274,13 @@ def test_build_email_tasks_prepublication(self): with test_app.app_context(): handler = reminders.PrepublicationHandler() actual = reminders.build_email_tasks( - [(self.feature_template, 100)], 'Action requested - Verify %s', - handler.EMAIL_TEMPLATE_PATH, - self.current_milestone_info, handler.should_escalate_notification) + [(self.feature_template, 100)], + 'Action requested - Verify %s', + handler.EMAIL_TEMPLATE_PATH, + self.current_milestone_info, + handler.should_escalate_notification, + handler.is_accuracy_email, + ) self.assertEqual(1, len(actual)) task = actual[0] self.assertEqual('feature_owner@example.com', task['to']) @@ -222,6 +295,14 @@ class FeatureAccuracyHandlerTest(testing_config.CustomTestCase): def setUp(self): self.feature_1, self.feature_2, self.feature_3 = make_test_features() self.handler = reminders.FeatureAccuracyHandler() + self.owner_user_pref_1 = UserPref( + email='owner_1@example.com', + notify_as_starrer=False) + self.owner_user_pref_1.put() + self.owner_user_pref_2 = UserPref( + email='owner_2@example.com', + notify_as_starrer=False) + self.owner_user_pref_2.put() def tearDown(self): self.feature_1.key.delete() @@ -229,6 +310,9 @@ def tearDown(self): self.feature_3.key.delete() for stage in Stage.query(): stage.key.delete() + self.owner_user_pref_1.key.delete() + self.owner_user_pref_2.key.delete() + @mock.patch('requests.get') def test_determine_features_to_notify__no_features(self, mock_get): @@ -281,11 +365,8 @@ def test_determine_features_to_notify__escalated(self, mock_get): with test_app.app_context(): result = self.handler.get_template_data() # More email tasks should be created to notify extended contributors. - expected_message = ('5 email(s) sent or logged.\n' + expected_message = ('2 email(s) sent or logged.\n' 'Recipients:\n' - 'feature_editor@example.com\n' - 'jrobbins-test@googlegroups.com\n' - 'mentor@example.com\n' 'owner_1@example.com\n' 'owner_2@example.com') expected = {'message': expected_message}