Merge lp:~allenap/launchpad/checkwatches-stuff into lp:launchpad

[Gavin Panella]

Commit after every successful bug watch update.

[Gavin Panella (allenap)]

The main change here is that BugWatchUpdater.updateBugWatches()
commits transactions regularly, instead of trying to keep them
open. It tried to do this to avoid calling an expensive function. I've
changed the code to use a different, new, function which means that
it's not necessary to hold the transaction open.

Looking at it again, I don't know that this was necessary; I could
have just added a commit in there and it would have worked. However,
the expensive function was really doing ugly things, so I feel that
the change was worth it anyway.

Lint free.

Test: bin/test -vvt 'checkwatch|bug-?watch'

There are two other small changes, detailed below.

lib/lp/bugs/doc/externalbugtracker.txt

  Amended this doctest to show the ways transactions are now handled
  in BugWatchUpdater.updateBugWatches().

lib/lp/bugs/interfaces/bugwatch.py

  New method getBugWatchesForRemoteBug().

lib/lp/bugs/interfaces/externalbugtracker.py

  Improved interface docstrings.

lib/lp/bugs/model/bugwatch.py

  Implementation of getBugWatchesForRemoteBug().

lib/lp/bugs/scripts/checkwatches.py

  Removed BugWatchUpdater._getBugWatchesByRemoteBug(). This method is
  reasonably expensive because it can end up iterating through 1000s
  of bug watches, each of which it queries individually from the
  database.

  It needed to be called after each aborted transaction (i.e. after an
  error updating another bug watch), and this has resulted in trying
  to hold transactions open for a long time instead of committing
  after each update.

  From reading the code, it's apparent that this can mean that a
  failure in one bug watch can cause nothing to be recorded for many
  other bug watches that are otherwise fine.

  I've changed updateBugWatches() to commit on every iteration of its
  main loop, and now use IBugWatchSet.getBugWatchesForRemoteBug()
  instead of self._getBugWatchesByRemoteBug().

  In updateBugWatches() I also renamed bug_id to remote_bug_id because
  it was confusing.

lib/canonical/launchpad/scripts/ftests/test_checkwatches.py =>
lib/lp/bugs/scripts/tests/test_checkwatches.py

  Moved this module into the lp tree, and renamed the only TestCase in
  it. I originally added some test code to this file, but later moved
  it elsewhere, but I want to land the move and rename anyway.

lib/lp/bugs/tests/test_bugwatch.py

  Refactored this because the test_suite() function was ugly and
  fragile. It would be easy to add a TestCase, forget to add it to the
  test_suite() method, and no one notice.

  I also added TestBugWatchSet, which currently only tests the new
  method, getBugWatchesForRemoteBug().

[Graham Binns (gmb)]

Hi Gavin,

I'm marking this as Needs Fixing for now because there's some
conflicty-oddness going on. Apart from that, though, and a naming issue,
I'm happy with this. Just let me know when the problems I've mentioned
are fixed.

> === modified file 'lib/lp/bugs/scripts/checkwatches.py'
> --- lib/lp/bugs/scripts/checkwatches.py 2009-12-16 16:50:24 +0000
> +++ lib/lp/bugs/scripts/checkwatches.py 2010-01-04 15:37:12 +0000
> @@ -414,26 +414,6 @@
> """Return the bug watch with id `bug_watch_id`."""
> return getUtility(IBugWatchSet).get(bug_watch_id)
>
> - def _getBugWatchesByRemoteBug(self, bug_watch_ids):
> - """Returns a dictionary of bug watches mapped to remote bugs.
> -
> - For each bug watch id fetches the corresponding bug watch and
> - appends it to a list of bug watches pointing to one remote
> - bug - the key of the returned mapping.
> - """
> - bug_watches_by_remote_bug = {}
> - for bug_watch_id in bug_watch_ids:
> - bug_watch = self._getBugWatch(bug_watch_id)
> - remote_bug = bug_watch.remotebug
> - # There can be multiple bug watches pointing to the same
> - # remote bug; because of that, we need to store lists of bug
> - # watches related to the remote bug, and later update the
> - # status of each one of them.
> - if remote_bug not in bug_watches_by_remote_bug:
> - bug_watches_by_remote_bug[remote_bug] = []
> - bug_watches_by_remote_bug[remote_bug].append(bug_watch)
> - return bug_watches_by_remote_bug
> -
> def _getExternalBugTrackersAndWatches(self, bug_tracker, bug_watches):
> """Return an `ExternalBugTracker` instance for `bug_tracker`."""
> remotesystem = externalbugtracker.get_external_bugtracker(
> @@ -686,6 +666,17 @@
> 'unmodified_remote_ids': unmodified_remote_ids,
> }
>
> + def getBugWatchesForRemoteBug(self, remote_bug_id, bug_watch_ids):

This should either start with an underscore or (if you have the time and
the pain threshold) you should remove all underscores from method names
in BugWatchUpdater, because they aren't relevant. However, that's a big
change and whilst I'd like it I wouldn't advise it.

