launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === added file 'lib/lp/snappy/browser/snapseries.py'
> --- lib/lp/snappy/browser/snapseries.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/snappy/browser/snapseries.py	2016-04-27 17:15:51 +0000
> @@ -0,0 +1,31 @@
> +# Copyright 2016 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""SnapSeries views."""
> +
> +from __future__ import absolute_import, print_function, unicode_literals
> +
> +__metaclass__ = type
> +__all__ = [
> +    'SnapSeriesNavigation',
> +    'SnapSeriesSetNavigation',
> +    ]
> +
> +from lp.services.webapp import (
> +    GetitemNavigation,
> +    Navigation,
> +    )
> +from lp.snappy.interfaces.snapseries import (
> +    ISnapSeries,
> +    ISnapSeriesSet,
> +    )
> +
> +
> +class SnapSeriesSetNavigation(GetitemNavigation):
> +    """Navigation methods for `ISnapSeriesSet`."""
> +    usedfor = ISnapSeriesSet
> +
> +
> +class SnapSeriesNavigation(Navigation):
> +    """Navigation methods for `ISnapSeries`."""
> +    usedfor = ISnapSeries

This doesn't seem terribly useful.

> 
> === added file 'lib/lp/snappy/interfaces/snapseries.py'
> --- lib/lp/snappy/interfaces/snapseries.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/snappy/interfaces/snapseries.py	2016-04-27 17:15:51 +0000
> @@ -0,0 +1,190 @@
> +# Copyright 2016 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Snap series interfaces."""
> +
> +from __future__ import absolute_import, print_function, unicode_literals
> +
> +__metaclass__ = type
> +__all__ = [
> +    'ISnapDistroSeries',
> +    'ISnapDistroSeriesSet',
> +    'ISnapSeries',
> +    'ISnapSeriesSet',
> +    'NoSuchSnapSeries',
> +    ]
> +
> +from lazr.restful.declarations import (
> +    call_with,
> +    collection_default_content,
> +    export_as_webservice_collection,
> +    export_as_webservice_entry,
> +    export_factory_operation,
> +    export_read_operation,
> +    exported,
> +    operation_for_version,
> +    operation_parameters,
> +    operation_returns_collection_of,
> +    operation_returns_entry,
> +    REQUEST_USER,
> +    )
> +from lazr.restful.fields import Reference
> +from zope.component import getUtility
> +from zope.interface import Interface
> +from zope.schema import (
> +    Choice,
> +    Datetime,
> +    Int,
> +    List,
> +    TextLine,
> +    )
> +
> +from lp import _
> +from lp.app.errors import NameLookupFailed
> +from lp.app.validators.name import name_validator
> +from lp.registry.interfaces.distroseries import IDistroSeries
> +from lp.registry.interfaces.series import SeriesStatus
> +from lp.services.fields import (
> +    ContentNameField,
> +    PublicPersonChoice,
> +    Title,
> +    )
> +
> +
> +class NoSuchSnapSeries(NameLookupFailed):
> +    """The requested `SnapSeries` does not exist."""
> +
> +    _message_prefix = "No such snap series"
> +
> +
> +class SnapSeriesNameField(ContentNameField):
> +    """Ensure that `ISnapSeries` has unique names."""
> +
> +    errormessage = _("%s is already in use by another series.")
> +
> +    @property
> +    def _content_iface(self):
> +        """See `UniqueField`."""
> +        return ISnapSeries
> +
> +    def _getByName(self, name):
> +        """See `ContentNameField`."""
> +        try:
> +            return getUtility(ISnapSeriesSet).getByName(name)
> +        except NoSuchSnapSeries:
> +            return None
> +
> +
> +class ISnapSeriesView(Interface):
> +    """`ISnapSeries` attributes that require launchpad.View permission."""
> +
> +    id = Int(title=_("ID"), required=True, readonly=True)
> +
> +    date_created = exported(Datetime(
> +        title=_("Date created"), required=True, readonly=True))
> +
> +    registrant = exported(PublicPersonChoice(
> +        title=_("Registrant"), required=True, readonly=True,
> +        vocabulary="ValidPersonOrTeam",
> +        description=_("The person who registered this snap package.")))
> +
> +
> +class ISnapSeriesEditableAttributes(Interface):
> +    """`ISnapSeries` attributes that can be edited.
> +
> +    These attributes need launchpad.View to see, and launchpad.Edit to change.

s/launchpad.View/zope.Public/

> +    """
> +
> +    name = exported(SnapSeriesNameField(
> +        title=_("Name"), required=True, readonly=False,
> +        constraint=name_validator))
> +
> +    display_name = exported(TextLine(
> +        title=_("Display name"), required=True, readonly=False))
> +
> +    title = Title(title=_("Title"), required=True, readonly=True)
> +
> +    status = exported(Choice(
> +        title=_("Status"), required=True, vocabulary=SeriesStatus))
> +
> +    usable_distro_series = exported(List(
> +        title=_("Usable distro series"),
> +        description=_(
> +            "The distro series that can be used for this snap series."),
> +        value_type=Reference(schema=IDistroSeries),
> +        required=True, readonly=False))

