launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/improve-personpicker-bugtaskaffiliation-798764 into lp:launchpad with lp:~sinzui/launchpad/person-picker-expand-0 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #798764 in Launchpad itself: "User affiliation with context in person picker is unclear"
  https://bugs.launchpad.net/launchpad/+bug/798764

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/improve-personpicker-bugtaskaffiliation-798764/+merge/70255

Improve the bugtask affiliation adaptor to account for:
- pillar driver
- pillar security contact
- pillar bug supervisor

The bug mentions adding affiliation checks for blueprints etc - this will be done in another branch and another bug will be created for that.

== Implementation ==

Simply update the getAffiliationBadge() method.

There are always 4 queries executed to do the core work, being:
select from Bug
select from BugTask
select from Person
select from Product/Distribution

The query counts stay the same regardless of whether affiliation checks are done for supervisor, security contact etc. The bug report mentions performance concerns but since the checks for supervisor, security contact do not add to the query count, it's ok to do these checks.

== Tests ==

Update lp/registry/tests/test_pillaraffiliation.py
As well as functional tests, tests are included to check the query counts.

== Lint ==

Linting changed files:
  lib/lp/registry/model/pillaraffiliation.py
  lib/lp/registry/tests/test_pillaraffiliation.py
  lib/lp/testing/factory.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/improve-personpicker-bugtaskaffiliation-798764/+merge/70255
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/improve-personpicker-bugtaskaffiliation-798764 into lp:launchpad.

=== modified file 'lib/lp/registry/model/pillaraffiliation.py'
--- lib/lp/registry/model/pillaraffiliation.py	2011-08-03 06:34:41 +0000
+++ lib/lp/registry/model/pillaraffiliation.py	2011-08-03 06:34:43 +0000
@@ -29,8 +29,11 @@
     )
 
 from canonical.launchpad.interfaces.launchpad import IHasIcon
+from lp.bugs.interfaces.bugsupervisor import IHasBugSupervisor
 from lp.bugs.interfaces.bugtask import IBugTask
+from lp.bugs.interfaces.securitycontact import IHasSecurityContact
 from lp.registry.interfaces.distribution import IDistribution
+from lp.registry.interfaces.role import IHasAppointedDriver
 
 
 class IHasAffiliation(Interface):
@@ -72,7 +75,20 @@
 
     def getAffiliationBadge(self, person):
         pillar = self.context.pillar
-        affiliated = person.inTeam(pillar.owner)
+        bug_supervisor = None
+        security_contact = None
+        driver = None
+        if IHasAppointedDriver.providedBy(pillar):
+            driver = pillar.driver
+        if IHasSecurityContact.providedBy(pillar):
+            security_contact = pillar.security_contact
+        if IHasBugSupervisor.providedBy(pillar):
+            bug_supervisor = pillar.bug_supervisor
+
+        affiliated = (person.inTeam(pillar.owner) or
+                    person.inTeam(bug_supervisor) or
+                    person.inTeam(security_contact) or
+                    person.inTeam(driver))
         if not affiliated:
             return None
 

=== modified file 'lib/lp/registry/tests/test_pillaraffiliation.py'
--- lib/lp/registry/tests/test_pillaraffiliation.py	2011-08-03 06:34:41 +0000
+++ lib/lp/registry/tests/test_pillaraffiliation.py	2011-08-03 06:34:43 +0000
@@ -5,29 +5,99 @@
 
 __metaclass__ = type
 
