launchpad-reviewers team mailing list archive

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

j.c.sackett has proposed merging lp:~jcsackett/launchpad/sharing-details-breadcrumb into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/sharing-details-breadcrumb/+merge/100498

Summary
=======
This branch adds breadcrumb info for the sharing details page. It also alters
the url for the sharing details page, slightly, from
+sharingdetails/$personname to +sharing/$personname, which is closer to the
url structure we agreed upon previously. Ideally, it would be
+sharing/details/person, but that doesn't seem entirely possible within zope's
traversal structure. The previous url (+sharingdetails) was believed to be
necessary at the time, that's not true.

Of note, this doesn't complete the breadcrumb setup as seen in mockups[1], but
as the managing disclosure view doesn't have any corresponding interface
beyond the pillar, it's not added into the traversed_objects for the Hierarchy
interface to convert into a breadcrumb. There are probably ways around this,
but sorting them out was delaying the useful bit of code contained in this
branch, i.e. a proper breadcrumb for the sharing details page.

[1]: http://people.canonical.com/~curtis/disclosure/details.html?context=albert

Preimp
======
None

Implementation
==============
This branch adds a simple IBreadCrumb adapter for the PillarPerson interface,
providing the name of the person in the pillar person as the breadcrumb text.

The zcml for the details page has been updated, as has the stepthrough
decorator for the NavigationMixin to alter the url from +sharingdetails to
+sharing.

Tests
=====
bin/test -vvct pillar_sharing

QA
==
Go to a sharing details page via +sharing/$personname; it should open fine,
verifying the url change.

Additionally, the page should have a breadcrumb of $pillarname >> $personname

Lint
====
Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/browser/configure.zcml
  lib/lp/registry/browser/pillar.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/sharing-details-breadcrumb/+merge/100498
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/sharing-details-breadcrumb into lp:launchpad.

=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2012-03-31 11:32:15 +0000
+++ lib/lp/registry/browser/configure.zcml	2012-04-02 19:02:24 +0000
@@ -1433,9 +1433,14 @@
         name="+sharing"
         class="lp.registry.browser.pillar.PillarSharingView"
         template="../templates/pillar-sharing.pt"/>
+    <adapter
+        provides="lp.services.webapp.interfaces.IBreadcrumb"
+        for="lp.registry.interfaces.pillar.IPillarPerson"
+        factory="lp.registry.browser.pillar.PillarPersonBreadcrumb"
+        permission="zope.Public"/>
     <browser:url
         for="lp.registry.interfaces.pillar.IPillarPerson"
-        path_expression="string:+sharingdetails/${person/name}"
+        path_expression="string:+sharing/${person/name}"
         rootsite="mainsite"
         attribute_to_parent="pillar"/>
     <browser:page

=== modified file 'lib/lp/registry/browser/pillar.py'
--- lib/lp/registry/browser/pillar.py	2012-03-31 11:32:15 +0000
+++ lib/lp/registry/browser/pillar.py	2012-04-02 19:02:24 +0000
@@ -6,7 +6,9 @@
 __metaclass__ = type
 
 __all__ = [
-    'InvolvedMenu', 'PillarBugsMenu', 'PillarView',
+    'InvolvedMenu',
+    'PillarBugsMenu',
+    'PillarView',
     'PillarNavigationMixin',
     'PillarPersonSharingView',
     'PillarSharingView',
@@ -61,6 +63,7 @@
     BatchNavigator,
     StormRangeFactory,
     )
+from lp.services.webapp.breadcrumb import Breadcrumb
 from lp.services.webapp.menu import (
     ApplicationMenu,
     enabled_with_permission,
@@ -75,9 +78,17 @@
     )
 
 
+class PillarPersonBreadcrumb(Breadcrumb):
+    """Builds a breadcrumb for an `IPillarPerson`."""
+
+    @property
+    def text(self):
+        return "%s" % self.context.person.displayname
+
+
 class PillarNavigationMixin:
 
-    @stepthrough('+sharingdetails')
+    @stepthrough('+sharing')
     def traverse_details(self, name):
         """Traverse to the sharing details for a given person."""
         person = getUtility(IPersonSet).getByName(name)

=== modified file 'lib/lp/registry/browser/tests/test_pillar_sharing.py'
--- lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-03-31 11:32:15 +0000
+++ lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-04-02 19:02:24 +0000
@@ -74,13 +74,13 @@
         return PillarPerson(self.pillar, person)
 
     def test_view_traverses_plus_sharingdetails(self):
-        # The traversed url in the app is pillar/+sharingdetails/person
+        # The traversed url in the app is pillar/+sharing/person
         with FeatureFixture(DETAILS_ENABLED_FLAG):
             # We have to do some fun url hacking to force the traversal a user
             # encounters.
             pillarperson = self.getPillarPerson()
             expected = pillarperson.person.displayname
-            url = 'http://launchpad.dev/%s/+sharingdetails/%s' % (
+            url = 'http://launchpad.dev/%s/+sharing/%s' % (
                 pillarperson.pillar.name, pillarperson.person.name)
             browser = self.getUserBrowser(user=self.owner, url=url)
             self.assertEqual(expected, browser.title)
@@ -92,7 +92,7 @@
             # We have to do some fun url hacking to force the traversal a user
             # encounters.
             pillarperson = self.getPillarPerson(with_sharing=False)
-            url = 'http://launchpad.dev/%s/+sharingdetails/%s' % (
+            url = 'http://launchpad.dev/%s/+sharing/%s' % (
                 pillarperson.pillar.name, pillarperson.person.name)
             browser = self.getUserBrowser(user=self.owner)
             self.assertRaises(NotFound, browser.open, url)