> + """Return a list of bug watches for the given remote bug.
> +
> + The returned watches will all be members of `bug_watch_ids`.
> +
> + This method exists primarily to be overridden during testing.
> + """
> + return list(
> + getUtility(IBugWatchSet).getBugWatchesForRemoteBug(
> + remote_bug_id, bug_watch_ids))
> +
> # XXX gmb 2008-11-07 [bug=295319]
> # This method is 186 lines long. It needs to be shorter.
> def updateBugWatches(self, remotesystem, bug_watches_to_update, now=None,
> @@ -752,10 +743,6 @@
> self.txn.commit()
> raise
>
> - self.txn.begin()
> - bug_watches_by_remote_bug = self._getBugWatchesByRemoteBug(
> - bug_watch_ids)
> -
> # Whether we can import and / or push comments is determined on
> # a per-bugtracker-type level....

review: Needs Fixing (code)

[Gavin Panella (allenap)]

Hi Graham, thanks for the review. I've merged devel again, changed the method name (not doing all of them; too much pain for now), and have fixed another test which I broke.

[Graham Binns (gmb)]

Approved pending the removal of the erroneous .moved file.

review: Approve (code)

=== removed file 'lib/canonical/launchpad/scripts/ftests/test_checkwatches.py'
--- lib/canonical/launchpad/scripts/ftests/test_checkwatches.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/scripts/ftests/test_checkwatches.py	1970-01-01 00:00:00 +0000
@@ -1,93 +0,0 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Tests for the checkwatches remote bug synchronisation code."""
-
-__metaclass__ = type
-__all__ = []
-
-import unittest
-
-from zope.component import getUtility
-
-from canonical.config import config
-from canonical.database.sqlbase import commit
-from canonical.launchpad.ftests import login
-from lp.bugs.tests.externalbugtracker import new_bugtracker
-from canonical.launchpad.interfaces import (BugTaskStatus, BugTrackerType,
-    IBugSet, IBugTaskSet, ILaunchpadCelebrities, IPersonSet,
-    IProductSet, IQuestionSet)
-from canonical.testing import LaunchpadZopelessLayer
-
-
-class TestCheckwatches(unittest.TestCase):
-    """Tests for the bugwatch updating system."""
-    layer = LaunchpadZopelessLayer
-
-    def setUp(self):
-        """Set up bugs, watches and questions to test with."""
-        super(TestCheckwatches, self).setUp()
-
-        # For test_can_update_bug_with_questions we need a bug that has
-        # a question linked to it.
-        bug_with_question = getUtility(IBugSet).get(10)
-        question = getUtility(IQuestionSet).get(1)
-
-        # XXX gmb 2007-12-11 bug 175545:
-        #     We shouldn't have to login() here, but since
-        #     database.buglinktarget.BugLinkTargetMixin.linkBug()
-        #     doesn't accept a user parameter, instead depending on the
-        #     currently logged in user, we get an exception if we don't.
-        login('[e-mail]')
-        question.linkBug(bug_with_question)
-
-        # We subscribe launchpad_developers to the question since this
-        # indirectly subscribes [e-mail] to it, too. We can
-        # then use this to test the updating of a question with indirect
-        # subscribers from a bug watch.
-        question.subscribe(
-            getUtility(ILaunchpadCelebrities).launchpad_developers)
-        commit()
-
-        # We now need to switch to the checkwatches DB user so that
-        # we're testing with the correct set of permissions.
-        self.layer.switchDbUser(config.checkwatches.dbuser)
-
-        # For test_can_update_bug_with_questions we also need a bug
-        # watch and by extension a bug tracker.
-        sample_person = getUtility(IPersonSet).getByEmail(
-            '[e-mail]')
-        bugtracker = new_bugtracker(BugTrackerType.ROUNDUP)
-        self.bugtask_with_question = getUtility(IBugTaskSet).createTask(
-            bug_with_question, sample_person,
-            product=getUtility(IProductSet).getByName('firefox'))
-        self.bugwatch_with_question = bug_with_question.addWatch(
-            bugtracker, '1', getUtility(ILaunchpadCelebrities).janitor)
-        self.bugtask_with_question.bugwatch = self.bugwatch_with_question
-        commit()
-
-    def test_can_update_bug_with_questions(self):
-        """Test whether bugs with linked questions can be updated.
-
-        This will also test whether indirect subscribers of linked
-        questions will be notified of the changes made when the bugwatch
-        is updated.
-        """
-        # We need to check that the bug task we created in setUp() is
-        # still being referenced by our bug watch.
-        self.assertEqual(self.bugwatch_with_question.bugtasks[0].id,
-            self.bugtask_with_question.id)
-
-        # We can now update the bug watch, which will in turn update the
-        # bug task and the linked question.
-        self.bugwatch_with_question.updateStatus('some status',
-            BugTaskStatus.INPROGRESS)
-        self.assertEqual(self.bugwatch_with_question.bugtasks[0].status,
-            BugTaskStatus.INPROGRESS,
-            "BugTask status is inconsistent. Expected %s but got %s" %
-            (BugTaskStatus.INPROGRESS.title,
-            self.bugtask_with_question.status.title))
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/bugs/doc/externalbugtracker.txt'
--- lib/lp/bugs/doc/externalbugtracker.txt	2009-11-28 08:31:15 +0000
+++ lib/lp/bugs/doc/externalbugtracker.txt	2010-01-05 16:11:20 +0000
@@ -42,7 +42,6 @@
     ...     InitializingExternalBugTracker(), [])
     COMMIT
     initializeRemoteBugDB() called: []
-    BEGIN
 
 
 === Choosing another ExternalBugTracker instance ===
@@ -318,7 +317,6 @@
     COMMIT
     getCurrentDBTime() called
     initializeRemoteBugDB() called: []
-    BEGIN
 
 If the difference between what we and the remote system think the time
 is, an error is raised.
@@ -370,7 +368,6 @@
     >>> bug_watch_updater.updateBugWatches(
     ...     CorrectTimeExternalBugTracker(), [], now=utc_now)
     COMMIT
-    BEGIN
 
 If the timezone is known, the local time time should be returned, rather
 than the UTC time.
@@ -383,7 +380,6 @@
     >>> bug_watch_updater.updateBugWatches(
     ...     LocalTimeExternalBugTracker(), [], now=utc_now)
     COMMIT
-    BEGIN
 
 If the remote server time is unknown, we will refuse to import any
 comments from it. Bug watches will still be updated, but a warning is
@@ -399,7 +395,6 @@
     COMMIT
     getCurrentDBTime() called
     initializeRemoteBugDB() called: []
-    BEGIN
     WARNING:...:Comment importing supported, but server time can't be
                 trusted. No comments will be imported.
 
@@ -494,9 +489,16 @@
     initializeRemoteBugDB() called: [u'1', u'2', u'3', u'4']
     BEGIN
     getRemoteStatus() called: u'1'
+    COMMIT
+    BEGIN
     getRemoteStatus() called: u'2'
+    COMMIT
+    BEGIN
     getRemoteStatus() called: u'3'
+    COMMIT
+    BEGIN
     getRemoteStatus() called: u'4'
+    COMMIT
 
 If the bug watches have the lastchecked attribute set, they will be
 passed to getModifiedRemoteBugs(). Only the bugs that have been modified
@@ -530,7 +532,12 @@
     initializeRemoteBugDB() called: [u'1', u'4']
     BEGIN
     getRemoteStatus() called: u'1'
+    COMMIT
+    BEGIN
     getRemoteStatus() called: u'4'
+    COMMIT
+    BEGIN
+    BEGIN
 
 The bug watches that are deemed as not being modified are still marked
 as being checked.
@@ -580,9 +587,16 @@
     initializeRemoteBugDB() called: [u'1', u'2', u'3', u'4']
     BEGIN
     getRemoteStatus() called: u'1'
+    COMMIT
+    BEGIN
     getRemoteStatus() called: u'2'
+    COMMIT
+    BEGIN
     getRemoteStatus() called: u'3'
+    COMMIT
+    BEGIN
     getRemoteStatus() called: u'4'
+    COMMIT
 
 As mentioned earlier, getModifiedRemoteBugs() is only called if we can
 get the current time of the remote system. If the time is unknown, we
@@ -600,9 +614,16 @@
     initializeRemoteBugDB() called: [u'1', u'2', u'3', u'4']
     BEGIN
     getRemoteStatus() called: u'1'
+    COMMIT
+    BEGIN
     getRemoteStatus() called: u'2'
+    COMMIT
+    BEGIN
     getRemoteStatus() called: u'3'
+    COMMIT
+    BEGIN
     getRemoteStatus() called: u'4'
+    COMMIT
 
 The only exception to the rule of only updating modified bugs is the set
 of bug watches which have comments that need to be pushed to the remote
@@ -1102,7 +1123,7 @@
     >>> bug_watch_updater.updateBugTrackers(
     ...     bug_tracker_names=[standard_bugzilla.name], batch_size=2)
     DEBUG Using a global batch size of 2
-    INFO Updating 4 watches for 2 bugs on http://example.com
+    INFO Updating 2 watches for 2 bugs on http://example.com
     initializeRemoteBugDB() called: [u'5', u'6']
     getRemoteStatus() called: u'5'
     getRemoteStatus() called: u'6'

=== modified file 'lib/lp/bugs/interfaces/bugwatch.py'
--- lib/lp/bugs/interfaces/bugwatch.py	2009-09-04 09:48:04 +0000
+++ lib/lp/bugs/interfaces/bugwatch.py	2010-01-05 16:11:20 +0000
@@ -268,6 +268,19 @@
         If no bug tracker type can be guessed, None is returned.
         """
 
+    def getBugWatchesForRemoteBug(self, remote_bug, bug_watch_ids=None):
+        """Returns bug watches referring to the given remote bug.
+
+        Returns a set of those bug watches, optionally limited to
+        those with IDs in `bug_watch_ids`, that refer to `remote_bug`.
+
+        :param remote_bug_id: The ID of the remote bug.
+        :type remote_bug_id: See `IBugWatch.remotebug`.
+
+        :param bug_watch_ids: A collection of `BugWatch` IDs.
+        :type bug_watch_ids: An iterable of `int`s, or `None`.
+        """
+
 
 class NoBugTrackerFound(Exception):
     """No bug tracker with the base_url is registered in Launchpad."""

=== modified file 'lib/lp/bugs/interfaces/externalbugtracker.py'
--- lib/lp/bugs/interfaces/externalbugtracker.py	2009-06-25 00:40:31 +0000
+++ lib/lp/bugs/interfaces/externalbugtracker.py	2010-01-05 16:11:20 +0000
@@ -43,35 +43,56 @@
         """Return the `ExternalBugTracker` instance to use.
 
         Probe the remote bug tracker and choose the right
-        `ExternalBugTracker` instance to use further on.
+        `ExternalBugTracker` instance to use further on. In most cases
+        this will simply return `self`.
         """
 
     def getCurrentDBTime():
         """Return the current time of the bug tracker's DB server.
 
         The current time will be returned as a timezone-aware datetime.
+
+        :return: `datetime.datetime` with timezone.
         """
 
     def getModifiedRemoteBugs(remote_bug_ids, last_checked):
         """Return the bug ids that have been modified.
 
         Return all ids if the modified bugs can't be determined.
+
+        :param remote_bug_ids: The remote bug IDs to be checked.
+        :type remote_bug_ids: `list` of strings
+
+        :param last_checked: The date and time since when a bug should
+            be considered modified.
+        :param last_checked: `datetime.datetime`
         """
 
     def initializeRemoteBugDB(remote_bug_ids):
-        """Do any initialization before each bug watch is updated."""
+        """Do any initialization before each bug watch is updated.
+
+        :param remote_bug_ids: The remote bug IDs that to be checked.
+        :type remote_bug_ids: `list` of strings
+        """
 
     def convertRemoteStatus(remote_status):
-        """Convert a remote status string to a BugTaskStatus item."""
+        """Convert a remote status string to a BugTaskStatus item.
+
+        :return: a member of `BugTaskStatus`
+        """
 
     def convertRemoteImportance(remote_importance):
-        """Convert a remote importance to a BugTaskImportance item."""
+        """Convert a remote importance to a BugTaskImportance item.
+
+        :return: a member of `BugTaskImportance`
+        """
 
     def getRemoteProduct(remote_bug):
         """Return the remote product for a given remote bug.
 
         :param remote_bug: The ID of the remote bug for which to return
             the remote product.
+        :type remote_bug: string
         :return: The remote product for `remote_bug`. If no remote
             product is recorded for `remote_bug` return None.
         :raise BugNotFound: If `remote_bug` doesn't exist for the bug
@@ -123,16 +144,19 @@
     def getBugReporter(remote_bug):
         """Return the person who submitted the given bug.
 
-        A tuple of (display name, email) is returned.
+        :return: `tuple` of (display name, email)
         """
 
     def getBugSummaryAndDescription(remote_bug):
-        """Return a tuple of summary and description for the given bug."""
+        """Return the summary and description for the given bug.
+
+        :return: `tuple` of (summary, description)
+        """
 
     def getBugTargetName(remote_bug):
         """Return the specific target name of the bug.
 
-        Return None if no target can be determined.
+        :return: string, or `None` if no target can be determined
         """
 
 

=== modified file 'lib/lp/bugs/model/bugwatch.py'
--- lib/lp/bugs/model/bugwatch.py	2009-12-24 06:44:57 +0000
+++ lib/lp/bugs/model/bugwatch.py	2010-01-05 16:11:20 +0000
@@ -17,7 +17,7 @@
 # SQL imports
 from sqlobject import ForeignKey, SQLObjectNotFound, StringCol
 
-from storm.expr import Desc, Not
+from storm.expr import Desc, In, Not
 from storm.store import Store
 
 from lazr.lifecycle.event import ObjectModifiedEvent
@@ -31,6 +31,7 @@
 from canonical.launchpad.database.message import Message
 from canonical.launchpad.helpers import shortlist
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.launchpad.interfaces.lpstorm import IStore
 from canonical.launchpad.validators.email import valid_email
 from canonical.launchpad.webapp import urlappend, urlsplit
 from canonical.launchpad.webapp.interfaces import NotFoundError
@@ -603,3 +604,10 @@
 
         raise UnrecognizedBugTrackerURL(url)
 
+    def getBugWatchesForRemoteBug(self, remote_bug, bug_watch_ids=None):
+        """See `IBugWatchSet`."""
+        query = IStore(BugWatch).find(
+            BugWatch, BugWatch.remotebug == remote_bug)
+        if bug_watch_ids is not None:
+            query = query.find(In(BugWatch.id, bug_watch_ids))
+        return query

=== modified file 'lib/lp/bugs/scripts/checkwatches.py'
--- lib/lp/bugs/scripts/checkwatches.py	2009-12-16 16:50:24 +0000
+++ lib/lp/bugs/scripts/checkwatches.py	2010-01-05 16:11:20 +0000
@@ -414,26 +414,6 @@
         """Return the bug watch with id `bug_watch_id`."""
         return getUtility(IBugWatchSet).get(bug_watch_id)
 
-    def _getBugWatchesByRemoteBug(self, bug_watch_ids):
-        """Returns a dictionary of bug watches mapped to remote bugs.
-
-        For each bug watch id fetches the corresponding bug watch and
-        appends it to a list of bug watches pointing to one remote
-        bug - the key of the returned mapping.
-        """
-        bug_watches_by_remote_bug = {}
-        for bug_watch_id in bug_watch_ids:
-            bug_watch = self._getBugWatch(bug_watch_id)
-            remote_bug = bug_watch.remotebug
-            # There can be multiple bug watches pointing to the same
-            # remote bug; because of that, we need to store lists of bug
-            # watches related to the remote bug, and later update the
-            # status of each one of them.
-            if remote_bug not in bug_watches_by_remote_bug:
-                bug_watches_by_remote_bug[remote_bug] = []
-            bug_watches_by_remote_bug[remote_bug].append(bug_watch)
-        return bug_watches_by_remote_bug
-
     def _getExternalBugTrackersAndWatches(self, bug_tracker, bug_watches):
         """Return an `ExternalBugTracker` instance for `bug_tracker`."""
         remotesystem = externalbugtracker.get_external_bugtracker(
@@ -686,6 +666,17 @@
             'unmodified_remote_ids': unmodified_remote_ids,
             }
 
+    def _getBugWatchesForRemoteBug(self, remote_bug_id, bug_watch_ids):
+        """Return a list of bug watches for the given remote bug.
+
+        The returned watches will all be members of `bug_watch_ids`.
+
+        This method exists primarily to be overridden during testing.
+        """
+        return list(
+            getUtility(IBugWatchSet).getBugWatchesForRemoteBug(
+                remote_bug_id, bug_watch_ids))
+
     # XXX gmb 2008-11-07 [bug=295319]
     #     This method is 186 lines long. It needs to be shorter.
     def updateBugWatches(self, remotesystem, bug_watches_to_update, now=None,
@@ -752,10 +743,6 @@
             self.txn.commit()
             raise
 
-        self.txn.begin()
-        bug_watches_by_remote_bug = self._getBugWatchesByRemoteBug(
-            bug_watch_ids)
-
         # Whether we can import and / or push comments is determined on
         # a per-bugtracker-type level.
         can_import_comments = (
@@ -788,26 +775,32 @@
             "local bugs: %(local_ids)s"
             )
 
-        for bug_id in all_remote_ids:
-            try:
-                bug_watches = bug_watches_by_remote_bug[bug_id]
-            except KeyError:
+        for remote_bug_id in all_remote_ids:
+            # Start a fresh transaction every time round the loop.
+            self.txn.begin()
+
+            bug_watches = self._getBugWatchesForRemoteBug(
+                remote_bug_id, bug_watch_ids)
+            if len(bug_watches) == 0:
                 # If there aren't any bug watches for this remote bug,
                 # just log a warning and carry on.
                 self.warning(
                     "Spurious remote bug ID: No watches found for "
-                    "remote bug %s on %s" % (bug_id, remotesystem.baseurl))
+                    "remote bug %s on %s" % (
+                        remote_bug_id, remotesystem.baseurl))
                 continue
 
             for bug_watch in bug_watches:
                 bug_watch.lastchecked = UTC_NOW
-            if bug_id in unmodified_remote_ids:
+            if remote_bug_id in unmodified_remote_ids:
                 continue
 
             # Save the remote bug URL in case we need to log an error.
             remote_bug_url = bug_watches[0].url
 
-            local_ids = ", ".join(str(watch.bug.id) for watch in bug_watches)
+            local_ids = ", ".join(
+                str(bug_id) for bug_id in sorted(
+                    watch.bug.id for watch in bug_watches))
             try:
                 new_remote_status = None
                 new_malone_status = None
@@ -820,12 +813,13 @@
                 #      necessary and can be refactored out when bug
                 #      136391 is dealt with.
                 try:
-                    new_remote_status = remotesystem.getRemoteStatus(bug_id)
+                    new_remote_status = (
+                        remotesystem.getRemoteStatus(remote_bug_id))
                     new_malone_status = self._convertRemoteStatus(
                         remotesystem, new_remote_status)
 
-                    new_remote_importance = remotesystem.getRemoteImportance(
-                        bug_id)
+                    new_remote_importance = (
+                        remotesystem.getRemoteImportance(remote_bug_id))
                     new_malone_importance = (
                         remotesystem.convertRemoteImportance(
                             new_remote_importance))
@@ -835,13 +829,13 @@
                         error, error_type_message_default)
                     self.warning(
                         message % {
-                            'bug_id': bug_id,
+                            'bug_id': remote_bug_id,
                             'base_url': remotesystem.baseurl,
                             'local_ids': local_ids,
                             },
                         properties=[
                             ('URL', remote_bug_url),
-                            ('bug_id', bug_id),
+                            ('bug_id', remote_bug_id),
                             ('local_ids', local_ids),
                             ] + self._getOOPSProperties(remotesystem),
                         info=sys.exc_info())
@@ -868,17 +862,11 @@
             except (KeyboardInterrupt, SystemExit):
                 # We should never catch KeyboardInterrupt or SystemExit.
                 raise
+
             except Exception, error:
-                # If something unexpected goes wrong, we shouldn't break the
-                # updating of the other bugs.
-
-                # Restart the transaction so that subsequent
-                # bug watches will get recorded.
+                # Restart transaction before recording the error.
                 self.txn.abort()
                 self.txn.begin()
-                bug_watches_by_remote_bug = self._getBugWatchesByRemoteBug(
-                    bug_watch_ids)
-
                 # We record errors against the bug watches and update
                 # their lastchecked dates so that we don't try to
                 # re-check them every time checkwatches runs.
@@ -889,17 +877,20 @@
                 # We need to commit the transaction, in case the next
                 # bug fails to update as well.
                 self.txn.commit()
-                self.txn.begin()
-
+                # Send the error to the log too.
                 self.error(
                     "Failure updating bug %r on %s (local bugs: %s)." %
-                            (bug_id, bug_tracker_url, local_ids),
+                            (remote_bug_id, bug_tracker_url, local_ids),
                     properties=[
                         ('URL', remote_bug_url),
-                        ('bug_id', bug_id),
+                        ('bug_id', remote_bug_id),
                         ('local_ids', local_ids)] +
                         self._getOOPSProperties(remotesystem))
 
+            else:
+                # All is well, save it now.
+                self.txn.commit()
+
     def importBug(self, external_bugtracker, bugtracker, bug_target,
                   remote_bug):
         """Import a remote bug into Launchpad.

=== modified file 'lib/lp/bugs/scripts/tests/test_bugimport.py'
--- lib/lp/bugs/scripts/tests/test_bugimport.py	2009-11-27 16:16:12 +0000
+++ lib/lp/bugs/scripts/tests/test_bugimport.py	2010-01-05 16:11:20 +0000
@@ -892,15 +892,18 @@
         """See `BugWatchUpdater`."""
         return [(TestExternalBugTracker(bug_tracker.baseurl), bug_watches)]
 
-    def _getBugWatch(self, bug_watch_id):
-        """Returns a mock bug watch object.
+    def _getBugWatchesForRemoteBug(self, remote_bug_id, bug_watch_ids):
+        """Returns a list of fake bug watch objects.
 
-        We override this method to force one of our two bug watches
-        to be returned. The first is guaranteed to trigger a db error,
-        the second should update successfuly.
+        We override this method so that we always return bug watches
+        from our list of fake bug watches.
         """
-        return self.bugtracker.getBugWatchesNeedingUpdate(0)[bug_watch_id - 1]
-
+        return [
+            bug_watch for bug_watch in (
+                self.bugtracker.getBugWatchesNeedingUpdate(0))
+            if (bug_watch.remotebug == remote_bug_id and
+                bug_watch.id in bug_watch_ids)
+            ]
 
 
 class CheckBugWatchesErrorRecoveryTestCase(unittest.TestCase):

=== modified file 'lib/lp/bugs/scripts/tests/test_checkwatches.py'
--- lib/lp/bugs/scripts/tests/test_checkwatches.py	2010-01-04 11:39:14 +0000
+++ lib/lp/bugs/scripts/tests/test_checkwatches.py	2010-01-05 16:11:20 +0000
@@ -9,14 +9,20 @@
 
 from zope.component import getUtility
 
+from canonical.config import config
+from canonical.database.sqlbase import commit
+from canonical.launchpad.ftests import login
+from canonical.launchpad.interfaces import (
+    BugTaskStatus, BugTrackerType, IBugSet, IBugTaskSet,
+    ILaunchpadCelebrities, IPersonSet, IProductSet, IQuestionSet)
 from canonical.launchpad.scripts.logger import QuietFakeLogger
-from canonical.launchpad.interfaces import ILaunchpadCelebrities
 from canonical.testing import LaunchpadZopelessLayer
 
 from lp.bugs.externalbugtracker.bugzilla import BugzillaAPI
 from lp.bugs.scripts import checkwatches
 from lp.bugs.scripts.checkwatches import CheckWatchesErrorUtility
-from lp.bugs.tests.externalbugtracker import TestBugzillaAPIXMLRPCTransport
+from lp.bugs.tests.externalbugtracker import (
+    TestBugzillaAPIXMLRPCTransport, new_bugtracker)
 from lp.testing import TestCaseWithFactory
 
 
@@ -38,37 +44,17 @@
     def getExternalBugTrackerToUse(self):
         return self
 
-    def getProductsForRemoteBugs(self, remote_bugs):
-        """Return the products for some remote bugs.
-
-        This method is basically the same as that of the superclass but
-        without the call to initializeRemoteBugDB().
-        """
-        bug_products = {}
-        for bug_id in bug_ids:
-            # If one of the bugs we're trying to get the product for
-            # doesn't exist, just skip it.
-            try:
-                actual_bug_id = self._getActualBugId(bug_id)
-            except BugNotFound:
-                continue
-
-            bug_dict = self._bugs[actual_bug_id]
-            bug_products[bug_id] = bug_dict['product']
-
-        return bug_products
-
 
 class NoBugWatchesByRemoteBugUpdater(checkwatches.BugWatchUpdater):
     """A subclass of BugWatchUpdater with methods overridden for testing."""
 
-    def _getBugWatchesByRemoteBug(self, bug_watch_ids):
-        """Return an empty dict.
+    def _getBugWatchesForRemoteBug(self, remote_bug_id, bug_watch_ids):
+        """Return an empty list.
 
-        This method overrides _getBugWatchesByRemoteBug() so that bug
+        This method overrides _getBugWatchesForRemoteBug() so that bug
         497141 can be regression-tested.
         """
-        return {}
+        return []
 
 
 class TestCheckwatchesWithSyncableGnomeProducts(TestCaseWithFactory):
@@ -146,5 +132,75 @@
             last_oops.value.startswith('Spurious remote bug ID'))
 
 
