Merge lp:~sinzui/launchpad/bug-contacts-1 into lp:launchpad

[Curtis Hovey]

This is my branch to refactor bug role view code for reuse in another view.

    lp:~sinzui/launchpad/bug-contacts-1
    Diff size: 797 (includes deletions of redundant tests)
    Launchpad bug: https://bugs.launchpad.net/bugs/133676
    Test command: ./bin/test -vv \
        -t test_bugsupervisor -t test_securitycontact -t initial-bug-contacts
    Pre-implementation: no one
    Target release: 10.05

Refactor bug role view code for reuse
-------------------------------------

+configure-bugtraker will the support bug supervisor and security contact
fields. The form views for those fields need refactoring so that common
validation and change behaviours can be shared.

Rules
-----

    * Extract the validate() and change actions of BugSupervisorEditView and
      SecurityContactEditView.
    * Use the same validation rules--security contact has no rules.
      This will fix bug 133676

QA
--

    * Visit http://bugs.edge.launchpad.net/launchpad-registry
    * Change the bug supervisor and security contact roles to persons,
      teams, and none to verify everything still works.

Lint
----

Linting changed files:
  lib/lp/bugs/browser/bugsupervisor.py
  lib/lp/bugs/browser/securitycontact.py
  lib/lp/bugs/browser/tests/test_bugsupervisor.py
  lib/lp/bugs/browser/tests/test_securitycontact.py
  lib/lp/bugs/stories/initial-bug-contacts/10-set-upstream-bugcontact.txt

Test
----

I created two tests to verify the behaviours imparted from the mixin and
the view under test. The two tests are similar because of the four implicit
states of validation and permission rules, but their messages and data were
too different to make a single test suite.

    * lib/lp/bugs/browser/tests/test_bugsupervisor.py
      * Test the validation and message behaviours.
    * lib/lp/bugs/browser/tests/test_securitycontact.py
      * Test the validation and message behaviours.
    * lib/lp/bugs/stories/initial-bug-contacts/10-set-upstream-bugcontact.txt
      * Deleted parts that tested the view, not the user experience.

Implementation
--------------

    * lib/lp/bugs/browser/bugsupervisor.py
      Extracted a mixin to do validation and data changes.
    * lib/lp/bugs/browser/securitycontact.py
      Used the mixin in the existing form. Removed the odd messages and
      assertion that the mixin controls.

[Henning Eggers (henninge)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 24.05.2010 20:20, schrieb Curtis Hovey:
> This is my branch to refactor bug role view code for reuse in another view.

Thanks for doing this!

>
> lp:~sinzui/launchpad/bug-contacts-1
> Diff size: 797 (includes deletions of redundant tests)
> Launchpad bug: https://bugs.launchpad.net/bugs/133676
> Test command: ./bin/test -vv \
> -t test_bugsupervisor -t test_securitycontact -t initial-bug-contacts
> Pre-implementation: no one

I guess I should trust you that this is the right thing to do. Sure looks
right but shouldn't the bugs team at least know about it?

> Target release: 10.05
>
>
> Refactor bug role view code for reuse
> -------------------------------------

[...]

> === modified file 'lib/lp/bugs/browser/bugsupervisor.py'

All good here except that I wonder if the Mixin should go in its own file?
(See next comment)

> === modified file 'lib/lp/bugs/browser/securitycontact.py'
> --- lib/lp/bugs/browser/securitycontact.py 2010-05-18 21:24:27 +0000
> +++ lib/lp/bugs/browser/securitycontact.py 2010-05-24 18:20:55 +0000
> @@ -7,11 +7,12 @@
> __all__ = ["SecurityContactEditView"]
>
> from lp.bugs.interfaces.securitycontact import IHasSecurityContact
> +from lp.bugs.browser.bugsupervisor import BugRoleMixin

from lp.bugs.browser.bugrole import BugRoleMixin

What do you think?

[...]

> === added file 'lib/lp/bugs/browser/tests/test_bugsupervisor.py'
> --- lib/lp/bugs/browser/tests/test_bugsupervisor.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/bugs/browser/tests/test_bugsupervisor.py 2010-05-24 18:20:55 +0000

Thanks for adding this test. ;)

> @@ -0,0 +1,166 @@
> +# Copyright10 Canonical Ltd. This software is licensed under the

# Copyright 2010 Canonical Ltd...

> +# GNU Affero General Public License version (see the file LICENSE).
> +
> +"""Unit tests for bug supervisor views."""
> +
> +__metaclass__ = type
> +
> +import unittest
> +
> +from zope.app.form.interfaces import ConversionError
> +
> +from lp.bugs.browser.bugsupervisor import BugSupervisorEditSchema
> +from lp.testing import login, login_person, TestCaseWithFactory
> +from lp.testing.views import create_initialized_view
> +from canonical.testing import DatabaseFunctionalLayer
> +
> +
> +class TestBugSupervisorEditView(TestCaseWithFactory):
> +
> + layer = DatabaseFunctionalLayer
> +
> + def setUp(self):
> + super(TestBugSupervisorEditView, self).setUp()
> + self.owner = self.factory.makePerson(
> + name='splat', displayname='<splat />')

I understand that you use specific names because they appear in the messages
that you test but using a "tag" displayname here seems a bit cryptic. All
those &lt; and &gt; make the messages harder to read. I'd suggest using proper
names here and use the "tags" when you need them. See my suggestion below.

> + self.product = self.factory.makeProduct(
> + name="boing", displayname='<boing />', owner=self.owner)

Same here.

> + self.team = self.factory.makeTeam(name='thud', owner=self.owner)
> + login_person(self.owner)
> +
> + def _makeForm(self, person):
> + if person is None:
> + ...

