Merge lp:~gmb/launchpad/cw-refactor-add-rbu-bug-568881 into lp:launchpad/db-devel

[Graham Binns]

The remote bug-specific part of CheckwatchesMaster has been moved into a RemoteBugUpdater class.

This branch is the second in the mad-checkwatches-refactoring process.
It moves the remote-bug-specific part of CheckwatchesMaster into a
RemoteBugUpdater class.

The majority of changes here are simple moves, though there are some
alterations to tests and callsites to make sure that everything works
properly.

[Gavin Panella (allenap)]

Hi Graham,

This mad refactorage is looking good :) I have a few comments but
nothing major, so r=me.

Gavin.

> === modified file 'lib/lp/bugs/doc/externalbugtracker.txt'
> --- lib/lp/bugs/doc/externalbugtracker.txt 2010-04-21 10:33:55 +0000
> +++ lib/lp/bugs/doc/externalbugtracker.txt 2010-04-29 10:42:49 +0000
> @@ -641,7 +641,7 @@
>
> === Converting statuses ===
>
> -Once it has retrieved the bugs from the remote server, CheckwatchesMaster
> +Once it has retrieved the bugs from the remote server, RemoteBugUpdater
> attempts to convert their statuses into Launchpad BugTaskStatuses by
> calling the convertRemoteStatus() method on the ExternalBugTracker via
> its own _convertRemoteStatus() method.
> @@ -660,17 +660,17 @@
> ... else:
> ... raise UnknownRemoteStatusError(remote_status)
>
> -CheckwatchesMaster._convertRemoteStatus() will handle these errors and will
> +RemoteBugUpdater._convertRemoteStatus() will handle these errors and will
> return BugTaskStatus.UNKNOWN when they occur. It will also log a
> warning.
>
> - >>> status = bug_watch_updater._convertRemoteStatus(
> - ... StatusConvertingExternalBugTracker(), 'new')
> + >>> remote_bug_updater = bug_watch_updater._makeRemoteBugUpdater(
> + ... StatusConvertingExternalBugTracker(), '1', [1], [])
> + >>> status = remote_bug_updater._convertRemoteStatus('new')
> >>> print status.title
> New
>
> - >>> status = bug_watch_updater._convertRemoteStatus(
> - ... StatusConvertingExternalBugTracker(), 'spam')
> + >>> status = remote_bug_updater._convertRemoteStatus('spam')
> WARNING...Unknown remote status 'spam'. (OOPS-...)
> >>> print status.title
> Unknown
>
> === modified file 'lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py'
> --- lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py 2010-04-26 12:34:47 +0000
> +++ lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py 2010-04-29 10:42:49 +0000
> @@ -26,9 +26,12 @@
> from lp.bugs.interfaces.bug import IBugSet
> from lp.bugs.scripts.checkwatches.base import (
> WorkingBase, commit_before)
> +from lp.bugs.scripts.checkwatches.utilities import (
> + get_remote_system_oops_properties)
> from lp.registry.interfaces.person import PersonCreationRationale
>
>
> +
> class BugWatchUpdater(WorkingBase):
> """Handles the updating of a single BugWatch for checkwatches."""
>
> @@ -81,10 +84,6 @@
> @commit_before
> def importBugComments(self):
> """Import all the comments from the remote bug."""
> - # Avoid circularity.
> - from lp.bugs.scripts.checkwatches.core import (
> - get_remote_system_oops_properties)
> -
> with self.transaction:
> local_bug_id = self.bug_watch.bug.id
> remote_bug_id = self.bug_watch.remotebug
>
> === modified file 'lib/lp/bugs/scripts/checkwatches/core.py'
> --- lib/lp/bugs/scripts/checkwatches/core.py 2010-04-26 12:34:47 +0000
> +++ lib/lp/bugs/scripts/checkwatches/core.py 2010-04-29 10:42:49 +0000
> @@ -37,22 +37,21 @@
> from canonical.database.constants import UTC_NOW
> from canonical.database.sqlbase import flush_database_updates
> f...

review: Approve

[Gavin Panella (allenap)]

review: Approve (code)

[Graham Binns (gmb)]