+class TestUpdateBugsWithLinkedQuestions(unittest.TestCase):
+    """Tests for updating bugs with linked questions."""
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        """Set up bugs, watches and questions to test with."""
+        super(TestUpdateBugsWithLinkedQuestions, self).setUp()
+
+        # For test_can_update_bug_with_questions we need a bug that has
+        # a question linked to it.
+        bug_with_question = getUtility(IBugSet).get(10)
+        question = getUtility(IQuestionSet).get(1)
+
+        # XXX gmb 2007-12-11 bug 175545:
+        #     We shouldn't have to login() here, but since
+        #     database.buglinktarget.BugLinkTargetMixin.linkBug()
+        #     doesn't accept a user parameter, instead depending on the
+        #     currently logged in user, we get an exception if we don't.
+        login('[e-mail]')
+        question.linkBug(bug_with_question)
+
+        # We subscribe launchpad_developers to the question since this
+        # indirectly subscribes [e-mail] to it, too. We can
+        # then use this to test the updating of a question with indirect
+        # subscribers from a bug watch.
+        question.subscribe(
+            getUtility(ILaunchpadCelebrities).launchpad_developers)
+        commit()
+
+        # We now need to switch to the checkwatches DB user so that
+        # we're testing with the correct set of permissions.
+        self.layer.switchDbUser(config.checkwatches.dbuser)
+
+        # For test_can_update_bug_with_questions we also need a bug
+        # watch and by extension a bug tracker.
+        sample_person = getUtility(IPersonSet).getByEmail(
+            '[e-mail]')
+        bugtracker = new_bugtracker(BugTrackerType.ROUNDUP)
+        self.bugtask_with_question = getUtility(IBugTaskSet).createTask(
+            bug_with_question, sample_person,
+            product=getUtility(IProductSet).getByName('firefox'))
+        self.bugwatch_with_question = bug_with_question.addWatch(
+            bugtracker, '1', getUtility(ILaunchpadCelebrities).janitor)
+        self.bugtask_with_question.bugwatch = self.bugwatch_with_question
+        commit()
+
+    def test_can_update_bug_with_questions(self):
+        """Test whether bugs with linked questions can be updated.
+
+        This will also test whether indirect subscribers of linked
+        questions will be notified of the changes made when the bugwatch
+        is updated.
+        """
+        # We need to check that the bug task we created in setUp() is
+        # still being referenced by our bug watch.
+        self.assertEqual(self.bugwatch_with_question.bugtasks[0].id,
+            self.bugtask_with_question.id)
+
+        # We can now update the bug watch, which will in turn update the
+        # bug task and the linked question.
+        self.bugwatch_with_question.updateStatus('some status',
+            BugTaskStatus.INPROGRESS)
+        self.assertEqual(self.bugwatch_with_question.bugtasks[0].status,
+            BugTaskStatus.INPROGRESS,
+            "BugTask status is inconsistent. Expected %s but got %s" %
+            (BugTaskStatus.INPROGRESS.title,
+            self.bugtask_with_question.status.title))
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/bugs/tests/test_bugwatch.py'
--- lib/lp/bugs/tests/test_bugwatch.py	2009-12-15 17:28:51 +0000
+++ lib/lp/bugs/tests/test_bugwatch.py	2010-01-05 16:11:20 +0000
@@ -11,18 +11,20 @@
 
 from zope.component import getUtility
 
