On Fri, Apr 23, 2010 at 01:56:40PM -0000, Gavin Panella wrote:
> Review: Approve
> You're my hero for taking this on.

Ta :)

> This is all good. I've made a few comments, and a bigger suggestion
> (including patch) to make the instantiation of BugWatchUpdater less of
> a pain (and to preserve the encapsulation that WorkingBase gives).
> 

Cool. Replies in-line, etc...

> 
> > === modified file 'lib/lp/bugs/doc/externalbugtracker-comment-imports.txt'
> > --- lib/lp/bugs/doc/externalbugtracker-comment-imports.txt	2010-04-21 10:30:24 +0000
> > +++ lib/lp/bugs/doc/externalbugtracker-comment-imports.txt	2010-04-23 09:29:42 +0000
> > @@ -85,10 +85,13 @@
> >  comment-importing ExternalBugTracker instance will result in the three
> >  comments in the comment_dict being imported into Launchpad.
> >
> > -    >>> from lp.bugs.scripts.checkwatches import CheckwatchesMaster
> > -    >>> bugwatch_updater = CheckwatchesMaster(LaunchpadZopelessLayer.txn)
> > -    >>> transaction.commit()
> > -    >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch)
> > +    >>> from canonical.launchpad.scripts.logger import log
> > +    >>> from lp.bugs.scripts.checkwatches.bugwatchupdater import (
> > +    ...     BugWatchUpdater)
> > +    >>> bugwatch_updater = BugWatchUpdater(
> > +    ...     '<email address hidden>', LaunchpadZopelessLayer.txn,
> 
> I think it's confusing to use LaunchpadZopelessLayer.txn in some
> places and transaction in others. ZopelessTransactionManager actually
> just calls through to transaction, so it's easier to just use
> transaction everywhere (or transaction.manager; the statement_logging
> context manager needs the real manager). Also, I irrationally loathe
> the "txn" spelling, but that should have no bearing on this review :)
> 

I'll switch everything to use transaction, then.

> '<email address hidden>' could be obtained from
> lp.bugs.scripts.checkwatches.core.LOGIN, but it's not necessary.
> 

I'll leave it as-is for now, then.

> > +    ...     log, bug_watch, external_bugtracker)
> > +    >>> bugwatch_updater.importBugComments()
> >      INFO:...:Imported 3 comments for remote bug 123456 on ...
> >
> >  These three comments will be linked to the bug watch from which they
> > @@ -118,7 +121,7 @@
> >
> >      >>> transaction.commit()
> >
> > -    >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch)
> > +    >>> bugwatch_updater.importBugComments()
> >      INFO:...:Imported 1 comments for remote bug 123456 on ...
> >
> >  Once again, the newly-imported comment will be linked to the bug watch
> > @@ -169,7 +172,7 @@
> >
> >      >>> transaction.commit()
> >
> > -    >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch)
> > +    >>> bugwatch_updater.importBugComments()
> >      INFO:...:Imported 1 comments for remote bug 123456 on ...
> >
> >      >>> bug.messages[-1].owner.name
> > @@ -187,7 +190,7 @@
> >
> >      >>> transaction.commit()
> >
> > -    >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch)
> > +    >>> bugwatch_updater.importBugComments()
> >      INFO:...:Imported 1 comments for remote bug 123456 on ...
> >
> >      >>> bug.messages[-1].owner.name
> > @@ -212,7 +215,7 @@
> >
> >      >>> transaction.commit()
> >
> > -    >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch)
> > +    >>> bugwatch_updater.importBugComments()
> >      WARNING:...:Unable to import remote comment author. No email address
> >      or display name found. (OOPS-...)
> >      INFO:...:Imported 0 comments for remote bug 123456 on ...
> > @@ -388,7 +391,8 @@
> >      >>> LaunchpadZopelessLayer.switchDbUser(config.checkwatches.dbuser)
> >      >>> external_bugtracker.remote_comments = {
> >      ...     '5':"A comment containing a CVE entry: CVE-1991-9911."}
> > -    >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch)
> > +    >>> bugwatch_updater.bug_watch = bug_watch
> 
> I don't think this is a good idea; I think a new BugWatchUpdater
> should be created here. This is a drag at the moment, but I think
> it'll be easy later on because BugTrackerUpdater can have a factory
> method.

Right; fixed.

> > +    >>> bugwatch_updater.importBugComments()
> 
> Do you think it's worth making importBugComments(), pushBugComments()
> and linkLaunchpadBug() private methods?

For now, I don't think there's much mileage in it. Maybe later when all
the other refactoring is done.

