Code review comment for lp:~jamalta/launchpad/bug-127489

Hi Jamal,

This branch looks really good, but I'm not quite comfortable having the makeKarmaCache() method do a switchDBUser('launchpad') after the entry is created, as a test calling it might have previously switched to a different DB user and not expect it to be changed to 'launchpad' after the method is called. However, I don't see a significantly better way of creating the KarmaCache entries without doing this, so I think we should rename the method to _makeKarmaCache() to make it clear that it's an internal API, drop the last switchDBUser() call and add a docstring to it, mentioning that after completion, the current transaction will be using the 'karma' db user and callsites must deal with it if they need. I also think we should have a comment explaining why the commit is necessary. Something like

    def _makeKarmaCache(...):
        """Create and return a KarmaCache entry with the given arguments.

        In order to create a KarmaCache entry we'll switch the DB user 'karma',
        so tests that need a different user after calling this method should do
        a switchDBUser() themselves.
        """
        ...
        # Must commit here so that the change is seen in other transactions
        # (e.g. when the callsite issues a switchDBUser() after we return).
        LaunchpadZopelessLayer.commit()
        return karmacache

Also, please run the 'make lint' command as I see some unused imports in test_person_view.py. That command will tell you which imports are unused (among other things) and then you can remove them.

Cheers,
Guilherme