+from canonical.launchpad.ftests import login, ANONYMOUS
 from canonical.launchpad.webapp import urlsplit
 from canonical.testing import (
-    DatabaseFunctionalLayer, LaunchpadFunctionalLayer)
+    DatabaseFunctionalLayer, LaunchpadFunctionalLayer, LaunchpadZopelessLayer)
 
 from lp.bugs.interfaces.bugtracker import BugTrackerType, IBugTrackerSet
 from lp.bugs.interfaces.bugwatch import (
     IBugWatchSet, NoBugTrackerFound, UnrecognizedBugTrackerURL)
 from lp.registry.interfaces.person import IPersonSet
-from lp.testing import ANONYMOUS, TestCaseWithFactory, login
-
-
-class ExtractBugTrackerAndBugTestBase(unittest.TestCase):
+
+from lp.testing import TestCaseWithFactory
+
+
+class ExtractBugTrackerAndBugTestBase:
     """Test base for testing BugWatchSet.extractBugTrackerAndBug."""
     layer = LaunchpadFunctionalLayer
 
@@ -85,7 +87,8 @@
                 "NoBugTrackerFound wasn't raised by extractBugTrackerAndBug")
 
 
-class MantisExtractBugTrackerAndBugTest(ExtractBugTrackerAndBugTestBase):
+class MantisExtractBugTrackerAndBugTest(
+    ExtractBugTrackerAndBugTestBase, unittest.TestCase):
     """Ensure BugWatchSet.extractBugTrackerAndBug works with Mantis URLs."""
 
     bugtracker_type = BugTrackerType.MANTIS