> >      INFO:...:Imported 1 comments for remote bug 123456...
> >
> >      >>> for cve in bug_watch.bug.cves:
> > @@ -410,7 +414,7 @@
> >
> >      >>> transaction.commit()
> >
> > -    >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch)
> > +    >>> bugwatch_updater.importBugComments()
> >      INFO:...:Imported 1 comments for remote bug 123456...
> >
> >      >>> for cve in sorted([cve.displayname for cve in bug_watch.bug.cves]):
> > @@ -460,7 +464,8 @@
> >
> >      >>> transaction.commit()
> >
> > -    >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch)
> > +    >>> bugwatch_updater.bug_watch = bug_watch
> > +    >>> bugwatch_updater.importBugComments()
> >      INFO:...:Imported 2 comments for remote bug 42 ...
> >
> >      >>> notifications = get_new_notifications(bug=bug_watch.bug)
> > @@ -516,7 +521,7 @@
> >
> >      >>> transaction.commit()
> >
> > -    >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch)
> > +    >>> bugwatch_updater.importBugComments()
> >      INFO:...:Imported 2 comments for remote bug 42 ...
> >
> >      >>> notifications = get_new_notifications(bug_watch.bug)
> >
> > === modified file 'lib/lp/bugs/doc/externalbugtracker-comment-pushing.txt'
> > --- lib/lp/bugs/doc/externalbugtracker-comment-pushing.txt	2010-04-21 10:30:24 +0000
> > +++ lib/lp/bugs/doc/externalbugtracker-comment-pushing.txt	2010-04-23 09:29:42 +0000
> > @@ -95,12 +95,14 @@
> >  each Launchpad comment that needs to be pushed to the remote bug
> >  tracker.
> >
> > -    >>> from lp.bugs.scripts.checkwatches import CheckwatchesMaster
> > -    >>> bugwatch_updater = CheckwatchesMaster(LaunchpadZopelessLayer.txn)
> > -
> > -    >>> transaction.commit()
> > -
> > -    >>> bugwatch_updater.pushBugComments(external_bugtracker, bug_watch)
> > +    >>> from canonical.launchpad.scripts.logger import log
> > +    >>> from lp.bugs.scripts.checkwatches.bugwatchupdater import (
> > +    ...     BugWatchUpdater)
> > +    >>> bugwatch_updater = BugWatchUpdater(
> > +    ...     '<email address hidden>', LaunchpadZopelessLayer.txn,
> > +    ...     log, bug_watch, external_bugtracker)
> > +
> > +    >>> bugwatch_updater.pushBugComments()
> >      Comment added as remote comment 1
> >      INFO:...:Pushed 1 comments to remote bug 1234 on ...
> >
> > @@ -122,7 +124,7 @@
> >
> >      >>> transaction.commit()
> >
> > -    >>> bugwatch_updater.pushBugComments(external_bugtracker, bug_watch)
> > +    >>> bugwatch_updater.pushBugComments()
> >      >>> transaction.commit()
> >
> >  If we now check the bug watch's unpushed_comments property, we will
> > @@ -148,7 +150,7 @@
> >      >>> bugmessage_three = bug.linkMessage(message_three, bug_watch)
> >      >>> transaction.commit()
> >
> > -    >>> bugwatch_updater.pushBugComments(external_bugtracker, bug_watch)
> > +    >>> bugwatch_updater.pushBugComments()
> >      Comment added as remote comment 2
> >      Comment added as remote comment 3
> >      INFO:...:Pushed 2 comments to remote bug 1234 on ...
> > @@ -171,7 +173,7 @@
> >      >>> bugmessage_four = bug.linkMessage(message_four)
> >      >>> transaction.commit()
> >
> > -    >>> bugwatch_updater.pushBugComments(external_bugtracker, bug_watch)
> > +    >>> bugwatch_updater.pushBugComments()
> >
> >      >>> print_bug_messages(bug, bug_watch)
> >      1: Pushing, for the purpose of.
> > @@ -222,7 +224,8 @@
> >
> >      >>> transaction.commit()
> >
> > -    >>> bugwatch_updater.importBugComments(external_bugtracker, bug_watch)
> > +    >>> bugwatch_updater.external_bugtracker = external_bugtracker
> > +    >>> bugwatch_updater.importBugComments()
> >      INFO:...:Imported 3 comments for remote bug 1234 on ...
> >
> >  Each of the imported comments has its remote_comment_id field set.
> > @@ -239,9 +242,7 @@
> >  Running pushBugComments() on the external bugtracker won't result in the
> >  comments being pushed because they have already been imported.
> >
> > -    >>> transaction.commit()
> > -
> > -    >>> bugwatch_updater.pushBugComments(external_bugtracker, bug_watch)
> > +    >>> bugwatch_updater.pushBugComments()
> >
> >  If the external bugtracker's addRemoteComment() method returns an
> >  invalid remote comment ID, an error will be raised:
> > @@ -264,8 +265,9 @@
> >      >>> broken_external_bugtracker = ErroringExternalBugTracker(
> >      ...     'http://example.com')
> >
> > -    >>> bugwatch_updater.pushBugComments(
> > -    ...     broken_external_bugtracker, bug_watch)
> > +    >>> bugwatch_updater.bug_watch = bug_watch
> > +    >>> bugwatch_updater.external_bugtracker = broken_external_bugtracker
> > +    >>> bugwatch_updater.pushBugComments()
> >      Traceback (most recent call last):
> >        ...
> >      AssertionError: A remote_comment_id must be specified.
> > @@ -310,8 +312,7 @@
> >  remote server. This allows us to include salient information, such as
> >  the comment author, with the pushed comment.
> >
> > -    >>> formatted_message = bugwatch_updater._formatRemoteComment(
> > -    ...     external_bugtracker, bug_watch, message)
> > +    >>> formatted_message = bugwatch_updater._formatRemoteComment(message)
> >      >>> print formatted_message
> >      Sample Person added the following comment to Launchpad bug #...:
> >      <BLANKLINE>
> > @@ -331,8 +332,8 @@
> >      ...     dirname(__file__), '../tests/testfiles/test_comment_template.txt')
> >      >>> external_bugtracker.comment_template = comment_template
> >
> > -    >>> formatted_message = bugwatch_updater._formatRemoteComment(
> > -    ...     external_bugtracker, bug_watch, message)
> > +    >>> bugwatch_updater.external_bugtracker = external_bugtracker
> > +    >>> formatted_message = bugwatch_updater._formatRemoteComment(message)
> >      >>> print formatted_message
> >      Egg and bacon
> >      Egg, sausage and bacon
> >
> > === modified file 'lib/lp/bugs/doc/externalbugtracker-linking-back.txt'
> > --- lib/lp/bugs/doc/externalbugtracker-linking-back.txt	2010-04-21 10:30:24 +0000
> > +++ lib/lp/bugs/doc/externalbugtracker-linking-back.txt	2010-04-23 09:29:42 +0000
> > @@ -47,13 +47,13 @@
> >      >>> LaunchpadZopelessLayer.switchDbUser('checkwatches')
> >
> >      >>> from lp.bugs.scripts.checkwatches import CheckwatchesMaster
> > -    >>> bug_watch_updater = CheckwatchesMaster(txn)
> > +    >>> checkwatches_master = CheckwatchesMaster(txn)
> >      >>> txn.commit()
> >
> >      >>> external_bugtracker = BackLinkingExternalBugTracker(
> >      ...     'http://example.com/')
> >
> > -    >>> bug_watch_updater.updateBugWatches(external_bugtracker, [bug_watch])
> > +    >>> checkwatches_master.updateBugWatches(external_bugtracker, [bug_watch])
> >      Getting Launchpad id for bug 42
> >      Setting Launchpad id for bug 42
> >
> > @@ -63,17 +63,17 @@
> >  For comment syncing and back-linking to be attempted, bug watches must
> >  be related to a bug task, not just a bug.
> >
> > -    >>> bug_watch_updater.updateBugWatches(
> > +    >>> checkwatches_master.updateBugWatches(
> >      ...     external_bugtracker, [bug_watch_without_bugtask])
> >
> >
> > -== CheckwatchesMaster.linkLaunchpadBug() ==
> > +== BugWatchUpdater.linkLaunchpadBug() ==
> >
> > -The CheckwatchesMaster method that does the work of setting the Launchpad
> > +The BugWatchUpdater method that does the work of setting the Launchpad
> >  bug link is linkLaunchpadBug(). This method first retrieves the
> >  current Launchpad bug ID for the remote bug. If the remote bug is
> >  already linked to a Launchpad bug other than the one that we're trying
> > -to link it to, the CheckwatchesMaster will check that the bug that is
> > +to link it to, the BugWatchUpdater will check that the bug that is
> >  already linked has a valid watch on the remote bug in question. If it
> >  does, the link will remain unchanged. Otherwise it will be updated.
> >
> > @@ -91,13 +91,18 @@
> >
> >      >>> transaction.commit()
> >
> > -    >>> bug_watch_updater.linkLaunchpadBug(
> > -    ...     external_bugtracker, bug_watch_2)
> > +    >>> from canonical.launchpad.scripts.logger import log
> > +    >>> from lp.bugs.scripts.checkwatches.bugwatchupdater import (
> > +    ...     BugWatchUpdater)
> > +    >>> bug_watch_updater = BugWatchUpdater(
> > +    ...     '<email address hidden>', LaunchpadZopelessLayer.txn,
> > +    ...     log, bug_watch, external_bugtracker)
> > +    >>> bug_watch_updater.linkLaunchpadBug()
> >      Getting Launchpad id for bug 42
> >
> >  However, if we set the current Launchpad bug ID on our
> >  BackLinkingExternalBugTracker to a Launchpad bug that doesn't link to
> > -the remote bug, CheckwatchesMaster.linkLaunchpadBug() will call
> > +the remote bug, BugWatchUpdater.linkLaunchpadBug() will call
> >  getLaunchpadBugId() and then, when it discovers that the current
> >  Launchpad bug ID isn't valid, setLaunchpadBugId() to correct the error.
> >
> > @@ -117,7 +122,7 @@
> >
> >      >>> transaction.commit()
> >
> > -    >>> bug_watch_updater.linkLaunchpadBug(external_bugtracker, bug_watch)
> > +    >>> bug_watch_updater.linkLaunchpadBug()
> >      Getting Launchpad id for bug 42
> >      Setting Launchpad id for bug 42
> >
> > @@ -127,6 +132,6 @@
> >  error.
> >
> >      >>> external_bugtracker.last_launchpad_bug_id = 0
> > -    >>> bug_watch_updater.linkLaunchpadBug(external_bugtracker, bug_watch)
> > +    >>> bug_watch_updater.linkLaunchpadBug()
> >      Getting Launchpad id for bug 42
> >      Setting Launchpad id for bug 42
> >
> > === added file 'lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py'
> > --- lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py	1970-01-01 00:00:00 +0000
> > +++ lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py	2010-04-23 09:29:42 +0000
> > @@ -0,0 +1,291 @@
> > +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> > +# GNU Affero General Public License version 3 (see the file LICENSE).
> > +
> > +"""Classes and logic for the checkwatches BugWatchUpdater."""
> > +
> > +from __future__ import with_statement
> > +
> > +__metaclass__ = type
> > +__all__ = [
> > +    'BugWatchUpdater',
> > +    ]
> > +
> > +import sys
> > +
> > +from zope.component import getUtility
> > +from zope.event import notify
> > +
> > +from canonical.database.sqlbase import flush_database_updates
> > +
> > +from canonical.launchpad.helpers import get_email_template
> > +from canonical.launchpad.interfaces.launchpad import (
> > +    ILaunchpadCelebrities, NotFoundError)
> > +from canonical.launchpad.interfaces.message import IMessageSet
> > +from canonical.launchpad.webapp.publisher import canonical_url
> > +
> > +from lazr.lifecycle.event import ObjectCreatedEvent
> > +
> > +from lp.bugs.interfaces.bug import CreateBugParams, IBugSet
> > +from lp.bugs.interfaces.bugwatch import IBugWatchSet
> > +from lp.bugs.scripts.checkwatches.base import (
> > +    WorkingBase, commit_before, with_interaction)
> > +from lp.registry.interfaces.distribution import IDistribution
> > +from lp.registry.interfaces.person import IPersonSet, PersonCreationRationale
> 
> There's a lot of lint in the imports here and in core.py.

Aaaah. Bloody make lint didn't pick it up. Not the first time that's
happened. I'll fix it.

> > +
> > +
> > +class BugWatchUpdater(WorkingBase):
> > +    """Handles the updating of a single BugWatch for checkwatches."""
> > +
> > +    def __init__(self, login, transaction_manager, logger, bug_watch,
> > +                 external_bugtracker):
> > +        super(BugWatchUpdater, self).__init__(
> > +            login, transaction_manager, logger)
> 
> This is really ugly and cumbersome :) Also, I think it's best to keep
> WorkingBase as the place that abstracts this stuff, so I've added an
> initFromParent() method on WorkingBase that cleans this up (imo).
> 
>   Diff: http://paste.ubuntu.com/421040/
> 
>   Branch: lp:~allenap/launchpad/cw-refactor-add-bwu-bug-567793 r10731
> 

