launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/person-affiliation-breakage-823644 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #823644 in Launchpad itself: "person picker affiliation breaks pickers"
  https://bugs.launchpad.net/launchpad/+bug/823644

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/person-affiliation-breakage-823644/+merge/70981

The person affiliation functionality renders an icon for the product/distro the person is affiliated with. There was a bug in the code to return the correct icon.

== Implementation ==

Fix bug - a one line typo.

== Tests ==

Add new tests to create a project and distro with non-default icons to provide the required coverage. Write test to specifically target the bug.

In test_pillaraffiliation:
  test_distro_badge_icon
  test_product_badge_icon
  test_pillar_badge_icon

== Lint ==

Checking for conflicts and issues in changed files.

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/person-affiliation-breakage-823644/+merge/70981
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/person-affiliation-breakage-823644 into lp:launchpad.

=== modified file 'lib/lp/registry/model/pillaraffiliation.py'
--- lib/lp/registry/model/pillaraffiliation.py	2011-08-09 02:06:15 +0000
+++ lib/lp/registry/model/pillaraffiliation.py	2011-08-10 02:57:34 +0000
@@ -96,7 +96,7 @@
                     icon_url = context.icon.getURL()
                     return icon_url
                 if IHasIcon.providedBy(pillar) and pillar.icon is not None:
-                    icon_url = context.icon.getURL()
+                    icon_url = pillar.icon.getURL()
                     return icon_url
                 return default_url
 

=== modified file 'lib/lp/registry/tests/test_pillaraffiliation.py'
--- lib/lp/registry/tests/test_pillaraffiliation.py	2011-08-09 12:14:42 +0000
+++ lib/lp/registry/tests/test_pillaraffiliation.py	2011-08-10 02:57:34 +0000
@@ -9,7 +9,10 @@
 from testtools.matchers import Equals
 from zope.component import getUtility
 
-from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
 from lp.registry.model.pillaraffiliation import IHasAffiliation
 from lp.services.worlddata.interfaces.language import ILanguageSet
 from lp.testing import (
@@ -22,12 +25,21 @@
 
 class TestPillarAffiliation(TestCaseWithFactory):
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
+
+    def test_distro_badge_icon(self):
+        # A distro's icon is used for the badge if present.
+        person = self.factory.makePerson()
+        icon = self.factory.makeLibraryFileAlias(
+            filename='smurf.png', content_type='image/png')
+        distro = self.factory.makeDistribution(
+            owner=person, name='pting', icon=icon)
+        [badge] = IHasAffiliation(distro).getAffiliationBadges([person])
+        self.assertEqual((icon.getURL(), "Pting maintainer"), badge)
 
     def _check_affiliated_with_distro(self, person, distro, role):
         [badge] = IHasAffiliation(distro).getAffiliationBadges([person])
-        self.assertEqual(
-            ("/@@/distribution-badge", "Pting %s" % role), badge)
+        self.assertEqual(("/@@/distribution-badge", "Pting %s" % role), badge)
 
     def test_distro_owner_affiliation(self):
         # A person who owns a distro is affiliated.
@@ -64,10 +76,30 @@
         self.assertIs(
             None, IHasAffiliation(distro).getAffiliationBadges([person])[0])
 
+    def test_product_badge_icon(self):
+        # A product's icon is used for the badge if present.
+        person = self.factory.makePerson()
+        icon = self.factory.makeLibraryFileAlias(
+            filename='smurf.png', content_type='image/png')
+        product = self.factory.makeProduct(
+            owner=person, name='pting', icon=icon)
+        [badge] = IHasAffiliation(product).getAffiliationBadges([person])
+        self.assertEqual((icon.getURL(), "Pting maintainer"), badge)
+
+    def test_pillar_badge_icon(self):
+        # A pillar's icon is used for the badge if the context has no icon.
+        person = self.factory.makePerson()
+        icon = self.factory.makeLibraryFileAlias(
+            filename='smurf.png', content_type='image/png')
+        product = self.factory.makeProduct(
+            owner=person, name='pting', icon=icon)
+        bugtask = self.factory.makeBugTask(target=product)
+        [badge] = IHasAffiliation(bugtask).getAffiliationBadges([person])
+        self.assertEqual((icon.getURL(), "Pting maintainer"), badge)
+
     def _check_affiliated_with_product(self, person, product, role):
         [badge] = IHasAffiliation(product).getAffiliationBadges([person])
-        self.assertEqual(
-            ("/@@/product-badge", "Pting %s" % role), badge)
+        self.assertEqual(("/@@/product-badge", "Pting %s" % role), badge)
 
     def test_product_driver_affiliation(self):
         # A person who is the driver for a product is affiliated.

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-08-09 04:09:33 +0000
+++ lib/lp/testing/factory.py	2011-08-10 02:57:34 +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, security_contact=None):
+        driver=None, security_contact=None, icon=None):
         """Create and return a new, arbitrary Product."""
         if owner is None:
             owner = self.makePerson()
@@ -980,7 +980,8 @@
             self.getUniqueString('description'),
             licenses=licenses,
             project=project,
-            registrant=registrant)
+            registrant=registrant,
+            icon=icon)
         naked_product = removeSecurityProxy(product)
         if official_malone is not None:
             naked_product.official_malone = official_malone
@@ -2332,7 +2333,7 @@
                          aliases=None, bug_supervisor=None, driver=None,
                          security_contact=None, publish_root_dir=None,
                          publish_base_url=None, publish_copy_base_url=None,
-                         no_pubconf=False):
+                         no_pubconf=False, icon=None):
         """Make a new distribution."""
         if name is None:
             name = self.getUniqueString(prefix="distribution")
@@ -2351,7 +2352,7 @@
             members = self.makeTeam(owner)
         distro = getUtility(IDistributionSet).new(
             name, displayname, title, description, summary, domainname,
-            members, owner, registrant)
+            members, owner, registrant, icon=icon)
         naked_distro = removeSecurityProxy(distro)
         if aliases is not None:
             naked_distro.setAliases(aliases)