On Thu, Apr 29, 2010 at 12:52:29PM -0000, Gavin Panella wrote:
> Review: Approve
> Hi Graham,
>
> This mad refactorage is looking good :) I have a few comments but
> nothing major, so r=me.
>
> Gavin.
>
>
> > @@ -168,6 +126,17 @@
> > else:
> > self._syncable_gnome_products = list(SYNCABLE_GNOME_PRODUCTS)
> >
> > + def _makeRemoteBugUpdater(self, external_bugtracker, remote_bug,
> > + bug_watch_ids, unmodified_remote_ids):
> > + """Create and return a `RemoteBugUpdater` instance.
> > +
> > + This method exists purely for the sake of being able to
> > + overrride it in tests.
>
> Have you been listening to Christina Aguilera recently? s/rrr/rr/

What's really sad is that you're confident enough about your knowledge
of her album names to make that gag. Shame on you.

> Another way to do this with less boiler-plate is to have a
> remote_bug_updater_factory class attribute. The signature of
> _makeRemoteBugUpdater() is identical to RemoteBugUpdater.__init__().

Right. After discussion on IRC I've done this.

> > + @commit_before
> > + def updateRemoteBug(self, can_import_comments=False,
> > + can_push_comments=False, can_back_link=False):
>
> The can_* arguments don't change for the life of the RemoteBugUpdater,
> so I think it makes sense to set them in __init__(). This could be
> done by just passing server_time in.

Agreed. Done.

> > + # Avoid circular imports
>
> Already avoided.
>

Oops, fixed.

> > + with self.transaction:
> > + bug_watches = self._getBugWatchesForRemoteBug()
> > + # If there aren't any bug watches for this remote bug,
> > + # just log a warning and carry on.
> > + if len(bug_watches) == 0:
> > + self.warning(
> > + "Spurious remote bug ID: No watches found for "
> > + "remote bug %s on %s" % (
> > + self.remote_bug, self.external_bugtracker.baseurl))
> > + return
> > + # Mark them all as checked.
> > + for bug_watch in bug_watches:
> > + bug_watch.lastchecked = UTC_NOW
> > + bug_watch.next_check = None
> > + # Next if this one is definitely unmodified.
>
> s/Next/Return/
>

Fixed.

> > + if self.remote_bug in self.unmodified_remote_ids:
> > + return
> > + # Save the remote bug URL for error reporting.
> > + remote_bug_url = bug_watches[0].url
> > + # Save the list of local bug IDs for error reporting.
> > + 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
> > + new_remote_importance = None
> > + new_malone_importance = None
> > + error = None
> > + oops_id = None
> > +
> > + # XXX: 2007-10-17 Graham Binns
> > + # This nested set of try:excepts isn't really
> > + ...

=== modified file 'lib/lp/bugs/doc/externalbugtracker.txt'
--- lib/lp/bugs/doc/externalbugtracker.txt	2010-04-23 11:19:49 +0000
+++ lib/lp/bugs/doc/externalbugtracker.txt	2010-05-05 10:09:40 +0000
@@ -391,12 +391,15 @@
     >>> from zope.interface import implements
     >>> from canonical.launchpad.interfaces import ISupportsCommentImport
     >>> class CommentImportExternalBugTracker(TimeUnknownExternalBugTracker):
+    ...     baseurl = 'http://whatever.com'
     ...     implements(ISupportsCommentImport)
     ...     sync_comments = True
-    >>> bug_watch_updater.updateBugWatches(
-    ...     CommentImportExternalBugTracker(), [], now=utc_now)
-    getCurrentDBTime() called
-    initializeRemoteBugDB() called: []
+
+    >>> checkwatches_master = CheckwatchesMaster(
+    ...     transaction, syncable_gnome_products=[])
+    >>> remote_bug_updater = checkwatches_master.remote_bug_updater_factory(
+    ...     checkwatches_master, CommentImportExternalBugTracker(), '1',
+    ...     [], [], server_time=None)
     WARNING:...:Comment importing supported, but server time can't be
                 trusted. No comments will be imported. (OOPS-...)
 
@@ -641,7 +644,7 @@
 
 === Converting statuses ===
 
-Once it has retrieved the bugs from the remote server, CheckwatchesMaster
+Once it has retrieved the bugs from the remote server, RemoteBugUpdater
 attempts to convert their statuses into Launchpad BugTaskStatuses by
 calling the convertRemoteStatus() method on the ExternalBugTracker via
 its own _convertRemoteStatus() method.
@@ -660,17 +663,18 @@
     ...         else:
     ...             raise UnknownRemoteStatusError(remote_status)
 
-CheckwatchesMaster._convertRemoteStatus() will handle these errors and will
+RemoteBugUpdater._convertRemoteStatus() will handle these errors and will
 return BugTaskStatus.UNKNOWN when they occur. It will also log a
 warning.
 
-    >>> status = bug_watch_updater._convertRemoteStatus(
-    ...     StatusConvertingExternalBugTracker(), 'new')
+    >>> remote_bug_updater = bug_watch_updater.remote_bug_updater_factory(
+    ...     bug_watch_updater, StatusConvertingExternalBugTracker(),
+    ...     '1', [1], [], utc_now)
+    >>> status = remote_bug_updater._convertRemoteStatus('new')
     >>> print status.title
     New
 
-    >>> status = bug_watch_updater._convertRemoteStatus(
-    ...     StatusConvertingExternalBugTracker(), 'spam')
+    >>> status = remote_bug_updater._convertRemoteStatus('spam')
     WARNING...Unknown remote status 'spam'. (OOPS-...)
     >>> print status.title
     Unknown
 
=== modified file 'lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py'
--- lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py	2010-04-26 12:34:47 +0000
+++ lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py	2010-05-05 10:09:40 +0000
@@ -26,9 +26,12 @@
 from lp.bugs.interfaces.bug import IBugSet
 from lp.bugs.scripts.checkwatches.base import (
     WorkingBase, commit_before)
+from lp.bugs.scripts.checkwatches.utilities import (
+    get_remote_system_oops_properties)
 from lp.registry.interfaces.person import PersonCreationRationale
 
 
+
 class BugWatchUpdater(WorkingBase):
     """Handles the updating of a single BugWatch for checkwatches."""
 
@@ -81,10 +84,6 @@
     @commit_before
     def importBugComments(self):
         """Import all the comments from the remote bug."""
-        # Avoid circularity.
-        from lp.bugs.scripts.checkwatches.core import (
-            get_remote_system_oops_properties)
-
         with self.transaction:
             local_bug_id = self.bug_watch.bug.id
             remote_bug_id = self.bug_watch.remotebug
 
=== modified file 'lib/lp/bugs/scripts/checkwatches/core.py'
--- lib/lp/bugs/scripts/checkwatches/core.py	2010-04-30 03:10:17 +0000
+++ lib/lp/bugs/scripts/checkwatches/core.py	2010-05-05 10:09:40 +0000
@@ -37,22 +37,21 @@
 from canonical.database.constants import UTC_NOW
 from canonical.database.sqlbase import flush_database_updates
 from canonical.launchpad.interfaces import (
-    BugTaskStatus, BugWatchActivityStatus, CreateBugParams,
-    IBugTrackerSet, IBugWatchSet, IDistribution, ILaunchpadCelebrities,
-    IPersonSet, ISupportsCommentImport, ISupportsCommentPushing,
-    PersonCreationRationale, UNKNOWN_REMOTE_STATUS)
+    CreateBugParams, IBugTrackerSet, IBugWatchSet, IDistribution,
+    ILaunchpadCelebrities, IPersonSet, ISupportsCommentImport,
+    ISupportsCommentPushing, PersonCreationRationale)
 from canonical.launchpad.scripts.logger import log as default_log
 
 from lp.bugs import externalbugtracker
 from lp.bugs.externalbugtracker import (
-    BATCH_SIZE_UNLIMITED, BugNotFound, BugTrackerConnectError,
-    BugWatchUpdateError, InvalidBugId, PrivateRemoteBug,
-    UnknownBugTrackerTypeError, UnknownRemoteStatusError, UnparseableBugData,
-    UnparseableBugTrackerVersion, UnsupportedBugTrackerVersion)
+    BATCH_SIZE_UNLIMITED, BugWatchUpdateError,
+    UnknownBugTrackerTypeError)
 from lp.bugs.interfaces.externalbugtracker import ISupportsBackLinking
 from lp.bugs.scripts.checkwatches.base import (
     WorkingBase, commit_before, with_interaction)
-from lp.bugs.scripts.checkwatches.bugwatchupdater import BugWatchUpdater
+from lp.bugs.scripts.checkwatches.remotebugupdater import RemoteBugUpdater
+from lp.bugs.scripts.checkwatches.utilities import (
+    get_bugwatcherrortype_for_error)
 from lp.services.scripts.base import LaunchpadCronScript
 
 
@@ -77,30 +76,6 @@
     """Time difference between ourselves and the remote server is too much."""
 
 
-_exception_to_bugwatcherrortype = [
-   (BugTrackerConnectError, BugWatchActivityStatus.CONNECTION_ERROR),
-   (PrivateRemoteBug, BugWatchActivityStatus.PRIVATE_REMOTE_BUG),
-   (UnparseableBugData, BugWatchActivityStatus.UNPARSABLE_BUG),
-   (UnparseableBugTrackerVersion,
-    BugWatchActivityStatus.UNPARSABLE_BUG_TRACKER),
-   (UnsupportedBugTrackerVersion,
-    BugWatchActivityStatus.UNSUPPORTED_BUG_TRACKER),
-   (UnknownBugTrackerTypeError,
-    BugWatchActivityStatus.UNSUPPORTED_BUG_TRACKER),
-   (InvalidBugId, BugWatchActivityStatus.INVALID_BUG_ID),
-   (BugNotFound, BugWatchActivityStatus.BUG_NOT_FOUND),
-   (PrivateRemoteBug, BugWatchActivityStatus.PRIVATE_REMOTE_BUG),
-   (socket.timeout, BugWatchActivityStatus.TIMEOUT)]
-
-def get_bugwatcherrortype_for_error(error):
-    """Return the correct `BugWatchActivityStatus` for a given error."""
-    for exc_type, bugwatcherrortype in _exception_to_bugwatcherrortype:
-        if isinstance(error, exc_type):
-            return bugwatcherrortype
-    else:
-        return BugWatchActivityStatus.UNKNOWN
-
-
 def unique(iterator):
     """Generate only unique items from an iterator."""
     seen = set()
@@ -126,26 +101,11 @@
             int(SUGGESTED_BATCH_SIZE_PROPORTION * num_watches))
 
 
-def get_remote_system_oops_properties(remote_system):
-    """Return (name, value) tuples describing a remote system.
-
-    Each item in the list is intended for use as an OOPS property.
-
-    :remote_system: The `ExternalBugTracker` instance from which the
-        OOPS properties should be extracted.
-    """
-    return [
-        ('batch_size', remote_system.batch_size),
-        ('batch_query_threshold', remote_system.batch_query_threshold),
-        ('sync_comments', remote_system.sync_comments),
-        ('externalbugtracker', remote_system.__class__.__name__),
-        ('baseurl', remote_system.baseurl)
-        ]
-
-
 class CheckwatchesMaster(WorkingBase):
     """Takes responsibility for updating remote bug watches."""
 
+    remote_bug_updater_factory = RemoteBugUpdater
+
     def __init__(self, transaction_manager, logger=default_log,
                  syncable_gnome_products=None):
         """Initialize a CheckwatchesMaster.
