launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #09691
Re: [Merge] lp:~wallyworld/launchpad/make-private-branches-527900 into lp:launchpad
Thanks for reviewing!
>
> I have two broad questions:
> 1. I think this branch allows Lp admins to make branches public even though the public branches are forbidden. This contradicts the safety feature. I think the Lp Admin must first allow the project to have public branches so that we do not introduce data inconsistencies. I would like to think the Lp admin would then consider if making the branch public is very wrong and abandon the effort.
>
This was deliberate because I thought admins were to have super powers.
I can see thought that a mistake could be made to disclose information.
So I can remove the admin user check from the canBePublic() method.
> 2. I expected canBePrivate to check of the branch target is a project with a commercial subscription. Doing this allows new projects to create a private trunk and start working minutes after registration.
>
The feature you want is partly provided as part of the solution. The
private vocab items which are rendered include Proprietary if the branch
is associated with a project with a commercial subscription. But the
user who created the branch may not be given permission to make the
branch private. If I add the commercial subscription check to
canBePrivate(), this will allow the user to also make the branch
embargoed security or user data as well as proprietary. Do I need to
restrict the choice to proprietary in this case? ie if the
canBePrivate() check as currently written returns False but the branch
is for a commercial project, should the only private choice be
proprietary and not embargoes security or user data?
> This branch also fixes
> https://bugs.launchpad.net/launchpad/+bug/409898
>
Partly. We need bzr support to allow a user to register a private branch
straight up. Even with this mp, they need to push an empty branch and
flip the private toggle before they commit anything to it.
>> === modified file 'lib/lp/bugs/interfaces/bugtask.py'
>> --- lib/lp/bugs/interfaces/bugtask.py 2012-06-14 21:50:59 +0000
>> +++ lib/lp/bugs/interfaces/bugtask.py 2012-07-06 03:42:22 +0000
>> @@ -1178,7 +1179,8 @@
>> structural_subscriber=None, modified_since=None,
>> created_since=None, exclude_conjoined_tasks=False, cve=None,
>> upstream_target=None, milestone_dateexpected_before=None,
>> - milestone_dateexpected_after=None, created_before=None):
>> + milestone_dateexpected_after=None, created_before=None,
>> + information_types=None):
>
> This is a partial fix for
> https://bugs.launchpad.net/launchpad/+bug/66206
> https://bugs.launchpad.net/launchpad/+bug/741792
>
So it is. I can add the necessary fields to the search form as a followup.
> This glorious fix enables API users to get valuable reports. Alas the
> Advanced search for is crafted, someone needs to craft the template and
> the view to show and pass the field to search.
>
> We need to update these bugs after release to let everyone know that API
> works and that adding the UI can be done in a single branch.
>
>
> The comment in test_unauthorised_user_cannot_change_branch_to_private()
> does not make sense.
>
Bah. Cut and paste typo.
>> === modified file 'lib/lp/code/interfaces/branch.py'
>> --- lib/lp/code/interfaces/branch.py 2012-05-24 04:21:10 +0000
>> +++ lib/lp/code/interfaces/branch.py 2012-07-06 03:42:22 +0000
>> @@ -968,6 +968,25 @@
> ...
>> + def canBePrivate(user):
>> + """Can this branch be private?
>> +
>> + A branch can be made private if:
>> + - the branch has a visibility policy which allows it
>> + - the user is an admin or bzr expert
>> + - the branch is owned by a private team
>> + (The branch is already implicitly private)
>> + - the branch is linked to a private bug the user can access
>> + """
>
> I expected this to also mention that if the branch target is a project
> with a commercial subscription, it can be private.
>
Yes, will fix.
> === modified file 'lib/lp/code/model/branch.py'
> --- lib/lp/code/model/branch.py 2012-06-27 22:31:38 +0000
> +++ lib/lp/code/model/branch.py 2012-07-06 03:42:22 +0000
> @@ -1286,6 +1287,26 @@
> user, checked_branches)
> return can_access
>
> + def canBePublic(self, user):
> + """See `IBranch`."""
> + policy = IBranchNamespacePolicy(self.namespace)
> + return (policy.canBranchesBePublic() or
> + user_has_special_branch_access(user))
>
> Does this allow an Lp admin's to make an organisation's series branch
> public even though public is forbidden? I think the admins have to remove
> the forbidden rule first.
>
Yes, as per above, will remove the admin check.
> + def canBePrivate(self, user):
> + """See `IBranch`."""
> + policy = IBranchNamespacePolicy(self.namespace)
> + # Do the easy checks first.
> + if (policy.canBranchesBePrivate() or
> + user_has_special_branch_access(user) or
> + user.visibility == PersonVisibility.PRIVATE):
> + return True
> + params = BugTaskSearchParams(
> + user=user, linked_branches=self.id,
> + information_types=PRIVATE_INFORMATION_TYPES)
> + bug_ids = getUtility(IBugTaskSet).searchBugIds(params)
> + return bug_ids.count() > 0
>
> If my project has a commercial subscription, I should be permitted to
> make any of my branches private. I do not see this rule in the code.
> We don't want to setup BVPs.
>
Yes.
>> === modified file 'lib/lp/code/model/tests/test_branch.py'
>> --- lib/lp/code/model/tests/test_branch.py 2012-06-11 10:25:46 +0000
>> +++ lib/lp/code/model/tests/test_branch.py 2012-07-06 03:42:22 +0000
>> @@ -2504,6 +2508,106 @@
>> +class TestBranchCanBePrivate(TestCaseWithFactory):
>
> I expected a test that shows that if the project has a commercial subscription,
> any branch can be made private, and specifically proprietary. I guess the
> specific case is the job of the vocab, not canBePrivate().
>
Yes, this is mainly in the vocab. And there is a test for that.
>> +class TestBranchCanBePublic(TestCaseWithFactory):
> ...
>> + def test_arbitary_branch(self):
> s/arbitary/arbitrary/
>
>> + def test_admin(self):
>> + # Admins can make a branch public.
>> + team = self.factory.makeTeam(
>> + subscription_policy=TeamSubscriptionPolicy.RESTRICTED)
>> + product = self.factory.makeProduct()
>> + product.setBranchVisibilityTeamPolicy(
>> + team, BranchVisibilityRule.PRIVATE_ONLY)
>> + branch = self.factory.makeBranch(
>> + product=product, owner=team,
>> + information_type=InformationType.USERDATA)
>> + admin = getUtility(ILaunchpadCelebrities).admin
>> + self.assertTrue(branch.canBePublic(admin))
>
> This is wrong. I do not think we want an LP Admin to have the power to make
> an organisations's proprietary information public. The PRIVATE_ONLY flag
> must be disabled first so that we are certain that someone gave consideration
> to the issue.
>
Yes, will fix.
--
https://code.launchpad.net/~wallyworld/launchpad/make-private-branches-527900/+merge/113532
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
References