launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code

This looks fine apart from some potential breadcrumb improvements.

Diff comments:

> === modified file 'lib/lp/registry/browser/configure.zcml'
> --- lib/lp/registry/browser/configure.zcml	2015-01-29 10:35:21 +0000
> +++ lib/lp/registry/browser/configure.zcml	2015-02-06 13:47:42 +0000
> @@ -1,4 +1,4 @@
> -<!-- Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
> +<!-- Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>       GNU Affero General Public License version 3 (see the file LICENSE).
>  -->
>  
> @@ -2154,6 +2154,19 @@
>          />
>  
>      <browser:url
> +        for="lp.registry.interfaces.persondistributionsourcepackage.IPersonDistributionSourcePackage"
> +        path_expression="string:${distro_source_package/distribution/name}/+source/${distro_source_package/sourcepackagename/name}"
> +        attribute_to_parent="person"/>
> +    <browser:navigation
> +        module="lp.registry.browser.persondistributionsourcepackage"
> +        classes="
> +            PersonDistributionSourcePackageNavigation"/>
> +    <browser:menus
> +        classes="
> +            PersonDistributionSourcePackageFacets"
> +        module="lp.registry.browser.persondistributionsourcepackage"/>
> +
> +    <browser:url
>          for="lp.registry.interfaces.personproduct.IPersonProduct"
>          path_expression="product/name"
>          attribute_to_parent="person"/>
> 
> === modified file 'lib/lp/registry/browser/person.py'
> --- lib/lp/registry/browser/person.py	2015-01-06 10:47:37 +0000
> +++ lib/lp/registry/browser/person.py	2015-02-06 13:47:42 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Person-related view classes."""
> @@ -154,6 +154,7 @@
>  from lp.registry.enums import PersonVisibility
>  from lp.registry.errors import VoucherAlreadyRedeemed
>  from lp.registry.interfaces.codeofconduct import ISignedCodeOfConductSet
> +from lp.registry.interfaces.distribution import IDistribution
>  from lp.registry.interfaces.gpg import IGPGKeySet
>  from lp.registry.interfaces.irc import IIrcIDSet
>  from lp.registry.interfaces.jabber import (
> @@ -172,6 +173,9 @@
>      IPersonClaim,
>      IPersonSet,
>      )
> +from lp.registry.interfaces.persondistributionsourcepackage import (
> +    IPersonDistributionSourcePackageFactory,
> +    )
>  from lp.registry.interfaces.personproduct import IPersonProductFactory
>  from lp.registry.interfaces.persontransferjob import (
>      IPersonDeactivateJobSource,
> @@ -357,7 +361,9 @@
>          raise NotFoundError
>  
>      def traverse(self, pillar_name):
> -        # If the pillar is a product, then return the PersonProduct.
> +        # If the pillar is a product, then return the PersonProduct; if it
> +        # is a distribution and further segments provide a source package,
> +        # then return the PersonDistributionSourcePackage.
>          pillar = getUtility(IPillarNameSet).getByName(pillar_name)
>          if IProduct.providedBy(pillar):
>              person_product = getUtility(IPersonProductFactory).create(
> @@ -369,6 +375,25 @@
>                      status=301)
>              getUtility(IOpenLaunchBag).add(pillar)
>              return person_product
> +        elif IDistribution.providedBy(pillar):
> +            if (len(self.request.stepstogo) >= 2 and
> +                self.request.stepstogo.peek() == "+source"):
> +                self.request.stepstogo.consume()
> +                spn_name = self.request.stepstogo.consume()
> +                dsp = IDistribution(pillar).getSourcePackage(spn_name)
> +                if dsp is not None:
> +                    factory = getUtility(
> +                        IPersonDistributionSourcePackageFactory)
> +                    person_dsp = factory.create(self.context, dsp)
> +                    # If accessed through an alias, redirect to the proper
> +                    # name.
> +                    if pillar.name != pillar_name:
> +                        return self.redirectSubTree(
> +                            canonical_url(person_dsp, request=self.request),
> +                            status=301)
> +                    getUtility(IOpenLaunchBag).add(pillar)
> +                    return person_dsp
> +
>          # Otherwise look for a branch.
>          try:
>              branch = getUtility(IBranchNamespaceSet).traverse(
> 
> === added file 'lib/lp/registry/browser/persondistributionsourcepackage.py'
> --- lib/lp/registry/browser/persondistributionsourcepackage.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/browser/persondistributionsourcepackage.py	2015-02-06 13:47:42 +0000
> @@ -0,0 +1,69 @@
> +# Copyright 2015 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Views, menus and traversal related to PersonDistributionSourcePackages."""
> +
> +__metaclass__ = type
> +__all__ = [
> +    'PersonDistributionSourcePackageBreadcrumb',
> +    'PersonDistributionSourcePackageFacets',
> +    'PersonDistributionSourcePackageNavigation',
> +    ]
> +
> +
> +from zope.component import queryAdapter
> +from zope.interface import implements
> +from zope.traversing.interfaces import IPathAdapter
> +
> +from lp.app.errors import NotFoundError
> +from lp.registry.interfaces.persondistributionsourcepackage import (
> +    IPersonDistributionSourcePackage,
> +    )
> +from lp.services.webapp import (
> +    canonical_url,
> +    Navigation,
> +    StandardLaunchpadFacets,
> +    )
> +from lp.services.webapp.breadcrumb import Breadcrumb
> +from lp.services.webapp.interfaces import IMultiFacetedBreadcrumb
> +
> +
> +class PersonDistributionSourcePackageNavigation(Navigation):
> +    usedfor = IPersonDistributionSourcePackage
> +
> +    def traverse(self, branch_name):
> +        # XXX cjwatson 2015-02-06: This will look for Git repositories in
> +        # the person/DSP namespace, but for now it does nothing.
> +        raise NotFoundError
> +
> +
> +# XXX cjwatson 2015-01-29: Do we need two breadcrumbs, one for the
> +# distribution and one for the source package?

