Code review comment for lp:~sinzui/launchpad/oil-and-pigment

Hi Curtis,

overall, a nice branch. I have though a few qestions regarding the
filtering of inactive projects for the karma display, so I'm marking
the MP as "needs information".

> === modified file 'lib/lp/registry/model/person.py'
> --- lib/lp/registry/model/person.py 2010-04-19 21:16:12 +0000
> +++ lib/lp/registry/model/person.py 2010-04-22 18:05:27 +0000
> @@ -850,12 +850,16 @@
> def getProjectsAndCategoriesContributedTo(self, limit=5):
> """See `IPerson`."""
> contributions = []
> - results = self._getProjectsWithTheMostKarma(limit=limit)
> - for pillar_name, karma in results:
> + # Pillars names have no concept of active. Extra pillars names are
> + # requested because deactivated pillars will be filtered out.
> + extra_limit = limit + 5
> + results = self._getProjectsWithTheMostKarma(limit=extra_limit)
> + for pillar_name, karma in results[:limit]:

While you retrieve 5 extra items calling _getProjectsWithTheMostKarma(),
you remove these extra items here ;)

> pillar = getUtility(IPillarNameSet).getByName(pillar_name)
> - contributions.append(
> - {'project': pillar,
> - 'categories': self._getContributedCategories(pillar)})
> + if pillar.active:
> + contributions.append(
> + {'project': pillar,
> + 'categories': self._getContributedCategories(pillar)})
> return contributions

I think that should be

          return contributions[:limit]

combined with a loop over the full result set.

But: Did you consider to change the SQL query in the method
_getProjectsWithTheMostKarma() itself so that only activ projects
are returned (if necessary, using an optional method parameter
"active_only=False")?

While adding five extra items to the result set looks reasonable,
there is the (admittedly largely theoretical) situation that
the result set contains more than five inactive projects, so that
the length of the "effective" list is shorter than expected.

And I would prefer it anyway to do the filtering in
_getProjectsWithTheMostKarma(), even if it would be too cumbersome
to implement in SQL.

[...]

> === modified file 'lib/lp/registry/tests/test_person.py'
> --- lib/lp/registry/tests/test_person.py 2010-02-11 19:26:26 +0000
> +++ lib/lp/registry/tests/test_person.py 2010-04-22 18:05:27 +0000

[...]

> @@ -597,5 +601,103 @@
> assignee=self.user)
>
>
> +class TestPersonKarma(TestCaseWithFactory):
> +
> + layer = DatabaseFunctionalLayer
> +
> + def setUp(self):
> + super(TestPersonKarma, self).setUp()
> + self.person = self.factory.makePerson()
> + a_product = self.factory.makeProduct(name='aa')
> + b_product = self.factory.makeProduct(name='bb')
> + self.c_product = self.factory.makeProduct(name='cc')
> + self.cache_manager = getUtility(IKarmaCacheManager)
> + self._makeKarmaCache(
> + self.person, a_product, [('bugs', 10)])
> + self._makeKarmaCache(
> + self.person, b_product, [('answers', 50)])
> + self._makeKarmaCache(
> + self.person, self.c_product, [('code', 100), (('bugs', 50))])
> +
> + def _makeKarmaCache(self, person, product, category_name_values):
> + """Create a KarmaCache entry with the given arguments.
> +
> + In order to create the KarmaCache record we must switch to the DB
> + user 'karma'. This requires a commit and invalidates the product
> + instance.
> + """
> + transaction.commit()
> + reconnect_stores('karmacacheupdater')
> + total = 0
> + # Insert category total for person and project.
> + for category_name_value in category_name_values:
> + category_name, value = category_name_value

Just an aesthetical remark, but I would prefer a simple

           for category_name, value in category_name_values: