Merge lp:~wallyworld/launchpad/sharee-display-details-1002954 into lp:launchpad

[Ian Booth]

Display team icon and launchpad name of grantee in sharing table.

== Implementation ==

Add the icon url to the json data sent when the client asks for grantee data to display.
Update the mustache template used to render the grantee row.
The table now shows the icon (if there is one) plus the Luanchpad id of the grantee.

== Demo and QA ==

http://people.canonical.com/~ianb/sharee-table-icons.png

== Tests ==

Update shareetable yui test.
Update sharing service jsonData tests.
Add 1 to view query count tests since there is a new query to cache the icon info.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/browser/tests/test_pillar_sharing.py
  lib/lp/registry/javascript/sharing/shareetable.js
  lib/lp/registry/javascript/sharing/tests/test_shareetable.js
  lib/lp/registry/services/sharingservice.py
  lib/lp/registry/services/tests/test_sharingservice.py

[William Grant (wgrant)]

137 + grantee_ids = [grantee[0].id for grantee in grant_permissions]

This is not an entirely insignificant change: it causes grant_permissions to be iterated before the main loop. It looks like all callsites currently pass in a list, but if it happens to be a ResultSet then the pre-consumption will cause jsonShareeData to return an empty list. Hopefully not a concern here, though.

148 + image_url = display_api.custom_icon_url() or ''

I think it feels slightly nicer to use None instead of ''. I'd also consider s/image/icon/.

149 + sprite_css = display_api.sprite_css() or 'sprite bullet'

Can sprite_css for a person or team ever return something that evaluates to false? This seems like over-nice handling of a case that should at best crash.

review: Approve (code)

[Ian Booth (wallyworld)]

On 30/05/12 17:35, William Grant wrote:
> Review: Approve code
>
> 137 + grantee_ids = [grantee[0].id for grantee in grant_permissions]
>
> This is not an entirely insignificant change: it causes grant_permissions to be iterated before the main loop. It looks like all callsites currently pass in a list, but if it happens to be a ResultSet then the pre-consumption will cause jsonShareeData to return an empty list. Hopefully not a concern here, though.
>

Yes, this method is to be considered internal to the service and is
meant to be called with a list representing the current batch of results
to be rendered.

> 148 + image_url = display_api.custom_icon_url() or ''
>
> I think it feels slightly nicer to use None instead of ''. I'd also consider s/image/icon/.
>

Ok. I'll make that change.

> 149 + sprite_css = display_api.sprite_css() or 'sprite bullet'
>
> Can sprite_css for a person or team ever return something that evaluates to false? This seems like over-nice handling of a case that should at best crash.

This code was taken from the TAL formatter but I guess we are always
guaranteed to be operating with a person or team so it can be simplified.