review: Approve (code)

[Curtis Hovey (sinzui)]

On Tue, 2010-05-25 at 16:34 +0000, Henning Eggers wrote:
...
> > === modified file 'lib/lp/bugs/browser/securitycontact.py'
> > --- lib/lp/bugs/browser/securitycontact.py 2010-05-18 21:24:27 +0000
> > +++ lib/lp/bugs/browser/securitycontact.py 2010-05-24 18:20:55 +0000
> > @@ -7,11 +7,12 @@
> > __all__ = ["SecurityContactEditView"]
> >
> > from lp.bugs.interfaces.securitycontact import IHasSecurityContact
> > +from lp.bugs.browser.bugsupervisor import BugRoleMixin
>
> from lp.bugs.browser.bugrole import BugRoleMixin
>
> What do you think?

I was thinking of __init__ since it is common. I did not create a module
because it would inflate the diff. Deryck agrees with
lp.bugs.browser.bugroles.

> [...]
>
> > === added file 'lib/lp/bugs/browser/tests/test_bugsupervisor.py'
> > --- lib/lp/bugs/browser/tests/test_bugsupervisor.py 1970-01-01 00:00:00 +0000
> > +++ lib/lp/bugs/browser/tests/test_bugsupervisor.py 2010-05-24 18:20:55 +0000
>
> Thanks for adding this test. ;)
>
> > @@ -0,0 +1,166 @@
> > +# Copyright10 Canonical Ltd. This software is licensed under the
>
> # Copyright 2010 Canonical Ltd...
>
> > +# GNU Affero General Public License version (see the file LICENSE).
> > +
> > +"""Unit tests for bug supervisor views."""
> > +
> > +__metaclass__ = type
> > +
> > +import unittest
> > +
> > +from zope.app.form.interfaces import ConversionError
> > +
> > +from lp.bugs.browser.bugsupervisor import BugSupervisorEditSchema
> > +from lp.testing import login, login_person, TestCaseWithFactory
> > +from lp.testing.views import create_initialized_view
> > +from canonical.testing import DatabaseFunctionalLayer
> > +
> > +
> > +class TestBugSupervisorEditView(TestCaseWithFactory):
> > +
> > + layer = DatabaseFunctionalLayer
> > +
> > + def setUp(self):
> > + super(TestBugSupervisorEditView, self).setUp()
> > + self.owner = self.factory.makePerson(
> > + name='splat', displayname='<splat />')
>
> I understand that you use specific names because they appear in the messages
> that you test but using a "tag" displayname here seems a bit cryptic. All
> those &lt; and &gt; make the messages harder to read. I'd suggest using proper
> names here and use the "tags" when you need them. See my suggestion below.

I rather not since these *are* only used when they are needed. I am not
a bug domain expert, but I am certain that showing a notification for
the change, and including links in it is a strong indication something
is fundamentally wrong with these roles. It is clear that changing the
roles is dangerous and the UI did not set my expectation *before* I made
the change. The markup has historically been an XSS issue and the
testing for this is weak, Each name is tested exactly once for each
message. I think this qualifies for just enough testing.

> > + self.product = self.factory.makeProduct(
> > + name="boing", displayname='<boing />', owner=self.owner)
>
> Same here.

^ as above.

> > + self.team = self.factory.makeTeam(name='thud', owner=self.owner)
> > + login_person(self.owner)
> > +
> > + def _makeForm(self, person):
> > + if person is None:
> > + name = ''
> ...

