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. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
