Code review comment for lp:~brian-murray/launchpad/modify-official-bug-tags-permissions

Hi Brian,

This branch looks good. I have a few comments below for changes I would like.

-Edwin

>=== modified file 'lib/canonical/launchpad/security.py'
>--- lib/canonical/launchpad/security.py 2010-09-14 18:28:53 +0000
>+++ lib/canonical/launchpad/security.py 2010-10-06 17:27:19 +0000
>@@ -61,6 +61,7 @@
> IBuildFarmJobOld,
> )
> from lp.buildmaster.interfaces.packagebuild import IPackageBuild
>+from lp.bugs.interfaces.bugtarget import IOfficialBugTagTargetRestricted
> from lp.code.interfaces.branch import (
> IBranch,
> user_has_special_branch_access,
>@@ -656,7 +657,7 @@
>
> def checkAuthenticated(self, user):
> """Only the Launchpad admins need this, we are only going to use it
>- for connecting up series and distroseriess where we did not have
>+ for connecting up series and distroseries where we did not have
> them."""

According to PEP8, if the docstring is more than one line, the
ending triple-quotes should appear on a line by itself.

> return user.in_admin
>
>@@ -897,6 +898,18 @@
> user.in_admin)
>
>
>+class EditProductOfficialBugTagsByOwnerBugSupervisorOrAdmins(AuthorizationBase):

This line is too long. It can be wrapped like
AdminDistroSeriesLanguagePacks.

>+ """The owner of a product and its bug supervisor should be able to
>+ edit its official bug tags."""

The first sentence of the docstring should fit on a single line, and
there should be a blank line separating it from any additional text.

>+ permission = 'launchpad.BugSupervisor'
>+ usedfor = IOfficialBugTagTargetRestricted
>+
>+ def checkAuthenticated(self, user):
>+ return (user.inTeam(self.obj.bug_supervisor) or
>+ user.inTeam(self.obj.owner) or
>+ user.in_admin)
>+
>+
> class AdminDistroSeries(AdminByAdminsTeam):
> """Soyuz involves huge chunks of data in the archive and librarian,
> so for the moment we are locking down admin and edit on distributions
>=== modified file 'lib/lp/bugs/stories/bug-tags/xx-official-bug-tags.txt'
>--- lib/lp/bugs/stories/bug-tags/xx-official-bug-tags.txt 2010-10-03 15:30:06 +0000
>+++ lib/lp/bugs/stories/bug-tags/xx-official-bug-tags.txt 2010-10-06 17:27:19 +0000
>@@ -52,13 +52,32 @@
> ...
> Unauthorized...
>
>+The link is also available for the bug supervisor.
>+
>+ >>> from lp.testing.sampledata import ADMIN_EMAIL
>+ >>> from canonical.launchpad.ftests import login, logout
>+ >>> login(ADMIN_EMAIL)
>+ >>> d_owner = factory.makePerson()
>+ >>> logout()
>+ >>> login_person(d_owner)
>+ >>> distro = factory.makeDistribution(name='youbuntu', owner=d_owner)
>+ >>> supervisor_team = factory.makeTeam(owner=d_owner)
>+ >>> supervisor_member = factory.makePerson(password='g00dpassword')
>+ >>> added = supervisor_team.addMember(supervisor_member, d_owner)
>+ >>> distro.setBugSupervisor(supervisor_team, d_owner)
>+ >>> bug_super_browser = setupBrowser(
>+ ... auth='Basic %s:g00dpassword' %
>+ ... supervisor_member.preferredemail.email)
>+ >>> logout()
>+ >>> bug_super_browser.open(
>+ ... 'http://bugs.launchpad.dev/youbuntu/+manage-official-tags')

You're missing a test to see if the "Edit official tags" link
appears on the http://bugs.launchpad.dev/firefox page. Instead of
opening +manage-official-tags, you can click the link and check where
bug_super_browser.url ends up.

> == Official Tags on Bug Pages ==
>
> Official tags are displayed using a different style from unofficial ones.
> They are grouped together at the beginning of the list.
>
>- >>> from canonical.launchpad.ftests import login, logout
> >>> from canonical.launchpad.webapp import canonical_url
> >>> from lp.bugs.tests.bug import print_bug_tag_anchors
> >>> import transaction
>
>=== modified file 'lib/lp/registry/configure.zcml'
>--- lib/lp/registry/configure.zcml 2010-10-04 20:46:55 +0000
>+++ lib/lp/registry/configure.zcml 2010-10-06 17:27:19 +0000
>@@ -1143,13 +1145,15 @@
> freshmeatproject homepage_content
> homepageurl icon license_info licenses
> logo mugshot official_answers
>- official_blueprints official_bug_tags
>- official_codehosting official_malone owner
>- programminglang project
>- redeemSubscriptionVoucher releaseroot
>- remote_product screenshotsurl
>- security_contact sourceforgeproject
>- summary title wikiurl"/>
>+ official_blueprints official_codehosting
>+ official_malone owner programminglang
>+ project redeemSubscriptionVoucher
>+ releaseroot remote_product screenshotsurl
>+ security_contact sourceforgeproject summary
>+ title wikiurl"/>

This list is a pain to read. Can you put each item on a line by
itself and sort it?

>+ <require
>+ permission="launchpad.BugSupervisor"
>+ set_attributes="official_bug_tags"/>
>
> <!-- mark 2006-04-10 I put "name" in the admin group because
> with Bazaar now in place, lots of people can have personal