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

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

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

Lint free.

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

There are two other small changes, detailed below.

lib/lp/bugs/doc/externalbugtracker.txt

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

lib/lp/bugs/interfaces/bugwatch.py

  New method getBugWatchesForRemoteBug().

lib/lp/bugs/interfaces/externalbugtracker.py

  Improved interface docstrings.

lib/lp/bugs/model/bugwatch.py

  Implementation of getBugWatchesForRemoteBug().

lib/lp/bugs/scripts/checkwatches.py

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

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

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

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

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

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

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

lib/lp/bugs/tests/test_bugwatch.py

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

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

Hi Gavin,

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

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

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

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

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

Approved pending the removal of the erroneous .moved file.