launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/launchpad/bug-727540 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~lifeless/launchpad/bug-727540/+merge/54293

team /members also needs to be eager loaded. Code reuse ftw this is nice and simple.
-- 
https://code.launchpad.net/~lifeless/launchpad/bug-727540/+merge/54293
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/bug-727540 into lp:launchpad.

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2011-03-18 03:56:36 +0000
+++ lib/lp/registry/interfaces/person.py	2011-03-22 04:46:30 +0000
@@ -1420,10 +1420,12 @@
         "The number of real people who are members of this team.")
     # activemembers.value_type.schema will be set to IPerson once
     # IPerson is defined.
-    activemembers = exported(
+    activemembers = Attribute('List of direct members with ADMIN or APPROVED status')
+    # For the API we need eager loading
+    api_activemembers = exported(
         doNotSnapshot(
             CollectionField(
-                title=_("List of members with ADMIN or APPROVED status"),
+                title=_("List of direct members with ADMIN or APPROVED status"),
                 value_type=Reference(schema=Interface))),
         exported_as='members')
     adminmembers = exported(
@@ -2375,7 +2377,7 @@
 # Fix value_type.schema of IPersonViewRestricted attributes.
 for name in [
     'all_members_prepopulated',
-    'activemembers',
+    'api_activemembers',
     'adminmembers',
     'proposedmembers',
     'invited_members',

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2011-03-12 08:52:47 +0000
+++ lib/lp/registry/model/distroseries.py	2011-03-22 04:46:30 +0000
@@ -432,7 +432,7 @@
         # NB: precaching objects like this method tries to do has a very poor
         # hit rate with storm - many queries will still be executed; consider
         # ripping this out and instead allowing explicit inclusion of things
-        # like Person._all_members does - returning a cached object graph.
+        # like Person._members does - returning a cached object graph.
         # -- RBC 20100810
         # Avoid circular import failures.
         from lp.registry.model.product import Product

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-03-18 03:56:36 +0000
+++ lib/lp/registry/model/person.py	2011-03-22 04:46:30 +0000
@@ -1212,7 +1212,7 @@
     @cachedproperty
     def karma(self):
         """See `IPerson`."""
-        # May also be loaded from _all_members
+        # May also be loaded from _members
         cache = KarmaTotalCache.selectOneBy(person=self)
         if cache is None:
             # Newly created accounts may not be in the cache yet, meaning the
@@ -1608,14 +1608,14 @@
     @property
     def allmembers(self):
         """See `IPerson`."""
-        return self._all_members()
+        return self._members(direct=False)
 
     @property
     def all_members_prepopulated(self):
         """See `IPerson`."""
-        return self._all_members(need_karma=True, need_ubuntu_coc=True,
-            need_location=True, need_archive=True, need_preferred_email=True,
-            need_validity=True)
+        return self._members(direct=False, need_karma=True,
+            need_ubuntu_coc=True, need_location=True, need_archive=True,
+            need_preferred_email=True, need_validity=True)
 
     @staticmethod
     def _validity_queries(person_table=None):
@@ -1681,11 +1681,12 @@
             tables=columns,
             decorators=decorators)
 
-    def _all_members(self, need_karma=False, need_ubuntu_coc=False,
+    def _members(self, direct, need_karma=False, need_ubuntu_coc=False,
         need_location=False, need_archive=False, need_preferred_email=False,
         need_validity=False):
         """Lookup all members of the team with optional precaching.
 
+        :param direct: If True only direct members are returned.
         :param need_karma: The karma attribute will be cached.
         :param need_ubuntu_coc: The is_ubuntu_coc_signer attribute will be
             cached.
@@ -1699,15 +1700,25 @@
         #       The difference between the two is that
         #       getMembersWithPreferredEmails includes self, which is arguably
         #       wrong, but perhaps deliberate.
-        origin = [
-            Person,
-            Join(TeamParticipation, TeamParticipation.person == Person.id),
-            ]
-        conditions = And(
-            # Members of this team,
-            TeamParticipation.team == self.id,
-            # But not the team itself.
-            TeamParticipation.person != self.id)
+        origin = [Person]
+        if not direct:
+            origin.append(Join(
+                TeamParticipation, TeamParticipation.person == Person.id))
+            conditions = And(
+                # Members of this team,
+                TeamParticipation.team == self.id,
+                # But not the team itself.
+                TeamParticipation.person != self.id)
+        else:
+            origin.append(Join(
+                TeamMembership, TeamMembership.personID == Person.id))
+            conditions = And(
+                # Membership in this team,
+                TeamMembership.team == self.id,
+                # And approved or admin status
+                TeamMembership.status.is_in([
+                    TeamMembershipStatus.APPROVED,
+                    TeamMembershipStatus.ADMIN]))
         # Use a PersonSet object that is not security proxied to allow
         # manipulation of the object.
         person_set = PersonSet()
@@ -1814,6 +1825,13 @@
             self.adminmembers, orderBy=self._sortingColumnsForSetOperations)
 
     @property
+    def api_activemembers(self):
+        """See `IPerson`."""
+        return self._members(direct=True, need_karma=True,
+            need_ubuntu_coc=True, need_location=True, need_archive=True,
+            need_preferred_email=True, need_validity=True)
+
+    @property
     def active_member_count(self):
         """See `IPerson`."""
         return self.activemembers.count()
@@ -2634,7 +2652,7 @@
     @cachedproperty
     def is_ubuntu_coc_signer(self):
         """See `IPerson`."""
-        # Also assigned to by self._all_members.
+        # Also assigned to by self._members.
         store = Store.of(self)
         query = And(SignedCodeOfConduct.ownerID == self.id,
             Person._is_ubuntu_coc_signer_condition())

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2011-03-07 07:05:39 +0000
+++ lib/lp/soyuz/model/archive.py	2011-03-22 04:46:30 +0000
@@ -1979,7 +1979,7 @@
     def getPPAOwnedByPerson(self, person, name=None, statuses=None,
                             has_packages=False):
         """See `IArchiveSet`."""
-        # See Person._all_members which also directly queries this.
+        # See Person._members which also directly queries this.
         store = Store.of(person)
         clause = [
             Archive.purpose == ArchivePurpose.PPA,