launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #796889 in Launchpad itself: "ValidPersonOrTeamVocabulary preloads all IRC nicks into everybody"
  https://bugs.launchpad.net/launchpad/+bug/796889

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

ValidPersonOrTeamVocabulary's IrcID preloading is buggy: it attaches all found IrcIDs to all Persons with IrcIDs, and doesn't populate the cache when there are none. This branch fixes it to only attach the relevant ones, and to leave an empty list when there are none.

I've added a test to confirm that the values and queries are correct.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-796889/+merge/64490
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-796889 into lp:launchpad.

=== modified file 'lib/lp/registry/tests/test_person_vocabularies.py'
--- lib/lp/registry/tests/test_person_vocabularies.py	2011-06-08 07:03:50 +0000
+++ lib/lp/registry/tests/test_person_vocabularies.py	2011-06-14 04:11:08 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 
 from storm.store import Store
+from testtools.matchers import Equals
 from zope.component import getUtility
 from zope.schema.vocabulary import getVocabularyRegistry
 from zope.security.proxy import removeSecurityProxy
@@ -15,14 +16,19 @@
     DatabaseFunctionalLayer,
     LaunchpadZopelessLayer,
     )
+from lp.registry.interfaces.irc import IIrcIDSet
 from lp.registry.interfaces.person import (
     PersonVisibility,
     TeamSubscriptionPolicy,
     )
 from lp.registry.interfaces.karma import IKarmaCacheManager
 from lp.services.features.testing import FeatureFixture
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    StormStatementRecorder,
+    TestCaseWithFactory,
+    )
 from lp.testing.dbuser import dbuser
+from lp.testing.matchers import HasQueryCount
 
 
 PERSON_AFFILIATION_RANK_FLAG = {
@@ -125,6 +131,43 @@
         self.assertKarmaContextConstraint(None, None)
 
 
+class TestValidPersonOrTeamPreloading(VocabularyTestBase,
+                                      TestCaseWithFactory):
+    """Tests for ValidPersonOrTeamVocabulary's preloading behaviour."""
+
+    layer = DatabaseFunctionalLayer
+    vocabulary_name = 'ValidPersonOrTeam'
+
+    def test_preloads_irc_nicks_and_preferredemail(self):
+        """Test that IRC nicks and preferred email addresses are preloaded."""
+        # Create three people with IRC nicks, and one without.
+        people = []
+        for num in range(3):
+            person = self.factory.makePerson(displayname='foobar %d' % num)
+            getUtility(IIrcIDSet).new(person, 'launchpad', person.name)
+            people.append(person)
+        people.append(self.factory.makePerson(displayname='foobar 4'))
+
+        # Remember the current values for checking later, and throw out
+        # the cache.
+        expected_nicks = dict(
+            (person.id, list(person.ircnicknames)) for person in people)
+        expected_emails = dict(
+            (person.id, person.preferredemail) for person in people)
+        Store.of(people[0]).invalidate()
+
+        with FeatureFixture(PERSON_AFFILIATION_RANK_FLAG):
+            results = list(self.searchVocabulary(None, u'foobar'))
+        with StormStatementRecorder() as recorder:
+            self.assertEquals(4, len(results))
+            for person in results:
+                self.assertEqual(
+                    expected_nicks[person.id], person.ircnicknames)
+                self.assertEqual(
+                    expected_emails[person.id], person.preferredemail)
+        self.assertThat(recorder, HasQueryCount(Equals(0)))
+
+
 class TeamMemberVocabularyTestBase(VocabularyTestBase):
 
     def test_open_team_cannot_be_a_member_of_a_closed_team(self):

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2011-06-11 09:49:37 +0000
+++ lib/lp/registry/vocabularies.py	2011-06-14 04:11:08 +0000
@@ -866,15 +866,15 @@
                 for email in emails
                 if email.status == EmailAddressStatus.PREFERRED)
 
-            # The irc nicks.
-            nicks = bulk.load_referencing(IrcID, persons, ['personID'])
-            nicks_by_person = dict((nick.personID, nicks)
-                for nick in nicks)
-
             for person in persons:
                 cache = get_property_cache(person)
                 cache.preferredemail = email_by_person.get(person.id, None)
-                cache.ircnicknames = nicks_by_person.get(person.id, None)
+                cache.ircnicknames = []
+
+            # The irc nicks.
+            nicks = bulk.load_referencing(IrcID, persons, ['personID'])
+            for nick in nicks:
+                get_property_cache(nick.person).ircnicknames.append(nick)
 
         return DecoratedResultSet(result, pre_iter_hook=pre_iter_hook)