I've applied the patch; thanks!

> > +        self.logger = logger
> > +        self.bug_watch = bug_watch
> > +        self.external_bugtracker = external_bugtracker
> > +
> > +    @commit_before
> > +    def updateBugWatch(self, new_remote_status, new_malone_status,
> > +                       new_remote_importance, new_malone_importance,
> > +                       can_import_comments, can_push_comments, can_back_link,
> 
> When there is a BugTrackerUpdater class, these can_* attributes could
> be obtained from the given parent in __init__(), assuming you adopt
> the initFromParent() patch.
> 

Yep, that's my plan.

> > +                       error, oops_id):
> > +        """Update the BugWatch."""
> > +        with self.transaction:
> > +            self.bug_watch.last_error_type = error
> > +            if new_malone_status is not None:
> > +                self.bug_watch.updateStatus(
> > +                    new_remote_status, new_malone_status)
> > +            if new_malone_importance is not None:
> > +                self.bug_watch.updateImportance(
> > +                    new_remote_importance, new_malone_importance)
> > +            # Only sync comments and backlink if there was no
> > +            # earlier error, the local bug isn't a duplicate,
> > +            # *and* if the bug watch is associated with a bug
> > +            # task. This helps us to avoid spamming upstream.
> > +            do_sync = (
> > +                error is None and
> > +                self.bug_watch.bug.duplicateof is None and
> > +                len(self.bug_watch.bugtasks) > 0
> > +                )
> > +
> > +        # XXX: Gavin Panella 2010-04-19 bug=509223:
> > +        # Exception handling is all wrong! If any of these
> > +        # throw an exception, *all* the watches in
> > +        # self.bug_watches, even those that have not errored,
> > +        # will have negative activity added.
> > +        if do_sync:
> > +            if can_import_comments:
> > +                self.importBugComments()
> > +            if can_push_comments:
> > +                self.pushBugComments()
> > +            if can_back_link:
> > +                self.linkLaunchpadBug()
> > +
> > +        with self.transaction:
> > +            self.bug_watch.addActivity(
> > +                result=error, oops_id=oops_id)
> > +
> > +    @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
> > +
> > +        # Construct a list of the comment IDs we want to import; i.e.
> > +        # those which we haven't already imported.
> > +        all_comment_ids = self.external_bugtracker.getCommentIds(
> > +            remote_bug_id)
> > +
> > +        with self.transaction:
> > +            comment_ids_to_import = [
> > +                comment_id for comment_id in all_comment_ids
> > +                if not self.bug_watch.hasComment(comment_id)]
> > +
> > +        self.external_bugtracker.fetchComments(
> > +            remote_bug_id, comment_ids_to_import)
> > +
> > +        with self.transaction:
> > +            previous_imported_comments = (
> > +                self.bug_watch.getImportedBugMessages())
> > +            is_initial_import = previous_imported_comments.count() == 0
> > +            imported_comments = []
> > +
> > +            for comment_id in comment_ids_to_import:
> > +                displayname, email = (
> > +                    self.external_bugtracker.getPosterForComment(
> > +                        remote_bug_id, comment_id))
> > +
> > +                if displayname is None and email is None:
> > +                    # If we don't have a displayname or an email address
> > +                    # then we can't create a Launchpad Person as the author
> > +                    # of this comment. We raise an OOPS and continue.
> > +                    self.warning(
> > +                        "Unable to import remote comment author. No email "
> > +                        "address or display name found.",
> > +                        get_remote_system_oops_properties(
> > +                            self.external_bugtracker),
> > +                        sys.exc_info())
> > +                    continue
> > +
> > +                poster = self.bug_watch.bugtracker.ensurePersonForSelf(
> > +                    displayname, email, PersonCreationRationale.BUGIMPORT,
> > +                    "when importing comments for %s." % self.bug_watch.title)
> > +
> > +                comment_message = (
> > +                    self.external_bugtracker.getMessageForComment(
> > +                        remote_bug_id, comment_id, poster))
> > +
> > +                bug_message = self.bug_watch.addComment(
> > +                    comment_id, comment_message)
> > +                imported_comments.append(bug_message)
> > +
> > +            if len(imported_comments) > 0:
> > +                self.bug_watch_updater = (
> > +                    getUtility(ILaunchpadCelebrities).bug_watch_updater)
> > +                if is_initial_import:
> > +                    notification_text = get_email_template(
> > +                        'bugwatch-initial-comment-import.txt') % dict(
> > +                            num_of_comments=len(imported_comments),
> > +                            bug_watch_url=self.bug_watch.url)
> > +                    comment_text_template = get_email_template(
> > +                        'bugwatch-comment.txt')
> > +
> > +                    for bug_message in imported_comments:
> > +                        comment = bug_message.message
> > +                        notification_text += comment_text_template % dict(
> > +                            comment_date=comment.datecreated.isoformat(),
> > +                            commenter=comment.owner.displayname,
> > +                            comment_text=comment.text_contents,
> > +                            comment_reply_url=canonical_url(comment))
> > +                    notification_message = getUtility(IMessageSet).fromText(
> > +                        subject=self.bug_watch.bug.followup_subject(),
> > +                        content=notification_text,
> > +                        owner=self.bug_watch_updater)
> > +                    self.bug_watch.bug.addCommentNotification(
> > +                        notification_message)
> > +                else:
> > +                    for bug_message in imported_comments:
> > +                        notify(ObjectCreatedEvent(
> > +                            bug_message,
> > +                            user=self.bug_watch_updater))
> > +
> > +            self.logger.info("Imported %(count)i comments for remote bug "
> > +                "%(remotebug)s on %(bugtracker_url)s into Launchpad bug "
> > +                "%(bug_id)s." %
> > +                {'count': len(imported_comments),
> > +                 'remotebug': remote_bug_id,
> > +                 'bugtracker_url': self.external_bugtracker.baseurl,
> > +                 'bug_id': local_bug_id})
> > +
> > +    def _formatRemoteComment(self, message):
> > +        """Format a comment for a remote bugtracker and return it."""
> > +        comment_template = get_email_template(
> > +            self.external_bugtracker.comment_template)
> > +
> > +        return comment_template % {
> > +            'launchpad_bug': self.bug_watch.bug.id,
> > +            'comment_author': message.owner.displayname,
> > +            'comment_body': message.text_contents,
> > +            }
> > +
> > +    @commit_before
> > +    def pushBugComments(self):
> > +        """Push Launchpad comments to the remote bug.
> > +
> > +        :param self.external_bugtracker: An external bugtracker which
> > +            implements `ISupportsCommentPushing`.
> > +        :param self.bug_watch: The bug watch to which the comments should be
> > +            pushed.
> > +        """
> > +        pushed_comments = 0
> > +
> > +        with self.transaction:
> > +            local_bug_id = self.bug_watch.bug.id
> > +            remote_bug_id = self.bug_watch.remotebug
> > +            unpushed_comments = list(self.bug_watch.unpushed_comments)
> > +
> > +        # Loop over the unpushed comments for the bug watch.
> > +        # We only push those comments that haven't been pushed
> > +        # already. We don't push any comments not associated with
> > +        # the bug watch.
> > +        for unpushed_comment in unpushed_comments:
> > +            with self.transaction:
> > +                message = unpushed_comment.message
> > +                message_rfc822msgid = message.rfc822msgid
> > +                # Format the comment so that it includes information
> > +                # about the Launchpad bug.
> > +                formatted_comment = self._formatRemoteComment(message)
> > +
> > +            remote_comment_id = (
> > +                self.external_bugtracker.addRemoteComment(
> > +                    remote_bug_id, formatted_comment,
> > +                    message_rfc822msgid))
> > +
> > +            assert remote_comment_id is not None, (
> > +                "A remote_comment_id must be specified.")
> > +            with self.transaction:
> > +                unpushed_comment.remote_comment_id = remote_comment_id
> > +
> > +            pushed_comments += 1
> > +
> > +        if pushed_comments > 0:
> > +            self.logger.info("Pushed %(count)i comments to remote bug "
> > +                "%(remotebug)s on %(bugtracker_url)s from Launchpad bug "
> > +                "%(bug_id)s" %
> > +                {'count': pushed_comments,
> > +                 'remotebug': remote_bug_id,
> > +                 'bugtracker_url': self.external_bugtracker.baseurl,
> > +                 'bug_id': local_bug_id})
> > +
> > +    @commit_before
> > +    def linkLaunchpadBug(self):
> > +        """Link a Launchpad bug to a given remote bug."""
> > +        with self.transaction:
> > +            local_bug_id = self.bug_watch.bug.id
> > +            local_bug_url = canonical_url(self.bug_watch.bug)
> > +            remote_bug_id = self.bug_watch.remotebug
> > +
> > +        current_launchpad_id = self.external_bugtracker.getLaunchpadBugId(
> > +            remote_bug_id)
> > +
> > +        if current_launchpad_id is None:
> > +            # If no bug is linked to the remote bug, link this one and
> > +            # then stop.
> > +            self.external_bugtracker.setLaunchpadBugId(
> > +                remote_bug_id, local_bug_id, local_bug_url)
> > +            return
> > +
> > +        elif current_launchpad_id == local_bug_id:
> > +            # If the current_launchpad_id is the same as the ID of the bug
> > +            # we're trying to link, we can stop.
> > +            return
> > +
> > +        else:
> > +            # If the current_launchpad_id isn't the same as the one
> > +            # we're trying to link, check that the other bug actually
> > +            # links to the remote bug. If it does, we do nothing, since
> > +            # the first valid link wins. Otherwise we link the bug that
> > +            # we've been passed, overwriting the previous value of the
> > +            # Launchpad bug ID for this remote bug.
> > +            try:
> > +                with self.transaction:
> > +                    other_launchpad_bug = getUtility(IBugSet).get(
> > +                        current_launchpad_id)
> > +                    other_bug_watch = other_launchpad_bug.getBugWatch(
> > +                        self.bug_watch.bugtracker, remote_bug_id)
> > +            except NotFoundError:
> > +                # If we can't find the bug that's referenced by
> > +                # current_launchpad_id we simply set other_self.bug_watch to
> > +                # None so that the Launchpad ID of the remote bug can be
> > +                # set correctly.
> > +                other_bug_watch = None
> > +
> > +            if other_bug_watch is None:
> > +                self.external_bugtracker.setLaunchpadBugId(
> > +                    remote_bug_id, local_bug_id, local_bug_url)
> 
> I haven't looked much at the method above because, as far as I can
> tell, they've just been moved. Let me know if there are changes in
> there I should look at. And sorry for being too lazy to diff them.
> 

