Merge lp:~gmb/launchpad/fix-bugwatch-message-shenanigans into lp:launchpad

[Graham Binns]

Users should no longer be able to attempt to delete a BugWatch that is linked to BugComments.

This branch began as an attempt to fix bug 575911 (Trying to delete a
bug watch results in a non-OOPSing IntegrityError). The error was being
raised because we were allowing people to attempt to delete bug watches
which had comments linked to them. Since we don't do a cascading delete
on comments linked to watches, Postgres threw its toys out of the pram.

It turned out that the simplest way to stop the error from being raise
was to stop people from ever getting to the point where they could try
to delete a linked watch in the first place. The fact that the error
didn't OOPS was due to it occurring when the transaction was being
committed, which is (apparently) outside application code and so not
handled by the OOPS machinery.

Nevertheless I've followed Gavin's suggestion of adding a store.flush()
call to the end of BugWatch.destroySelf(). Whilst it should no longer be
possible for someone to try to delete a linked watch without getting a
sane error message the flush() should force any DB errors to be handled
by the OOPS machinery, thus making life easier should anything else go
wrong.

Here's a list of my changes:

== lib/lp/bugs/browser/bugwatch.py ==

 - I've updated the bugWatchIsUnlinked() method, which is used to
   determine whether ot display the delete action on the page. It now
   checks that the watch has no comments linked to it.

== lib/lp/bugs/browser/tests/test_bugwatch_views.py ==

 - I've added a couple of tests to cover the changes to
   bugWatchIsUnlinked() above.

== lib/lp/bugs/doc/bugmessage.txt ==

 - I've added tests to ensure that comments aren't linked to a watch
   when a watch is created from a URL in a comment. This was originally
   thought to be the source of (part of) the bug I was attempting to
   fix. That turned out not to be the case but it seemed sensible to
   leave the test in place.

== lib/lp/bugs/model/bugwatch.py ==

 - BugWatch.destroySelf() now raises a BugWatchDeletionError if the
   watch is linked to a task or comments. I've added a call to
   store.flush() as described above.

== lib/lp/bugs/stories/bugwatches/xx-delete-bugwatch.txt ==

 - I've updated the narrative for the deletion story.

== lib/lp/bugs/tests/test_bugwatch.py ==

 - I've added a test to check that BugWatch.destroySelf() raises a
   BugWatchDeletionError as described above.

[Edwin Grubbs (edwin-grubbs)]

Hi Graham,

This branch looks good. Can you make sure there is a bug open about the oops machinery not reporting late integrity errors?

-Edwin

review: Approve (code)

[Graham Binns (gmb)]

On 29 September 2010 18:41, Edwin Grubbs <email address hidden> wrote:
> Review: Approve code
> Hi Graham,
>
> This branch looks good. Can you make sure there is a bug open about the oops machinery not reporting late integrity errors?

Gary filed bug 647103 about this problem. I've added an XXX referring
to it above the flush(), since we shouldn't have to do it.

[Graham Binns (gmb)]

On 29 September 2010 18:41, Edwin Grubbs <email address hidden> wrote:
> Review: Approve code
> Hi Graham,
>
> This branch looks good. Can you make sure there is a bug open about the oops machinery not reporting late integrity errors?

Gary filed bug 647103 about this problem. I've added an XXX referring
to it above the flush(), since we shouldn't have to do it.

=== modified file 'lib/lp/bugs/browser/bugwatch.py'
--- lib/lp/bugs/browser/bugwatch.py	2010-09-24 14:24:06 +0000
+++ lib/lp/bugs/browser/bugwatch.py	2010-09-29 19:17:49 +0000
@@ -29,6 +29,7 @@
 from canonical.launchpad.webapp.menu import structured
 from canonical.widgets.textwidgets import URIWidget
 from lp.bugs.browser.bugtask import get_comments_for_bugtask
