launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01300
[Merge] lp:~gmb/launchpad/fix-bugwatch-message-shenanigans into lp:launchpad/devel
Graham Binns has proposed merging lp:~gmb/launchpad/fix-bugwatch-message-shenanigans into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers): code
Related bugs:
#575911 Trying to delete a bug watch results in a non-OOPSing IntegrityError
https://bugs.launchpad.net/bugs/575911
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.
--
https://code.launchpad.net/~gmb/launchpad/fix-bugwatch-message-shenanigans/+merge/36972
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/fix-bugwatch-message-shenanigans into lp:launchpad/devel.
=== 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 11:21:07 +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 11:21:07 +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 11:21:07 +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 11:21:07 +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,13 @@
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.")
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 11:21:07 +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 11:21:07 +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."""
