Code review comment for lp:~ursinha/launchpad/bug-391569-add-last-changed-column

Hi Ursula,

Nice branch! I've got a few comments but they're all stylistic nitpicks, so I'm happy to approve this with the changes I've requested.

> === modified file 'lib/lp/registry/model/productseries.py'
> --- lib/lp/registry/model/productseries.py 2009-10-16 15:00:55 +0000
> +++ lib/lp/registry/model/productseries.py 2009-11-03 15:00:31 +0000
> @@ -526,10 +528,21 @@
> POTemplate.iscurrent==True,
> Language.id!=english.id).group_by(Language)
>
> - for (language, imported, changed, new, unreviewed) in (
> - query.order_by(['Language.englishname'])):
> + # XXX: Ursinha 2009-11-02: The Max(POFile.date_changed) result
> + # here is a naive datetime. My guess is that it happens
> + # because UTC awareness is attibuted to the field in the POFile
> + # model class, and in this case the Max function deals directly
> + # with the value returned from the database without
> + # instantiating it.
> + # This seems to be irrelevant to what we're trying to achieve
> + # here, but making a note either way.
> +
> + for (language, imported, changed, new, unreviewed,
> + last_changed) in (
> + query.order_by(['Language.englishname'])):

This is a little hard to read. Maybe something like:

    ordered_results = query.order_by(['Language.englishname'])
    for (language, imported, changed, new, unreviewed,
        last_changed) in ordered_results:
        ...

Would be a little easier.

Also note that the indent on long loop definitions should still be four
spaces.

> psl = ProductSeriesLanguage(self, language)
> - psl.setCounts(total, imported, changed, new, unreviewed)
> + psl.setCounts(total, imported, changed, new, unreviewed,
> + last_changed)

When we wrap long method calls, we should wrap them like this:

    psl.setCounts(
        total, imported, changed, new, unreviewed, last_changed)

> results.append(psl)
>
> return results
>
> === modified file 'lib/lp/translations/tests/test_productserieslanguage.py'
> --- lib/lp/translations/tests/test_productserieslanguage.py 2009-09-12 16:07:05 +0000
> +++ lib/lp/translations/tests/test_productserieslanguage.py 2009-11-03 15:00:31 +0000
> @@ -46,6 +46,7 @@
> # Add a Serbian translation.
> serbian = getUtility(ILanguageSet).getLanguageByCode('sr')
> sr_pofile = self.factory.makePOFile(serbian.code, potemplate)
> +
> psls = list(self.productseries.productserieslanguages)
> self.assertEquals(len(psls), 1)
>
> @@ -111,7 +112,8 @@
> removeSecurityProxy(potemplate).messagecount = number_of_potmsgsets
> return potemplate
>
> - def setPOFileStatistics(self, pofile, imported, changed, new, unreviewed):
> + def setPOFileStatistics(self, pofile, imported, changed, new, unreviewed,
> + date_changed):

The wrapped lines of a method definition should start under the first
argument, thus:

    def setPOFileStatistics(self, pofile, imported, changed, new, unreviewed,
                            date_changed):

> # Instead of creating all relevant translation messages, we
> # just fake cached statistics instead.
> naked_pofile = removeSecurityProxy(pofile)
> @@ -136,20 +139,22 @@
> psl.currentCount(),
> psl.rosettaCount(),
> psl.updatesCount(),
> - psl.unreviewedCount()),
> + psl.unreviewedCount(),
> + psl.last_changed_date),
> stats)
>
> def test_DummyProductSeriesLanguage(self):
> # With no templates all counts are zero.
> psl = self.psl_set.getDummy(self.productseries, self.language)
> self.failUnless(verifyObject(IProductSeriesLanguage, psl))
> - self.assertPSLStatistics(psl, (0, 0, 0, 0, 0, 0))
> + self.assertPSLStatistics(psl, (0, 0, 0, 0, 0, 0, None))
>
> # Adding a single template with 10 messages makes the total
> # count of messages go up to 10.
> potemplate = self.createPOTemplateWithPOTMsgSets(10)
> psl = self.psl_set.getDummy(self.productseries, self.language)
> - self.assertPSLStatistics(psl, (10, 0, 0, 0, 0, 0))
> + self.assertPSLStatistics(psl, (10, 0, 0, 0, 0, 0,
> + None))

Again, this should be wrapped as:

        self.assertPSLStatistics(
            psl, (10, 0, 0, 0, 0, 0, None))

>
> def test_OneTemplate(self):
> # With only one template, statistics match those of the POFile.