code review 3459043: godashboard: support submitting projects with non-ascii...

godashboard: support submitting projects with non-ascii names

Fixes issue 1314.

[rmmh, 2010-12-08 23:24:28]

Hello adg (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[adg, 2010-12-08 23:43:55]

So this makes it easier to edit a project with a utf8 name? is that
right? How have you encountered this issue?

On 9 December 2010 10:24,  <hitchmanr@gmail.com> wrote:
> Reviewers: adg,
>
> Message:
> Hello adg (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> godashboard: support submitting projects with non-ascii names
>
> Please review this at http://codereview.appspot.com/3459043/
>
> Affected files:
>  M misc/dashboard/godashboard/package.py
>  M misc/dashboard/godashboard/project-notify.txt
>  A misc/dashboard/godashboard/toutf8.py
>
>
> Index: misc/dashboard/godashboard/package.py
> ===================================================================
> --- a/misc/dashboard/godashboard/package.py
> +++ b/misc/dashboard/godashboard/package.py
> @@ -30,6 +30,11 @@
>  import urllib2
>  import sets
>
> +# local imports
> +import toutf8
> +
> +template.register_template_library('toutf8')
> +
>  # Storage model for package info recorded on server.
>  # Just path, count, and time of last install.
>  class Package(db.Model):
> Index: misc/dashboard/godashboard/project-notify.txt
> ===================================================================
> --- a/misc/dashboard/godashboard/project-notify.txt
> +++ b/misc/dashboard/godashboard/project-notify.txt
> @@ -5,5 +5,5 @@
>  URL: {{project.web_url}}
>
>  To edit/approve/delete:
> -http://godashboard.appspot.com/project/edit?name={{project.name|urlencode}}
>
+http://godashboard.appspot.com/project/edit?name={{project.name|toutf8|urlencode}}
>
> Index: misc/dashboard/godashboard/toutf8.py
> ===================================================================
> new file mode 100644
> --- /dev/null
> +++ b/misc/dashboard/godashboard/toutf8.py
> @@ -0,0 +1,14 @@
> +# Copyright 2010 The Go Authors. All rights reserved.
> +# Use of this source code is governed by a BSD-style
> +# license that can be found in the LICENSE file.
> +
> +# This is a Django custom template filter to work around the
> +# fact that GAE's urlencode filter doesn't handle unicode strings.
> +
> +from google.appengine.ext import webapp
> +
> +register = webapp.template.create_template_register()
> +
> +@register.filter
> +def toutf8(value):
> +    return value.encode("utf-8")
>
>
>

[rmmh, 2010-12-09 00:00:50]

It's a fix for http://code.google.com/p/go/issues/detail?id=1314

Specifically, {{project.name|urlencode}} throws an error if project.name is a
unicode string. This patch adds a filter that encodes unicode strings so
urlencode can handle them.

On 2010/12/08 23:43:55, adg wrote:
> So this makes it easier to edit a project with a utf8 name? is that
> right? How have you encountered this issue?

[adg, 2010-12-09 00:44:45]

On 9 December 2010 11:00,  <hitchmanr@gmail.com> wrote:
> It's a fix for http://code.google.com/p/go/issues/detail?id=1314

Please 'hg change 3459043' and add "Fixes issue 1314." to the description.

> Specifically, {{project.name|urlencode}} throws an error if project.name
> is a unicode string. This patch adds a filter that encodes unicode
> strings so urlencode can handle them.

Have you tested it?

Thanks for this. I'll likely submit it once my dashboard CL has been submitted.

[rmmh, 2010-12-09 03:11:03]

On 2010/12/09 00:44:45, adg wrote:
> Please 'hg change 3459043' and add "Fixes issue 1314." to the description.

Done.

> Have you tested it?

Yes, with dev_appserver.py.

[rsc, 2010-12-09 15:13:33]

I think the solution should be to make
project.name be a UTF-8-encoded string
already instead of adding explicit toutf8
conversions in whatever places we identify.

See the "UNICODE STRING HANDLING"
comment at http://golang.org/lib/codereview/codereview.py

Russ

[rmmh, 2010-12-09 19:22:37]

On 2010/12/09 15:13:33, rsc wrote:
> I think the solution should be to make
> project.name be a UTF-8-encoded string
> already instead of adding explicit toutf8
> conversions in whatever places we identify.

The root of the problem is that GAE's Django 0.96 template engine has incomplete
Unicode handling. *Most* of the template system other than urlencode works
properly with Unicode. The only place you'd have to use toutf8 is before a
urlencode.

Converting project.name to a UTF-8 bytestring wouldn't work, as GAE
StringProperty types (what project.name is stored as) store either ASCII text as
str or Unicode text as unicode.

http://code.google.com/appengine/docs/python/datastore/entitiesandmodels.html...
"A short string value can be either a unicode value or a str value. If the value
is a str, an encoding of 'ascii' is assumed."

[adg, 2010-12-12 23:29:39]

http://codereview.appspot.com/3459043/diff/2001/misc/dashboard/godashboard/pr...
File misc/dashboard/godashboard/project-notify.txt (right):

http://codereview.appspot.com/3459043/diff/2001/misc/dashboard/godashboard/pr...
misc/dashboard/godashboard/project-notify.txt:8:
http://godashboard.appspot.com/project/edit?name={{project.name|toutf8|urlenc...
Has something been dropped from this CL? Surely this is not the only place the
project name need be escaped. (I'm the only one who ever sees this
notification.)

[adg, 2010-12-15 00:52:34]

Ping

[rmmh, 2010-12-15 00:54:54]

I'm pretty sure this is the only place it needs to be escaped. urlencode is the
only filter being used that doesn't handle Unicode properly, and a URL
containing the project name is only made in the mail template.

[adg, 2010-12-15 01:07:39]

*** Submitted as http://code.google.com/p/go/source/detail?r=8f047bce6638 ***

godashboard: support submitting projects with non-ascii names

Fixes issue 1314.

R=adg, rsc
CC=golang-dev
http://codereview.appspot.com/3459043

Committer: Andrew Gerrand <adg@golang.org>

[adg, 2010-12-15 01:07:47]

LGTM

Alrighty, makes sense.

On 15 December 2010 11:54,  <hitchmanr@gmail.com> wrote:
> I'm pretty sure this is the only place it needs to be escaped. urlencode
> is the only filter being used that doesn't handle Unicode properly, and
> a URL containing the project name is only made in the mail template.
>
> http://codereview.appspot.com/3459043/
>