Hi Ian.

I have a few concerns about the safety and maintainability of of two changes. I have some suggestions and questions too.

> === modified file 'lib/lp/bugs/browser/bugtask.py'
> --- lib/lp/bugs/browser/bugtask.py	2011-10-19 14:54:07 +0000
> +++ lib/lp/bugs/browser/bugtask.py	2011-10-25 08:17:26 +0000
> @@ -1757,6 +1758,38 @@
>          self.updateContextFromData(data)
>  
>  
> +class BugTaskDeletionView(LaunchpadFormView):
> +    """Used to delete a bugtask."""
> +
> +    schema = IBugTask
> +    field_names = []
> +
> +    label = 'Remove bug task'
> +    page_title = label
> +
> +    @property
> +    def confirmation_message(self):
> +        return ('<p>You are about to mark bug %s<br>'
> +                'as no longer affecting %s.</p>'
> +                '<p>This operation will be permanent and cannot be '
> +                'undone.</p>'
> +                % (self.context.bug.title,
> +                   self.context.target.bugtargetdisplayname))

This is dangerous. Both title and displayname may *always* contain
markup. We use structured() to build safe interpolated messages.

Would direct language be more effective at conveying the consequences of the
action:
    Deletion is permanent, it cannot be undone.

...

> @@ -3552,6 +3585,8 @@
>              row_css_class='highlight' if is_primary else None,
>              target_link=canonical_url(self.context.target),
>              target_link_title=self.target_link_title,
> +            user_can_delete=self.user_can_delete_bugtask,
> +            delete_link=link + '/+delete',

While I am certain this link will work, but it is a crafted link we need to
maintain with extra testing.
    canonical_url(self.context.target, view_name='+delete')
will raise an exception if the named view is unregistered.

> === modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
> --- lib/lp/bugs/browser/tests/test_bugtask.py	2011-10-05 18:02:45 +0000
> +++ lib/lp/bugs/browser/tests/test_bugtask.py	2011-10-25 08:17:26 +0000
> @@ -606,6 +614,99 @@
>          self.assertIn(series.product.displayname, content)
>  
>  
> +class TestBugTaskDeleteLinks(TestCaseWithFactory):
> +    """ Test that the delete icons/links for each relevant bug task are
> +        correctly rendered.
> +
> +        Bug task deletion is protected by a feature flag.
> +        """

The first line may not wrap according to PEP 256.

+    layer = DatabaseFunctionalLayer
+
+    def test_cannot_delete_only_bugtask(self):
+        # The last bugtask cannot be deleted.
+        bug = self.factory.makeBug()
+        login_person(bug.owner)
+        view = create_initialized_view(
+            bug, name='+bugtasks-and-nominations-table')
+        row_view = view._getTableRowView(bug.default_bugtask, False, False)
+        self.assertFalse(row_view.user_can_delete_bugtask)
+        del get_property_cache(row_view).user_can_delete_bugtask
+        with FeatureFixture(DELETE_BUGTASK_ENABLED):
+            self.assertFalse(row_view.user_can_delete_bugtask)
+
+    def test_can_delete_bugtask_if_authorised(self):
+        # The bugtask can be deleted if the user if authorised.
+        bug = self.factory.makeBug()
+        bugtask = self.factory.makeBugTask(bug=bug)
+        login_person(bugtask.owner)
+        view = create_initialized_view(
+            bug, name='+bugtasks-and-nominations-table')
+        row_view = view._getTableRowView(bugtask, False, False)
+        self.assertFalse(row_view.user_can_delete_bugtask)
+        del get_property_cache(row_view).user_can_delete_bugtask
+        with FeatureFixture(DELETE_BUGTASK_ENABLED):
+            self.assertTrue(row_view.user_can_delete_bugtask)
+
+    def test_bugtask_delete_icon(self):
+        # The bugtask delete icon is rendered correctly for those tasks the
+        # user is allowed to delete.
+        bug = self.factory.makeBug()
+        bugtask = self.factory.makeBugTask(bug=bug)
+        with FeatureFixture(DELETE_BUGTASK_ENABLED):
+            login_person(bugtask.owner)
+            url = canonical_url(bugtask, rootsite='bugs')
+            browser = self.getUserBrowser(url, user=bugtask.owner)
+            # bugtask can be deleted because the user owns it.
+            delete_icon = find_tag_by_id(
+                browser.contents, 'bugtask-delete-task%d' % bugtask.id)
+            self.assertEqual(url + '/+delete', delete_icon['href'])
+            # default_bugtask cannot be deleted.
+            delete_icon = find_tag_by_id(
+                browser.contents,
+                'bugtask-delete-task%d' % bug.default_bugtask.id)
+            self.assertIsNone(delete_icon)

The TestBrowser is an indirect means to test rendering. The view does that.
you can render any view (that had the principal argument passed) by calling
it; view(). Launchpad views add striping rules so __call__ calls render().
You can render a view's templates using view() or view.render(). It is much
faster than TestBrowser.

    def test_bugtask_delete_icon(self):
        # The bugtask delete icon is rendered correctly for those tasks the
        # user is allowed to delete.
        bug = self.factory.makeBug()
        bugtask = self.factory.makeBugTask(bug=bug)
        with FeatureFixture(DELETE_BUGTASK_ENABLED):
            login_person(bugtask.owner)
             view = create_initialized_view(
                bug, name='+bugtasks-and-nominations-table'
                principal=bugtask.owner)
            content = view.render()
            # bugtask can be deleted because the user owns it.
            delete_icon = find_tag_by_id(
                content, 'bugtask-delete-task%d' % bugtask.id)
            self.assertEqual(url + '/+delete', delete_icon['href'])
            # default_bugtask cannot be deleted.
            delete_icon = find_tag_by_id(
                content, 'bugtask-delete-task%d' % bug.default_bugtask.id)
            self.assertIsNone(delete_icon)