No, there are no changes other than ones related to the refactor.

> > +
> >
> > === modified file 'lib/lp/bugs/scripts/checkwatches/core.py'
> > --- lib/lp/bugs/scripts/checkwatches/core.py	2010-04-21 10:33:55 +0000
> > +++ lib/lp/bugs/scripts/checkwatches/core.py	2010-04-23 09:29:42 +0000
> > @@ -59,6 +59,7 @@
> >  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.services.scripts.base import LaunchpadCronScript
> >
> >
> > @@ -895,215 +871,18 @@
> >                  reporter, summary, description, subscribe_owner=False,
> >                  filed_by=getUtility(ILaunchpadCelebrities).bug_watch_updater))
> >          [added_task] = bug.bugtasks
> > -        bug_watch = getUtility(IBugWatchSet).createBugWatch(
> > +        self.bug_watch = getUtility(IBugWatchSet).createBugWatch(
> 
> Gah! Was this just for testing/development? I don't think the bug
> watch should be saved on self.
> 

Search-and-replace error. Fixed.

>[...]
> > === added file 'utilities/less-oops.sh'
> > --- utilities/less-oops.sh	1970-01-01 00:00:00 +0000
> > +++ utilities/less-oops.sh	2010-04-23 09:29:42 +0000
> > @@ -0,0 +1,9 @@
> > +#!/bin/bash
> > +#
> > +# Copyright 2009 Canonical Ltd.  This software is licensed under the
> > +# GNU Affero General Public License version 3 (see the file LICENSE).
> > +#
> > +# Outputs the contents of the OOPS report for a given OOPS ID.
> > +
> > +OOPS_ID=$1
> > +grep -rl "$OOPS_ID" /var/tmp/lperr* | xargs less
> >
> 
> Nice :)
> 

Ta :)