@@ -455,37 +415,6 @@
                 self.logger.debug(
                     "No watches to update on %s" % bug_tracker.baseurl)
 
-    def _convertRemoteStatus(self, remotesystem, remote_status):
-        """Convert a remote bug status to a Launchpad status and return it.
-
-        :param remotesystem: The `IExternalBugTracker` instance
-            representing the remote system.
-        :param remote_status: The remote status to be converted into a
-            Launchpad status.
-
-        If the remote status cannot be mapped to a Launchpad status,
-        BugTaskStatus.UNKNOWN will be returned and a warning will be
-        logged.
-        """
-        # We don't bother trying to convert UNKNOWN_REMOTE_STATUS.
-        if remote_status == UNKNOWN_REMOTE_STATUS:
-            return BugTaskStatus.UNKNOWN
-
-        try:
-            launchpad_status = remotesystem.convertRemoteStatus(
-                remote_status)
-        except UnknownRemoteStatusError:
-            # We log the warning, since we need to know about statuses
-            # that we don't handle correctly.
-            self.warning(
-                "Unknown remote status '%s'." % remote_status,
-                get_remote_system_oops_properties(remotesystem),
-                sys.exc_info())
-
-            launchpad_status = BugTaskStatus.UNKNOWN
-
-        return launchpad_status
-
     def _getRemoteIdsToCheck(self, remotesystem, bug_watches,
                              server_time=None, now=None, batch_size=None):
         """Return the remote bug IDs to check for a set of bug watches.
@@ -607,17 +536,6 @@
             'unmodified_remote_ids': sorted(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.
     @commit_before
@@ -678,140 +596,11 @@
                     bug_watch_ids, get_bugwatcherrortype_for_error(error))
             raise
 
-        # Whether we can import and / or push comments is determined
-        # on a per-bugtracker-type level.
-        can_import_comments = (
-            ISupportsCommentImport.providedBy(remotesystem) and
-            remotesystem.sync_comments)
-        can_push_comments = (
-            ISupportsCommentPushing.providedBy(remotesystem) and
-            remotesystem.sync_comments)
-        can_back_link = (
-            ISupportsBackLinking.providedBy(remotesystem) and
-            remotesystem.sync_comments)
-
-        if can_import_comments and server_time is None:
-            can_import_comments = False
-            self.warning(
-                "Comment importing supported, but server time can't be"
-                " trusted. No comments will be imported.")
-
-        error_type_messages = {
-            BugWatchActivityStatus.INVALID_BUG_ID:
-                ("Invalid bug %(bug_id)r on %(base_url)s "
-                 "(local bugs: %(local_ids)s)."),
-            BugWatchActivityStatus.BUG_NOT_FOUND:
-                ("Didn't find bug %(bug_id)r on %(base_url)s "
-                 "(local bugs: %(local_ids)s)."),
-            BugWatchActivityStatus.PRIVATE_REMOTE_BUG:
-                ("Remote bug %(bug_id)r on %(base_url)s is private "
-                 "(local bugs: %(local_ids)s)."),
-            }
-        error_type_message_default = (
-            "remote bug: %(bug_id)r; "
-            "base url: %(base_url)s; "
-            "local bugs: %(local_ids)s"
-            )
-
         for remote_bug_id in all_remote_ids:
-            with self.transaction:
-                bug_watches = self._getBugWatchesForRemoteBug(
-                    remote_bug_id, bug_watch_ids)
-                # If there aren't any bug watches for this remote bug,
-                # just log a warning and carry on.
-                if len(bug_watches) == 0:
-                    self.warning(
-                        "Spurious remote bug ID: No watches found for "
-                        "remote bug %s on %s" % (
-                            remote_bug_id, remotesystem.baseurl))
-                    continue
-                # Mark them all as checked.
-                for bug_watch in bug_watches:
-                    bug_watch.lastchecked = UTC_NOW
-                    bug_watch.next_check = None
-                # Next if this one is definitely unmodified.
-                if remote_bug_id in unmodified_remote_ids:
-                    continue
-                # Save the remote bug URL for error reporting.
-                remote_bug_url = bug_watches[0].url
-                # Save the list of local bug IDs for error reporting.
-                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
-                new_remote_importance = None
-                new_malone_importance = None
-                error = None
-                oops_id = None
-
-                # XXX: 2007-10-17 Graham Binns
-                #      This nested set of try:excepts isn't really
-                #      necessary and can be refactored out when bug
-                #      136391 is dealt with.
-                try:
-                    new_remote_status = (
-                        remotesystem.getRemoteStatus(remote_bug_id))
-                    new_malone_status = self._convertRemoteStatus(
-                        remotesystem, new_remote_status)
-                    new_remote_importance = (
-                        remotesystem.getRemoteImportance(remote_bug_id))
-                    new_malone_importance = (
-                        remotesystem.convertRemoteImportance(
-                            new_remote_importance))
-                except (InvalidBugId, BugNotFound, PrivateRemoteBug), ex:
-                    error = get_bugwatcherrortype_for_error(ex)
-                    message = error_type_messages.get(
-                        error, error_type_message_default)
-                    oops_id = self.warning(
-                        message % {
-                            'bug_id': remote_bug_id,
-                            'base_url': remotesystem.baseurl,
-                            'local_ids': local_ids,
-                            },
-                        properties=[
-                            ('URL', remote_bug_url),
-                            ('bug_id', remote_bug_id),
-                            ('local_ids', local_ids),
-                            ] + get_remote_system_oops_properties(
-                                remotesystem),
-                        info=sys.exc_info())
-
-                for bug_watch in bug_watches:
-                    bug_watch_updater = BugWatchUpdater(
-                        self, bug_watch, remotesystem)
-
-                    bug_watch_updater.updateBugWatch(
-                        new_remote_status, new_malone_status,
-                        new_remote_importance, new_malone_importance,
-                        can_import_comments, can_push_comments,
-                        can_back_link, error, oops_id)
-
-            except (KeyboardInterrupt, SystemExit):
-                # We should never catch KeyboardInterrupt or SystemExit.
-                raise
-
-            except Exception, error:
-                # Send the error to the log.
-                oops_id = self.error(
-                    "Failure updating bug %r on %s (local bugs: %s)." %
-                            (remote_bug_id, bug_tracker_url, local_ids),
-                    properties=[
-                        ('URL', remote_bug_url),
-                        ('bug_id', remote_bug_id),
-                        ('local_ids', local_ids)] +
-                        get_remote_system_oops_properties(remotesystem))
-                # 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.
-                error_type = get_bugwatcherrortype_for_error(error)
-                with self.transaction:
-                    getUtility(IBugWatchSet).bulkSetError(
-                        bug_watches, error_type)
-                    getUtility(IBugWatchSet).bulkAddActivity(
-                        bug_watches, result=error_type, oops_id=oops_id)
+            remote_bug_updater = self.remote_bug_updater_factory(
+                self, remotesystem, remote_bug_id, bug_watch_ids,
+                unmodified_remote_ids, server_time)
+            remote_bug_updater.updateRemoteBug()
 
     def importBug(self, external_bugtracker, bugtracker, bug_target,
                   remote_bug):
 
=== added file 'lib/lp/bugs/scripts/checkwatches/remotebugupdater.py'
--- lib/lp/bugs/scripts/checkwatches/remotebugupdater.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/scripts/checkwatches/remotebugupdater.py	2010-05-05 10:09:40 +0000
@@ -0,0 +1,218 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Classes and logic for the remote bug updater."""
+
+from __future__ import with_statement
+
+__metaclass__ = type
+__all__ = [
+    'RemoteBugUpdater',
+    ]
+
+import sys
+
+from zope.component import getUtility
+
+from canonical.database.constants import UTC_NOW
+
+from lp.bugs.externalbugtracker import (
+    BugNotFound, InvalidBugId, PrivateRemoteBug, UnknownRemoteStatusError)
+
+from lp.bugs.interfaces.bugtask import BugTaskStatus
+from lp.bugs.interfaces.bugwatch import BugWatchActivityStatus, IBugWatchSet
+from lp.bugs.interfaces.externalbugtracker import (
+    ISupportsBackLinking, ISupportsCommentImport,
+    ISupportsCommentPushing, UNKNOWN_REMOTE_STATUS)
+from lp.bugs.scripts.checkwatches.base import WorkingBase, commit_before
+from lp.bugs.scripts.checkwatches.bugwatchupdater import BugWatchUpdater
+from lp.bugs.scripts.checkwatches.utilities import (
+    get_bugwatcherrortype_for_error, get_remote_system_oops_properties)
+
+
+class RemoteBugUpdater(WorkingBase):
+
+    def __init__(self, parent, external_bugtracker, remote_bug,
+                 bug_watch_ids, unmodified_remote_ids, server_time):
+        self.initFromParent(parent)
+        self.external_bugtracker = external_bugtracker
+        self.bug_tracker_url = external_bugtracker.baseurl
+        self.remote_bug = remote_bug
+        self.bug_watch_ids = bug_watch_ids
+        self.unmodified_remote_ids = unmodified_remote_ids
+
+        self.error_type_messages = {
+            BugWatchActivityStatus.INVALID_BUG_ID:
+                ("Invalid bug %(bug_id)r on %(base_url)s "
+                 "(local bugs: %(local_ids)s)."),
+            BugWatchActivityStatus.BUG_NOT_FOUND:
+                ("Didn't find bug %(bug_id)r on %(base_url)s "
+                 "(local bugs: %(local_ids)s)."),
+            BugWatchActivityStatus.PRIVATE_REMOTE_BUG:
+                ("Remote bug %(bug_id)r on %(base_url)s is private "
+                 "(local bugs: %(local_ids)s)."),
+            }
+        self.error_type_message_default = (
+            "remote bug: %(bug_id)r; "
+            "base url: %(base_url)s; "
+            "local bugs: %(local_ids)s"
+            )
+
+        # Whether we can import and / or push comments is determined
+        # on a per-bugtracker-type level.
+        self.can_import_comments = (
+            ISupportsCommentImport.providedBy(external_bugtracker) and
+            external_bugtracker.sync_comments)
+        self.can_push_comments = (
+            ISupportsCommentPushing.providedBy(external_bugtracker) and
+            external_bugtracker.sync_comments)
+        self.can_back_link = (
+            ISupportsBackLinking.providedBy(external_bugtracker) and
+            external_bugtracker.sync_comments)
+
+        if self.can_import_comments and server_time is None:
+            self.can_import_comments = False
+            self.warning(
+                "Comment importing supported, but server time can't be "
+                "trusted. No comments will be imported.")
+
+    def _getBugWatchesForRemoteBug(self):
+        """Return a list of bug watches for the current remote bug.
+
+        The returned watches will all be members of `self.bug_watch_ids`.
+
+        This method exists primarily to be overridden during testing.
+        """
+        return list(
+            getUtility(IBugWatchSet).getBugWatchesForRemoteBug(
+                self.remote_bug, self.bug_watch_ids))
+
+    @commit_before
+    def updateRemoteBug(self):
+        with self.transaction:
+            bug_watches = self._getBugWatchesForRemoteBug()
+            # If there aren't any bug watches for this remote bug,
+            # just log a warning and carry on.
+            if len(bug_watches) == 0:
+                self.warning(
+                    "Spurious remote bug ID: No watches found for "
+                    "remote bug %s on %s" % (
+                        self.remote_bug, self.external_bugtracker.baseurl))
+                return
+            # Mark them all as checked.
+            for bug_watch in bug_watches:
+                bug_watch.lastchecked = UTC_NOW
+                bug_watch.next_check = None
+            # Return if this one is definitely unmodified.
+            if self.remote_bug in self.unmodified_remote_ids:
+                return
+            # Save the remote bug URL for error reporting.
+            remote_bug_url = bug_watches[0].url
+            # Save the list of local bug IDs for error reporting.
+            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
+            new_remote_importance = None
+            new_malone_importance = None
+            error = None
+            oops_id = None
+
+            # XXX: 2007-10-17 Graham Binns
+            #      This nested set of try:excepts isn't really
+            #      necessary and can be refactored out when bug
+            #      136391 is dealt with.
+            try:
+                new_remote_status = (
+                    self.external_bugtracker.getRemoteStatus(
+                        self.remote_bug))
+                new_malone_status = self._convertRemoteStatus(
+                    new_remote_status)
+                new_remote_importance = (
+                    self.external_bugtracker.getRemoteImportance(
+                        self.remote_bug))
+                new_malone_importance = (
+                    self.external_bugtracker.convertRemoteImportance(
+                        new_remote_importance))
+            except (InvalidBugId, BugNotFound, PrivateRemoteBug), ex:
+                error = get_bugwatcherrortype_for_error(ex)
+                message = self.error_type_messages.get(
+                    error, self.error_type_message_default)
+                oops_id = self.warning(
+                    message % {
+                        'bug_id': self.remote_bug,
+                        'base_url': self.external_bugtracker.baseurl,
+                        'local_ids': local_ids,
+                        },
+                    properties=[
+                        ('URL', remote_bug_url),
+                        ('bug_id', self.remote_bug),
+                        ('local_ids', local_ids),
+                        ] + get_remote_system_oops_properties(
+                            self.external_bugtracker),
+                    info=sys.exc_info())
+
+            for bug_watch in bug_watches:
+                bug_watch_updater = BugWatchUpdater(
+                    self, bug_watch, self.external_bugtracker)
+
+                bug_watch_updater.updateBugWatch(
+                    new_remote_status, new_malone_status,
+                    new_remote_importance, new_malone_importance,
+                    self.can_import_comments, self.can_push_comments,
+                    self.can_back_link, error, oops_id)
+
+        except Exception, error:
+            # Send the error to the log.
+            oops_id = self.error(
+                "Failure updating bug %r on %s (local bugs: %s)." %
+                        (self.remote_bug, self.bug_tracker_url, local_ids),
+                properties=[
+                    ('URL', remote_bug_url),
+                    ('bug_id', self.remote_bug),
+                    ('local_ids', local_ids)] +
+                    get_remote_system_oops_properties(
+                        self.external_bugtracker))
+            # 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.
+            error_type = get_bugwatcherrortype_for_error(error)
+            with self.transaction:
+                getUtility(IBugWatchSet).bulkSetError(
+                    bug_watches, error_type)
+                getUtility(IBugWatchSet).bulkAddActivity(
+                    bug_watches, result=error_type, oops_id=oops_id)
+
+    def _convertRemoteStatus(self, remote_status):
+        """Convert a remote bug status to a Launchpad status and return it.
+
+        :param remote_status: The remote status to be converted into a
+            Launchpad status.
+
+        If the remote status cannot be mapped to a Launchpad status,
+        BugTaskStatus.UNKNOWN will be returned and a warning will be
+        logged.
+        """
+        # We don't bother trying to convert UNKNOWN_REMOTE_STATUS.
+        if remote_status == UNKNOWN_REMOTE_STATUS:
+            return BugTaskStatus.UNKNOWN
+
+        try:
+            launchpad_status = self.external_bugtracker.convertRemoteStatus(
+                remote_status)
+        except UnknownRemoteStatusError:
+            # We log the warning, since we need to know about statuses
+            # that we don't handle correctly.
+            self.warning(
+                "Unknown remote status '%s'." % remote_status,
+                get_remote_system_oops_properties(
+                    self.external_bugtracker),
+                sys.exc_info())
+
+            launchpad_status = BugTaskStatus.UNKNOWN
+
+        return launchpad_status
+
 