+from storm.store import Store
+from testtools.matchers import Equals
+
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.registry.model.pillaraffiliation import IHasAffiliation
-from lp.testing import TestCaseWithFactory
-
-
-class TestPillarAffiliation(TestCaseWithFactory):
+from lp.testing import StormStatementRecorder, TestCaseWithFactory
+from lp.testing.matchers import HasQueryCount
+
+
+class TestBugTaskPillarAffiliation(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
 
-    def test_bugtask_distro_affiliation(self):
+    def _check_affiliated_with_distro(self, person, distro):
+        bugtask = self.factory.makeBugTask(target=distro)
+        badge = IHasAffiliation(bugtask).getAffiliationBadge(person)
+        self.assertEqual(
+            badge, ("/@@/distribution-badge", "Affiliated with Pting"))
+
+    def test_bugtask_distro_owner_affiliation(self):
         # A person who owns a bugtask distro is affiliated.
         person = self.factory.makePerson()
         distro = self.factory.makeDistribution(owner=person, name='pting')
-        bugtask = self.factory.makeBugTask(target=distro)
+        self._check_affiliated_with_distro(person, distro)
+
+    def test_bugtask_distro_security_contact_affiliation(self):
+        # A person who is the security contact for a bugtask distro is
+        # affiliated.
+        person = self.factory.makePerson()
+        distro = self.factory.makeDistribution(
+            security_contact=person, name='pting')
+        self._check_affiliated_with_distro(person, distro)
+
+    def test_bugtask_distro_bug_supervisor_affiliation(self):
+        # A person who is the bug supervisor for a bugtask distro is
+        # affiliated.
+        person = self.factory.makePerson()
+        distro = self.factory.makeDistribution(
+            bug_supervisor=person, name='pting')
+        self._check_affiliated_with_distro(person, distro)
+
+    def _check_affiliated_with_product(self, person, product):
+        bugtask = self.factory.makeBugTask(target=product)
         badge = IHasAffiliation(bugtask).getAffiliationBadge(person)
         self.assertEqual(
-            badge, ("/@@/distribution-badge", "Affiliated with Pting"))
-
-    def test_bugtask_product_affiliation(self):
+            badge, ("/@@/product-badge", "Affiliated with Pting"))
+
+    def test_bugtask_product_driver_affiliation(self):
+        # A person who is the driver for a bugtask product is affiliated.
+        person = self.factory.makePerson()
+        product = self.factory.makeProduct(driver=person, name='pting')
+        self._check_affiliated_with_product(person, product)
+
+    def test_bugtask_product_security_contact_affiliation(self):
+        # A person who is the security contact for a bugtask product is
+        # affiliated.
+        person = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            security_contact=person, name='pting')
+        self._check_affiliated_with_product(person, product)
+
+    def test_bugtask_product_bug_supervisor_affiliation(self):
+        # A person who is the bug supervisor for a bugtask product is
+        # affiliated.
+        person = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            bug_supervisor=person, name='pting')
+        self._check_affiliated_with_product(person, product)
+
+    def test_bugtask_product_owner_affiliation(self):
         # A person who owns a bugtask product is affiliated.
         person = self.factory.makePerson()
         product = self.factory.makeProduct(owner=person, name='pting')
+        self._check_affiliated_with_product(person, product)
+
+    def test_bugtask_product_affiliation_query_count(self):
+        # Only 4 queries are expected, selects from:
+        # - Bug, BugTask, Product, Person
+        person = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=person, name='pting')
         bugtask = self.factory.makeBugTask(target=product)
-        badge = IHasAffiliation(bugtask).getAffiliationBadge(person)
-        self.assertEqual(
-            badge, ("/@@/product-badge", "Affiliated with Pting"))
+        Store.of(bugtask).invalidate()
+        with StormStatementRecorder() as recorder:
+            IHasAffiliation(bugtask).getAffiliationBadge(person)
+        self.assertThat(recorder, HasQueryCount(Equals(4)))
+
+    def test_bugtask_distro_affiliation_query_count(self):
+        # Only 4 queries are expected, selects from:
+        # - Bug, BugTask, Distribution, Person
+        person = self.factory.makePerson()
+        distro = self.factory.makeDistribution(owner=person, name='pting')
+        bugtask = self.factory.makeBugTask(target=distro)
+        Store.of(bugtask).invalidate()
+        with StormStatementRecorder() as recorder:
+            IHasAffiliation(bugtask).getAffiliationBadge(person)
+        self.assertThat(recorder, HasQueryCount(Equals(4)))

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-08-02 07:06:41 +0000
+++ lib/lp/testing/factory.py	2011-08-03 06:34:43 +0000
@@ -954,7 +954,7 @@
         licenses=None, owner=None, registrant=None,
         title=None, summary=None, official_malone=None,
         translations_usage=None, bug_supervisor=None,
-        driver=None):
+        driver=None, security_contact=None):
         """Create and return a new, arbitrary Product."""
         if owner is None:
             owner = self.makePerson()
@@ -990,6 +990,8 @@
             naked_product.bug_supervisor = bug_supervisor
         if driver is not None:
             naked_product.driver = driver
+        if security_contact is not None:
+            naked_product.security_contact = security_contact
         return product
 
     def makeProductSeries(self, product=None, name=None, owner=None,
@@ -2325,8 +2327,9 @@
     def makeDistribution(self, name=None, displayname=None, owner=None,
                          registrant=None, members=None, title=None,
                          aliases=None, bug_supervisor=None,
-                         publish_root_dir=None, publish_base_url=None,
-                         publish_copy_base_url=None, no_pubconf=False):
+                         security_contact=None, publish_root_dir=None,
+                         publish_base_url=None, publish_copy_base_url=None,
+                         no_pubconf=False):
         """Make a new distribution."""
         if name is None:
             name = self.getUniqueString(prefix="distribution")
@@ -2346,11 +2349,13 @@
         distro = getUtility(IDistributionSet).new(
             name, displayname, title, description, summary, domainname,
             members, owner, registrant)
+        naked_distro = removeSecurityProxy(distro)
         if aliases is not None:
-            removeSecurityProxy(distro).setAliases(aliases)
+            naked_distro.setAliases(aliases)
         if bug_supervisor is not None:
-            naked_distro = removeSecurityProxy(distro)
             naked_distro.bug_supervisor = bug_supervisor
+        if security_contact is not None:
+            naked_distro.security_contact = security_contact
         if not no_pubconf:
             self.makePublisherConfig(
                 distro, publish_root_dir, publish_base_url,