> +class TestBugTaskDeleteView(TestCaseWithFactory):
> +    """Test the bug task delete form."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def test_delete_view_rendering(self):
> +        # Test the view rendering, including confirmation message, cancel url.
> +        bug = self.factory.makeBug()
> +        bugtask = self.factory.makeBugTask(bug=bug)
> +        with FeatureFixture(DELETE_BUGTASK_ENABLED):
> +            login_person(bugtask.owner)
> +            view = create_initialized_view(
> +                bugtask, name='+delete', principal=bugtask.owner)
> +            matches = MatchesRegex(
> +                '.*You are about to mark bug.*'
> +                'This operation will be permanent and cannot be undone.*')
> +            self.assertThat(normalize_whitespace(view.render()), matches)
> +            url = canonical_url(bugtask, rootsite='bugs')
> +            self.assertEqual(view.cancel_url, url)

And I see you did you view.render() here. I do not care for exact testing
of user text because it tends to change often and get tested more than once.
I put ids on the text fragment, id="deletion-waring-message", and just test
for the presence. This permits non technical users to make trivial changes
to the text while the code ensures that the intent of the message is still
in the content.

> === modified file 'lib/lp/bugs/model/bugtask.py'
> --- lib/lp/bugs/model/bugtask.py	2011-10-19 23:46:54 +0000
> +++ lib/lp/bugs/model/bugtask.py	2011-10-25 08:17:26 +0000
> @@ -630,6 +632,31 @@
>  
>          return num_bugtasks > 1
>  
> +    def userCanDelete(self, user):
> +        """See `IBugTask`."""
> +        if user is None:
> +            return False
> +
> +        # Admins can always delete bugtasks.
> +        if user.inTeam(getUtility(ILaunchpadCelebrities).admin):
> +            return True
> +
> +        delete_allowed = bool(features.getFeatureFlag(
> +            'disclosure.delete_bugtask.enabled'))
> +        if not delete_allowed:
> +            return False
> +
> +        owner = None
> +        if IHasOwner.providedBy(self.pillar):
> +            owner = self.pillar.owner
> +        bugsupervisor = None
> +        if IHasBugSupervisor.providedBy(self.pillar):
> +            bugsupervisor = self.pillar.bug_supervisor
> +        return (
> +            user.inTeam(owner) or
> +            user.inTeam(bugsupervisor) or
> +            user.inTeam(self.owner))
> +

This code is repeating the mistakes of Bugs past. The code embeds roles
and celebrities into model code. This is the primary reason bug permissions
are fucked up. I believe this logic belongs in lp.bugs.security, where I
think this came from. userCanDelete() could do this:
    def userCanDelete(self):
        return check_permission('launchpad.Delete', self)

The user is gotten from the interaction.

> === added file 'lib/lp/bugs/templates/bugtask-delete.pt'
> --- lib/lp/bugs/templates/bugtask-delete.pt	1970-01-01 00:00:00 +0000
> +++ lib/lp/bugs/templates/bugtask-delete.pt	2011-10-25 08:17:26 +0000
> @@ -0,0 +1,20 @@
> +<html
> +  xmlns="http://www.w3.org/1999/xhtml"
> +  xmlns:tal="http://xml.zope.org/namespaces/tal"
> +  xmlns:metal="http://xml.zope.org/namespaces/metal"
> +  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> +  metal:use-macro="view/macro:page/main_only"
> +  i18n:domain="launchpad">
> +<body>
> +
> +<div metal:fill-slot="main">
> +  <div metal:use-macro="context/@@launchpad_form/form">
> +    <div metal:fill-slot="extra_info">
> +      <p tal:content="structure view/confirmation_message">
> +          Are you sure?
> +      </p>
> +    </div>
> +  </div>
> +</div>
> +</body>
> +</html>

Why did you choose to create HTML in BugTaskDeletionView.confirmation_message
rather than using it in the template where escaping is easier to manage?

> === modified file 'lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt'
> --- lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt	2011-08-19 08:55:04 +0000
> +++ lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt	2011-10-25 08:17:26 +0000
> @@ -33,6 +33,12 @@
>            Edit
>          </button>
>          <div class="yui3-activator-message-box yui3-activator-hidden"></div>
> +        <tal:delete-icon condition="data/user_can_delete">
> +            <a tal:attributes="
> +                id string:bugtask-delete-${data/form_row_id};
> +                href data/delete_link"
> +               class="sprite remove bugtask-delete" style="margin-left: 4px"></a>
> +        </tal:delete-icon>
The condition is always process first in TALES. The outer markup is not
needed.

        <a tal:condition="data/user_can_delete"
            tal:attributes="
            id string:bugtask-delete-${data/form_row_id};
            href data/delete_link"
           class="sprite remove bugtask-delete" style="margin-left: 4px"></a>

I believe the style attr is unneeded. We can update style-3.0.css with a rule
like:
    .bugtask-delete {
        margin-left: 4px;
        }
.