=== modified file 'lib/lp/bugs/scripts/checkwatches/tests/test_core.py'
--- lib/lp/bugs/scripts/checkwatches/tests/test_core.py	2010-04-23 11:19:49 +0000
+++ lib/lp/bugs/scripts/checkwatches/tests/test_core.py	2010-05-05 10:09:40 +0000
@@ -11,6 +11,7 @@
 
 import transaction
 
+from datetime import datetime
 from zope.component import getUtility
 
 from canonical.config import config
@@ -24,9 +25,11 @@
 from lp.bugs.externalbugtracker.bugzilla import BugzillaAPI
 from lp.bugs.interfaces.bugtracker import IBugTrackerSet
 from lp.bugs.scripts import checkwatches
+from lp.bugs.scripts.checkwatches.base import (
+    CheckWatchesErrorUtility, WorkingBase)
 from lp.bugs.scripts.checkwatches.core import (
-    CheckwatchesMaster, TwistedThreadScheduler)
-from lp.bugs.scripts.checkwatches.base import CheckWatchesErrorUtility
+    CheckwatchesMaster, LOGIN, TwistedThreadScheduler)
+from lp.bugs.scripts.checkwatches.remotebugupdater import RemoteBugUpdater
 from lp.bugs.tests.externalbugtracker import (
     TestBugzillaAPIXMLRPCTransport, TestExternalBugTracker, new_bugtracker)
 from lp.testing import TestCaseWithFactory, ZopeTestInSubProcess
