launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> 
> === added file 'lib/lp/snappy/browser/widgets/storechannels.py'
> --- lib/lp/snappy/browser/widgets/storechannels.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/snappy/browser/widgets/storechannels.py	2017-03-27 19:30:58 +0000
> @@ -0,0 +1,152 @@
> +# Copyright 2017 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +__metaclass__ = type
> +
> +__all__ = [
> +    'StoreChannelsWidget',
> +    ]
> +
> +from z3c.ptcompat import ViewPageTemplateFile
> +from zope.formlib.interfaces import IInputWidget, WidgetInputError
> +from zope.formlib.utility import setUpWidget
> +from zope.formlib.widget import (
> +    BrowserWidget,
> +    CustomWidgetFactory,
> +    InputErrors,
> +    InputWidget,
> +    )
> +from zope.interface import implementer
> +from zope.schema import (
> +    Choice,
> +    List,
> +    TextLine,
> +    )
> +
> +from lp import _
> +from lp.app.errors import UnexpectedFormData
> +from lp.app.validators import LaunchpadValidationError
> +from lp.app.widgets.itemswidgets import LabeledMultiCheckBoxWidget
> +from lp.services.webapp.interfaces import (
> +    IAlwaysSubmittedWidget,
> +    ISingleLineWidgetLayout,
> +    )
> +from lp.snappy.validators.channels import (
> +    channel_components_delimiter,
> +    split_channel_name,
> +    )
> +
> +
> +@implementer(ISingleLineWidgetLayout, IAlwaysSubmittedWidget, IInputWidget)
> +class StoreChannelsWidget(BrowserWidget, InputWidget):
> +
> +    template = ViewPageTemplateFile("templates/storechannels.pt")
> +    display_label = False
> +    _separator = channel_components_delimiter
> +    _default_track = 'latest'
> +    _widgets_set_up = False
> +
> +    def __init__(self, field, value_type, request):
> +        # We don't use value_type.
> +        super(StoreChannelsWidget, self).__init__(field, request)
> +
> +    def setUpSubWidgets(self):
> +        if self._widgets_set_up:
> +            return
> +        fields = [
> +            TextLine(__name__="track", title=u"Track", required=False,
> +                     description=_(
> +                         "Track defines a series for your software. "
> +                         "If not specified, the default track is assumed. "

Should we mention that the default track is "latest"?

> +                         "Tracks should be requested to the store admins.")

"requested to" isn't quite good English here, and this also doesn't give users much of an idea of how to get in touch with the store admins.  I'd go with something like:

  To open a new track, ask the store admins (https://snapcraft.io/community).

(Ideally make this a proper link if you can - it may not be possible to do that via the field description, and you might have to put it in the template instead.)

> +                     ),
> +            List(__name__="risks", title=u"Risk", required=False,
> +                 value_type=Choice(vocabulary="SnapStoreChannel"),
> +                 description=_(
> +                     "Risks denote the stability of your software.")),
> +            ]
> +
> +        self.risks_widget = CustomWidgetFactory(LabeledMultiCheckBoxWidget)
> +        for field in fields:
> +            setUpWidget(
> +                self, field.__name__, field, IInputWidget, prefix=self.name)
> +        self.risks_widget.orientation = 'horizontal'
> +        self._widgets_set_up = True
> +
> +    @property
> +    def has_risks_vocabulary(self):
> +        risks_widget = getattr(self, 'risks_widget', None)
> +        return risks_widget and bool(risks_widget.vocabulary)
> +
> +    def buildChannelName(self, track, risk):
> +        """Return channel name composed from given track and risk."""
> +        channel = risk
> +        if track and track != self._default_track:
> +            channel = track + self._separator + risk
> +        return channel
> +
> +    def splitChannelName(self, channel):
> +        """Return extracted track and risk from given channel name."""
> +        try:
> +            track, risk = split_channel_name(channel)
> +        except ValueError:
> +            raise AssertionError("Not a valid value: %r" % channel)
> +        return track, risk
> +
> +    def setRenderedValue(self, value):
> +        """See `IWidget`."""
> +        self.setUpSubWidgets()
> +        if value:
> +            # NOTE: atm target channels must belong to the same track
> +            tracks = set()
> +            risks = []
> +            for channel in value:
> +                track, risk = self.splitChannelName(channel)
> +                tracks.add(track)
> +                risks.append(risk)
> +            if len(tracks) != 1:
> +                raise AssertionError("Not a valid value: %r" % value)
> +            track = tracks.pop()
> +            self.track_widget.setRenderedValue(track)
> +            self.risks_widget.setRenderedValue(risks)
> +        else:
> +            self.track_widget.setRenderedValue(None)
> +            self.risks_widget.setRenderedValue(None)
> +
> +    def hasInput(self):
> +        """See `IInputWidget`."""
> +        return ("%s.risks" % self.name) in self.request.form
> +
> +    def hasValidInput(self):
> +        """See `IInputWidget`."""
> +        try:
> +            self.getInputValue()
> +            return True
> +        except (InputErrors, UnexpectedFormData):
> +            return False
> +
> +    def getInputValue(self):
> +        """See `IInputWidget`."""
> +        self.setUpSubWidgets()
> +        risks = self.risks_widget.getInputValue()
> +        track = self.track_widget.getInputValue()
> +        if track and self._separator in track:
> +            error_msg = "Track name cannot include '%s'." % self._separator
> +            raise WidgetInputError(
> +                self.name, self.label, LaunchpadValidationError(error_msg))
> +        channels = [self.buildChannelName(track, risk) for risk in risks]
> +        return channels
> +
> +    def error(self):
> +        """See `IBrowserWidget`."""
> +        try:
> +            if self.hasInput():
> +                self.getInputValue()
> +        except InputErrors as error:
> +            self._error = error
> +        return super(StoreChannelsWidget, self).error()
> +
> +    def __call__(self):
> +        """See `IBrowserWidget`."""
> +        self.setUpSubWidgets()
> +        return self.template()
> 
> === modified file 'lib/lp/snappy/interfaces/snap.py'
> --- lib/lp/snappy/interfaces/snap.py	2017-02-06 14:34:35 +0000
> +++ lib/lp/snappy/interfaces/snap.py	2017-03-27 19:30:58 +0000
> @@ -537,11 +538,12 @@
>              "authorize uploads of this snap package."))
>  
>      store_channels = exported(List(
> -        value_type=Choice(vocabulary="SnapStoreChannel"),
> -        title=_("Store channels"), required=False, readonly=False,
> +        title=_("Store channels"),
> +        required=False, readonly=False, constraint=channels_validator,
>          description=_(
>              "Channels to release this snap package to after uploading it to "
> -            "the store.")))
> +            "the store. A channel is defined by a combination of an optional "
> +            " track and and a risk, e.g. '2.1/stable', or 'stable'.")))

Repeated "and".

>  
>  
>  class ISnapAdminAttributes(Interface):
> 
> === added directory 'lib/lp/snappy/validators'
> === added file 'lib/lp/snappy/validators/__init__.py'
> --- lib/lp/snappy/validators/__init__.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/snappy/validators/__init__.py	2017-03-27 19:30:58 +0000
> @@ -0,0 +1,2 @@
> +# Copyright 2017 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).

I usually just make these entirely empty, but as you like. :-)



-- 
https://code.launchpad.net/~matiasb/launchpad/upload-snap-to-track-based-channels/+merge/320855
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.