Code review comment for lp:~sinzui/launchpad/closed-teams-0

On Thu, 2010-12-02 at 17:12 +0000, Gavin Panella wrote:
> Review: Approve
> Hi Curtis. Some incredibly minor comments, +1.
>
>
> [1]
>
> js = js_template % simplejson.dumps(args)
> # If the YUI widget or javascript is not supported in the browser,
> # it will degrade to being this "Find..." link instead of the
> @@ -154,8 +163,7 @@
> css = ''
> return ('<span class="%s">(<a id="%s" href="/people/">'
> 'Find&hellip;</a>)</span>'
> - '\n<script>\n%s\n</script>'
> - ) % (css, self.show_widget_id, js)
> + '\n<script>\n%s\n</script>') % (css, self.show_widget_id, js)
>
> It's not part of your change, but using simplejson.dumps() can be
> hazardous for embedding javascript into <script> tags. See:
>
> http://code.google.com/p/simplejson/issues/detail?id=66
>
> Instead the following should be used:
>
> simplejson.dumps(args, cls=simplejson.encoder.JSONEncoderForHTML)
>
> Or:
>
> simplejson.encoder.JSONEncoderForHTML().encode(obj)

Interesting. We cannot do this right now because Lp is using 2.0.9. I
can see it my version of 2.1.1

> [2]
>
> + Y.lp.registry.team.setup_add_member_handler(
> + '${step_title}');
>
> The lack of escaping and all that jazz makes me a little uneasy, but
> it's not user-supplied so I'm not too worried.

This content is in a tal:content rule and is escaped. A single-quote
will break the script. I updated the method to escape quotes and added a
test.

> [3]
>
> + def test_open_team_cannot_be_a_member_or_a_closed_team(self):
>
> s/or/of/

fixed

> [4]
>
> + def test_open_team_can_be_a_member_or_an_open_team(self):
>
> s/or/of/

fixed

> [5]
>
> + return 'Search for a restricted or moderated team or person'
>
> This could be parsed as:
>
> Search for a (restricted or moderated) (team or person)
>
> Perhaps the following would be less ambiguous:
>
> Search for a restricted team, a moderated team, or a person

I accept your proposal.

> [6]
>
> + Person.subscriptionpolicy != TeamSubscriptionPolicy.OPEN)
>
> Maybe it's worth taking a defensive approach here (to someone adding
> another TeamSubscriptionPolicy member) instead:
>
> In(Person.subscriptionpolicy, (
> TeamSubscriptionPolicy.RESTRICTED,
> TeamSubscriptionPolicy.MODERATED))

We have not changed the policies in 5 years. I do not think we will be
adding other policies. If I am wrong, we may need to update the proposed
revision. Since this issue is about OPEN, I think the clause should
remain as is.

--
__Curtis C. Hovey_________
http://launchpad.net/