launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

Overall very good, thanks!  I think this needs a few tweaks especially around making sure that invalid values can't be set over the API, but it's pretty close.

Diff comments:

> === modified file 'lib/lp/snappy/browser/snap.py'
> --- lib/lp/snappy/browser/snap.py	2017-03-08 22:45:20 +0000
> +++ lib/lp/snappy/browser/snap.py	2017-03-23 20:53:26 +0000
> @@ -192,12 +192,7 @@
>  
>      @cachedproperty

This can probably just become a @property now, since it isn't doing anything potentially expensive any more.

>      def store_channels(self):
> -        vocabulary = getUtility(
> -            IVocabularyFactory, name='SnapStoreChannel')(self.context)
> -        channel_titles = []
> -        for channel in self.context.store_channels:
> -            channel_titles.append(vocabulary.getTermByToken(channel).title)
> -        return ', '.join(channel_titles)
> +        return ', '.join(self.context.store_channels)
>  
>  
>  def builds_for_snap(snap):
> 
> === modified file 'lib/lp/snappy/browser/tests/test_snap.py'
> --- lib/lp/snappy/browser/tests/test_snap.py	2017-02-03 11:30:05 +0000
> +++ lib/lp/snappy/browser/tests/test_snap.py	2017-03-23 20:53:26 +0000
> @@ -946,12 +947,14 @@
>              registrant=self.person, owner=self.person,
>              distroseries=self.distroseries, store_upload=True,
>              store_series=self.snappyseries, store_name=u"one",
> -            store_channels=["edge"])
> +            store_channels=["track/edge"])
>          view_url = canonical_url(snap, view_name="+edit")
>          browser = self.getNonRedirectingBrowser(url=view_url, user=self.person)
>          browser.getControl("Registered store package name").value = "two"
> +        self.assertEqual(browser.getControl("Track").value, "track")

Launchpad style is self.assertEqual(expected, observed), so switch the argument order here and anywhere else relevant in this MP.

> +        self.assertTrue(browser.getControl("Edge").selected)
> +        browser.getControl("Track").value = ""
>          browser.getControl("Stable").selected = True
> -        self.assertTrue(browser.getControl("Edge").selected)
>          root_macaroon = Macaroon()
>          root_macaroon.add_third_party_caveat(
>              urlsplit(config.launchpad.openid_provider_root).netloc, "",
> 
> === 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-23 20:53:26 +0000
> @@ -0,0 +1,137 @@
> +# 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.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,
> +    )
> +
> +
> +@implementer(ISingleLineWidgetLayout, IAlwaysSubmittedWidget, IInputWidget)
> +class StoreChannelsWidget(BrowserWidget, InputWidget):
> +
> +    template = ViewPageTemplateFile("templates/storechannels.pt")
> +    display_label = False
> +    _separator = '/'
> +    _default_track = 'latest'
> +    _widgets_set_up = False
> +
> +    def __init__(self, field, value_type, request):
> +        super(StoreChannelsWidget, self).__init__(field, request)
> +
> +    def setUpSubWidgets(self):
> +        if self._widgets_set_up:
> +            return
> +        fields = [
> +            TextLine(__name__="track", title=u"Track", required=False),
> +            List(__name__="risks", title=u"Risk", required=False,
> +                 value_type=Choice(vocabulary="SnapStoreChannel")),
> +            ]

I wonder if these need more detailed descriptions of some kind to explain what tracks and risks are?

> +
> +        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
> +
> +    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."""
> +        components = channel.split(self._separator)
> +        if len(components) == 2:
> +            track, risk = components
> +        elif len(components) == 1:
> +            track = None
> +            risk = components[0]
> +        else:
> +            raise AssertionError("Not a valid value: %r" % channel)
> +        return track, risk
> +
> +    def setRenderedValue(self, value):
> +        """See `IWidget`."""
> +        self.setUpSubWidgets()
> +        if value:
> +            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()

It's probably worth a comment somewhere around here to note the slight oddity that not all values of store_channels are valid.

> +            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-23 20:53:26 +0000
> @@ -537,11 +537,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,
>          description=_(
>              "Channels to release this snap package to after uploading it to "
> -            "the store.")))
> +            "the store. If track is not specified, the default track will be "
> +            "assumed.")))

This shows up in the generated API documentation, so it should probably explain the track/risk serialisation in some way.

Since this field can be set over the API, we should also have a constraint here to ensure that it's not possible to set a list of channels with non-matching tracks.

>  
>  
>  class ISnapAdminAttributes(Interface):
> 
> === modified file 'lib/lp/snappy/templates/snap-edit.pt'
> --- lib/lp/snappy/templates/snap-edit.pt	2016-07-28 12:42:21 +0000
> +++ lib/lp/snappy/templates/snap-edit.pt	2017-03-23 20:53:26 +0000
> @@ -86,8 +86,7 @@
>                  <tal:widget define="widget nocall:view/widgets/store_name">
>                    <metal:block use-macro="context/@@launchpad_form/widget_row" />
>                  </tal:widget>
> -                <tal:widget define="widget nocall:view/widgets/store_channels"
> -                            condition="widget/context/value_type/vocabulary">
> +                <tal:widget define="widget nocall:view/widgets/store_channels">

I'd keep the conditions here and in snap-new.pt with appropriate modifications (maybe adding a StoreChannelsWidget.has_risks_vocabulary property or similar), since the widget isn't going to work properly if the vocabulary is empty for whatever reason.

>                    <metal:block use-macro="context/@@launchpad_form/widget_row" />
>                  </tal:widget>
>                </table>


-- 
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.