@@ -94,7 +97,8 @@
     bug_id = '3224'
 
 
-class BugzillaExtractBugTrackerAndBugTest(ExtractBugTrackerAndBugTestBase):
+class BugzillaExtractBugTrackerAndBugTest(
+    ExtractBugTrackerAndBugTestBase, unittest.TestCase):
     """Ensure BugWatchSet.extractBugTrackerAndBug works with Bugzilla URLs."""
 
     bugtracker_type = BugTrackerType.BUGZILLA
@@ -103,7 +107,8 @@
     bug_id = '3224'
 
 
-class IssuezillaExtractBugTrackerAndBugTest(ExtractBugTrackerAndBugTestBase):
+class IssuezillaExtractBugTrackerAndBugTest(
+    ExtractBugTrackerAndBugTestBase, unittest.TestCase):
     """Ensure BugWatchSet.extractBugTrackerAndBug works with Issuezilla.
 
     Issuezilla is practically the same as Buzilla, so we treat it as a
@@ -116,7 +121,8 @@
     bug_id = '3224'
 
 
-class RoundUpExtractBugTrackerAndBugTest(ExtractBugTrackerAndBugTestBase):
+class RoundUpExtractBugTrackerAndBugTest(
+    ExtractBugTrackerAndBugTestBase, unittest.TestCase):
     """Ensure BugWatchSet.extractBugTrackerAndBug works with RoundUp URLs."""
 
     bugtracker_type = BugTrackerType.ROUNDUP
@@ -125,7 +131,8 @@
     bug_id = '377'
 
 
-class TracExtractBugTrackerAndBugTest(ExtractBugTrackerAndBugTestBase):
+class TracExtractBugTrackerAndBugTest(
+    ExtractBugTrackerAndBugTestBase, unittest.TestCase):
     """Ensure BugWatchSet.extractBugTrackerAndBug works with Trac URLs."""
 
     bugtracker_type = BugTrackerType.TRAC
@@ -134,7 +141,8 @@
     bug_id = '42'
 
 
-class DebbugsExtractBugTrackerAndBugTest(ExtractBugTrackerAndBugTestBase):
+class DebbugsExtractBugTrackerAndBugTest(
+    ExtractBugTrackerAndBugTestBase, unittest.TestCase):
     """Ensure BugWatchSet.extractBugTrackerAndBug works with Debbugs URLs."""
 
     bugtracker_type = BugTrackerType.DEBBUGS
@@ -144,7 +152,7 @@
 
 
 class DebbugsExtractBugTrackerAndBugShorthandTest(
-    ExtractBugTrackerAndBugTestBase):
+    ExtractBugTrackerAndBugTestBase, unittest.TestCase):
     """Ensure extractBugTrackerAndBug works for short Debbugs URLs."""
 
     bugtracker_type = BugTrackerType.DEBBUGS
@@ -156,7 +164,8 @@
         # bugs.debian.org is already registered, so no dice.
         pass
 
-class SFExtractBugTrackerAndBugTest(ExtractBugTrackerAndBugTestBase):
+class SFExtractBugTrackerAndBugTest(
+    ExtractBugTrackerAndBugTestBase, unittest.TestCase):
     """Ensure BugWatchSet.extractBugTrackerAndBug works with SF URLs.
 
     We have only one SourceForge tracker registered in Launchpad, so we