@@ -57,10 +60,10 @@
         return self
 
 
-class NoBugWatchesByRemoteBugUpdater(checkwatches.CheckwatchesMaster):
-    """A subclass of CheckwatchesMaster with methods overridden for testing."""
+class NoBugWatchesByRemoteBugUpdater(RemoteBugUpdater):
+    """A subclass of RemoteBugUpdater with methods overridden for testing."""
 
-    def _getBugWatchesForRemoteBug(self, remote_bug_id, bug_watch_ids):
+    def _getBugWatchesForRemoteBug(self):
         """Return an empty list.
 
         This method overrides _getBugWatchesForRemoteBug() so that bug
@@ -154,10 +157,8 @@
 
     def test_bug_497141(self):
         # Regression test for bug 497141. KeyErrors raised in
-        # CheckwatchesMaster.updateBugWatches() shouldn't cause
+        # RemoteBugUpdater.updateRemoteBug() shouldn't cause
         # checkwatches to abort.
-        updater = NoBugWatchesByRemoteBugUpdater(
-            transaction.manager, QuietFakeLogger())
 
         # Create a couple of bug watches for testing purposes.
         bug_tracker = self.factory.makeBugTracker()
@@ -170,17 +171,27 @@
         remote_system = NonConnectingBugzillaAPI(
             bug_tracker.baseurl, xmlrpc_transport=test_transport)
 
-        # Calling updateBugWatches() shouldn't raise a KeyError, even
-        # though with our broken updater _getExternalBugTrackersAndWatches()
-        # will return an empty dict.
-        updater.updateBugWatches(remote_system, bug_watches)
-
-        # An error will have been logged instead of the KeyError being
-        # raised.
-        error_utility = CheckWatchesErrorUtility()
-        last_oops = error_utility.getLastOopsReport()
-        self.assertTrue(
-            last_oops.value.startswith('Spurious remote bug ID'))
+        working_base = WorkingBase()
+        working_base.init(LOGIN, transaction.manager, QuietFakeLogger())
+
+        for bug_watch in bug_watches:
+            updater = NoBugWatchesByRemoteBugUpdater(
+                working_base, remote_system, bug_watch.remotebug,
+                [bug_watch.id], [], datetime.now())
+
+            # Calling updateRemoteBug() shouldn't raise a KeyError,
+            # even though with our broken updater
+            # _getExternalBugTrackersAndWatches() will return an empty
+            # dict.
+            updater.updateRemoteBug()
+
+            # An error will have been logged instead of the KeyError being
+            # raised.
+            error_utility = CheckWatchesErrorUtility()
+            last_oops = error_utility.getLastOopsReport()
+            self.assertTrue(
+                last_oops.value.startswith('Spurious remote bug ID'),
+                "Unexpected last OOPS value: %s" % last_oops.value)
 
     def test_suggest_batch_size(self):
         class RemoteSystem: pass
 
=== added file 'lib/lp/bugs/scripts/checkwatches/tests/test_remotebugupdater.py'
--- lib/lp/bugs/scripts/checkwatches/tests/test_remotebugupdater.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/scripts/checkwatches/tests/test_remotebugupdater.py	2010-05-05 10:09:40 +0000
@@ -0,0 +1,59 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the checkwatches remotebugupdater module."""
+
+__metaclass__ = type
+
+import transaction
+import unittest
+
+from canonical.testing import LaunchpadZopelessLayer
+
+from lp.bugs.externalbugtracker.bugzilla import Bugzilla
+from lp.bugs.scripts.checkwatches.core import CheckwatchesMaster
+from lp.bugs.scripts.checkwatches.remotebugupdater import RemoteBugUpdater
+from lp.testing import TestCaseWithFactory
+
+
+class RemoteBugUpdaterTestCase(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def test_create(self):
+        # CheckwatchesMaster.remote_bug_updater_factory points to the
+        # RemoteBugUpdater class, so it can be used to create
+        # RemoteBugUpdaters.
+        remote_system = Bugzilla('http://example.com')
+        remote_bug_id = '42'
+        bug_watch_ids = [1, 2]
+        unmodified_remote_ids = ['76']
+
+        checkwatches_master = CheckwatchesMaster(transaction)
+        updater = checkwatches_master.remote_bug_updater_factory(
+            checkwatches_master, remote_system, remote_bug_id,
+            bug_watch_ids, unmodified_remote_ids, None)
+
+        self.assertTrue(
+            isinstance(updater, RemoteBugUpdater),
+            "updater should be an instance of RemoteBugUpdater.")
+        self.assertEqual(
+            remote_system, updater.external_bugtracker,
+            "Unexpected external_bugtracker for RemoteBugUpdater.")
+        self.assertEqual(
+            remote_bug_id, updater.remote_bug,
+            "RemoteBugUpdater's remote_bug should be '%s', was '%s'" %
+            (remote_bug_id, updater.remote_bug))
+        self.assertEqual(
+            bug_watch_ids, updater.bug_watch_ids,
+            "RemoteBugUpdater's bug_watch_ids should be '%s', were '%s'" %
+            (bug_watch_ids, updater.bug_watch_ids))
+        self.assertEqual(
+            unmodified_remote_ids, updater.unmodified_remote_ids,
+            "RemoteBugUpdater's unmodified_remote_ids should be '%s', "
+            "were '%s'" % 
+            (unmodified_remote_ids, updater.unmodified_remote_ids))
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)
 
