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
> > +            #      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,
> > +                    can_import_comments, can_push_comments,
> > +                    can_back_link, error, oops_id)
> 
> I didn't notice this before; the error and oops from syncing the
> status are passed into BugWatchUpdater to do things with, yet we also
> update bug watches in this function. Not something to change in this
> branch, it's just something else to consider when fixing up the
> exception handling.
> 

Right.

> > +
> > +        except (KeyboardInterrupt, SystemExit):
> > +            # We should never catch KeyboardInterrupt or SystemExit.
> > +            raise
> > +
> 
> Now that KeyboardInterrupt and SystemExit are not subclasses of
> Exception this handler (in this context) is basically a no-op, so
> consider removing it.

Done.

> > +        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:
> > +                for bug_watch in bug_watches:
> > +                    bug_watch.lastchecked = UTC_NOW
> > +                    bug_watch.next_check = None
> > +                    bug_watch.last_error_type = error_type
> > +                    bug_watch.addActivity(
> > +                        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-21 21:06:56 +0000
> > +++ lib/lp/bugs/scripts/checkwatches/tests/test_core.py	2010-04-29 10:42:49 +0000
> > @@ -24,9 +24,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 +59,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 +156,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 +170,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], [])
> > +
> > +            # Calling updateRemoteBug() shouldn't raise a KeyError,
> > +            # even though with our broken updater
> > +            # _getExternalBugTrackersAndWatches() will return an empty
> > +            # dict.
> > +            updater.updateRemoteBug(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'),
> > +                "Unexpected last OOPS value: %s" % last_oops.value)
> >
> >      def test_suggest_batch_size(self):
> >          class RemoteSystem: pass
> > @@ -197,6 +207,37 @@
> >          checkwatches.core.suggest_batch_size(remote_system, 99999)
> >          self.failUnlessEqual(247, remote_system.batch_size)
> >
> > +    def test__makeRemoteBugUpdater(self):
> > +        # CheckwatchesMaster._makeRemoteBugUpdater() will return a
> > +        # RemoteBugUpdater instance for a given remote bug, external bug
> > +        # tracker and set of bug watches.
> > +        remote_system = BugzillaAPI('http://example.com')
> > +        remote_bug_id = '42'
> > +        bug_watch_ids = [1, 2]
> > +        unmodified_remote_ids = ['76']
> > +
> > +        checkwatches_master = CheckwatchesMaster(transaction)
> > +        updater = checkwatches_master._makeRemoteBugUpdater(
> > +            remote_system, remote_bug_id, bug_watch_ids,
> > +            unmodified_remote_ids)
> > +
> > +        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))
> 
> If you go for the remote_bug_updater_factory idea from earlier, then
> this test could be moved into a new test_remotebugupdater module, and
> be called test_create().

Done.

> > +
> >
> >  class TestUpdateBugsWithLinkedQuestions(unittest.TestCase):
> >      """Tests for updating bugs with linked questions."""
> >
> > === 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-04-29 10:42:49 +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-21 10:30:24 +0000
> > +++ lib/lp/bugs/scripts/tests/test_bugimport.py	2010-04-29 10:42:49 +0000
> 
> Consider moving this into checkwatches/tests.
> 

Considered; I will do it in a subsequent branch.

> > @@ -28,6 +28,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
> > @@ -887,6 +888,27 @@
> >          """
> >          return BugTaskImportance.UNKNOWN
> >
> 
> 2 blank lines.
> 

Oops.

> > +class TestRemoteBugUpdater(RemoteBugUpdater):
> 
> We need to use a new prefix for test doubles, because "Test" is also
> used for test cases. It's confusing! Unfortunately, it's also
> prevalent.
> 
> Ordinarily I would think about bringing this up at a reviewer meeting,
> but I'm on Landscape next week. If I add an agenda item, can I put
> your name by it?

Yes, please do.

> > +
> > +    def __init__(self, parent, external_bugtracker, remote_bug,
> > +                 bug_watch_ids, unmodified_remote_ids, bugtracker):
> > +        super(TestRemoteBugUpdater, self). __init__(
> > +            parent, external_bugtracker, remote_bug, bug_watch_ids,
> > +            unmodified_remote_ids)
> > +        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):
> > @@ -901,27 +923,20 @@
> >          """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 _makeRemoteBugUpdater(self, external_bugtracker, remote_bug,
> > +                              bug_watch_ids, unmodified_remote_ids):
> > +        return TestRemoteBugUpdater(
> > +            self, external_bugtracker, remote_bug, bug_watch_ids,
> > +            unmodified_remote_ids, 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)
> >
> 
> If possible, can you move any relevant unit tests for
> remotebugupdater.py and utilities.py into new test modules named for
> them?

I will, but again, I'll do that in a subsequent branch (I'll add a
cleanup card for this).