launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/fix-person-product-code-breadcrumbs into lp:launchpad.

Commit message:
Fixes the breadcrumbs for personproduct code pages.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #612387 in Launchpad itself: "Breadcrumb from PersonProduct active reviews page gives 404"
  https://bugs.launchpad.net/launchpad/+bug/612387

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/fix-person-product-code-breadcrumbs/+merge/125533

Summary
=======
Breadcrumbs on personproduct code pages are a little broken. If you're looking
at a branch or the activereviews off a person product, the breadcrumb with the
product name tries to take you to the person product overview page--which
neither exists, nor should. The code vshot breadcrumb is only for the person,
as person can adapt to the vhost and person product can't.

This corrects the personproduct breadcrumb to take you to the product
overview, and allows personproduct to adapt the code vhost breadcrumb so you
can get to the person product code page via breadcrumbs.

Preimp
======
Spoke with Curtis Hovey.

Implementation
==============
The breadcrumb adapter for PersonProduct has been changed to return the url
for the product.

IPersonProduct now inherits from IHasBranches as well as
IHasMergeProposals--given it does have branches, this is appropriate, and lets
the code breadcrumb be setup to direct to the person product branch collection
page.

Tests have been added and a number of other cleanups have been done to reduce
the LoC for this branch.

Tests
=====
bin/test -vvct personproduct

QA
==
Make sure the breadcrumbs on the page linked to from the bug work.

LoC
===
This branch is +1 LoC. I have -30 LoC credit.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/interfaces/personproduct.py
  lib/lp/code/browser/tests/test_branchmergeproposallisting.py
  lib/lp/registry/browser/personproduct.py
  lib/lp/code/model/tests/test_branchcollection.py
  lib/lp/code/adapters/tests/test_branchcollection.py
  lib/lp/registry/tests/test_personproduct.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/fix-person-product-code-breadcrumbs/+merge/125533
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/fix-person-product-code-breadcrumbs into lp:launchpad.

=== modified file 'lib/lp/code/adapters/tests/test_branchcollection.py'
--- lib/lp/code/adapters/tests/test_branchcollection.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/adapters/tests/test_branchcollection.py	2012-09-20 17:04:19 +0000
@@ -25,13 +25,10 @@
         product = self.factory.makeProduct()
         person = self.factory.makePerson()
         person_product = PersonProduct(person, product)
-        random_branch = self.factory.makeBranch()
-        product_branch = self.factory.makeProductBranch(product=product)
-        person_branch = self.factory.makeBranch(owner=person)
+        self.factory.makeBranch()
+        self.factory.makeProductBranch(product=product)
+        self.factory.makeBranch(owner=person)
         person_product_branch = self.factory.makeProductBranch(
             owner=person, product=product)
         branches = IBranchCollection(person_product).getBranches()
-        self.assertNotIn(random_branch, branches)
-        self.assertNotIn(product_branch, branches)
-        self.assertNotIn(person_branch, branches)
-        self.assertIn(person_product_branch, branches)
+        self.assertEqual([person_product_branch], [b for b in branches])

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py'
--- lib/lp/code/browser/tests/test_branchmergeproposallisting.py	2012-05-31 03:54:13 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposallisting.py	2012-09-20 17:04:19 +0000
@@ -199,8 +199,7 @@
         """The merges view should be enabled for PersonProduct."""
         personproduct = PersonProduct(
             self.factory.makePerson(), self.factory.makeProduct())
-        self.getViewBrowser(personproduct, '+merges',
-                rootsite='code')
+        self.getViewBrowser(personproduct, '+merges', rootsite='code')
 
     def test_DistributionSourcePackage(self):
         """The merges view should be enabled for DistributionSourcePackage."""

=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py	2012-09-06 00:01:38 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py	2012-09-20 17:04:19 +0000
@@ -58,47 +58,36 @@
 
     layer = DatabaseFunctionalLayer
 
+    def assertCollection(self, target):
+        self.assertIsNot(None, IBranchCollection(target, None))
+
     def test_product(self):
         # A product can be adapted to a branch collection.
-        product = self.factory.makeProduct()
-        collection = IBranchCollection(product, None)
-        self.assertIsNot(None, collection)
+        self.assertCollection(self.factory.makeProduct())
 
     def test_project(self):
         # A project can be adapted to a branch collection.
-        project = self.factory.makeProject()
-        collection = IBranchCollection(project, None)
-        self.assertIsNot(None, collection)
+        self.assertCollection(self.factory.makeProject())
 
     def test_person(self):
         # A person can be adapted to a branch collection.
-        person = self.factory.makePerson()
-        collection = IBranchCollection(person, None)
-        self.assertIsNot(None, collection)
+        self.assertCollection(self.factory.makePerson())
 
     def test_distribution(self):
         # A distribution can be adapted to a branch collection.
