launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> diff --git a/lib/lp/registry/personmerge.py b/lib/lp/registry/personmerge.py
> index 7873a61..86bd92b 100644
> --- a/lib/lp/registry/personmerge.py
> +++ b/lib/lp/registry/personmerge.py
> @@ -917,6 +935,11 @@ def merge_people(from_person, to_person, reviewer, delete=False):
>      _mergeSnap(cur, from_person, to_person)
>      skip.append(('snap', 'owner'))
>  
> +    # XXX pappacena 2021-02-18: add tests for this once we have
> +    # SnapSubscription model in place.
> +    _mergeSnapSubscription(cur, from_id, to_id)
> +    skip.append(('snapsubscription', 'person'))

If you adjust your DB branch as I suggested, you could perhaps postpone the full personmerge changes until the branch where you put the actual SnapSubscription model in place, which might be more comfortable since then you could test it at the same time.

> +
>      _mergeOCIRecipe(cur, from_person, to_person)
>      skip.append(('ocirecipe', 'owner'))
>  
> diff --git a/lib/lp/snappy/browser/snap.py b/lib/lp/snappy/browser/snap.py
> index 3b06a89..7fdbdda 100644
> --- a/lib/lp/snappy/browser/snap.py
> +++ b/lib/lp/snappy/browser/snap.py
> @@ -520,8 +529,10 @@ class SnapAddView(
>              kwargs = {'git_ref': self.context}
>          else:
>              kwargs = {'branch': self.context}
> -        private = not getUtility(
> -            ISnapSet).isValidPrivacy(False, data['owner'], **kwargs)
> +        private = not getUtility(ISnapSet).isValidPrivacy(
> +            False, data['owner'], **kwargs)
> +        information_type = (InformationType.PROPRIETARY if private else
> +                            InformationType.PUBLIC)

This broad categorization may remain useful for something like a snap owned by a private team that uses a public repository for some reason; but if the branch/repository is private, we should probably follow its more specific information type by default.

It also feels as though the pillar is an important thing to consider in determining valid information types.  Maybe this is for a later branch, but have you considered adding something like Product:+new-snap (e.g. /launchpad/+new-snap) so that the pillar is known when setting up the vocabulary for the information_type field?

>          if not data.get('auto_build', False):
>              data['auto_build_archive'] = None
>              data['auto_build_pocket'] = None
> @@ -612,25 +624,36 @@ class BaseSnapEditView(LaunchpadEditFormView, SnapAuthorizeMixin):
>  
>      def validate(self, data):
>          super(BaseSnapEditView, self).validate(data)
> -        if data.get('private', self.context.private) is False:
> -            if 'private' in data or 'owner' in data:
> +        info_type = data.get('information_type', self.context.information_type)
> +        editing_info_type = 'information_type' in data
> +        private = info_type in PRIVATE_INFORMATION_TYPES
> +        if private is False:
> +            # These are the requirements for public snaps.
> +            if 'information_type' in data or 'owner' in data:
>                  owner = data.get('owner', self.context.owner)
>                  if owner is not None and owner.private:
>                      self.setFieldError(
> -                        'private' if 'private' in data else 'owner',
> +                        'information_type' if editing_info_type else 'owner',
>                          'A public snap cannot have a private owner.')
> -            if 'private' in data or 'branch' in data:
> +            if 'information_type' in data or 'branch' in data:
>                  branch = data.get('branch', self.context.branch)
>                  if branch is not None and branch.private:
>                      self.setFieldError(
> -                        'private' if 'private' in data else 'branch',
> +                        'information_type' if editing_info_type else 'branch',
>                          'A public snap cannot have a private branch.')
> -            if 'private' in data or 'git_ref' in data:
> +            if 'information_type' in data or 'git_ref' in data:
>                  ref = data.get('git_ref', self.context.git_ref)
>                  if ref is not None and ref.private:
>                      self.setFieldError(
> -                        'private' if 'private' in data else 'git_ref',
> +                        'information_type' if editing_info_type else 'git_ref',
>                          'A public snap cannot have a private repository.')
> +        else:
> +            # Requirements for private snaps.
> +            project = data.get('project', self.context.project)
> +            if project is None:
> +                msg = ('Private Snap recipes should be associated '

"must" rather than "should".

> +                       'with a project.')
> +                self.setFieldError('project', msg)
>  
>      def _needStoreReauth(self, data):
>          """Does this change require reauthorizing to the store?"""
> @@ -696,16 +719,31 @@ class SnapAdminView(BaseSnapEditView):
>  
>      page_title = 'Administer'
>  
> -    field_names = ['private', 'require_virtualized', 'allow_internet']
> +    field_names = [
> +        'project', 'information_type', 'require_virtualized', 'allow_internet']

We restricted Snap.private to +admin to begin with because it clearly wasn't yet mature (see https://code.launchpad.net/~cprov/launchpad/snap-privacy-take1/+merge/284109 for the history here), but with these changes I think we should be considering moving project and information_type to +edit - they aren't the sort of thing that's usually on +admin.

(It would be fine for this to be in a later branch if more work is required first, but if so please leave an XXX comment.)

> +
> +    custom_widget_information_type = CustomWidgetFactory(
> +        LaunchpadRadioWidgetWithDescription,
> +        vocabulary=InformationTypeVocabulary(
> +            types=FREE_INFORMATION_TYPES + PROPRIETARY_INFORMATION_TYPES))

Not all these information types are necessarily permitted for any given snap: snaps without a pillar should only permit FREE_INFORMATION_TYPES, and snaps with a pillar should follow the pillar's branch sharing policy.  The conventional thing to do here would be for the model to have a getAllowedInformationTypes method which can be used to work out which ones are available depending on the pillar.  Compare GitRepositoryEditFormView.schema, for instance.

> +
> +    @property
> +    def initial_values(self):
> +        """Set initial values for the form."""
> +        # XXX pappacena 2021-02-12: Until we back fill information_type
> +        # database column, it will be NULL, but snap.information_type
> +        # property has a fallback to check "private" property. This should
> +        # be removed once we back fill snap.information_type.
> +        return {'information_type': self.context.information_type}
>  
>      def validate(self, data):
>          super(SnapAdminView, self).validate(data)
>          # BaseSnapEditView.validate checks the rules for 'private' in
>          # combination with other attributes.
> -        if data.get('private', None) is True:
> +        if data.get('information_type', None) in PRIVATE_INFORMATION_TYPES:
>              if not getFeatureFlag(SNAP_PRIVATE_FEATURE_FLAG):
>                  self.setFieldError(
> -                    'private',
> +                    'information_type',
>                      'You do not have permission to create private snaps.')
>  
>  
> diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
> index 9b19b1b..4829775 100644
> --- a/lib/lp/snappy/interfaces/snap.py
> +++ b/lib/lp/snappy/interfaces/snap.py
> @@ -884,6 +884,13 @@ class ISnapSet(Interface):
>  
>      @call_with(registrant=REQUEST_USER)
>      @operation_parameters(
> +        # Redefining information_type param to make it optional on the API
> +        # (although it is mandatory on the UI).
> +        information_type=Choice(
> +            title=_("Information type"), vocabulary=InformationType,
> +            required=False, default=InformationType.PUBLIC,
> +            description=_(
> +                "The type of information contained in this Snap recipe.")),

Maybe `information_type=copy_field(ISnap["information_type"], required=False)` instead.

>          processors=List(
>              value_type=Reference(schema=IProcessor), required=False))
>      @export_factory_operation(


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