@@ -216,7 +225,8 @@
     bug_id = '1568562'
 
 
-class XForgeExtractBugTrackerAndBugTest(ExtractBugTrackerAndBugTestBase):
+class XForgeExtractBugTrackerAndBugTest(
+    ExtractBugTrackerAndBugTestBase, unittest.TestCase):
     """Ensure extractBugTrackerAndBug works with SourceForge-like URLs.
     """
 
@@ -228,7 +238,8 @@
     bug_id = '90812'
 
 
-class RTExtractBugTrackerAndBugTest(ExtractBugTrackerAndBugTestBase):
+class RTExtractBugTrackerAndBugTest(
+    ExtractBugTrackerAndBugTestBase, unittest.TestCase):
     """Ensure BugWatchSet.extractBugTrackerAndBug works with RT URLs."""
 
     bugtracker_type = BugTrackerType.RT
@@ -237,7 +248,8 @@
     bug_id = '2379'
 
 
-class CpanExtractBugTrackerAndBugTest(ExtractBugTrackerAndBugTestBase):
+class CpanExtractBugTrackerAndBugTest(
+    ExtractBugTrackerAndBugTestBase, unittest.TestCase):
     """Ensure BugWatchSet.extractBugTrackerAndBug works with CPAN URLs."""
 
     bugtracker_type = BugTrackerType.RT