"usable" seems like a bad term, but I can't think of anything obviously better.

> +
> +
> +class ISnapSeries(ISnapSeriesView, ISnapSeriesEditableAttributes):
> +    """A series for snap packages in the store."""
> +
> +    # XXX cjwatson 2016-04-13 bug=760849: "beta" is a lie to get WADL
> +    # generation working.  Individual attributes must set their version to
> +    # "devel".
> +    export_as_webservice_entry(plural_name="snap_serieses", as_of="beta")
> +
> +
> +class ISnapDistroSeries(Interface):
> +    """A snap/distro series link."""
> +
> +    snap_series = Reference(ISnapSeries, title=_("Snap series"), readonly=True)
> +    distro_series = Reference(
> +        IDistroSeries, title=_("Distro series"), readonly=True)
> +
> +    title = Title(title=_("Title"), required=True, readonly=True)

Is this a useful interface to have? Seems similar to ArchiveArch, which is just a storage backend for methods and properties on IArchive.

The distinction between SnapSeries and SnapDistroSeries also occasionally confuses even me, so best we don't expose it to normal people.

> +
> +
> +class ISnapSeriesSetEdit(Interface):
> +    """`ISnapSeriesSet` methods that require launchpad.Edit permission."""
> +
> +    @call_with(registrant=REQUEST_USER)
> +    @export_factory_operation(ISnapSeries, ["name", "display_name", "status"])
> +    @operation_for_version("devel")
> +    def new(registrant, name, display_name, status, date_created=None):
> +        """Create an `ISnapSeries`."""
> +
> +
> +class ISnapSeriesSet(ISnapSeriesSetEdit):
> +    """Interface representing the set of snap series."""
> +
> +    export_as_webservice_collection(ISnapSeries)
> +
> +    def __iter__():
> +        """Iterate over `ISnapSeries`."""
> +
> +    def __getitem__(name):
> +        """Return the `ISnapSeries` with this name."""
> +
> +    @operation_parameters(
> +        name=TextLine(title=_("Snap series name"), required=True))
> +    @operation_returns_entry(ISnapSeries)
> +    @export_read_operation()
> +    @operation_for_version("devel")
> +    def getByName(name):
> +        """Return the `ISnapSeries` with this name.
> +
> +        :raises NoSuchSnapSeries: if no snap series exists with this name.
> +        """
> +
> +    @operation_parameters(
> +        distro_series=Reference(
> +            IDistroSeries, title=_("Distro series"), required=True))
> +    @operation_returns_collection_of(ISnapSeries)
> +    @export_read_operation()
> +    @operation_for_version("devel")
> +    def getByDistroSeries(distro_series):
> +        """Return all `ISnapSeries` usable with this `IDistroSeries`."""
> +
> +    @collection_default_content()
> +    def getAll():
> +        """Return all `ISnapSeries`."""
> +
> +
> +class ISnapDistroSeriesSet(Interface):
> +    """Interface representing the set of snap/distro series links."""
> +
> +    def getByDistroSeries(distro_series):
> +        """Return all `SnapDistroSeries` for this `IDistroSeries`."""

The web UI is likely to want the inverse as well; I can imagine a user will say "I want my snap to build for Ubuntu Core 16" and then choose a distroseries from that, rather than looking up the 16 -> xenial mapping elsewhere.

