Code review comment for lp:~jcsackett/launchpad/projectgroup-branches-652156

Hi Jon,

This looks good to me; I only have a couple comments below.

 review approve ui*

On Wed, 2010-10-13 at 18:49 +0000, j.c.sackett wrote:
>
> Preimplementation talk
> ======================
>
> Spoke with Edwin Grubbs. We brought up the possiblity of linking out
> to all the products for the group so someone could configure the
> products, but to do so sanely (i.e. only show the links for products
> the viewer can configure) requires doing database checks for every
> product; this could lead to death by SQL timeouts.

I think that a project group that doesn't have a single project using LP
for hosting code is unlikely to have more than a handful projects, so I
think we could afford these extra SQL queries in the name of a better
user experience.

That said, it's fine if you want to implement that in a separate branch,
as I might be wrong.

> === modified file 'lib/lp/code/templates/project-branches.pt'
> --- lib/lp/code/templates/project-branches.pt 2009-09-17 00:27:40 +0000
> +++ lib/lp/code/templates/project-branches.pt 2010-10-13 18:43:40 +0000
> @@ -17,8 +17,17 @@
> tal:condition="link/enabled"
> tal:content="structure link/render" />
> </div>
> -
> - <tal:branchlisting content="structure branches/@@+branch-listing" />
> + <tal:no-branches
> + condition="not:context/has_branches">
> + <div id="no-branchtable">
> + <strong>None of <tal:project replace="context/displayname"/>'s

I've read somewhere
(<http://en.wikipedia.org/wiki/Apostrophe#cite_note-13>, I think) that
to form the possessive we should add an 's regardless of the final
consonant, and your change is in line with that, but I'm not sure that's
the preferred style in Launchpad (in case we have one), so I'd like to
know what Curtis thinks.

> + projects are using Launchpad to host their code.</strong>
> + </div>
> + </tal:no-branches>

> === modified file 'lib/lp/registry/model/projectgroup.py'
> --- lib/lp/registry/model/projectgroup.py 2010-10-07 13:26:53 +0000
> +++ lib/lp/registry/model/projectgroup.py 2010-10-13 18:43:40 +0000
> @@ -222,6 +222,10 @@
> """See `IProjectGroup`."""
> return self.translatables().count() != 0
>
> + def has_branches(self):
> + """ See `IProjectGroup`."""
> + return self.getBranches().count() != 0

Can you use .is_empty() instead of .count() here? It is faster and we
don't really need the count here.
(I know this was supposed to be a UI review, but I thought it was worth
mentioning)

> +
> def _getBaseQueryAndClauseTablesForQueryingSprints(self):
> query = """
> Product.project = %s
>

--
Guilherme Salgado <https://launchpad.net/~salgado>