launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/deal-with-unsynched-bugwatch-comments into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #301740 in Launchpad itself: "It should be possible to delete an auto-generated bugwatch that has comments attached"
  https://bugs.launchpad.net/launchpad/+bug/301740

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/deal-with-unsynched-bugwatch-comments/+merge/131293

When deleting a bugwatch that has a message which does not contain a remote ID, it is shown in the UI as unsynchronized, and if it is the only bugmessage that references that bugwatch when that bugwatch is deleted, an OOPS will result due to a foreign key violation. I have dealt with that by adding a new method onto IBugWatch, and making use of it in the view.

This branch does not close the linked bug entirely, but it destroys the cause of the OOPS, so it can drop from Critical to Low.

I have destroyed an evil doctest for LoC credit, as well as refactoring the current method on IBugWatch.
-- 
https://code.launchpad.net/~stevenk/launchpad/deal-with-unsynched-bugwatch-comments/+merge/131293
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/deal-with-unsynched-bugwatch-comments into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugwatch.py'
--- lib/lp/bugs/browser/bugwatch.py	2011-12-30 06:14:56 +0000
+++ lib/lp/bugs/browser/bugwatch.py	2012-10-25 00:02:22 +0000
@@ -133,7 +133,7 @@
         """Return whether the bug watch is unlinked."""
         return (
             len(self.context.bugtasks) == 0 and
-            self.context.getImportedBugMessages().is_empty())
+            self.context.getBugMessages().is_empty())
 
     @action('Delete Bug Watch', name='delete', condition=bugWatchIsUnlinked)
     def delete_action(self, action, data):

=== modified file 'lib/lp/bugs/browser/tests/test_bugwatch_views.py'
--- lib/lp/bugs/browser/tests/test_bugwatch_views.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/browser/tests/test_bugwatch_views.py	2012-10-25 00:02:22 +0000
@@ -8,6 +8,7 @@
 from zope.component import getUtility
 
 from lp.services.messages.interfaces.message import IMessageSet
+from lp.services.webapp.interfaces import ILaunchBag
 from lp.testing import (
     login,
     login_person,
@@ -32,15 +33,27 @@
         self.bug_watch = self.factory.makeBugWatch(
             bug=self.bug_task.bug)
 
+    def test_can_delete_watch(self):
+        # An unlinked bugwatch can be deleted.
+        form = {'field.actions.delete': 'Delete Bug Watch'}
+        getUtility(ILaunchBag).add(self.bug_task.bug)
+        view = create_initialized_view(self.bug_watch, '+edit', form=form)
+        self.assertContentEqual([], view.errors)
+
+    def test_can_not_delete_unlinked_watch_with_unsynched_comments(self):
+        # If a bugwatch is unlinked, but has imported comments that are
+        # awaiting synch, it can not be deleted.
+        self.factory.makeBugComment(
+            bug=self.bug_task.bug.id, bug_watch=self.bug_watch)
+        view = create_initialized_view(self.bug_watch, '+edit')
+        self.assertFalse(view.bugWatchIsUnlinked(None))
+
     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.")
+        self.assertFalse(view.bugWatchIsUnlinked(None))
 
     def test_cannot_delete_watch_if_linked_to_comment(self):
         # It isn't possible to delete a bug watch that's linked to a bug
@@ -54,7 +67,4 @@
         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.")
+        self.assertFalse(view.bugWatchIsUnlinked(None))

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2012-10-15 02:32:30 +0000
+++ lib/lp/bugs/configure.zcml	2012-10-25 00:02:22 +0000
@@ -879,6 +879,7 @@
                     remotestatus
                     title
                     url
+                    getBugMessages
                     getImportedBugMessages"/>
             <require
                 permission="launchpad.AnyPerson"

=== modified file 'lib/lp/bugs/interfaces/bugwatch.py'
--- lib/lp/bugs/interfaces/bugwatch.py	2012-10-09 05:48:57 +0000
+++ lib/lp/bugs/interfaces/bugwatch.py	2012-10-25 00:02:22 +0000
@@ -263,6 +263,12 @@
         :param message: The imported comment as a Launchpad Message object.
         """
 
+    def getBugMessages(clauses):
+        """Return all the `IBugMessage`s that reference this BugWatch.
+        
+        :param clauses: A iterable of Storm clauses to limit the messages.
+        """
+
     def getImportedBugMessages():
         """Return all the `IBugMessage`s that have been imported."""
 

=== modified file 'lib/lp/bugs/model/bugwatch.py'
--- lib/lp/bugs/model/bugwatch.py	2012-10-09 04:47:36 +0000
+++ lib/lp/bugs/model/bugwatch.py	2012-10-25 00:02:22 +0000
@@ -283,19 +283,14 @@
             remote_comment_id=comment_id)
         return bug_message
 
+    def getBugMessages(self, clauses=[]):
+        return Store.of(self).find(
+            BugMessage, BugMessage.bug == self.bug.id,
+            BugMessage.bugwatch == self.id, *clauses)
+
     def getImportedBugMessages(self):
         """See `IBugWatch`."""
-        store = Store.of(self)
-        # If a comment is linked to a bug watch and has a
-        # remote_comment_id, it means it's imported.
-        # XXX gmb 2008-12-09 bug 244768:
-        #     The Not() needs to be in this find() call due to bug
-        #     244768; we should remove it once that is solved.
-        return store.find(
-            BugMessage,
-            BugMessage.bug == self.bug.id,
-            BugMessage.bugwatch == self.id,
-            Not(BugMessage.remote_comment_id == None))
+        return self.getBugMessages([BugMessage.remote_comment_id != None])
 
     def addActivity(self, result=None, message=None, oops_id=None):
         """See `IBugWatch`."""

=== removed file 'lib/lp/bugs/stories/bugwatches/xx-delete-bugwatch.txt'
--- lib/lp/bugs/stories/bugwatches/xx-delete-bugwatch.txt	2010-09-29 11:07:13 +0000
+++ lib/lp/bugs/stories/bugwatches/xx-delete-bugwatch.txt	1970-01-01 00:00:00 +0000
@@ -1,34 +0,0 @@
-= Delete a Bug Watch =
-
-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')
-    >>> for li in bug_watches('li'):
-    ...     print li.findAll('a')[0].renderContents()
-    mozilla.org #123543
-    mozilla.org #2000
-    mozilla.org #42
-    debbugs #304014
-
-    >>> user_browser.getLink(url='/+watch/3').click()
-    >>> user_browser.title
-    'Edit bug watch for bug 123543 in The Mozilla.org Bug Tracker on bug #1'
-    >>> print find_portlet(
-    ...     user_browser.contents, 'Bug watch links').span.renderContents()
-    There are currently no links to this bug watch.
-
-    >>> user_browser.getControl('Delete Bug Watch').click()
-
-    >>> for message in find_tags_by_class(user_browser.contents, 'message'):
-    ...     print message.renderContents()
-    The <a href="https://bugzilla.mozilla.org/...123543";>mozilla.org #123543</a>
-    bug watch has been deleted.
-
-    >>> bug_watches = find_portlet(user_browser.contents, 'Remote bug watches')
-    >>> for li in bug_watches('li'):
-    ...     print li.findAll('a')[0].renderContents()
-    mozilla.org #2000
-    mozilla.org #42
-    debbugs #304014