Merge lp://staging/~sinzui/launchpad/oil-and-pigment into lp://staging/launchpad

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....

On Fri, 2010-04-23 at 13:41 +0000, Abel Deuring wrote:
> Review: Needs Information code
> 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 ;)

I am incompetent. I removed the limit on the iterator. I added a guard
in the loop to break at the limit.

> > 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.

I prefer a guard in the loop. The loop may make 5 extra SQL queries for
data that will not be used. _getContributedCategories() is a call to the
db.

> 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")?

Yes, the query would insane because it is often nothing to join on. We
are querying pillar names, not objects. that is why our code always
filters *after* the pillar_name is used to get the pillar (product,
distribution, project group)

> 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.

Yes. This hack is the same approach taken when getting a short list of
bugs that may have private ones filtered out. The complaints from users
is about 1 deactivated project in the list. We are providing coverage
for 5 deactivated projects.

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

I do not think that is practical. As I said above, ...

OK, looks good now. (Though I would still prefer to move the filtering to _getprojectsWithMostKarma(), even if it is implemented in Python)