> +
> +    def getByBothSeries(snap_series, distro_series):
> +        """Return a `SnapDistroSeries` for this pair of series, or None."""
> 
> === modified file 'lib/lp/snappy/vocabularies.py'
> --- lib/lp/snappy/vocabularies.py	2015-09-18 14:14:34 +0000
> +++ lib/lp/snappy/vocabularies.py	2016-04-27 17:15:51 +0000
> @@ -29,3 +39,57 @@
>  
>      def __len__(self):
>          return len(self.context.getAllowedArchitectures())
> +
> +
> +class SnapSeriesVocabulary(StormVocabularyBase):
> +    """A vocabulary for searching snap series."""
> +
> +    _table = SnapSeries
> +    _clauses = [SnapSeries.status.is_in(ACTIVE_STATUSES)]
> +    _order_by = Desc(SnapSeries.date_created)
> +
> +
> +class SnapDistroSeriesVocabulary(StormVocabularyBase):
> +    """A vocabulary for searching snap/distro series combinations."""
> +
> +    _table = SnapDistroSeries
> +    _clauses = [
> +        SnapDistroSeries.snap_series_id == SnapSeries.id,
> +        SnapDistroSeries.distro_series_id == DistroSeries.id,
> +        DistroSeries.distributionID == Distribution.id,
> +        ]
> +
> +    @property
> +    def _entries(self):
> +        tables = [SnapDistroSeries, SnapSeries, DistroSeries, Distribution]
> +        entries = IStore(self._table).using(*tables).find(
> +            self._table, *self._clauses)
> +        return entries.order_by(
> +            Distribution.display_name, Desc(DistroSeries.date_created),
> +            Desc(SnapSeries.date_created))
> +
> +    def toTerm(self, obj):
> +        """See `IVocabulary`."""
> +        token = "%s/%s/%s" % (
> +            obj.distro_series.distribution.name, obj.distro_series.name,
> +            obj.snap_series.name)
> +        return SimpleTerm(obj, token, obj.title)
> +
> +    def __contains__(self, value):
> +        """See `IVocabulary`."""
> +        return value in self._entries
> +
> +    def getTerm(self, value):
> +        """See `IVocabulary`."""
> +        if value not in self:
> +            raise LookupError(value)
> +        return self.toTerm(value)
> +
> +
> +class BuildableSnapDistroSeriesVocabulary(SnapDistroSeriesVocabulary):
> +    """A vocabulary for searching active snap/distro series combinations."""
> +
> +    _clauses = SnapDistroSeriesVocabulary._clauses + [
> +        SnapSeries.status.is_in(ACTIVE_STATUSES),
> +        DistroSeries.status.is_in(ACTIVE_STATUSES),
> +        ]

I mean maybe, but 15.04 is a bit of a counterexample. Though ubuntu/vivid is still supported, I guess. Any reason to restrict it on anything more than SnapSeries.status?

> 
> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py	2016-04-04 10:06:33 +0000
> +++ lib/lp/testing/factory.py	2016-04-27 17:15:51 +0000
> @@ -4672,6 +4673,21 @@
>          return ProxyFactory(
>              SnapFile(snapbuild=snapbuild, libraryfile=libraryfile))
>  
> +    def makeSnapSeries(self, registrant=None, name=None, display_name=None,
> +                       status=SeriesStatus.DEVELOPMENT, date_created=DEFAULT):
> +        """Make a new SnapSeries."""
> +        if registrant is None:
> +            registrant = self.makePerson()
> +        if name is None:
> +            name = self.getUniqueString(u"snap-series-name")
> +        if display_name is None:
> +            display_name = SPACE.join(
> +                word.capitalize() for word in name.split('-'))
> +        snap_series = getUtility(ISnapSeriesSet).new(
> +            registrant, name, display_name, status, date_created=date_created)
> +        IStore(snap_series).flush()
> +        return snap_series

Seems like callsites will almost always want to set usable_distro_series.

> +
>  
>  # Some factory methods return simple Python types. We don't add
>  # security wrappers for them, as well as for objects created by


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-series/+merge/292516
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.