launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/fast-contact-via-web into lp:launchpad.

Commit message:
Do not load all the team members to learn the count of who will be contacted.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1084588 in Launchpad itself: "Admins of large teams cannot view their team page"
  https://bugs.launchpad.net/launchpad/+bug/1084588

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/fast-contact-via-web/+merge/136986

OOPS-6c1d4799395a99f0d86db9e729eaab5d shows a timeout caused by lost
time in a python call related to getMembersWithPreferredEmails(). The
link to contact team members needs to know the count, but it is getting
all 18363 members -- they are not needed. A recent change to fix an oops
contact teams without admins changed the call to get the count to use a
cached property...but this property is not already loaded, and in fact
only needs to be called when sending the member email messages.


--------------------------------------------------------------------

RULES

    Pre-implementation: no one
    * Revert the change to ContactViaWebNotificationRecipientSet.__len__
      so that the TO_MEMBERS check uses:
      recipient.getMembersWithPreferredEmailsCount()
    * Keep the change for TO_ADMINS.


QA

    As a member of https://qastaging.launchpad.net/~ubuntu-lococouncil:
    * Visit https://qastaging.launchpad.net/~locoteams
    * Verify the page loads.


LINT

    lib/lp/registry/browser/person.py
    lib/lp/registry/browser/tests/test_person_contact.py



TEST

    ./bin/test -vvc lp.registry.browser.tests.test_person_contact


IMPLEMENTATION

I reverted the check for TO_MEMBERS which was much faster than the
needless loading of all the members. I forgot that pages that link to
the contact-team page use the ContactViaWebNotificationRecipientSet to
get the link description, they do not need the members, so they were not
already cached. I added a test to verify that instantiating the
recipient set and calling length makes less than three db calls -- it
currently checks the user's
relationship to the team, then gets the member count.
    lib/lp/registry/browser/person.py
    lib/lp/registry/browser/tests/test_person_contact.py
-- 
https://code.launchpad.net/~sinzui/launchpad/fast-contact-via-web/+merge/136986
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/fast-contact-via-web into lp:launchpad.

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2012-11-27 16:04:54 +0000
+++ lib/lp/registry/browser/person.py	2012-11-29 16:45:34 +0000
@@ -4066,7 +4066,11 @@
         """The number of recipients in the set."""
         if self._count_recipients is None:
             recipient = self._primary_recipient
-            if self.primary_reason in (self.TO_MEMBERS, self.TO_ADMINS):
+            if self.primary_reason is self.TO_MEMBERS:
+                # Get the count without loading all the members.
+                self._count_recipients = (
+                    recipient.getMembersWithPreferredEmailsCount())
+            elif self.primary_reason is self.TO_ADMINS:
                 self._count_recipients = len(self._all_recipients)
             elif recipient.is_valid_person_or_team:
                 self._count_recipients = 1

=== modified file 'lib/lp/registry/browser/tests/test_person_contact.py'
--- lib/lp/registry/browser/tests/test_person_contact.py	2012-11-27 19:43:48 +0000
+++ lib/lp/registry/browser/tests/test_person_contact.py	2012-11-29 16:45:34 +0000
@@ -4,6 +4,8 @@
 
 __metaclass__ = type
 
+from testtools.matchers import LessThan
+
 from lp.app.browser.tales import DateTimeFormatterAPI
 from lp.registry.browser.person import (
     ContactViaWebLinksMixin,
@@ -13,9 +15,11 @@
 from lp.services.messages.interfaces.message import IDirectEmailAuthorization
 from lp.testing import (
     person_logged_in,
+    StormStatementRecorder,
     TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.matchers import HasQueryCount
 from lp.testing.views import create_initialized_view
 
 
@@ -52,8 +56,11 @@
         member = self.factory.makePerson()
         sender_team = self.factory.makeTeam(members=[member])
         owner = sender_team.teamowner
-        self.assertEqual(
-            2, len(ContactViaWebNotificationRecipientSet(owner, sender_team)))
+        with StormStatementRecorder() as recorder:
+            total = len(
+                ContactViaWebNotificationRecipientSet(owner, sender_team))
+            self.assertThat(recorder, HasQueryCount(LessThan(3)))
+        self.assertEqual(2, total)
         with person_logged_in(owner):
             owner.leave(sender_team)
         self.assertEqual(