launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/snappy/browser/snap.py'
> --- lib/lp/snappy/browser/snap.py	2016-05-24 04:45:38 +0000
> +++ lib/lp/snappy/browser/snap.py	2016-05-24 05:16:28 +0000
> @@ -354,9 +388,20 @@
>          private = not getUtility(
>              ISnapSet).isValidPrivacy(False, data['owner'], **kwargs)
>          snap = getUtility(ISnapSet).new(
> -            self.user, data['owner'], data['distro_series'], data['name'],
> -            private=private, **kwargs)
> -        self.next_url = canonical_url(snap)
> +            self.user, data['owner'],
> +            data['store_distro_series'].distro_series, data['name'],
> +            private=private, store_upload=data['store_upload'],
> +            store_series=data['store_distro_series'].snappy_series,
> +            store_name=data['store_name'], **kwargs)
> +        if data['store_upload']:
> +            try:
> +                self.next_url = SnapAuthorizeView.requestAuthorization(
> +                    snap, self.request)
> +            except BadRequestPackageUploadResponse as e:
> +                self.setFieldError(
> +                    'store_upload', 'Cannot upload this package: %s' % e)

Raw exceptions in field errors sound like a bad idea.

> +        else:
> +            self.next_url = canonical_url(snap)
>  
>      def validate(self, data):
>          super(SnapAddView, self).validate(data)
> @@ -388,6 +433,10 @@
>                  render_radio_widget_part(widget, value, current_value)
>                  for value in (VCSType.BZR, VCSType.GIT)]
>  
> +    @property
> +    def has_snappy_distro_series(self):
> +        return not getUtility(ISnappyDistroSeriesSet).getAll().is_empty()

Is has_snappy_distro_series meaningfully different from True?

> +
>      def validate_widgets(self, data, names=None):
>          """See `LaunchpadFormView`."""
>          if self.widgets.get('vcs') is not None:
> @@ -418,8 +488,20 @@
>                  self.context.setProcessors(
>                      new_processors, check_permissions=True, user=self.user)
>              del data['processors']
> +        store_upload = data.get('store_upload', False)
> +        if not store_upload:
> +            data['store_name'] = None
> +        need_store_reauth = self._needStoreReauth(data)
>          self.updateContextFromData(data)
> -        self.next_url = canonical_url(self.context)
> +        if need_store_reauth:
> +            try:
> +                self.next_url = SnapAuthorizeView.requestAuthorization(
> +                    self.context, self.request)
> +            except BadRequestPackageUploadResponse as e:
> +                self.setFieldError(
> +                    'store_upload', 'Cannot upload this package: %s' % e)
> +        else:
> +            self.next_url = canonical_url(self.context)

This block is slightly non-trivial and duplicated with the add form.

>  
>      @property
>      def adapters(self):
> @@ -478,11 +568,18 @@
>  
>      @property
>      def initial_values(self):
> +        initial_values = {}
> +        if self.context.store_series is None:
> +            # XXX cjwatson 2016-04-26: Remove this case once all existing
> +            # Snaps have had a store_series backfilled.

Is it sensible to have it always be set?

> +            sds_set = getUtility(ISnappyDistroSeriesSet)
> +            initial_values['store_distro_series'] = sds_set.getByDistroSeries(
> +                self.context.distro_series).first()
>          if self.context.git_ref is not None:
> -            vcs = VCSType.GIT
> +            initial_values['vcs'] = VCSType.GIT
>          else:
> -            vcs = VCSType.BZR
> -        return {'vcs': vcs}
> +            initial_values['vcs'] = VCSType.BZR
> +        return initial_values
>  
>      def validate(self, data):
>          super(SnapEditView, self).validate(data)
> 
> === modified file 'lib/lp/snappy/model/snap.py'
> --- lib/lp/snappy/model/snap.py	2016-05-17 12:47:34 +0000
> +++ lib/lp/snappy/model/snap.py	2016-05-24 05:16:28 +0000
> @@ -291,6 +292,18 @@
>                      or not self.require_virtualized))]
>  
>      @property
> +    def store_distro_series(self):
> +        if self.store_series is None:
> +            return None
> +        return getUtility(ISnappyDistroSeriesSet).getByBothSeries(
> +            self.store_series, self.distro_series)

Will hilarity ensure if a SnappyDistroSeries is ever deleted? I'm thinking something like rolling-core.

> +
> +    @store_distro_series.setter
> +    def store_distro_series(self, value):
> +        self.distro_series = value.distro_series
> +        self.store_series = value.snappy_series
> +
> +    @property
>      def can_upload_to_store(self):
>          return (
>              config.snappy.store_upload_url is not None and


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-store-add-edit-views/+merge/294524
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.