=== added file 'lib/lp/bugs/scripts/checkwatches/utilities.py'
--- lib/lp/bugs/scripts/checkwatches/utilities.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/scripts/checkwatches/utilities.py	2010-05-05 10:09:40 +0000
@@ -0,0 +1,61 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Utility functions for checkwatches."""
+
+__metaclass__ = type
+__all__ = [
+    'get_bugwatcherrortype_for_error',
+    'get_remote_system_oops_properties',
+    ]
+
+import socket
+
+from lp.bugs.externalbugtracker import (
+    BugNotFound, BugTrackerConnectError, InvalidBugId, PrivateRemoteBug,
+    UnknownBugTrackerTypeError, UnparseableBugData,
+    UnparseableBugTrackerVersion, UnsupportedBugTrackerVersion)
+
+from lp.bugs.interfaces.bugwatch import BugWatchActivityStatus
+
+
+_exception_to_bugwatcherrortype = [
+   (BugTrackerConnectError, BugWatchActivityStatus.CONNECTION_ERROR),
+   (PrivateRemoteBug, BugWatchActivityStatus.PRIVATE_REMOTE_BUG),
+   (UnparseableBugData, BugWatchActivityStatus.UNPARSABLE_BUG),
+   (UnparseableBugTrackerVersion,
+    BugWatchActivityStatus.UNPARSABLE_BUG_TRACKER),
+   (UnsupportedBugTrackerVersion,
+    BugWatchActivityStatus.UNSUPPORTED_BUG_TRACKER),
+   (UnknownBugTrackerTypeError,
+    BugWatchActivityStatus.UNSUPPORTED_BUG_TRACKER),
+   (InvalidBugId, BugWatchActivityStatus.INVALID_BUG_ID),
+   (BugNotFound, BugWatchActivityStatus.BUG_NOT_FOUND),
+   (PrivateRemoteBug, BugWatchActivityStatus.PRIVATE_REMOTE_BUG),
+   (socket.timeout, BugWatchActivityStatus.TIMEOUT)]
+
+
+def get_bugwatcherrortype_for_error(error):
+    """Return the correct `BugWatchActivityStatus` for a given error."""
+    for exc_type, bugwatcherrortype in _exception_to_bugwatcherrortype:
+        if isinstance(error, exc_type):
+            return bugwatcherrortype
+    else:
+        return BugWatchActivityStatus.UNKNOWN
+
+
+def get_remote_system_oops_properties(remote_system):
+    """Return (name, value) tuples describing a remote system.
+
+    Each item in the list is intended for use as an OOPS property.
+
+    :remote_system: The `ExternalBugTracker` instance from which the
+        OOPS properties should be extracted.
+    """
+    return [
+        ('batch_size', remote_system.batch_size),
+        ('batch_query_threshold', remote_system.batch_query_threshold),
+        ('sync_comments', remote_system.sync_comments),
+        ('externalbugtracker', remote_system.__class__.__name__),
+        ('baseurl', remote_system.baseurl)
+        ]
 
=== modified file 'lib/lp/bugs/scripts/tests/test_bugimport.py'
--- lib/lp/bugs/scripts/tests/test_bugimport.py	2010-04-29 10:09:24 +0000
+++ lib/lp/bugs/scripts/tests/test_bugimport.py	2010-05-05 10:09:40 +0000
@@ -30,6 +30,7 @@
 from lp.bugs.scripts import bugimport
 from lp.bugs.scripts.bugimport import ET
 from lp.bugs.scripts.checkwatches import CheckwatchesMaster
+from lp.bugs.scripts.checkwatches.remotebugupdater import RemoteBugUpdater
 from lp.registry.interfaces.person import IPersonSet, PersonCreationRationale
 from lp.registry.interfaces.product import IProductSet
 from lp.registry.model.person import generate_nick
@@ -892,6 +893,29 @@
         return BugTaskImportance.UNKNOWN
 
 
+class TestRemoteBugUpdater(RemoteBugUpdater):
+
+    def __init__(self, parent, external_bugtracker, remote_bug,
+                 bug_watch_ids, unmodified_remote_ids, server_time,
+                 bugtracker):
+        super(TestRemoteBugUpdater, self). __init__(
+            parent, external_bugtracker, remote_bug, bug_watch_ids,
+            unmodified_remote_ids, server_time)
+        self.bugtracker = bugtracker
+
+    def _getBugWatchesForRemoteBug(self):
+        """Returns a list of fake bug watch objects.
+
+        We override this method so that we always return bug watches
+        from our list of fake bug watches.
+        """
+        return [
+            bug_watch for bug_watch in (
+                self.bugtracker.watches_needing_update)
+            if (bug_watch.remotebug == self.remote_bug and
+                bug_watch.id in self.bug_watch_ids)
+            ]
+
 
 class TestCheckwatchesMaster(CheckwatchesMaster):
     """A mock `CheckwatchesMaster` object."""
@@ -905,28 +929,21 @@
         """See `CheckwatchesMaster`."""
         return [(TestExternalBugTracker(bug_tracker.baseurl), bug_watches)]
 
-    def _getBugWatchesForRemoteBug(self, remote_bug_id, bug_watch_ids):
-        """Returns a list of fake bug watch objects.
-
-        We override this method so that we always return bug watches
-        from our list of fake bug watches.
-        """
-        return [
-            bug_watch for bug_watch in (
-                self.bugtracker.watches_needing_update)
-            if (bug_watch.remotebug == remote_bug_id and
-                bug_watch.id in bug_watch_ids)
-            ]
-
-
-class CheckBugWatchesErrorRecoveryTestCase(unittest.TestCase):
+    def remote_bug_updater_factory(self, parent, external_bugtracker,
+                                   remote_bug, bug_watch_ids,
+                                   unmodified_remote_ids, server_time):
+        return TestRemoteBugUpdater(
+            self, external_bugtracker, remote_bug, bug_watch_ids,
+            unmodified_remote_ids, server_time, self.bugtracker)
+
+
+class CheckwatchesErrorRecoveryTestCase(unittest.TestCase):
     """Test that errors in the bugwatch import process don't
     invalidate the entire run.
     """
     layer = LaunchpadZopelessLayer
 
-    def test_checkbugwatches_error_recovery(self):
-
+    def test_checkwatches_error_recovery(self):
         firefox = getUtility(IProductSet).get(4)
         foobar = getUtility(IPersonSet).get(16)
         params = CreateBugParams(