Here's a diff of my changes

=== modified file 'lib/lp/bugs/doc/externalbugtracker-comment-imports.txt'
--- lib/lp/bugs/doc/externalbugtracker-comment-imports.txt	2010-04-22 13:57:12 +0000
+++ lib/lp/bugs/doc/externalbugtracker-comment-imports.txt	2010-04-26 12:01:20 +0000
@@ -86,11 +86,16 @@
 comments in the comment_dict being imported into Launchpad.
 
     >>> from canonical.launchpad.scripts.logger import log
+    >>> from lp.bugs.scripts.checkwatches.base import (
+    ...     WorkingBase)
     >>> from lp.bugs.scripts.checkwatches.bugwatchupdater import (
     ...     BugWatchUpdater)
+
+    >>> working_base = WorkingBase()
+    >>> working_base.init(
+    ...     '<email address hidden>', transaction, log)
     >>> bugwatch_updater = BugWatchUpdater(
-    ...     '<email address hidden>', LaunchpadZopelessLayer.txn,
-    ...     log, bug_watch, external_bugtracker)
+    ...     working_base, bug_watch, external_bugtracker)
     >>> bugwatch_updater.importBugComments()
     INFO:...:Imported 3 comments for remote bug 123456 on ...
 
@@ -391,7 +396,8 @@
     >>> LaunchpadZopelessLayer.switchDbUser(config.checkwatches.dbuser)
     >>> external_bugtracker.remote_comments = {
     ...     '5':"A comment containing a CVE entry: CVE-1991-9911."}
