launchpad-reviewers team mailing list archive

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

Pushed the requested changes.

Diff comments:

> diff --git a/lib/lp/registry/model/accesspolicy.py b/lib/lp/registry/model/accesspolicy.py
> index 2e8fddf..472509f 100644
> --- a/lib/lp/registry/model/accesspolicy.py
> +++ b/lib/lp/registry/model/accesspolicy.py
> @@ -156,18 +162,20 @@ class AccessArtifact(StormBase):
>          insert_values = []
>          for concrete in needed:
>              if IBug.providedBy(concrete):
> -                insert_values.append((concrete, None, None, None))
> +                insert_values.append((concrete, None, None, None, None))
>              elif IBranch.providedBy(concrete):
> -                insert_values.append((None, concrete, None, None))
> +                insert_values.append((None, concrete, None, None, None))
>              elif IGitRepository.providedBy(concrete):
> -                insert_values.append((None, None, concrete, None))
> +                insert_values.append((None, None, concrete, None, None))
>              elif ISpecification.providedBy(concrete):
> -                insert_values.append((None, None, None, concrete))
> +                insert_values.append((None, None, None, concrete, None))
> +            elif ISnap.providedBy(concrete):
> +                insert_values.append((None, None, None, None, concrete))

I agree. I've tried to keep the order, but totally failed to do so in this method. Fixing it!

>              else:
>                  raise ValueError("%r is not a supported artifact" % concrete)
> -        new = create(
> -            (cls.bug, cls.branch, cls.gitrepository, cls.specification),
> -            insert_values, get_objects=True)
> +        columns = (cls.bug, cls.branch, cls.gitrepository, cls.specification,
> +                   cls.snap)
> +        new = create(columns, insert_values, get_objects=True)
>          return list(existing) + new
>  
>      @classmethod
> diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
> index ce14933..c3ba48a 100644
> --- a/lib/lp/snappy/model/snap.py
> +++ b/lib/lp/snappy/model/snap.py
> @@ -1350,7 +1374,8 @@ class SnapSet:
>                      private_snap == False,
>                      Snap.owner_id.is_in(Select(
>                          TeamParticipation.teamID,
> -                        TeamParticipation.person == visible_by_user)))
> +                        TeamParticipation.person == visible_by_user)),
> +                    *get_private_snap_subscriber_filter(visible_by_user))

> The owner should be given an access grant, so it shouldn't be necessary to consider that case separately.
Right, I was not subscribing the owner automatically. I'll do that in this MP.

In production, we will need to run `snap.subscribe(snap.owner, snap.registrant)` for every snap before releasing it to everyone.

I agree with removing _findSnapVisibilityClause and using only the helper function.

>  
>      def findByURL(self, url, owner=None, visible_by_user=None):
>          """See `ISnapSet`."""
> @@ -1547,3 +1572,31 @@ class SnapStoreSecretsEncryptedContainer(NaClEncryptedContainerBase):
>                  config.snappy.store_secrets_private_key.encode("UTF-8"))
>          else:
>              return None
> +
> +
> +def get_private_snap_subscriber_filter(user):

Renaming it.

> +    """Returns the filter for all private Snaps that the given user is
> +    subscribed to (that is, has access without being directly an owner).
> +    """
> +    artifact_grant_query = Coalesce(
> +        ArrayIntersects(
> +            SQL("%s.access_grants" % Snap.__storm_table__),
> +            Select(
> +                ArrayAgg(TeamParticipation.teamID),
> +                tables=TeamParticipation,
> +                where=(TeamParticipation.person == user)
> +            )), False)
> +
> +    policy_grant_query = Coalesce(
> +        ArrayIntersects(
> +            Array(SQL("%s.access_policy" % Snap.__storm_table__)),
> +            Select(
> +                ArrayAgg(AccessPolicyGrant.policy_id),
> +                tables=(AccessPolicyGrant,
> +                        Join(TeamParticipation,
> +                             TeamParticipation.teamID ==
> +                             AccessPolicyGrant.grantee_id)),
> +                where=(TeamParticipation.person == user)
> +            )), False)
> +
> +    return [Or(artifact_grant_query, policy_grant_query)]


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