@@ -246,7 +258,8 @@
     bug_id = '2379'
 
 
-class SavannahExtractBugTrackerAndBugTest(ExtractBugTrackerAndBugTestBase):
+class SavannahExtractBugTrackerAndBugTest(
+    ExtractBugTrackerAndBugTestBase, unittest.TestCase):
     """Ensure BugWatchSet.extractBugTrackerAndBug works with Savannah URLs.
     """
 
@@ -261,7 +274,8 @@
         pass
 
 
-class SavaneExtractBugTrackerAndBugTest(ExtractBugTrackerAndBugTestBase):
+class SavaneExtractBugTrackerAndBugTest(
+    ExtractBugTrackerAndBugTestBase, unittest.TestCase):
     """Ensure BugWatchSet.extractBugTrackerAndBug works with Savane URLs.
     """
 
@@ -272,7 +286,7 @@
 
 
 class EmailAddressExtractBugTrackerAndBugTest(
-    ExtractBugTrackerAndBugTestBase):
+    ExtractBugTrackerAndBugTestBase, unittest.TestCase):
     """Ensure BugWatchSet.extractBugTrackerAndBug works with email addresses.
     """
 
@@ -290,7 +304,7 @@
 
 
 class PHPProjectBugTrackerExtractBugTrackerAndBugTest(
-    ExtractBugTrackerAndBugTestBase):
+    ExtractBugTrackerAndBugTestBase, unittest.TestCase):
     """Ensure BugWatchSet.extractBugTrackerAndBug works with PHP bug URLs.
     """
 