-    >>> bugwatch_updater.bug_watch = bug_watch
+    >>> bugwatch_updater = BugWatchUpdater(
+    ...     working_base, bug_watch, external_bugtracker)
     >>> bugwatch_updater.importBugComments()
     INFO:...:Imported 1 comments for remote bug 123456...
 
@@ -464,7 +470,8 @@
 
     >>> transaction.commit()
 
-    >>> bugwatch_updater.bug_watch = bug_watch
+    >>> bugwatch_updater = BugWatchUpdater(
+    ...     working_base, bug_watch, external_bugtracker)
     >>> bugwatch_updater.importBugComments()
     INFO:...:Imported 2 comments for remote bug 42 ...
 

=== modified file 'lib/lp/bugs/doc/externalbugtracker-comment-pushing.txt'
--- lib/lp/bugs/doc/externalbugtracker-comment-pushing.txt	2010-04-22 13:57:12 +0000
+++ lib/lp/bugs/doc/externalbugtracker-comment-pushing.txt	2010-04-26 12:15:05 +0000
@@ -96,11 +96,16 @@
 tracker.
 
     >>> from canonical.launchpad.scripts.logger import log
+    >>> from lp.bugs.scripts.checkwatches.base import (
+    ...     WorkingBase)
     >>> from lp.bugs.scripts.checkwatches.bugwatchupdater import (
     ...     BugWatchUpdater)
+
+    >>> working_base = WorkingBase()
+    >>> working_base.init(
+    ...     '<email address hidden>', transaction, log)
     >>> bugwatch_updater = BugWatchUpdater(
-    ...     '<email address hidden>', LaunchpadZopelessLayer.txn,
-    ...     log, bug_watch, external_bugtracker)
+    ...     working_base, bug_watch, external_bugtracker)
 
     >>> bugwatch_updater.pushBugComments()
     Comment added as remote comment 1
@@ -265,7 +270,8 @@
     >>> broken_external_bugtracker = ErroringExternalBugTracker(
     ...     'http://example.com')
 
-    >>> bugwatch_updater.bug_watch = bug_watch
+    >>> bugwatch_updater = BugWatchUpdater(
+    ...     working_base, bug_watch, external_bugtracker)
     >>> bugwatch_updater.external_bugtracker = broken_external_bugtracker
     >>> bugwatch_updater.pushBugComments()
     Traceback (most recent call last):