=== added file 'lib/lp/bugs/browser/bugrole.py'
--- lib/lp/bugs/browser/bugrole.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/browser/bugrole.py	2010-05-25 21:21:40 +0000
@@ -0,0 +1,127 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version (see the file LICENSE).
+
+"""Common classes to support bug roles."""
+
+__metaclass__ = type
+
+__all__ = [
+    'BugRoleMixin',
+    ]
+
+from canonical.launchpad.webapp.menu import structured
+from canonical.launchpad.webapp.publisher import canonical_url
+
+
+class BugRoleMixin:
+
+    INVALID_PERSON = object()
+    OTHER_USER = object()
+    OTHER_TEAM = object()
+    OK = object()
+
+    def _getFieldState(self, field_name, data):
+        """Return the enum that summarises the field state."""
+        # The field_name will not be in the data if the user did not enter
+        # a person in the ValidPersonOrTeam vocabulary.
+        if field_name not in data:
+            return self.INVALID_PERSON
+        role = data[field_name]
+        user = self.user
+        # The user may assign the role to None, himself, or a team he admins.
+        if role is None or self.context.userCanAlterSubscription(role, user):
+            return self.OK
+        # The user is not an admin of the team, or he entered another user.
+        if role.isTeam():
+            return self.OTHER_TEAM
+        else:
+            return self.OTHER_USER
+
+    def validateBugSupervisor(self, data):
+        """Validates the new bug supervisor.
+
+        Verify that the value is None, the user, or a team he administers,
+        otherwise, set a field error.
+        """
+        field_state = self._getFieldState('bug_supervisor', data)
+        if field_state is self.INVALID_PERSON:
+            error = (
+                'You must choose a valid person or team to be the '
+                'bug supervisor for %s.' % self.context.displayname)
+        elif field_state is self.OTHER_TEAM:
+            supervisor = data['bug_supervisor']
+            team_url = canonical_url(
+                supervisor, rootsite='mainsite', view_name='+members')
+            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, notify one of the '
+                '<a href="%(url)s">%(team)s administrators</a>. See '
+                '<a href="https://help.launchpad.net/BugSupervisors">'
+                'the help wiki</a> for information about setting a bug '
+                'supervisor.',
+                team=supervisor.displayname,
+                target=self.context.displayname,
+                url=team_url)
+        elif field_state is self.OTHER_USER:
+            error = structured(
+                'You cannot set another person as the bug supervisor for '
+                '%(target)s.<br />See '
+                '<a href="https://help.launchpad.net/BugSupervisors">'
+                'the help wiki</a> for information about setting a bug '
+                'supervisor.',
+                target=self.context.displayname)
+        else:
+            # field_state is self.OK.
+            return
+        self.setFieldError('bug_supervisor', error)
+
+    def changeBugSupervisor(self, bug_supervisor):
+        self.context.setBugSupervisor(bug_supervisor, self.user)
+        if bug_supervisor is not None:
+            self.request.response.addNotification(structured(
+                'Successfully changed the bug supervisor to '
+                '<a href="%(supervisor_url)s">%(displayname)s</a>.'
+                '<br /><a href="%(supervisor_url)s">%(displayname)s</a> '
+                'has also been subscribed to bug notifications for '
+                '%(targetname)s.<br />You can '
+                '<a href="%(targeturl)s/+subscribe">change the '
+                'subscriptions</a> for %(targetname)s at any time.',
+                supervisor_url=canonical_url(bug_supervisor),
+                displayname=bug_supervisor.displayname,
+                targetname=self.context.displayname,
+                targeturl=canonical_url(self.context)))
+
+    def validateSecurityContact(self, data):
+        """Validates the new security contact.
+
+        Verify that the value is None, the user, or a team he administers,
+        otherwise, set a field error.
+        """
+        field_state = self._getFieldState('security_contact', data)
+        if field_state is self.INVALID_PERSON:
+            error = (
+                'You must choose a valid person or team to be the '
+                'security contact for %s.' % self.context.displayname)
+        elif field_state is self.OTHER_TEAM:
+            supervisor = data['security_contact']
+            team_url = canonical_url(
+                supervisor, rootsite='mainsite', view_name='+members')
+            error = structured(
+                'You cannot set %(team)s as the security contact for '
+                '%(target)s because you are not an administrator of that '
+                'team.<br />If you believe that %(team)s should be the '
+                'security contact for %(target)s, notify one of the '
+                '<a href="%(url)s">%(team)s administrators</a>.',
+                team=supervisor.displayname,
+                target=self.context.displayname,
+                url=team_url)
+        elif field_state is self.OTHER_USER:
+            error = structured(
+                'You cannot set another person as the security contact for '
+                '%(target)s.', target=self.context.displayname)
+        else:
+            # field_state is self.OK.
+            return
+        self.setFieldError('security_contact', error)

=== modified file 'lib/lp/bugs/browser/bugsupervisor.py'
--- lib/lp/bugs/browser/bugsupervisor.py	2010-04-29 15:32:46 +0000
+++ lib/lp/bugs/browser/bugsupervisor.py	2010-05-25 21:21:40 +0000
@@ -1,19 +1,23 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Browser view for bug supervisor."""
 
 __metaclass__ = type
 
-__all__ = ['BugSupervisorEditView']
+__all__ = [
+    'BugSupervisorEditView',
+    ]
 
-from canonical.launchpad.webapp import (
-    action, canonical_url, LaunchpadEditFormView)
-from canonical.launchpad.webapp.menu import structured
-from lazr.restful.interface import copy_field, use_template
 from zope.interface import Interface
 
+from lazr.restful.interface import copy_field
+
+from canonical.launchpad.webapp.launchpadform import (
+    action, LaunchpadEditFormView)
+from canonical.launchpad.webapp.publisher import canonical_url
 from lp.bugs.interfaces.bugsupervisor import IHasBugSupervisor
+from lp.bugs.browser.bugrole import BugRoleMixin
 
 
 class BugSupervisorEditSchema(Interface):
@@ -26,7 +30,7 @@
         IHasBugSupervisor['bug_supervisor'], readonly=False)
 
 
-class BugSupervisorEditView(LaunchpadEditFormView):
+class BugSupervisorEditView(BugRoleMixin, LaunchpadEditFormView):
     """Browser view class for editing the bug supervisor."""
 
     schema = BugSupervisorEditSchema
@@ -47,99 +51,23 @@
         """See `LaunchpadFormView`"""
         return {BugSupervisorEditSchema: self.context}
 
+    @property
+    def next_url(self):
+        """See `LaunchpadFormView`."""
+        return canonical_url(self.context)
+
+    cancel_url = next_url
+
+    def validate(self, data):
+        """See `LaunchpadFormView`."""
+        self.validateBugSupervisor(data)
+
     @action('Change', name='change')
     def change_action(self, action, data):
         """Redirect to the target page with a success message."""
-        target = self.context
         bug_supervisor = data['bug_supervisor']
-        target.setBugSupervisor(bug_supervisor, self.user)
-
-        if bug_supervisor is not None:
-            self.request.response.addNotification(structured(
-                'Successfully changed the bug supervisor to '
-                '<a href="%(supervisor_url)s">%(displayname)s</a>.'
-                '<br />'
-                '<a href="%(supervisor_url)s">%(displayname)s</a> '
-                'has also been '
-                'subscribed to bug notifications for %(targetname)s. '
-                '<br />'
-                'You can '
-                '<a href="%(targeturl)s/+subscribe">'
-                'change the subscriptions</a> for '
-                '%(targetname)s at any time.',
-                supervisor_url=canonical_url(bug_supervisor),
-                displayname=bug_supervisor.displayname,
-                targetname=self.context.displayname,
-                targeturl=canonical_url(self.context)))
-        else:
+        self.changeBugSupervisor(bug_supervisor)
+        if bug_supervisor is None:
             self.request.response.addNotification(
                 "Successfully cleared the bug supervisor. "
                 "You can set the bug supervisor again at any time.")
-
-        self.request.response.redirect(canonical_url(target))
-
-    def validate(self, data):
-        """Validates the new bug supervisor.
-
-        The following values are valid as bug supervisors:
-            * None, indicating that the bug supervisor field for the target
-              should be cleared in change_action().
-            * A valid Person (email address or launchpad id).
-            * A valid Team of which the current user is an administrator.
-
-        If the bug supervisor entered does not meet any of the above
-        criteria then the submission will fail and the user will be notified
-        of the error.
-        """
-
-        # `data` will not have a bug_supervisor entry in cases where the
-        # bug_supervisor the user entered is valid according to the
-        # ValidPersonOrTeam vocabulary
-        # (i.e. is not a Person, Team or None).
-        if not data.has_key('bug_supervisor'):
-            self.setFieldError(
-                'bug_supervisor',
-                'You must choose a valid person or team to be the'
-                ' bug supervisor for %s.' %
-                self.context.displayname)
-
-            return
-
-        supervisor = data['bug_supervisor']
-
-        # 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>. See "
-                    "<a href=\"https://help.launchpad.net/BugSupervisors\">"
-                    "the help wiki</a> for information about setting a bug "
-                    "supervisor.",
-                    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 />See "
-                    "<a href=\"https://help.launchpad.net/BugSupervisors\">"
-                    "the help wiki</a> for information about setting a bug "
-                    "supervisor.",
-                    person=supervisor.displayname,
-                    target=self.context.displayname)
-            self.setFieldError('bug_supervisor', error)
-
-    def cancel_url(self):
-        """See `LaunchpadFormView`."""
-        return canonical_url(self.context)
-
-

=== modified file 'lib/lp/bugs/browser/securitycontact.py'
--- lib/lp/bugs/browser/securitycontact.py	2010-05-18 21:24:27 +0000
+++ lib/lp/bugs/browser/securitycontact.py	2010-05-25 21:21:40 +0000
@@ -1,17 +1,22 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Browser view classes for security contacts."""
 
 __metaclass__ = type
