launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-766561 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #766561 in Launchpad itself: "Bug notifications broken for teams with only non-preferred email addresses"
  https://bugs.launchpad.net/launchpad/+bug/766561

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-766561/+merge/58421

emailPeople was recently replaced by get_recipients, a significantly less inefficient implementation. But it has a bug: it outer joins EmailAddress onto Person, and returns Persons where the EmailAddress is preferred or (there is no EmailAddress and the Person is a team). This breaks when a team has a non-preferred EmailAddress, as it meets the join condition but then fails both in the WHERE clause.

This branch fixes it to join only preferred email addresses, and simplifies the WHERE clause to allow any row where an EmailAddress address is present or the Person is a team.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-766561/+merge/58421
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-766561 into lp:launchpad.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-04-08 21:42:59 +0000
+++ lib/lp/registry/model/person.py	2011-04-20 02:19:26 +0000
@@ -4599,20 +4599,25 @@
                          Join(Person,
                               TeamMembership.personID==Person.id),
                          LeftJoin(EmailAddress,
-                                  EmailAddress.person == Person.id))
+                                  And(
+                                      EmailAddress.person == Person.id,
+                                      EmailAddress.status ==
+                                        EmailAddressStatus.PREFERRED)))
     pending_team_ids = [team.id]
     recipient_ids = set()
     seen = set()
     while pending_team_ids:
+        # Find Persons that have a preferred email address, or are a
+        # team, or both.
         intermediate_transitive_results = source.find(
             (TeamMembership.personID, EmailAddress.personID),
             In(TeamMembership.status,
                [TeamMembershipStatus.ADMIN.value,
                 TeamMembershipStatus.APPROVED.value]),
             In(TeamMembership.teamID, pending_team_ids),
-            Or(EmailAddress.status == EmailAddressStatus.PREFERRED,
-               And(Not(Person.teamownerID == None),
-                   EmailAddress.status == None))).config(distinct=True)
+            Or(
+                EmailAddress.personID != None,
+                Person.teamownerID != None)).config(distinct=True)
         next_ids = []
         for (person_id,
              preferred_email_marker) in intermediate_transitive_results:

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2011-04-09 02:27:34 +0000
+++ lib/lp/registry/tests/test_person.py	2011-04-20 02:19:26 +0000
@@ -67,6 +67,7 @@
     Person,
     )
 from lp.services.openid.model.openididentifier import OpenIdIdentifier
+from lp.services.propertycache import clear_property_cache
 from lp.soyuz.enums import (
     ArchivePurpose,
     ArchiveStatus,
@@ -1302,7 +1303,7 @@
 class TestGetRecipients(TestCaseWithFactory):
     """Tests for get_recipients"""
 
-    layer = LaunchpadFunctionalLayer
+    layer = DatabaseFunctionalLayer
 
     def setUp(self):
         super(TestGetRecipients, self).setUp()
@@ -1326,6 +1327,19 @@
         recipients = get_recipients(super_team)
         self.assertEqual(set([team]), set(recipients))
 
+    def test_get_recipients_team_with_unvalidated_address(self):
+        """Ensure get_recipients handles teams with non-preferred addresses.
+
+        If there is no preferred address but one or more non-preferred ones,
+        email should still be sent to the members.
+        """
+        owner = self.factory.makePerson(email='foo@xxxxxxx')
+        team = self.factory.makeTeam(owner, email='team@xxxxxxx')
+        self.assertContentEqual([team], get_recipients(team))
+        team.preferredemail.status = EmailAddressStatus.NEW
+        clear_property_cache(team)
+        self.assertContentEqual([owner], get_recipients(team))
+
     def makePersonWithNoPreferredEmail(self, **kwargs):
         kwargs['email_address_status'] = EmailAddressStatus.NEW
         return self.factory.makePerson(**kwargs)