-        distribution = self.factory.makeDistribution()
-        collection = IBranchCollection(distribution, None)
-        self.assertIsNot(None, collection)
+        self.assertCollection(self.factory.makeDistribution())
 
     def test_distro_series(self):
         # A distro series can be adapted to a branch collection.
-        distro_series = self.factory.makeDistroSeries()
-        collection = IBranchCollection(distro_series, None)
-        self.assertIsNot(None, collection)
+        self.assertCollection(self.factory.makeDistroSeries())
 
     def test_source_package(self):
         # A source package can be adapted to a branch collection.
-        source_package = self.factory.makeSourcePackage()
-        collection = IBranchCollection(source_package, None)
-        self.assertIsNot(None, collection)
+        self.assertCollection(self.factory.makeSourcePackage())
 
     def test_distribution_source_package(self):
         # A distribution source pakcage can be adapted to a branch collection.
-        distro_source_package = self.factory.makeDistributionSourcePackage()
-        collection = IBranchCollection(distro_source_package, None)
-        self.assertIsNot(None, collection)
+        self.assertCollection(self.factory.makeDistributionSourcePackage())
 
 
 class TestGenericBranchCollection(TestCaseWithFactory):

=== modified file 'lib/lp/registry/browser/personproduct.py'
--- lib/lp/registry/browser/personproduct.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/browser/personproduct.py	2012-09-20 17:04:19 +0000
@@ -22,6 +22,7 @@
     Navigation,
     StandardLaunchpadFacets,
     )
+from lp.services.webapp import canonical_url
 from lp.services.webapp.breadcrumb import Breadcrumb
 
 
@@ -48,6 +49,13 @@
         return self.context.product.displayname
 
     @property
+    def url(self):
+        if self._url is None:
+            return canonical_url(self.context.product, rootsite=self.rootsite)
+        else:
+            return self._url
+
+    @property
     def icon(self):
         return queryAdapter(
             self.context.product, IPathAdapter, name='image').icon()

=== modified file 'lib/lp/registry/interfaces/personproduct.py'
--- lib/lp/registry/interfaces/personproduct.py	2011-12-19 23:38:16 +0000
+++ lib/lp/registry/interfaces/personproduct.py	2012-09-20 17:04:19 +0000
@@ -15,18 +15,19 @@
 from zope.interface import Interface
 from zope.schema import TextLine
 
-from lp.code.interfaces.hasbranches import IHasMergeProposals
+from lp.code.interfaces.hasbranches import (
+    IHasBranches,
+    IHasMergeProposals,
+    )
 from lp.registry.interfaces.person import IPerson
 from lp.registry.interfaces.product import IProduct
 
 
-class IPersonProduct(IHasMergeProposals):
+class IPersonProduct(IHasMergeProposals, IHasBranches):
     """A person's view on a product."""
 
     person = Reference(IPerson)
-
     product = Reference(IProduct)
-
     displayname = TextLine()
 
 

=== modified file 'lib/lp/registry/tests/test_personproduct.py'
--- lib/lp/registry/tests/test_personproduct.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/tests/test_personproduct.py	2012-09-20 17:04:19 +0000
@@ -6,23 +6,30 @@
 __metaclass__ = type
 
 from lp.registry.model.personproduct import PersonProduct
+from lp.services.webapp.interfaces import IBreadcrumb
 from lp.services.webapp.publisher import canonical_url
-from lp.services.webapp.url import urlappend
 from lp.testing import TestCaseWithFactory
 from lp.testing.layers import DatabaseFunctionalLayer
 
 
-class TestPersonProductCanonicalUrl(TestCaseWithFactory):
-    """Tests for the canonical url of `IPersonProduct`s."""
+class TestPersonProduct(TestCaseWithFactory):
+    """Tests for `IPersonProduct`s."""
 
     layer = DatabaseFunctionalLayer
 
+    def _makePersonProduct(self):
+        person = self.factory.makePerson()
+        product = self.factory.makeProduct()
+        return PersonProduct(person, product)
+
     def test_canonical_url(self):
         # The canonical_url of a person product is ~person/product.
-        person = self.factory.makePerson()
-        product = self.factory.makeProduct()
-        pp = PersonProduct(person, product)
+        pp = self._makePersonProduct()
         self.assertEqual(
-            urlappend(canonical_url(person),
-                      product.name),
-            canonical_url(pp))
+                '~%s/%s' % pp.person.name, pp.product.name, canonical_url(pp))
+
+    def test_breadcrumb(self):
+        # Person products give the product as their breadcrumb url.
+        pp = self._makePersonProduct()
+        breadcrumb = IBreadcrumb(pp, None)
+        self.assertEqual(canonical_url(pp.product), breadcrumb.url)