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

Hi Jamal,

Thanks a lot for fixing this bug, and going through the effort for
writing a test for the fix. The bug itself was indeed easy to fix, but
now it's quite clear the test for it was anything but easy.

I have some suggestions for improvement below, most of them are stylish
though and they should all be easy to deal with. If you have any
comments/questions, just ping me on IRC or leave a comment here.

Cheers,

 review needs-fixing

On Thu, 2009-08-27 at 03:20 +0000, Jamal Fanaian wrote:
> === modified file 'lib/lp/registry/browser/person.py'
> --- lib/lp/registry/browser/person.py 2009-07-30 22:35:16 +0000
> +++ lib/lp/registry/browser/person.py 2009-08-05 19:55:13 +0000
> @@ -2517,7 +2517,9 @@
> categories = set()
> for contrib in self.contributions:
> categories.update(category for category in contrib['categories'])
> - return sorted(categories, key=attrgetter('title'))
> + sort = {'code': 0, 'bugs': 1, 'blueprints': 2, 'translations': 3,
> + 'answers': 4, 'specs': 5, 'soyuz': 6}
> + return sorted(categories, key=lambda category: sort[category.name])
>
> @cachedproperty
> def context_is_probably_a_team(self):
>
> === added file 'lib/lp/registry/browser/tests/test_person_view.py'
> --- lib/lp/registry/browser/tests/test_person_view.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/browser/tests/test_person_view.py 2009-08-27 02:59:16 +0000
> @@ -0,0 +1,71 @@
> +# Copyright 2009 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +__metaclass__ = type
> +
> +import unittest
> +
> +from canonical.config import config
> +from canonical.launchpad.webapp.servers import LaunchpadTestRequest
> +from canonical.testing import LaunchpadZopelessLayer
> +from canonical.database.sqlbase import cursor, sqlvalues
> +from lp.registry.browser.person import PersonView
> +from lp.testing import TestCaseWithFactory
> +from zope.component import getUtility
> +from canonical.launchpad.interfaces import IKarmaCacheManager
> +

There should be one extra blank line here. Our style guide[1] has this
and other conventions we use.

[1] https://dev.launchpad.net/PythonStyleGuide

> +class TestPerson(TestCaseWithFactory):

TestPersonView would be a better name for this.

> + """Test Person view."""

Since the docstring is nearly identical to the class' name, we can get
rid of it.

> +
> + layer = LaunchpadZopelessLayer
> +
> + def setUp(self):
> + # Create a person

This comment doesn't add much either, so it can be removed.

> + TestCaseWithFactory.setUp(self)
> + self.person = self.factory.makePerson()
> + self.view = PersonView(self.person,
> + LaunchpadTestRequest())
> +
> + def test_karma_category_sort(self):
> + # Add karma to some categories for the user

Same here.

> + self.makeKarmaCache(person=self.person, category_id=2)
> + self.makeKarmaCache(person=self.person, category_id=7)
> + self.makeKarmaCache(person=self.person, category_id=8)

I'd move these lines to the setUp() method above.

> +
> + # Get contributed categories for the factory user

I'd drop this comment too.

> + categories = self.view.contributed_categories
> + category_names = []
> + for category in categories:
> + category_names.append(category.name)
> +
> + # Assert that the contributed categories are sorted correctly

And this one as well.

> + self.assertEqual(category_names, [u'code', u'bugs', u'answers'],
> + 'Categories are not sorted correctly')
> +
> + def makeKarmaCache(self, person=None, value=10, category_id=2,

Since this method will fail if a callsite doesn't pass in a person, it
doesn't make sense to specify a default value for the 'person' arg.

I don't see any reason for having a default value for category_id,
either -- it should be a required argument. Also, the argument should
be called 'category' and callsites should pass an IKarmaCategory object
instead of its id.

> + product_id=5):

Similarly, this should be renamed to 'product' and the method must
expect an IProduct instead of an integer. You can specify None as the
default value for that argument and have this method create a product
(using self.factory) when the callsite doesn't specify one.

> + # Create KarmaCache Record

This comment doesn't add much either, but you could add one explaining
that it's only the karmacacheupdater who has write access to the
KarmaCache table, so we switch to that user here.

> + LaunchpadZopelessLayer.switchDbUser(config.karmacacheupdater.dbuser)
> + karmacache = getUtility(IKarmaCacheManager).new(
> + person_id=person.id, value=value, category_id=category_id,
> + product_id=product_id)

After the changes to this method's arguments, this is what the above
would look like

    if product is None:
        product = self.factory.makeProduct()
    karmacache = getUtility(IKarmaCacheManager).new(
        value, person.id, category.id, product_id=product.id)

> + LaunchpadZopelessLayer.commit()
> +
> + # Update totals for this product
> + query = """
> + INSERT INTO KarmaCache
> + (person, category, karmavalue, product, distribution,
> + sourcepackagename, project)
> + SELECT person, NULL, SUM(karmavalue), product, NULL, NULL, NULL
> + FROM KarmaCache
> + WHERE product = %(product)s
> + AND person = %(person)s
> + GROUP BY person, product
> + """ % sqlvalues(person=person.id, product=product_id)
> + cur = cursor()
> + cur.execute(query)

Instead of doing this with raw SQL, you could do the following:

    try:
        cache_manager.updateKarmaValue(
            value, person.id, category_id=None, product_id=product.id)
    except NotFoundError:
        cache_manager.new(
            value, person.id, category_id=None, product_id=product.id)

which is a bit simpler. :)

> +

Here at the end we must also do another switchDbUser() to restore the DB
user that was in use when this method was called. If we don't do that,
other (completely unrelated) tests will fail because the wrong DB user
would be used for them.

> + return karmacache
> +
> +def test_suite():
> + return unittest.TestLoader().loadTestsFromName(__name__)
--
Guilherme Salgado <email address hidden>