launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Review: Needs Information code

Hi Ian.

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.

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.

This branch also fixes
https://bugs.launchpad.net/launchpad/+bug/409898

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

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.

> === modified file 'lib/lp/code/browser/tests/test_branch.py'
> --- lib/lp/code/browser/tests/test_branch.py	2012-06-22 05:52:17 +0000
> +++ lib/lp/code/browser/tests/test_branch.py	2012-07-06 03:42:22 +0000
> @@ -946,7 +951,33 @@
...
> +    def test_unauthorised_user_cannot_change_branch_to_private(self):
> +        # An authorised user can make the information type private.
> +        user = self.factory.makePerson()
> +        with person_logged_in(user):
> +            branch = self.factory.makeBranch(owner=user)
> +            browser = self.getUserBrowser(
> +                canonical_url(branch) + '/+edit', user=user)
> +        self.assertRaises(
> +            LookupError, browser.getControl, "Embargoed Security")

The comment in test_unauthorised_user_cannot_change_branch_to_private()
does not make sense.

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

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

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

> === 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().

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

-- 
https://code.launchpad.net/~wallyworld/launchpad/make-private-branches-527900/+merge/113532
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.