Code review comment for lp:~thumper/launchpad/allow-admin-to-set-branch-privacy

Hi Tim,

I think this branch is basically OK and will make Tom hate us at least
8% less :-)

> === modified file 'lib/lp/code/model/branch.py'
> --- lib/lp/code/model/branch.py 2009-11-20 15:58:09 +0000
> +++ lib/lp/code/model/branch.py 2009-11-26 02:41:00 +0000
> @@ -98,16 +98,18 @@
>
> private = BoolCol(default=False, notNull=True)
>
> - def setPrivate(self, private):
> + def setPrivate(self, private, user):
> """See `IBranch`."""
> if private == self.private:
> return
> - policy = IBranchNamespacePolicy(self.namespace)
> + # Only check the privacy if the user is not special.

I think "privacy policy" is clearer than just "privacy" here.

> + if (not user_has_special_branch_access(user)):
> + policy = IBranchNamespacePolicy(self.namespace)
>
> - if private and not policy.canBranchesBePrivate():
> - raise BranchCannotBePrivate()
> - if not private and not policy.canBranchesBePublic():
> - raise BranchCannotBePublic()
> + if private and not policy.canBranchesBePrivate():
> + raise BranchCannotBePrivate()
> + if not private and not policy.canBranchesBePublic():
> + raise BranchCannotBePublic()
> self.private = private
>
> registrant = ForeignKey(

> === modified file 'lib/lp/code/stories/branches/xx-branch-edit-privacy.txt'
> --- lib/lp/code/stories/branches/xx-branch-edit-privacy.txt 2009-05-06 15:13:39 +0000
> +++ lib/lp/code/stories/branches/xx-branch-edit-privacy.txt 2009-11-26 02:41:00 +0000
> @@ -1,4 +1,5 @@
> -= Changing branch privacy =
> +Changing branch privacy
> +=======================
>
> Which users can have private branches is controlled by
> the branch visibility policy (see doc/branch-visibility-policy.txt).
> @@ -9,22 +10,46 @@
> Once the branch is created, the user is able to change a public branch
> to private or a private branch to public in certain circumstances.
>
> -
> -== Branches with no visibility policy ==
> + >>> login('<email address hidden>')
> + >>> eric = factory.makePerson(
> + ... name='eric', displayname='Eric the Viking',
> + ... <email address hidden>', password='test')
> + >>> fooix = factory.makeProduct(
> + ... name='fooix', displayname='Fooix', owner=eric)
> + >>> trunk = factory.makeProductBranch(
> + ... owner=eric, product=fooix, name='trunk')
> + >>> logout()

Yay for killing off sampledata usage.

> +Branches with no visibility policy
> +----------------------------------
>
> If there is no policy set, then the user is not able to change the visiblity
> of the branch.
>
> - >>> browser = setupBrowser(auth='Basic <email address hidden>:test')
> - >>> browser.open('http://code.launchpad.dev/~name12/firefox/main')
> + >>> browser = setupBrowser(auth='Basic <email address hidden>:test')
> + >>> browser.open('http://code.launchpad.dev/~eric/fooix/trunk')
> >>> browser.getLink('Change branch details').click()
> >>> browser.getControl('Keep branch confidential')
> Traceback (most recent call last):
> ...
> LookupError: label 'Keep branch confidential'
>
> -
> -== Private branches allowed ==
> +However admin's can.

You don't want an apostrophe there. But "However an admin can" might
be better in any case.

> + >>> admin_browser.open('http://code.launchpad.dev/~eric/fooix/trunk')
> + >>> admin_browser.getLink('Change branch details').click()
> + >>> admin_browser.getControl('Keep branch confidential').click()
> + >>> admin_browser.getControl('Change Branch').click()
> +
> +The branch is now private, so it assumes the standard private presentation.
> +
> + >>> print_tag_with_id(admin_browser.contents, 'privacy')
> + This branch is private
> +
> +
> +Private branches allowed
> +------------------------
>
> Allow the Warty Security Team to have private firefox branches.
>
> @@ -40,6 +65,7 @@
> Now Sample Person, who is a member of the Warty Security Team should be
> able to mark the branch as private.
>
> + >>> browser = setupBrowser(auth='Basic <email address hidden>:test')
> >>> browser.open('http://code.launchpad.dev/~name12/firefox/main')
> >>> browser.getLink('Change branch details').click()
> >>> browser.getControl('Keep branch confidential').selected = True
> @@ -71,7 +97,8 @@
> The branch is now publicly accessible.
>
>
> -== Private Only branches ==
> +Private Only branches
> +---------------------
>
> If the policy specifies private only, then the user is able to make
> a public branch private, but not vice versa.

So just two trivial prose fixes. Thanks for doing this!

Cheers,
mwh