launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

On Thu, Feb 04, 2016 at 11:58:36AM -0000, Celso Providelo wrote:
> 1) After checking the code behaviour, it seems to me that SnapBuilds for private archives will remain protected even if their parent Snap become public. So I don't see why ISnapSet.isValidPrivacy() would change. I am probably missing something.

You're right; please disregard that comment on your isValidPrivacy
implementation.  I was forgetting that the archive was an attribute of
the build, not the snap, so it isn't correct for us to constrain snap
privacy by archive privacy.

> 2) Requiring lp.Admin for Snap.private uncovered an issue that I am not sure how to solve. Commercial_admins and ppa_admins cannot admin snaps owned by private teams, it endup allowing only LP admins to administrate private snaps. See `test_admin_snap_privacy_mismatch`.

Commercial admins should be able to, because they can view any private
team (see ViewPublicOrPrivateTeamMembers.checkAuthenticated); but PPA
admins indeed won't be able to.  I think that's fine and correct.

Are you sure you tried using commercial_admin in that test, not just
ppa_admin?

> 3) Lastly, I am not happy with the code that inspect the privacy portlet (first_tag_by_class()). Is there anything better ?

You have id="privacy" set on the div element at the top level of the
portlet, so I'd use find_tag_by_id(content, "privacy").  There are some
other tests that do this, albeit doctests: see for example
lib/lp/bugs/stories/bug-privacy/xx-presenting-private-bugs.txt.

-- 
https://code.launchpad.net/~cprov/launchpad/snap-privacy-take1/+merge/284109
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.