launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/dsp-getpersonsbyemail-stab-stab-stab into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/dsp-getpersonsbyemail-stab-stab-stab/+merge/72528

Purge DistributionSourcePackage.getPersonsByEmail(). My thoughts on the placement of this method are highly censored.

I have refactored it into IPersonSet.getByEmails(), and made IPersonSet.getByEmail() a wrapper around it. I have not exported the former over the API, since I don't like the thoughts of harvesting that it brings to mind.
-- 
https://code.launchpad.net/~stevenk/launchpad/dsp-getpersonsbyemail-stab-stab-stab/+merge/72528
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/dsp-getpersonsbyemail-stab-stab-stab into lp:launchpad.

=== modified file 'lib/lp/registry/browser/distributionsourcepackage.py'
--- lib/lp/registry/browser/distributionsourcepackage.py	2011-08-03 12:29:09 +0000
+++ lib/lp/registry/browser/distributionsourcepackage.py	2011-08-23 06:31:29 +0000
@@ -304,13 +304,11 @@
         # case the emails in the changelog will be obfuscated anyway and thus
         # cause no database lookups.
         if self.user:
-            unique_emails = extract_email_addresses(the_changelog)
-            # The method below returns a [(EmailAddress,Person]] result set.
-            result_set = self.context.getPersonsByEmail(unique_emails)
-            # Ignore the persons who want their email addresses hidden.
             self._person_data = dict(
-                [(email.email, person) for (email, person) in result_set
-                 if not person.hide_email_addresses])
+                [(email.email, person) for (email, person) in
+                    getUtility(IPersonSet).getByEmails(
+                        extract_email_addresses(the_changelog),
+                        show_hidden=False)])
         else:
             self._person_data = None
         # Collate diffs for relevant SourcePackageReleases

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2011-08-18 20:35:41 +0000
+++ lib/lp/registry/configure.zcml	2011-08-23 06:31:29 +0000
@@ -487,7 +487,6 @@
                 enable_bugfiling_duplicate_search
                 findRelatedArchivePublications
                 findRelatedArchives
-                getPersonsByEmail
                 getMergeProposals
                 getReleasesAndPublishingHistory
                 getUsedBugTags

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2011-08-11 20:37:16 +0000
+++ lib/lp/registry/interfaces/person.py	2011-08-23 06:31:29 +0000
@@ -2117,6 +2117,10 @@
         Return None if there is no person with the given email address.
         """
 
+    def getByEmails(addresses):
+        """Return a dict of `IPerson`, `IEmailAddress` for people with
+        the given email address."""
+
     def getByName(name, ignore_merged=True):
         """Return the person with the given name, ignoring merged persons if
         ignore_merged is True.

=== modified file 'lib/lp/registry/model/distributionsourcepackage.py'
--- lib/lp/registry/model/distributionsourcepackage.py	2011-06-22 06:28:38 +0000
+++ lib/lp/registry/model/distributionsourcepackage.py	2011-08-23 06:31:29 +0000
@@ -517,23 +517,6 @@
             sourcepackagename=self.sourcepackagename,
             language_code=language_code, language=language)
 
-    @staticmethod
-    def getPersonsByEmail(email_addresses):
-        """[(EmailAddress,Person), ..] iterable for given email addresses."""
-        if email_addresses is None or len(email_addresses) < 1:
-            return EmptyResultSet()
-        # Perform basic sanitization of email addresses.
-        email_addresses = [
-            address.lower().strip() for address in email_addresses]
-        store = IStore(Person)
-        origin = [
-            Person, Join(EmailAddress, EmailAddress.personID == Person.id)]
-        # Get all persons whose email addresses are in the list.
-        result_set = store.using(*origin).find(
-            (EmailAddress, Person),
-            EmailAddress.email.lower().is_in(email_addresses))
-        return result_set
-
     @classmethod
     def _get(cls, distribution, sourcepackagename):
         return Store.of(distribution).find(

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-08-17 18:08:36 +0000
+++ lib/lp/registry/model/person.py	2011-08-23 06:31:29 +0000
@@ -3338,11 +3338,22 @@
 
     def getByEmail(self, email):
         """See `IPersonSet`."""
-        email = ensure_unicode(email).strip().lower()
-        return IStore(Person).find(
+        return self.getByEmails([email]).one()[0]
+
+    def getByEmails(self, addresses, show_hidden=True):
+        """See `IPersonSet`."""
+        addresses = [
+            ensure_unicode(address.lower().strip())
+            for address in addresses]
+        extra_query = True
+        if not show_hidden:
+            extra_query = Person.hide_email_addresses == True
+        return IStore(Person).using(
             Person,
-            Person.id == EmailAddress.personID,
-            EmailAddress.email.lower() == email).one()
+            Join(EmailAddress, EmailAddress.personID == Person.id)
+        ).find(
+            (Person, EmailAddress),
+            EmailAddress.email.lower().is_in(addresses), extra_query)
 
     def latest_teams(self, limit=5):
         """See `IPersonSet`."""