=== modified file 'lib/lp/bugs/doc/externalbugtracker-linking-back.txt'
--- lib/lp/bugs/doc/externalbugtracker-linking-back.txt	2010-04-22 14:57:46 +0000
+++ lib/lp/bugs/doc/externalbugtracker-linking-back.txt	2010-04-26 12:15:05 +0000
@@ -30,7 +30,7 @@
 The methods are called by the CheckwatchesMaster class:
 
     >>> from canonical.testing import LaunchpadZopelessLayer
-    >>> txn = LaunchpadZopelessLayer.txn
+    >>> txn = transaction
 
     >>> LaunchpadZopelessLayer.switchDbUser('launchpad')
 
@@ -92,11 +92,16 @@
     >>> transaction.commit()
 
     >>> from canonical.launchpad.scripts.logger import log
+    >>> from lp.bugs.scripts.checkwatches.base import (
+    ...     WorkingBase)
     >>> from lp.bugs.scripts.checkwatches.bugwatchupdater import (
     ...     BugWatchUpdater)
+
+    >>> working_base = WorkingBase()
+    >>> working_base.init(
+    ...     '<email address hidden>', transaction, log)
     >>> bug_watch_updater = BugWatchUpdater(
-    ...     '<email address hidden>', LaunchpadZopelessLayer.txn,
-    ...     log, bug_watch, external_bugtracker)
+    ...     working_base, bug_watch, external_bugtracker)
     >>> bug_watch_updater.linkLaunchpadBug()
     Getting Launchpad id for bug 42
 

=== modified file 'lib/lp/bugs/scripts/checkwatches/base.py'
--- lib/lp/bugs/scripts/checkwatches/base.py	2010-04-21 08:58:25 +0000
+++ lib/lp/bugs/scripts/checkwatches/base.py	2010-04-26 10:13:13 +0000
@@ -119,7 +119,7 @@
 class WorkingBase:
     """A base class for writing a long-running process."""
 
