Only copy assiged reviewers if rules match. (#3487)

internals/approval_defs.py

@@ -199,6 +199,7 @@ def auto_assign_reviewer(gate):
     other_afd = APPROVAL_FIELDS_BY_ID[other_gate.gate_type]
     if (other_gate.key.integer_id() != gate.key.integer_id() and
         other_afd.team_name == afd.team_name and
+        other_afd.rule == afd.rule and
         other_gate.assignee_emails):
       gate.assignee_emails = other_gate.assignee_emails
       gate.put()

internals/approval_defs_test.py

@@ -19,6 +19,7 @@
 from unittest import mock
 
 from internals import approval_defs
+from internals import core_enums
 from internals.review_models import Gate, GateDef, Vote
 
 
@@ -63,6 +64,51 @@ def test__error(self, mock_get, mock_err):
     mock_get.assert_called_once_with('https://example.com')
 
 
+class AutoAssignmentTest(testing_config.CustomTestCase):
+
+  def setUp(self):
+    self.feature_id = 123456789
+    self.gate_1 = Gate(
+        feature_id=self.feature_id, stage_id=12300, state=0,
+        gate_type=core_enums.GATE_PRIVACY_ORIGIN_TRIAL)
+    self.gate_1.put()
+    self.gate_2 = Gate(
+        feature_id=self.feature_id, stage_id=12399, state=0,
+        gate_type=core_enums.GATE_PRIVACY_SHIP,
+        assignee_emails=['[email protected]'])
+    self.gate_3 = Gate(
+        feature_id=self.feature_id, stage_id=12399, state=0,
+        gate_type=core_enums.GATE_SECURITY_SHIP)
+
+  def test__with_prior_assignment__match(self):
+    """If there was a prior assignement, use it."""
+    self.gate_2.put()
+    approval_defs.auto_assign_reviewer(self.gate_1)
+    self.assertEqual(['[email protected]'], self.gate_1.assignee_emails)
+
+  def test__with_prior_assignment__wrong_rule(self):
+    """If there was a prior assignement, but a different rule, bail."""
+    self.gate_1.gate_type = core_enums.GATE_API_SHIP
+    self.gate_1.put()
+    self.gate_2.gate_type = core_enums.GATE_API_ORIGIN_TRIAL  # Different rule
+    self.gate_2.put()
+    approval_defs.auto_assign_reviewer(self.gate_1)
+    self.assertEqual([], self.gate_1.assignee_emails)
+
+  def test__no_prior_assignment__no_gate_def(self):
+    """If there is no prior assigned and members are not in NDB, bail."""
+    # Note that gate_2 and gate_3 not saved to NDB.
+    approval_defs.auto_assign_reviewer(self.gate_1)
+    self.assertEqual([], self.gate_1.assignee_emails)
+
+    self.gate_2.assignee_emails = []
+    self.gate_2.put()  # Same team, but it has not assignement.
+    self.assertEqual([], self.gate_1.assignee_emails)
+
+    self.gate_3.put()  # Gate for a different team
+    self.assertEqual([], self.gate_1.assignee_emails)
+
+
 
 MOCK_APPROVALS_BY_ID = {
     1: approval_defs.ApprovalFieldDef(