-__all__ = ["SecurityContactEditView"]
-
+
+__all__ = [
+    "SecurityContactEditView",
+    ]
+
+from canonical.launchpad.webapp.launchpadform import (
+    action, LaunchpadFormView)
+from canonical.launchpad.webapp.publisher import canonical_url
 from lp.bugs.interfaces.securitycontact import IHasSecurityContact
-from canonical.launchpad.webapp import (
-    canonical_url, LaunchpadFormView, action)
-
-
-class SecurityContactEditView(LaunchpadFormView):
+from lp.bugs.browser.bugrole import BugRoleMixin
+
+
+class SecurityContactEditView(BugRoleMixin, LaunchpadFormView):
     """Browser view for editing the security contact.
 
     self.context is assumed to implement IHasSecurityContact.
@@ -35,6 +40,10 @@
         return {
             'security_contact': self.context.security_contact}
 
+    def validate(self, data):
+        """See `LaunchpadFormView`."""
+        self.validateSecurityContact(data)
+
     @action('Change', name='change')
     def change_action(self, action, data):
         security_contact = data['security_contact']
@@ -43,25 +52,12 @@
 
         self.context.security_contact = security_contact
         if security_contact:
-            security_contact_display_value = None
-            if security_contact.preferredemail:
-                # The security contact was set to a new person or team.
-                security_contact_display_value = (
-                    security_contact.preferredemail.email)
-            else:
-                # The security contact doesn't have a preferred email address,
-                # so it must be a team.
-                assert security_contact.isTeam(), (
-                    "Expected security contact with no email address "
-                    "to be a team.")
-                security_contact_display_value = security_contact.displayname
-
             self.request.response.addNotification(
-                "Successfully changed the security contact to %s" %
-                security_contact_display_value)
+                "Successfully changed the security contact to %s." %
+                security_contact.displayname)
         else:
             self.request.response.addNotification(
-                "Successfully removed the security contact")
+                "Successfully removed the security contact.")
 
     @property
     def next_url(self):

=== added file 'lib/lp/bugs/browser/tests/test_bugsupervisor.py'
--- lib/lp/bugs/browser/tests/test_bugsupervisor.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/browser/tests/test_bugsupervisor.py	2010-05-25 21:21:40 +0000
@@ -0,0 +1,167 @@
+# Copyright10 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version (see the file LICENSE).
+
+"""Unit tests for bug supervisor views."""
+
+__metaclass__ = type
+
+import unittest
+
+from zope.app.form.interfaces import ConversionError
+
+from lp.bugs.browser.bugsupervisor import BugSupervisorEditSchema
+from lp.testing import login, login_person, TestCaseWithFactory
+from lp.testing.views import create_initialized_view
+from canonical.testing import DatabaseFunctionalLayer
+
+
+class TestBugSupervisorEditView(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestBugSupervisorEditView, self).setUp()
+        self.owner = self.factory.makePerson(
+            name='splat', displayname='<splat />')
+        self.product = self.factory.makeProduct(
+            name="boing", displayname='<boing />', owner=self.owner)
+        self.team = self.factory.makeTeam(name='thud', owner=self.owner)
+        login_person(self.owner)
+
+    def _makeForm(self, person):
+        if person is None:
+            name = ''
+        else:
+            name = person.name
+        return {
+            'field.bug_supervisor': name,
+            'field.actions.change': 'Change',
+            }
+
+    def test_view_attributes(self):
+        self.product.displayname = 'Boing'
+        view = create_initialized_view(
+            self.product, name='+bugsupervisor')
+        label = 'Edit bug supervisor for Boing'
+        self.assertEqual(label, view.label)
+        self.assertEqual(label, view.page_title)
+        fields = ['bug_supervisor']
+        self.assertEqual(fields, view.field_names)
+        adapter, context = view.adapters.popitem()
+        self.assertEqual(BugSupervisorEditSchema, adapter)
+        self.assertEqual(self.product, context)
+        self.assertEqual('http://launchpad.dev/boing', view.next_url)
+        self.assertEqual('http://launchpad.dev/boing', view.cancel_url)
+
+    def test_owner_appoint_self_from_none(self):
+        # This also verifies that displaynames are escaped.
+        form = self._makeForm(self.owner)
+        view = create_initialized_view(
+            self.product, name='+bugsupervisor', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(self.product.bug_supervisor, self.owner)
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        expected = (
+            'Successfully changed the bug supervisor to '
+            '<a href="http://launchpad.dev/~splat">&lt;splat /&gt;</a>.'
+            '<br /><a href="http://launchpad.dev/~splat">&lt;splat /&gt;</a> '
+            'has also been subscribed to bug notifications for '
+            '&lt;boing /&gt;.<br />You can '
+            '<a href="http://launchpad.dev/boing/+subscribe">change '
+            'the subscriptions</a> for &lt;boing /&gt; at any time.')
+        self.assertEqual(expected, notifications.pop().message)
+
+    def test_owner_appoint_self_from_another(self):
+        self.product.setBugSupervisor(self.team, self.owner)
+        form = self._makeForm(self.owner)
+        view = create_initialized_view(
+            self.product, name='+bugsupervisor', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(self.owner, self.product.bug_supervisor)
+
+    def test_owner_appoint_none(self):
+        self.product.setBugSupervisor(self.owner, self.owner)
+        form = self._makeForm(None)
+        view = create_initialized_view(
+            self.product, name='+bugsupervisor', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(self.product.bug_supervisor, None)
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        expected = (
+            'Successfully cleared the bug supervisor. '
+            'You can set the bug supervisor again at any time.')
+        self.assertEqual(expected, notifications.pop().message)
+
+    def test_owner_appoint_his_team(self):
+        form = self._makeForm(self.team)
+        view = create_initialized_view(
+            self.product, name='+bugsupervisor', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(self.team, self.product.bug_supervisor)
+
+    def test_owner_cannot_appoint_another_team(self):
+        team = self.factory.makeTeam(name='smack', displayname='<smack />')
+        form = self._makeForm(team)
+        view = create_initialized_view(
+            self.product, name='+bugsupervisor', form=form)
+        self.assertEqual(1, len(view.errors))
+        expected = (
+            'You cannot set &lt;smack /&gt; as the bug supervisor for '
+            '&lt;boing /&gt; because you are not an administrator of that '
+            'team.<br />If you believe that &lt;smack /&gt; should be the '
+            'bug supervisor for &lt;boing /&gt;, notify one of the '
+            '<a href="http://launchpad.dev/~smack/+members">&lt;smack /&gt; '
+            'administrators</a>. See '
+            '<a href="https://help.launchpad.net/BugSupervisors">the '
+            'help wiki</a> for information about setting a bug supervisor.')
+        self.assertEqual(expected, view.errors.pop())
+
+    def test_owner_cannot_appoint_a_nonvalid_user(self):
+        # The vocabulary only accepts valid users.
+        form = self._makeForm(None)
+        form['field.bug_supervisor'] = 'fnord'
+        view = create_initialized_view(
+            self.product, name='+bugsupervisor', form=form)
+        self.assertEqual(2, len(view.errors))
+        expected = (
+            'You must choose a valid person or team to be the bug supervisor '
+            'for &lt;boing /&gt;.')
+        self.assertEqual(expected, view.errors.pop())
+        self.assertTrue(isinstance(view.errors.pop(), ConversionError))
+
+    def test_owner_cannot_appoint_another_user(self):
+        another_user = self.factory.makePerson()
+        form = self._makeForm(another_user)
+        view = create_initialized_view(
+            self.product, name='+bugsupervisor', form=form)
+        self.assertEqual(1, len(view.errors))
+        expected = (
+            'You cannot set another person as the bug supervisor for '
+            '&lt;boing /&gt;.<br />See '
+            '<a href="https://help.launchpad.net/BugSupervisors">the help '
+            'wiki</a> for information about setting a bug supervisor.')
+        self.assertEqual(expected, view.errors.pop())
+
+    def test_admin_appoint_another_user(self):
+        another_user = self.factory.makePerson()
+        login('[e-mail]')
+        form = self._makeForm(another_user)
+        view = create_initialized_view(
+            self.product, name='+bugsupervisor', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(another_user, self.product.bug_supervisor)
+
+    def test_admin_appoint_another_team(self):
+        another_team = self.factory.makeTeam()
+        login('[e-mail]')
+        form = self._makeForm(another_team)
+        view = create_initialized_view(
+            self.product, name='+bugsupervisor', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(another_team, self.product.bug_supervisor)
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)

=== added file 'lib/lp/bugs/browser/tests/test_securitycontact.py'
--- lib/lp/bugs/browser/tests/test_securitycontact.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/browser/tests/test_securitycontact.py	2010-05-25 21:21:40 +0000
@@ -0,0 +1,154 @@
+# Copyright10 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version (see the file LICENSE).
+
+"""Unit tests for security contact views."""
+
+__metaclass__ = type
+
+import unittest
+
+from zope.app.form.interfaces import ConversionError
+
+from lp.testing import login, login_person, TestCaseWithFactory
+from lp.testing.views import create_initialized_view
+from canonical.testing import DatabaseFunctionalLayer
+
+
+class TestSecurityContactEditView(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestSecurityContactEditView, self).setUp()
+        self.owner = self.factory.makePerson(
+            name='splat', displayname='<splat />')
+        self.product = self.factory.makeProduct(
+            name="boing", displayname='<boing />', owner=self.owner)
+        self.team = self.factory.makeTeam(name='thud', owner=self.owner)
+        login_person(self.owner)
+
+    def _makeForm(self, person):
+        if person is None:
+            name = ''
+        else:
+            name = person.name
+        return {
+            'field.security_contact': name,
+            'field.actions.change': 'Change',
+            }
+
+    def test_view_attributes(self):
+        self.product.displayname = 'Boing'
+        view = create_initialized_view(
+            self.product, name='+securitycontact')
+        label = 'Edit Boing security contact'
+        self.assertEqual(label, view.label)
+        self.assertEqual(label, view.page_title)
+        fields = ['security_contact']
+        self.assertEqual(fields, view.field_names)
+        self.assertEqual('http://launchpad.dev/boing', view.next_url)
+        self.assertEqual('http://launchpad.dev/boing', view.cancel_url)
+
+    def test_owner_appoint_self_from_none(self):
+        # This also verifies that displaynames are escaped.
+        form = self._makeForm(self.owner)
+        view = create_initialized_view(
+            self.product, name='+securitycontact', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(self.product.security_contact, self.owner)
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        expected = (
+            'Successfully changed the security contact to &lt;splat /&gt;.')
+        self.assertEqual(expected, notifications.pop().message)
+
+    def test_owner_appoint_self_from_another(self):
+        self.product.security_contact = self.team
+        form = self._makeForm(self.owner)
+        view = create_initialized_view(
+            self.product, name='+securitycontact', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(self.owner, self.product.security_contact)
+
+    def test_owner_appoint_none(self):
+        self.product.security_contact = self.owner
+        form = self._makeForm(None)
+        view = create_initialized_view(
+            self.product, name='+securitycontact', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(self.product.security_contact, None)
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        expected = ('Successfully removed the security contact.')
+        self.assertEqual(expected, notifications.pop().message)
+
+    def test_owner_appoint_his_team(self):
+        form = self._makeForm(self.team)
+        view = create_initialized_view(
+            self.product, name='+securitycontact', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(self.product.security_contact, self.team)
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        expected = ('Successfully changed the security contact to Thud.')
+        self.assertEqual(expected, notifications.pop().message)
+
+    def test_owner_cannot_appoint_another_team(self):
+        team = self.factory.makeTeam(name='smack', displayname='<smack />')
+        form = self._makeForm(team)
+        view = create_initialized_view(
+            self.product, name='+securitycontact', form=form)
+        self.assertEqual(1, len(view.errors))
+        expected = (
+            'You cannot set &lt;smack /&gt; as the security contact for '
+            '&lt;boing /&gt; because you are not an administrator of that '
+            'team.<br />If you believe that &lt;smack /&gt; should be the '
+            'security contact for &lt;boing /&gt;, notify one of the '
+            '<a href="http://launchpad.dev/~smack/+members">&lt;smack /&gt; '
+            'administrators</a>.')
+        self.assertEqual(expected, view.errors.pop())
+
+    def test_owner_cannot_appoint_a_nonvalid_user(self):
+        form = self._makeForm(None)
+        form['field.security_contact'] = 'fnord'
+        view = create_initialized_view(
+            self.product, name='+securitycontact', form=form)
+        self.assertEqual(2, len(view.errors))
+        expected = (
+            'You must choose a valid person or team to be the '
+            'security contact for &lt;boing /&gt;.')
+        self.assertEqual(expected, view.errors.pop())
+        self.assertTrue(isinstance(view.errors.pop(), ConversionError))
+
+    def test_owner_cannot_appoint_another_user(self):
+        another_user = self.factory.makePerson()
+        form = self._makeForm(another_user)
+        view = create_initialized_view(
+            self.product, name='+securitycontact', form=form)
+        self.assertEqual(1, len(view.errors))
+        expected = (
+            'You cannot set another person as the security contact for '
+            '&lt;boing /&gt;.')
+        self.assertEqual(expected, view.errors.pop())
+
+    def test_admin_appoint_another_user(self):
+        another_user = self.factory.makePerson()
+        login('[e-mail]')
+        form = self._makeForm(another_user)
+        view = create_initialized_view(
+            self.product, name='+securitycontact', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(another_user, self.product.security_contact)
+
+    def test_admin_appoint_another_team(self):
+        another_team = self.factory.makeTeam()
+        login('[e-mail]')
+        form = self._makeForm(another_team)
+        view = create_initialized_view(
+            self.product, name='+securitycontact', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(another_team, self.product.security_contact)
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/bugs/stories/bugs/xx-malone-security-contacts.txt'
--- lib/lp/bugs/stories/bugs/xx-malone-security-contacts.txt	2009-06-12 16:36:02 +0000
+++ lib/lp/bugs/stories/bugs/xx-malone-security-contacts.txt	2010-05-25 21:21:40 +0000
@@ -6,17 +6,15 @@
     >>> browser.open("http://localhost:9000/ubuntu/+securitycontact")
     >>> browser.getControl("Security Contact").value = "name16"
     >>> browser.getControl("Change").click()
-    >>> print browser.contents
-    <!DOCTYPE...
-    ...Successfully changed the security contact to [e-mail]...
-    ...
+    >>> for message in get_feedback_messages(browser.contents):
+    ...     print extract_text(message)
+    Successfully changed the security contact to Foo Bar.
 
 Or on an upstream.
 
     >>> browser.open("http://localhost:9000/firefox/+securitycontact")
     >>> browser.getControl("Security Contact").value = "name21"
     >>> browser.getControl("Change").click()
-    >>> print browser.contents
-    <!DOCTYPE...
-    ...Successfully changed the security contact to Hoary Gnome Team...
-    ...
+    >>> for message in get_feedback_messages(browser.contents):
+    ...     print extract_text(message)
+    Successfully changed the security contact to Hoary Gnome Team.

=== 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-11-12 15:33:27 +0000
+++ lib/lp/bugs/stories/initial-bug-contacts/05-set-distribution-bugcontact.txt	2010-05-25 21:21:40 +0000
@@ -42,74 +42,11 @@
 
 And then we're redirected to the distribution page.
 
-  >>> colin_browser.url
-  'http://launchpad.dev/ubuntu'
-
-  >>> for tag in find_tags_by_class(colin_browser.contents,
-  ...                               "informational message"):
-  ...     print tag
-  <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's bug page page.
-
-  >>> 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. 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
-certain persons can be successfully selected. All users can set
-themselves self and teams they administer as the bug supervisor, as
-shown above. But ordinary users cannot appoint other persons and teams
-they do not administer.
-
-    >>> colin_browser.open("http://launchpad.dev/ubuntu/+bugsupervisor")
-    >>> colin_browser.getControl("Bug Supervisor").value = 'admins'
-    >>> colin_browser.getControl("Change").click()
-    >>> print extract_text(colin_browser.contents)
-    Edit bug supervisor for Ubuntu : Ubuntu
-    ...
-    You cannot set Launchpad Administrators as the bug supervisor for
-    Ubuntu because you are not an administrator of that team.
-    If you believe that Launchpad Administrators should be the bug
-    supervisor for Ubuntu, please notify one of the Launchpad
-    Administrators administrators.
-    ...
-
-But members of the Launchpad administration team can appoint anybody
-as bug supervisors, teams...
-
-    >>> admin_browser.open("http://launchpad.dev/ubuntu/+bugsupervisor")
-    >>> admin_browser.getControl("Bug Supervisor").value = 'ubuntu-translators'
-    >>> admin_browser.getControl("Change").click()
-    >>> print extract_text(admin_browser.contents)
-    Ubuntu Linux in Launchpad
-    ...
-    Successfully changed the bug supervisor to Ubuntu Translators.
-    ...
-
-...as well as persons.
-
-    >>> admin_browser.open("http://launchpad.dev/ubuntu/+bugsupervisor")
-    >>> admin_browser.getControl("Bug Supervisor").value = 'carlos'
-    >>> admin_browser.getControl("Change").click()
-    >>> print extract_text(admin_browser.contents)
-    Ubuntu Linux in Launchpad
-    ...
-    Successfully changed the bug supervisor to Carlos Perelló Marín.
-    ...
+    >>> colin_browser.url
+    'http://launchpad.dev/ubuntu'
+
+    >>> for message in get_feedback_messages(colin_browser.contents):
+    ...     print extract_text(message)
+    Successfully changed the bug supervisor to Colin Watson.
+    Colin Watson has also been subscribed to bug notifications for Ubuntu.
+    You can change the subscriptions for Ubuntu at any time.

=== modified file 'lib/lp/bugs/stories/initial-bug-contacts/10-set-upstream-bugcontact.txt'
--- lib/lp/bugs/stories/initial-bug-contacts/10-set-upstream-bugcontact.txt	2010-03-31 20:25:33 +0000
+++ lib/lp/bugs/stories/initial-bug-contacts/10-set-upstream-bugcontact.txt	2010-05-25 21:21:40 +0000
@@ -38,39 +38,6 @@
     Successfully changed the bug supervisor to Landscape Developers.
     ...
 
-If Sample Person tries to appoint another person, he'll get an error message.
-
-    >>> sample_browser.open("http://localhost:9000/firefox/+bugsupervisor")
-    >>> sample_browser.getControl(name="field.bug_supervisor").value = (
-    ...                           "[e-mail]")
-    >>> sample_browser.getControl("Change").click()
-    >>> print sample_browser.url
-    http://localhost:9000/firefox/+bugsupervisor
-    >>> print extract_text(sample_browser.contents)
-    Edit bug supervisor for Mozilla Firefox : Mozilla Firefox
-    ...
-    You cannot set another person as the bug supervisor for Mozilla Firefox.
-    See the help wiki for information about setting a bug supervisor.
-    The person or team responsible for bug management.
-    ...
-
-If he tries to appoint a team which he does not administer, he'll get
-too an an error message.
-
-    >>> sample_browser.getControl(name="field.bug_supervisor").value = (
-    ...                           "guadamen")
-    >>> sample_browser.getControl("Change").click()
-    >>> print sample_browser.url
-    http://localhost:9000/firefox/+bugsupervisor
-    >>> print extract_text(sample_browser.contents)
-    Edit bug supervisor for Mozilla Firefox : Mozilla Firefox
-    ...
-    You cannot set GuadaMen as the bug supervisor for Mozilla Firefox
-    because you are not an administrator of that team.
-    If you believe that GuadaMen should be the bug supervisor for Mozilla
-    Firefox, please notify one of the GuadaMen administrators.
-    ...
-
 Launchpad administrators can appoint anybody.
 
     >>> admin_browser = setupBrowser()
@@ -86,101 +53,3 @@
     ...
     Successfully changed the bug supervisor to Robert Collins.
     ...
-
-
-= Teams as Bug Supervisors =
-
-First, some setup. We create a new product owned by "Sample Person", using a
-separate browser instance to avoid breaking the other page tests.
-
-    >>> sample_browser.open('http://launchpad.dev/projects/+new')
-    >>> sample_browser.getControl('URL').value = 'testy'
-    >>> sample_browser.getControl('Name').value = 'Test Product'
-    >>> sample_browser.getControl('Title').value = (
-    ...     'A product for testing things')
-    >>> sample_browser.getControl('Summary').value = (
-    ...     'Testing team bug supervisors')
-    >>> sample_browser.getControl('Continue').click()
-
-    # XXX Add Description: field
-    >>> #sample_browser.getControl('Description').value = 'See bug 109652'
-    >>> sample_browser.getControl(name='field.licenses').value = ['GNU_GPL_V2']
-    >>> sample_browser.getControl(name='field.license_info').value = 'foo'
-    >>> sample_browser.getControl('Complete Registration').click()
-    >>> sample_browser.url
-    'http://launchpad.dev/testy'
-
-By default, a newly created project doesn't use Launchpad for bug
-tracking, and none of the bug lists or edit controls will be
-available.
-
-    >>> sample_browser.getLink('Bugs').click()
-    >>> uses_malone_p = find_tag_by_id(sample_browser.contents, 'no-malone')
-    >>> print extract_text(uses_malone_p)
-    A product for testing things does not use Launchpad for bug tracking.
-
-We can change the settings to use Launchpad for bug tracking.
-
-    >>> sample_browser.getLink(
-    ...     'Configure bug tracker').click()
-    >>> print sample_browser.title
-    Configure bug tracker : Bugs : Test Product
-    >>> sample_browser.getControl('In Launchpad').click()
-    >>> sample_browser.getControl('Change').click()
-
-Now, information about bug tracking (bug supervisor, security contact)
-and their edit controls are available on the page.
-
-    >>> bug_supervisor = find_tag_by_id(
-    ...     sample_browser.contents, 'bug-supervisor')
-    >>> print extract_text(bug_supervisor)
-    Bug supervisor:
-    None set
-
-Sample Person is a member of "Warty Security Team".  However, he should not be
-able to set Warty Security Team as a bug supervisor for Test Project as he's
-not an admin of that team.
-
-    >>> sample_browser.getLink('Change bug supervisor').click()
-    >>> sample_browser.url
-    'http://bugs.launchpad.dev/testy/+bugsupervisor'
-
-    >>> sample_browser.getControl('Bug Supervisor').value = 'name20'
-    >>> sample_browser.getControl('Change').click()
-    >>> 'Successfully changed the bug supervisor' not in sample_browser.contents
-    True
-
-    >>> print sample_browser.contents
-    <!DOCTYPE...
-    ...You cannot set Warty Security Team as the bug supervisor...
-    ...you are not an administrator of that team...
-
-In the warning, the user will be pointed to the Warty Security Team's
-membership page and advised to contact an administrator if she believes that
-the team should be a bug supervisor.
-
-    >>> sample_browser.getLink('Warty Security Team administrators').click()
-    >>> sample_browser.url
-    'http://launchpad.dev/~name20/+members'
-
-Launchpad admins can set a team as bug supervisor without any warnings,
-however, whether they're in the team or not.
-
-    >>> login('[e-mail]')
-
-    >>> from zope.component import getUtility
-    >>> from canonical.launchpad.interfaces import (
-    ...     IPersonSet)
-    >>> security_team = getUtility(IPersonSet).getByName('name20')
-    >>> foo_bar = getUtility(IPersonSet).getByName('name16')
-    >>> foo_bar.inTeam(security_team)
-    False
-
-    >>> logout()
-
-    >>> admin_browser.open('http://bugs.launchpad.dev/testy/+bugsupervisor')
-    >>> admin_browser.getControl('Bug Supervisor').value = 'name20'
-    >>> admin_browser.getControl('Change').click()
-    >>> 'Successfully changed the bug supervisor' in admin_browser.contents
-    True
-

=== modified file 'lib/lp/bugs/stories/initial-bug-contacts/25-file-distribution-bug.txt'
--- lib/lp/bugs/stories/initial-bug-contacts/25-file-distribution-bug.txt	2009-11-12 12:01:22 +0000
+++ lib/lp/bugs/stories/initial-bug-contacts/25-file-distribution-bug.txt	2010-05-25 21:21:40 +0000
@@ -19,9 +19,9 @@
     >>> print browser.url.replace(bug_id, "BUG-ID")
     http://bugs.launchpad.dev/ubuntu/+source/mozilla-firefox/+bug/BUG-ID
 
-We should have six subscribers now. The bug reporter (also a package
-subscriber), mark, the distro bug supervisor, ubuntu-team, and carlos
-and foobar, who are subscribed to the distribution.
+We should have three subscribers now. The bug reporter (also a package
+subscriber), mark, the distro bug supervisor kamion, and foobar, who is
+subscribed to the distribution.
 
     >>> from zope.component import getUtility
     >>> from canonical.launchpad.interfaces import IBugSet
@@ -36,8 +36,7 @@
 
     >>> bugset = getUtility(IBugSet)
     >>> subscriber_names(bugset.get(bug_id))
-    [u'carlos', u'kamion', u'mark', u'name16', u'ubuntu-team',
-    u'ubuntu-translators']
+    [u'kamion', u'mark', u'name16']
 
     >>> logout()