-    def __init__(self, login, transaction_manager, logger):
+    def init(self, login, transaction_manager, logger):
         self._login = login
         self._principal = (
             getUtility(IPlacelessAuthUtility).getPrincipalByLogin(
@@ -127,6 +127,12 @@
         self._transaction_manager = transaction_manager
         self.logger = logger
 
+    def initFromParent(self, parent):
+        self._login = parent._login
+        self._principal = parent._principal
+        self._transaction_manager = parent._transaction_manager
+        self.logger = parent.logger
+
     @property
     @contextmanager
     def interaction(self):

=== modified file 'lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py'
--- lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py	2010-04-22 13:57:12 +0000
+++ lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py	2010-04-26 12:34:47 +0000
@@ -15,8 +15,6 @@
 from zope.component import getUtility
 from zope.event import notify
 
-from canonical.database.sqlbase import flush_database_updates
-
 from canonical.launchpad.helpers import get_email_template
 from canonical.launchpad.interfaces.launchpad import (
     ILaunchpadCelebrities, NotFoundError)
@@ -25,22 +23,17 @@
 
 from lazr.lifecycle.event import ObjectCreatedEvent
 
-from lp.bugs.interfaces.bug import CreateBugParams, IBugSet
-from lp.bugs.interfaces.bugwatch import IBugWatchSet
+from lp.bugs.interfaces.bug import IBugSet
 from lp.bugs.scripts.checkwatches.base import (
-    WorkingBase, commit_before, with_interaction)
-from lp.registry.interfaces.distribution import IDistribution
-from lp.registry.interfaces.person import IPersonSet, PersonCreationRationale
+    WorkingBase, commit_before)
+from lp.registry.interfaces.person import PersonCreationRationale
 
 
 class BugWatchUpdater(WorkingBase):
     """Handles the updating of a single BugWatch for checkwatches."""
 
-    def __init__(self, login, transaction_manager, logger, bug_watch,
-                 external_bugtracker):
-        super(BugWatchUpdater, self).__init__(
-            login, transaction_manager, logger)
-        self.logger = logger
+    def __init__(self, parent, bug_watch, external_bugtracker):
+        self.initFromParent(parent)
         self.bug_watch = bug_watch
         self.external_bugtracker = external_bugtracker
 

=== modified file 'lib/lp/bugs/scripts/checkwatches/core.py'
--- lib/lp/bugs/scripts/checkwatches/core.py	2010-04-22 09:49:26 +0000
+++ lib/lp/bugs/scripts/checkwatches/core.py	2010-04-26 12:34:47 +0000
@@ -33,21 +33,15 @@
 from twisted.python.threadpool import ThreadPool
 
 from zope.component import getUtility
-from zope.event import notify
 
 from canonical.database.constants import UTC_NOW
 from canonical.database.sqlbase import flush_database_updates
-from lazr.lifecycle.event import ObjectCreatedEvent
-from canonical.launchpad.helpers import get_email_template
 from canonical.launchpad.interfaces import (
     BugTaskStatus, BugWatchActivityStatus, CreateBugParams,
     IBugTrackerSet, IBugWatchSet, IDistribution, ILaunchpadCelebrities,
     IPersonSet, ISupportsCommentImport, ISupportsCommentPushing,
     PersonCreationRationale, UNKNOWN_REMOTE_STATUS)
-from canonical.launchpad.interfaces.launchpad import NotFoundError
-from canonical.launchpad.interfaces.message import IMessageSet
 from canonical.launchpad.scripts.logger import log as default_log
-from canonical.launchpad.webapp.publisher import canonical_url
 
 from lp.bugs import externalbugtracker
 from lp.bugs.externalbugtracker import (
@@ -55,7 +49,6 @@
     BugWatchUpdateError, InvalidBugId, PrivateRemoteBug,
     UnknownBugTrackerTypeError, UnknownRemoteStatusError, UnparseableBugData,
     UnparseableBugTrackerVersion, UnsupportedBugTrackerVersion)
-from lp.bugs.interfaces.bug import IBugSet
 from lp.bugs.interfaces.externalbugtracker import ISupportsBackLinking
 from lp.bugs.scripts.checkwatches.base import (
     WorkingBase, commit_before, with_interaction)
@@ -167,8 +160,7 @@
             provides a similar interface.
 
         """
-        super(CheckwatchesMaster, self).__init__(
-            LOGIN, transaction_manager, logger)
+        self.init(LOGIN, transaction_manager, logger)
 
         # Override SYNCABLE_GNOME_PRODUCTS if necessary.
         if syncable_gnome_products is not None:
@@ -793,13 +785,13 @@
                             ('URL', remote_bug_url),
                             ('bug_id', remote_bug_id),
                             ('local_ids', local_ids),
-                            ] + get_remote_system_oops_properties(remotesystem),
+                            ] + get_remote_system_oops_properties(
+                                remotesystem),
                         info=sys.exc_info())
 
                 for bug_watch in bug_watches:
                     bug_watch_updater = BugWatchUpdater(
-                        LOGIN, self._transaction_manager,
-                        self.logger, bug_watch, remotesystem)
+                        self, bug_watch, remotesystem)
 
                     bug_watch_updater.updateBugWatch(
                         new_remote_status, new_malone_status,
@@ -871,12 +863,12 @@
                 reporter, summary, description, subscribe_owner=False,
                 filed_by=getUtility(ILaunchpadCelebrities).bug_watch_updater))
         [added_task] = bug.bugtasks
-        self.bug_watch = getUtility(IBugWatchSet).createBugWatch(
+        bug_watch = getUtility(IBugWatchSet).createBugWatch(
             bug=bug,
             owner=getUtility(ILaunchpadCelebrities).bug_watch_updater,
             bugtracker=bugtracker, remotebug=remote_bug)
 
-        added_task.bugwatch = self.bug_watch
+        added_task.bugwatch = bug_watch
         # Need to flush databse updates, so that the bug watch knows it
         # is linked from a bug task.
         flush_database_updates()

=== modified file 'lib/lp/bugs/scripts/checkwatches/tests/test_base.py'
--- lib/lp/bugs/scripts/checkwatches/tests/test_base.py	2010-04-21 08:42:13 +0000
+++ lib/lp/bugs/scripts/checkwatches/tests/test_base.py	2010-04-26 10:13:13 +0000
@@ -48,7 +48,8 @@
     def test_interaction(self):
         # The WorkingBase.interaction context manager will begin an
         # interaction on entry and end it on exit.
-        base = WorkingBase(self.email, transaction.manager, self.logger)
+        base = WorkingBase()
+        base.init(self.email, transaction.manager, self.logger)
         endInteraction()
         self.assertIs(None, queryInteraction())
         with base.interaction:
@@ -59,7 +60,8 @@
         # If an interaction is already in progress, the interaction
         # context manager will not begin a new interaction on entry,
         # nor will it end the interaction on exit.
-        base = WorkingBase(self.email, transaction.manager, self.logger)
+        base = WorkingBase()
+        base.init(self.email, transaction.manager, self.logger)
         endInteraction()
         self.assertIs(None, queryInteraction())
         with base.interaction:
@@ -74,7 +76,8 @@
         # transaction is in progress on entry, commits on a successful
         # exit, or aborts the transaction on failure.
         transaction_manager = StubTransactionManager()
-        base = WorkingBase(self.email, transaction_manager, self.logger)
+        base = WorkingBase()
+        base.init(self.email, transaction_manager, self.logger)
         transaction.commit()
         with base.transaction:
             self.assertFalse(is_transaction_in_progress())
@@ -88,7 +91,8 @@
         # On entry, WorkingBase.transaction will raise an exception if
         # a transaction is in progress.
         transaction_manager = StubTransactionManager()
-        base = WorkingBase(self.email, transaction_manager, self.logger)
+        base = WorkingBase()
+        base.init(self.email, transaction_manager, self.logger)
         self.assertTrue(is_transaction_in_progress())
         self.assertRaises(TransactionInProgress, base.transaction.__enter__)
 
@@ -97,7 +101,8 @@
         # manager is active, the transaction will be aborted and the
         # exception re-raised.
         transaction_manager = StubTransactionManager()
-        base = WorkingBase(self.email, transaction_manager, self.logger)
+        base = WorkingBase()
+        base.init(self.email, transaction_manager, self.logger)
         transaction.commit()
         try:
             with base.transaction:
@@ -111,7 +116,8 @@
     def test_statement_logging(self):
         # The WorkingBase.statement_logging context manager starts
         # statement logging on entry and stops it on exit.
-        base = WorkingBase(self.email, transaction.manager, self.logger)
+        base = WorkingBase()
+        base.init(self.email, transaction.manager, self.logger)
         self.factory.makeEmail('<email address hidden>', self.person)
         self.assertEqual(
             0, len(get_request_statements()),
@@ -130,6 +136,13 @@
             "SQL statement log not cleared on exit "
             "from base.statement_logging.")
 
+    def test_initFromParent(self):
+        base1 = WorkingBase()
+        base1.init(self.email, transaction.manager, self.logger)
+        base2 = WorkingBase()
+        base2.initFromParent(base1)
+        self.failUnlessEqual(base1.__dict__, base2.__dict__)
+
 
 class TestWorkingBaseErrorReporting(TestCaseWithFactory):
 
@@ -140,7 +153,8 @@
         person = self.factory.makePerson()
         email = person.preferredemail.email
         logger = QuietFakeLogger()
-        base = WorkingBase(email, transaction.manager, logger)
+        base = WorkingBase()
+        base.init(email, transaction.manager, logger)
         with base.statement_logging:
             self.factory.makeEmail('<email address hidden>', person)
             self.assertTrue(