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	2015-09-23 14:21:15 +0000
> +++ lib/lp/snappy/browser/snap.py	2015-09-26 00:51:11 +0000
> @@ -415,6 +422,15 @@
>      custom_widget('vcs', LaunchpadRadioWidget)
>      custom_widget('git_ref', GitRefWidget)
>  
> +    def setUpFields(self):
> +        """See `LaunchpadFormView`."""
> +        super(SnapEditView, self).setUpFields()
> +        self.form_fields += self.createEnabledProcessors(
> +            self.context.available_processors,
> +            u"The architectures that are available to be enabled or disabled "
> +            u"for this snap package. Some architectures are restricted and "
> +            u"may only be enabled or disabled by administrators.")

I'd s/The architectures that are available to be enabled or disabled for this snap package/The architectures on which this snap package can build/, or perhaps "for which this snap package builds". available_processors is the set that can be enabled or disabled, while this processors field is the set that is presently enabled.

> +
>      @property
>      def initial_values(self):
>          if self.context.git_ref is not None:
> @@ -437,6 +453,14 @@
>                          'this name.' % owner.displayname)
>              except NoSuchSnap:
>                  pass
> +        if 'processors' in data:
> +            available_processors = set(self.context.available_processors)
> +            for processor in self.context.processors:
> +                if (processor not in available_processors and
> +                        processor not in data['processors']):
> +                    # This processor is not currently available for
> +                    # selection, but is enabled.  Leave it untouched.
> +                    data['processors'].append(processor)

This needs the same disabled_items check as the archive fix last week.

>  
>  
>  class SnapDeleteView(BaseSnapEditView):
> 
> === modified file 'lib/lp/snappy/model/snap.py'
> --- lib/lp/snappy/model/snap.py	2015-09-18 14:14:34 +0000
> +++ lib/lp/snappy/model/snap.py	2015-09-26 00:51:11 +0000
> @@ -177,23 +187,55 @@
>          else:
>              return None
>  
> +    @property
> +    def available_processors(self):
> +        """See `ISnap`."""
> +        processors = Store.of(self).find(
> +            Processor,
> +            Processor.id == DistroArchSeries.processor_id,
> +            DistroArchSeries.id.is_in(
> +                self.distro_series.buildable_architectures.get_select_expr(
> +                    DistroArchSeries.id)),
> +            DistroArchSeries.enabled)

buildable_architectures is a silly property -- avoiding build creation due to a missing chroot is weird, and affecting other things is madness. Any reason not to use enabled_architectures here?

> +        return processors.config(distinct=True)
> +
>      def _getProcessors(self):
>          return list(Store.of(self).find(
>              Processor,
>              Processor.id == SnapArch.processor_id,
>              SnapArch.snap == self))
>  
> -    def setProcessors(self, processors):
> +    def setProcessors(self, processors, check_permissions=False, user=None):
>          """See `ISnap`."""
> +        if check_permissions:
> +            can_modify = None
> +            if user is not None:
> +                roles = IPersonRoles(user)
> +                authz = lambda perm: getAdapter(self, IAuthorization, perm)
> +                if authz('launchpad.Admin').checkAuthenticated(roles):
> +                    can_modify = lambda proc: True
> +                elif authz('launchpad.Edit').checkAuthenticated(roles):
> +                    can_modify = lambda proc: not proc.restricted
> +            if can_modify is None:
> +                raise Unauthorized(
> +                    'Permission launchpad.Admin or launchpad.Edit required '
> +                    'on %s.' % self)
> +        else:
> +            can_modify = lambda proc: True
> +
>          enablements = dict(Store.of(self).find(
>              (Processor, SnapArch),
>              Processor.id == SnapArch.processor_id,
>              SnapArch.snap == self))
>          for proc in enablements:
>              if proc not in processors:
> +                if not can_modify(proc):
> +                    raise CannotModifySnapProcessor(proc)
>                  Store.of(self).remove(enablements[proc])
>          for proc in processors:
>              if proc not in self.processors:
> +                if not can_modify(proc):
> +                    raise CannotModifySnapProcessor(proc)
>                  snaparch = SnapArch()
>                  snaparch.snap = self
>                  snaparch.processor = proc


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-processors-for-everyone/+merge/272507
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.