Today's breadcrumb setup doesn't support this case well. My plan is for unofficial artifacts to have the normal target header (up to two breadcrumbs) plus a person, but that's not currently implemented.

In the current world, the heading should state the person. But it's not clear how the in-page breadcrumbs should look, as there is no PersonDistribution to link to. It may be worth introducing one.

> +class PersonDistributionSourcePackageBreadcrumb(Breadcrumb):
> +    """Breadcrumb for an `IPersonDistributionSourcePackage`."""
> +    implements(IMultiFacetedBreadcrumb)
> +
> +    @property
> +    def text(self):
> +        return self.context.distro_source_package.displayname
> +
> +    @property
> +    def url(self):
> +        if self._url is None:
> +            return canonical_url(
> +                self.context.distro_source_package, rootsite=self.rootsite)
> +        else:
> +            return self._url
> +
> +    @property
> +    def icon(self):
> +        return queryAdapter(
> +            self.context.distro_source_package, IPathAdapter,
> +            name='image').icon()
> +
> +
> +class PersonDistributionSourcePackageFacets(StandardLaunchpadFacets):
> +    """The links that will appear in the facet menu for an IPersonDSP."""
> +
> +    usedfor = IPersonDistributionSourcePackage
> +    enable_only = ['branches']
> 
> === modified file 'lib/lp/registry/configure.zcml'
> --- lib/lp/registry/configure.zcml	2015-01-29 16:28:30 +0000
> +++ lib/lp/registry/configure.zcml	2015-02-06 13:47:42 +0000
> @@ -1,4 +1,4 @@
> -<!-- Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
> +<!-- Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>       GNU Affero General Public License version 3 (see the file LICENSE).
>  -->
>  
> @@ -2065,6 +2065,27 @@
>              interface="lp.registry.interfaces.packaging.IPackagingUtil"/>
>      </securedutility>
>  
> +    <!-- PersonDistributionSourcePackage -->
> +
> +    <class
> +        class="lp.registry.model.persondistributionsourcepackage.PersonDistributionSourcePackage">
> +        <allow
> +            interface="lp.registry.interfaces.persondistributionsourcepackage.IPersonDistributionSourcePackageView" />
> +    </class>
> +
> +    <adapter
> +        provides="lp.services.webapp.interfaces.IBreadcrumb"
> +        for="lp.registry.interfaces.persondistributionsourcepackage.IPersonDistributionSourcePackage"
> +        factory="lp.registry.browser.persondistributionsourcepackage.PersonDistributionSourcePackageBreadcrumb"
> +        permission="zope.Public"/>
> +
> +    <securedutility
> +        component="lp.registry.model.persondistributionsourcepackage.PersonDistributionSourcePackage"
> +        provides="lp.registry.interfaces.persondistributionsourcepackage.IPersonDistributionSourcePackageFactory">
> +        <allow
> +            interface="lp.registry.interfaces.persondistributionsourcepackage.IPersonDistributionSourcePackageFactory"/>
> +    </securedutility>
> +
>      <!-- PersonNotification -->
>  
>      <class
> 
> === added file 'lib/lp/registry/interfaces/persondistributionsourcepackage.py'
> --- lib/lp/registry/interfaces/persondistributionsourcepackage.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/interfaces/persondistributionsourcepackage.py	2015-02-06 13:47:42 +0000
> @@ -0,0 +1,38 @@
> +# Copyright 2015 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""A person's view on a source package in a distribution."""
> +
> +__metaclass__ = type
> +__all__ = [
> +    'IPersonDistributionSourcePackage',
> +    'IPersonDistributionSourcePackageFactory',
> +    ]
> +
> +from lazr.restful.fields import Reference
> +from zope.interface import Interface
> +from zope.schema import TextLine
> +
> +from lp.registry.interfaces.person import IPerson
> +from lp.registry.interfaces.distributionsourcepackage import (
> +    IDistributionSourcePackage,
> +    )
> +
> +
> +class IPersonDistributionSourcePackageView(Interface):
> +    """Viewing a person's view on a source package in a distribution."""
> +
> +    person = Reference(IPerson)
> +    distro_source_package = Reference(IDistributionSourcePackage)
> +    displayname = TextLine()
> +
> +
> +class IPersonDistributionSourcePackage(IPersonDistributionSourcePackageView):
> +    """A person's view on a source package in a distribution."""
> +
> +
> +class IPersonDistributionSourcePackageFactory(Interface):
> +    """Creates `IPersonDistributionSourcePackage`s."""
> +
> +    def create(person, distro_source_package):
> +        """Create and return an `IPersonDistributionSourcePackage`."""
> 
> === added file 'lib/lp/registry/model/persondistributionsourcepackage.py'
> --- lib/lp/registry/model/persondistributionsourcepackage.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/model/persondistributionsourcepackage.py	2015-02-06 13:47:42 +0000
> @@ -0,0 +1,39 @@
> +# Copyright 2015 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""A person's view on a source package in a distribution."""
> +
> +__metaclass__ = type
> +__all__ = [
> +    'PersonDistributionSourcePackage',
> +    ]
> +
> +from zope.interface import (
> +    classProvides,
> +    implements,
> +    )
> +
> +from lp.registry.interfaces.persondistributionsourcepackage import (
> +    IPersonDistributionSourcePackage,
> +    IPersonDistributionSourcePackageFactory,
> +    )
> +
> +
> +class PersonDistributionSourcePackage:
> +
> +    implements(IPersonDistributionSourcePackage)
> +
> +    classProvides(IPersonDistributionSourcePackageFactory)
> +
> +    def __init__(self, person, distro_source_package):
> +        self.person = person
> +        self.distro_source_package = distro_source_package
> +
> +    @staticmethod
> +    def create(person, distro_source_package):
> +        return PersonDistributionSourcePackage(person, distro_source_package)
> +
> +    @property
> +    def displayname(self):
> +        return '%s in %s' % (
> +            self.person.displayname, self.distro_source_package.displayname)
> 
> === added file 'lib/lp/registry/tests/test_persondistributionsourcepackage.py'
> --- lib/lp/registry/tests/test_persondistributionsourcepackage.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/tests/test_persondistributionsourcepackage.py	2015-02-06 13:47:42 +0000
> @@ -0,0 +1,42 @@
> +# Copyright 2015 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Test the Person/DistributionSourcePackage non-database class."""
> +
> +__metaclass__ = type
> +
> +from lp.registry.model.persondistributionsourcepackage import (
> +    PersonDistributionSourcePackage,
> +    )
> +from lp.services.webapp.interfaces import IBreadcrumb
> +from lp.services.webapp.publisher import canonical_url
> +from lp.testing import TestCaseWithFactory
> +from lp.testing.layers import DatabaseFunctionalLayer
> +
> +
> +class TestPersonDistributionSourcePackage(TestCaseWithFactory):
> +    """Tests for `IPersonDistributionSourcePackage`s."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def _makePersonDistributionSourcePackage(self):
> +        person = self.factory.makePerson()
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        return PersonDistributionSourcePackage(person, dsp)
> +
> +    def test_canonical_url(self):
> +        # The canonical_url of a person DSP is
> +        # ~person/distribution/+source/sourcepackagename.
> +        pdsp = self._makePersonDistributionSourcePackage()
> +        dsp = pdsp.distro_source_package
> +        expected = 'http://launchpad.dev/~%s/%s/+source/%s' % (
> +            pdsp.person.name, dsp.distribution.name,
> +            dsp.sourcepackagename.name)
> +        self.assertEqual(expected, canonical_url(pdsp))
> +
> +    def test_breadcrumb(self):
> +        # Person DSPs give the DSP as their breadcrumb url.
> +        pdsp = self._makePersonDistributionSourcePackage()
> +        breadcrumb = IBreadcrumb(pdsp, None)
> +        self.assertEqual(
> +            canonical_url(pdsp.distro_source_package), breadcrumb.url)
> 


-- 
https://code.launchpad.net/~cjwatson/launchpad/person-dsp/+merge/248911
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.