Code review comment for lp:~adeuring/launchpad/bug-479331

Hi Abel,

Nice branch. Couple of minor issues to deal with - nothing major - and the
error message change that we discussed on IRC. Other than that, this is
good to go.

> === modified file 'lib/lp/bugs/browser/bugsupervisor.py'
> --- lib/lp/bugs/browser/bugsupervisor.py 2009-09-09 13:37:24 +0000
> +++ lib/lp/bugs/browser/bugsupervisor.py 2009-11-12 12:30:31 +0000
> @@ -11,7 +11,6 @@
> from lp.bugs.interfaces.bugsupervisor import IHasBugSupervisor
> from canonical.launchpad.webapp import (
> action, canonical_url, LaunchpadEditFormView)
> -from canonical.launchpad.webapp.authorization import check_permission
> from canonical.launchpad.webapp.menu import structured
>
>
> @@ -91,19 +90,31 @@
>
> supervisor = data['bug_supervisor']
>
> - if (supervisor is not None and supervisor.isTeam() and
> - supervisor not in self.user.getAdministratedTeams() and not
> - check_permission('launchpad.Admin', self.user)):
> - error = structured(
> - "You cannot set %(team)s as the bug supervisor for "
> - "%(target)s because you are not an administrator of that "
> - "team.<br />If you believe that %(team)s should be the bug"
> - " supervisor for %(target)s, please notify one of the "
> - "<a href=\"%(url)s\">%(team)s administrators</a>.",
> - team=supervisor.displayname,
> - target=self.context.displayname,
> - url=(canonical_url(supervisor, rootsite='mainsite') +
> - '/+members'))
> + # Making a person the bug supervisor implies subscribing him
> + # to all bug mail. Ensure that the current user can indeed
> + # do this.
> + if (supervisor is not None and
> + not self.context.userCanAlterSubscription(supervisor, self.user)):
> + if supervisor.isTeam():
> + error = structured(
> + "You cannot set %(team)s as the bug supervisor for "
> + "%(target)s because you are not an administrator of that "
> + "team.<br />If you believe that %(team)s should be the "
> + "bug supervisor for %(target)s, please notify one of the "
> + "<a href=\"%(url)s\">%(team)s administrators</a>.",
> + team=supervisor.displayname,
> + target=self.context.displayname,
> + url=(canonical_url(supervisor, rootsite='mainsite') +
> + '/+members'))
> + self.setFieldError('bug_supervisor', error)
> + else:
> + error = structured(
> + "You cannot set another person as the bug supervisor for "
> + "%(target)s.<br/> If you believe that %(person)s should "
> + "be the bug supervisor for %(target)s, please ask this "
> + "person to make itself the bug supervisor.",

As we discussed on IRC, this error message needs to be changed, since
people who aren't project admins or members of admin teams can't set
themselves as bug supervisor.

> + person=supervisor.displayname,
> + target=self.context.displayname)
> self.setFieldError('bug_supervisor', error)
>
> def cancel_url(self):
>
> === modified file 'lib/lp/bugs/stories/initial-bug-contacts/05-set-distribution-bugcontact.txt'
> --- lib/lp/bugs/stories/initial-bug-contacts/05-set-distribution-bugcontact.txt 2009-08-13 19:03:36 +0000
> +++ lib/lp/bugs/stories/initial-bug-contacts/05-set-distribution-bugcontact.txt 2009-11-12 12:30:31 +0000
> @@ -11,57 +11,105 @@
> ...
> LinkNotFoundError
>
> -Now we login as Mark...
> +Now we login as Colin...
>
> - >>> browser.addHeader('Authorization', 'Basic <email address hidden>:test')
> - >>> browser.reload()
> - >>> browser.url
> - 'http://launchpad.dev/ubuntu/+bugs'
> + >>> colin_browser = setupBrowser(
> + ... auth='Basic <email address hidden>:test')
> + >>> colin_browser.open('http://launchpad.dev/ubuntu/+bugs')
>
> ...and we can see that the link is there.
>
> - >>> bug_supervisor_link = browser.getLink("Change bug supervisor")
> + >>> bug_supervisor_link = colin_browser.getLink("Change bug supervisor")
> >>> bug_supervisor_link.url
> 'http://launchpad.dev/ubuntu/+bugsupervisor'
>
> Anyone with launchpad.Edit permission can edit the distribution bug
> -supervisor. In this example, we're using <email address hidden> to set the
> -distribution bug supervisor.
> +supervisor, but most users can select only themselves and the teams they
> +administer. In this example, Carlos will set himself as the distribution

s/Carlos/Colin here, I think.

> +bug supervisor.
>
> >>> bug_supervisor_link.click()
> - >>> browser.url
> + >>> colin_browser.url
> 'http://launchpad.dev/ubuntu/+bugsupervisor'
>
> The bug supervisor page takes just one simple value: the bug supervisor email
> -or nickname. Let's set <email address hidden> as the bug supervisor for
> +or nickname. Let's set <email address hidden> as the bug supervisor for
> Ubuntu.
>
> - >>> browser.getControl("Bug Supervisor").value = '<email address hidden>'
> - >>> browser.getControl("Change").click()
> + >>> colin_browser.getControl("Bug Supervisor").value = (
> + ... ' <email address hidden> ')
> + >>> colin_browser.getControl("Change").click()
>
> And then we're redirected to the distribution page.
>
> - >>> browser.url
> + >>> colin_browser.url
> 'http://launchpad.dev/ubuntu'
>
> - >>> for tag in find_tags_by_class(browser.contents, "informational message"):
> + >>> for tag in find_tags_by_class(colin_browser.contents,
> + ... "informational message"):
> ... print tag
> - <div...Successfully changed the bug supervisor to ...Carlos Perelló Marín...
> + <div...Successfully changed the bug supervisor to ...Colin Watson...
>
> Now that the bug supervisor is set, there's a link to him on the
> -distribution page.
> +distribution's bug page page.
>
> - >>> browser.getLink(
> - ... url='/~carlos').url
> - 'http://launchpad.dev/~carlos'
> + >>> browser.open('http://bugs.launchpad.dev/ubuntu')
> + >>> browser.getLink(url='/~kamion').url
> + 'http://launchpad.dev/~kamion'
>
> When the bug supervisor doesn't have an email address, the displayname is
> -shown instead in the feedback message.
> -
> - >>> browser.open("http://launchpad.dev/ubuntu/+bugsupervisor")
> - >>> browser.getControl("Bug Supervisor").value = 'ubuntu-team'
> - >>> browser.getControl("Change").click()
> -
> - >>> for tag in find_tags_by_class(browser.contents, "informational message"):
> - ... print tag
> - <div...Successfully changed the bug supervisor to ...Ubuntu Team...
> +shown instead in the feedback message. Let's appoint the Ubuntu Team as
> +the bug supervisor. Since Colin is an administrator of this team, he can
> +do it.
> +
> + >>> colin_browser.open("http://launchpad.dev/ubuntu/+bugsupervisor")
> + >>> colin_browser.getControl("Bug Supervisor").value = 'ubuntu-team'
> + >>> colin_browser.getControl("Change").click()
> +
> + >>> print extract_text(colin_browser.contents)
> + Ubuntu Linux in Launchpad
> + ...
> + Successfully changed the bug supervisor to Ubuntu Team.
> + ...
> +
> +While anybody with edit permissions can set the bug supervisor, only
> +only certain persons can be successfully selected. All users can set

You've got one "only" more than you need here.