launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

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.