@@ -301,7 +315,7 @@
 
 
 class GoogleCodeBugTrackerExtractBugTrackerAndBugTest(
-    ExtractBugTrackerAndBugTestBase):
+    ExtractBugTrackerAndBugTestBase, unittest.TestCase):
     """Ensure BugWatchSet.extractBugTrackerAndBug works for Google Code URLs.
     """
 
@@ -311,6 +325,39 @@
     bug_id = '12345'
 
 
+class TestBugWatchSet(TestCaseWithFactory):
+    """Tests for the bugwatch updating system."""
+
+    layer = LaunchpadZopelessLayer
+
+    def test_getBugWatchesForRemoteBug(self):
+        # getBugWatchesForRemoteBug() returns bug watches from that
+        # refer to the remote bug.
+        bug_watches_alice = [
+            self.factory.makeBugWatch(remote_bug="alice"),
+            ]
+        bug_watches_bob = [
+            self.factory.makeBugWatch(remote_bug="bob"),
+            self.factory.makeBugWatch(remote_bug="bob"),
+            ]
+        bug_watch_set = getUtility(IBugWatchSet)
+        # Passing in the remote bug ID gets us every bug watch that
+        # refers to that remote bug.
+        self.failUnlessEqual(
+            set(bug_watches_alice),
+            set(bug_watch_set.getBugWatchesForRemoteBug('alice')))
+        self.failUnlessEqual(
+            set(bug_watches_bob),
+            set(bug_watch_set.getBugWatchesForRemoteBug('bob')))
+        # The search can be narrowed by passing in a list or other
+        # iterable collection of bug watch IDs.
+        bug_watches_limited = bug_watches_alice + bug_watches_bob[:1]
+        self.failUnlessEqual(
+            set(bug_watches_bob[:1]),
+            set(bug_watch_set.getBugWatchesForRemoteBug('bob', [
+                        bug_watch.id for bug_watch in bug_watches_limited])))
+
+
 class TestBugWatchBugTasks(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
@@ -326,31 +373,4 @@
 
 
 def test_suite():
-    suite = unittest.TestSuite()
-    suite.addTest(unittest.makeSuite(BugzillaExtractBugTrackerAndBugTest))
-    suite.addTest(unittest.makeSuite(IssuezillaExtractBugTrackerAndBugTest))
-    suite.addTest(unittest.makeSuite(RoundUpExtractBugTrackerAndBugTest))
-    suite.addTest(unittest.makeSuite(TracExtractBugTrackerAndBugTest))
-    suite.addTest(unittest.makeSuite(DebbugsExtractBugTrackerAndBugTest))
-    suite.addTest(
-        unittest.makeSuite(DebbugsExtractBugTrackerAndBugShorthandTest))
-    suite.addTest(unittest.makeSuite(SFExtractBugTrackerAndBugTest))
-    suite.addTest(unittest.makeSuite(SFTracker2ExtractBugTrackerAndBugTest))
-    suite.addTest(unittest.makeSuite(XForgeExtractBugTrackerAndBugTest))
-    suite.addTest(unittest.makeSuite(MantisExtractBugTrackerAndBugTest))
-    suite.addTest(unittest.makeSuite(RTExtractBugTrackerAndBugTest))
-    suite.addTest(unittest.makeSuite(CpanExtractBugTrackerAndBugTest))
-    suite.addTest(unittest.makeSuite(SavannahExtractBugTrackerAndBugTest))
-    suite.addTest(unittest.makeSuite(SavaneExtractBugTrackerAndBugTest))
-    suite.addTest(unittest.makeSuite(EmailAddressExtractBugTrackerAndBugTest))
-    suite.addTest(unittest.makeSuite(
-       PHPProjectBugTrackerExtractBugTrackerAndBugTest))
-    suite.addTest(unittest.makeSuite(
-        GoogleCodeBugTrackerExtractBugTrackerAndBugTest))
-    suite.addTest(unittest.makeSuite(TestBugWatchBugTasks))
-    return suite
-
-
-if __name__ == '__main__':
-    unittest.main()
-
+    return unittest.TestLoader().loadTestsFromName(__name__)

=== removed file 'lib/lp/translations/scripts/tests/__init__.py.moved'