+from lp.bugs.interfaces.bugmessage import IBugMessageSet
 from lp.bugs.interfaces.bugwatch import (
     BUG_WATCH_ACTIVITY_SUCCESS_STATUSES,
     IBugWatch,
@@ -131,7 +132,9 @@
 
     def bugWatchIsUnlinked(self, action):
         """Return whether the bug watch is unlinked."""
-        return len(self.context.bugtasks) == 0
+        return (
+            len(self.context.bugtasks) == 0 and
+            self.context.getImportedBugMessages().is_empty())
 
     @action('Delete Bug Watch', name='delete', condition=bugWatchIsUnlinked)
     def delete_action(self, action, data):

=== added file 'lib/lp/bugs/browser/tests/test_bugwatch_views.py'
--- lib/lp/bugs/browser/tests/test_bugwatch_views.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/browser/tests/test_bugwatch_views.py	2010-09-29 19:17:49 +0000
@@ -0,0 +1,56 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for BugWatch views."""
+
+__metaclass__ = type
+
+from zope.component import getUtility
+
+from canonical.launchpad.interfaces.message import IMessageSet
+from canonical.testing import LaunchpadFunctionalLayer
+
+from lp.testing import login, login_person, TestCaseWithFactory
+from lp.testing.sampledata import ADMIN_EMAIL
+from lp.testing.views import create_initialized_view
+
+
+class TestBugWatchEditView(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestBugWatchEditView, self).setUp()
+        self.person = self.factory.makePerson()
+
+        login_person(self.person)
+        self.bug_task = self.factory.makeBug(
+            owner=self.person).default_bugtask
+        self.bug_watch = self.factory.makeBugWatch(
+            bug=self.bug_task.bug)
+
+    def test_cannot_delete_watch_if_linked_to_task(self):
+        # It isn't possible to delete a bug watch that's linked to a bug
+        # task.
+        self.bug_task.bugwatch = self.bug_watch
+        view = create_initialized_view(self.bug_watch, '+edit')
+        self.assertFalse(
+            view.bugWatchIsUnlinked(None),
+            "bugWatchIsUnlinked() returned True though there is a task "
+            "linked to the watch.")
+
+    def test_cannot_delete_watch_if_linked_to_coment(self):
+        # It isn't possible to delete a bug watch that's linked to a bug
+        # comment.
+        message = getUtility(IMessageSet).fromText(
+            "Example message", "With some example content to read.")
+        # We need to log in as an admin here as only admins can link a
+        # watch to a comment.
+        login(ADMIN_EMAIL)
+        bug_message = self.bug_watch.addComment('comment-id', message)
+        login_person(self.person)
+        view = create_initialized_view(self.bug_watch, '+edit')
+        self.assertFalse(
+            view.bugWatchIsUnlinked(None),
+            "bugWatchIsUnlinked() returned True though there is a comment "
+            "linked to the watch.")

=== modified file 'lib/lp/bugs/doc/bugmessage.txt'
--- lib/lp/bugs/doc/bugmessage.txt	2010-05-21 17:01:20 +0000
+++ lib/lp/bugs/doc/bugmessage.txt	2010-09-29 19:17:49 +0000
@@ -109,6 +109,16 @@
     http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=304014
     http://some.bugzilla/show_bug.cgi?id=9876
 
+Note that although the watch was created when the Message was added to
+the bug, the message and the watch are not linked because the message
+was not imported by the bug watch.
+
+    >>> bug_message = bug_one.bug_messages[-1]
+    >>> print bug_message.message == test_message
+    True
+    >>> print bug_message.bugwatch
+    None
+
 CVE watches and bug watches are also created, when a message is imported from
 an external bug tracker.
 

=== modified file 'lib/lp/bugs/model/bugwatch.py'
--- lib/lp/bugs/model/bugwatch.py	2010-08-26 12:12:00 +0000
+++ lib/lp/bugs/model/bugwatch.py	2010-09-29 19:17:49 +0000
@@ -7,6 +7,7 @@
 __all__ = [
     'BugWatch',
     'BugWatchActivity',
+    'BugWatchDeletionError',
     'BugWatchSet',
     ]
 
@@ -114,6 +115,10 @@
                 '%r is not a bug watch or an ID.' % (reference,))
 
 
+class BugWatchDeletionError(Exception):
+    """Raised when someone attempts to delete a linked watch."""
+
+
 class BugWatch(SQLBase):
     """See `IBugWatch`."""
     implements(IBugWatch)
@@ -223,8 +228,16 @@
 
     def destroySelf(self):
         """See `IBugWatch`."""
-        assert len(self.bugtasks) == 0, "Can't delete linked bug watches"
+        if (len(self.bugtasks) > 0 or
+            not self.getImportedBugMessages().is_empty()):
+            raise BugWatchDeletionError(
+                "Can't delete bug watches linked to tasks or comments.")
+        # XXX 2010-09-29 gmb bug=647103
+        #     We flush the store to make sure that errors bubble up and
+        #     are caught by the OOPS machinery.
         SQLBase.destroySelf(self)
+        store = Store.of(self)
+        store.flush()
 
     @property
     def unpushed_comments(self):

=== modified file 'lib/lp/bugs/stories/bugwatches/xx-delete-bugwatch.txt'
--- lib/lp/bugs/stories/bugwatches/xx-delete-bugwatch.txt	2009-09-04 00:37:37 +0000
+++ lib/lp/bugs/stories/bugwatches/xx-delete-bugwatch.txt	2010-09-29 19:17:49 +0000
@@ -1,7 +1,7 @@
 = Delete a Bug Watch =
 
-If a bug watch isn't linked to a bug task, it's possible to delete from
-the bug watch edit page.
+If a bug watch isn't linked to a bug task and no comments have been
+imported for it, it's possible to delete from the bug watch edit page.
 
     >>> user_browser.open('http://launchpad.dev/bugs/1')
     >>> bug_watches = find_portlet(user_browser.contents, 'Remote bug watches')
@@ -32,4 +32,3 @@
     mozilla.org #2000
     mozilla.org #42
     debbugs #304014
-

=== modified file 'lib/lp/bugs/tests/test_bugwatch.py'
--- lib/lp/bugs/tests/test_bugwatch.py	2010-08-26 13:10:24 +0000
+++ lib/lp/bugs/tests/test_bugwatch.py	2010-09-29 19:17:49 +0000
@@ -48,7 +48,10 @@
     NoBugTrackerFound,
     UnrecognizedBugTrackerURL,
     )
-from lp.bugs.model.bugwatch import get_bug_watch_ids
+from lp.bugs.model.bugwatch import (
+    BugWatchDeletionError,
+    get_bug_watch_ids,
+    )
 from lp.bugs.scripts.checkwatches.scheduler import MAX_SAMPLE_SIZE
 from lp.registry.interfaces.person import IPersonSet
 from lp.testing import (
@@ -465,6 +468,14 @@
         self.assertRaises(
             AssertionError, list, get_bug_watch_ids(['fred']))
 
+    def test_destroySelf_raise_error_when_linked_to_a_task(self):
+        # It's not possible to delete a bug watch that's linked to a
+        # task. Trying will result in a BugWatchDeletionError.
+        bug_watch = self.factory.makeBugWatch()
+        bug = bug_watch.bug
+        bug.default_bugtask.bugwatch = bug_watch
+        self.assertRaises(BugWatchDeletionError, bug_watch.destroySelf)
+
 
 class TestBugWatchSet(TestCaseWithFactory):
     """Tests for the bugwatch updating system."""