Code review comment for lp:~allenap/launchpad/checkwatches-stuff

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().