launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> diff --git a/lib/lp/snappy/browser/snap.py b/lib/lp/snappy/browser/snap.py
> index d4ba8bd..3a6d0c8 100644
> --- a/lib/lp/snappy/browser/snap.py
> +++ b/lib/lp/snappy/browser/snap.py
> @@ -138,6 +140,32 @@ class SnapNavigation(WebhookTargetNavigationMixin, Navigation):
>              return self.context.getSubscription(person)
>  
>  
> +class SnapFormMixin:
> +    def validateVCSWidgets(self, cls, data):
> +        """Validates if VCS sub-widgets."""
> +        if self.widgets.get('vcs') is not None:
> +            # Set widgets as required or optional depending on the vcs
> +            # field.
> +            super(cls, self).validate_widgets(data, ['vcs'])

`super(SnapFormMixin, self)` doesn't specifically mean the superclass of `SnapFormMixin`; rather, it walks back through `self.__class__.__mro__` starting immediately after `SnapFormMixin`.  The real problem here is that the MRO is wrong:

    >>> SnapAddView.__mro__
    (<class 'lp.snappy.browser.snap.SnapAddView'>, <class 'lp.app.browser.launchpadform.LaunchpadFormView'>, <class 'lp.services.webapp.publisher.LaunchpadView'>, <class 'lp.services.webapp.publisher.UserAttributeCache'>, <class 'lp.snappy.browser.snap.SnapAuthorizeMixin'>, <class 'lp.soyuz.browser.archive.EnableProcessorsMixin'>, <class 'lp.snappy.browser.snap.SnapInformationTypeMixin'>, <class 'lp.snappy.browser.snap.SnapFormMixin'>, <type 'object'>)

The `validate_widgets` method that ought to be called next is in `LaunchpadFormView`, but that doesn't work properly because the mixins appear after `LaunchpadFormView` in the MRO.  To fix this, put mixins to the left of the "primary" base class in class definitions: that ensures that this sort of superclass delegation works in the expected way.  The views in this file seem to get this the wrong way round - probably originally my fault.  If you reorder the base classes then `super(SnapFormMixin, self)` will work as intended here.

Of course, you may think that the superclass delegation is less confusing outside of this mixin anyway, and that's OK, but I think it's worth correcting the mixin ordering anyway since it's a bit of a footgun.

> +            vcs = data.get('vcs')
> +            if vcs == VCSType.BZR:
> +                self.widgets['branch'].context.required = True
> +                self.widgets['git_ref'].context.required = False
> +            elif vcs == VCSType.GIT:
> +                self.widgets['branch'].context.required = False
> +                self.widgets['git_ref'].context.required = True
> +            else:
> +                raise AssertionError("Unknown branch type %s" % vcs)
> +
> +    def setUpVCSWidgets(self):
> +        widget = self.widgets.get('vcs')
> +        if widget is not None:
> +            current_value = widget._getFormValue()
> +            self.vcs_bzr_radio, self.vcs_git_radio = [
> +                render_radio_widget_part(widget, value, current_value)
> +                for value in (VCSType.BZR, VCSType.GIT)]
> +
> +
>  class SnapInformationTypeMixin:
>      def getPossibleInformationTypes(self, snap, user):
>          """Get the information types to display on the edit form.
> @@ -537,7 +590,7 @@ class SnapAddView(LaunchpadFormView, SnapAuthorizeMixin, EnableProcessorsMixin,
>      @property
>      def initial_values(self):
>          store_name = None
> -        if self.has_snappy_distro_series:
> +        if self.has_snappy_distro_series and not self.is_project_context:

Since Branch:+new-snap and GitRef:+new-snap are unaffected, I think dealing with that in a future MP is reasonable.

>              # Try to extract Snap store name from snapcraft.yaml file.
>              try:
>                  snapcraft_data = getUtility(ISnapSet).getSnapcraftYaml(


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/399184
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:snap-pillar-edit.