launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Pushed the requested changes. This should be ready for another round of review.

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'))

Ok! I'll remove this implementation, and add it only on https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/397755.

> +
>      _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)

I'll refactor this to set the information_type of the Snap being created to:
- git repo/bzr branch's information_type, if the repo/branch is private;
- PROPRIETARY, if the owner is private
- PUBLIC, otherwise

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

Ok!

> +                       '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']

Adding a XXX comment to make it clear that 'project' and 'information_type' fields in +admin are temporary, and should be moved to +edit as soon as the whole privacy work is done.

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

Ok! I'll add it.

> +
> +    @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.")),

Way better.

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