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

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.
> can_import_comments = (
> @@ -788,6 +775,7 @@
> "local bugs: %(local_ids)s"
> )
>
> +<<<<<<< TREE
> for bug_id in all_remote_ids:
> try:
> bug_watches = bug_watches_by_remote_bug[bug_id]
> @@ -799,9 +787,17 @@
> "remote bug %s on %s" % (bug_id, remotesystem.baseurl))
> continue
>
> +=======
> + for remote_bug_id in all_remote_ids:
> + # Start a fresh transaction every time round the loop.
> + self.txn.begin()
> +
> + bug_watches = self.getBugWatchesForRemoteBug(
> + remote_bug_id, bug_watch_ids)
> +>>>>>>> MERGE-SOURCE

You appear to have committed a conflict (or maybe my diff's out of
date).

> for bug_watch in bug_watches:
> bug_watch.lastchecked = UTC_NOW
> - if bug_id in unmodified_remote_ids:
> + if remote_bug_id in unmodified_remote_ids:
> continue
>
> # Save the remote bug URL in case we need to log an error.
> === renamed file 'lib/canonical/launchpad/scripts/ftests/test_checkwatches.py' => 'lib/lp/bugs/scripts/tests/test_checkwatches.py.moved'
> --- lib/canonical/launchpad/scripts/ftests/test_checkwatches.py 2009-06-25 05:30:52 +0000
> +++ lib/lp/bugs/scripts/tests/test_checkwatches.py.moved 2010-01-04 15:37:12 +0000

It appears that you've committed the .moved file rather than merge it
into the existing python module. However, on IRC you've said this isn't
the case. Can you find out what's gone on and let me know?