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