Code review comment for lp:~stevanr/linaro-license-protection/bug-1085007

Hi Stevan,

Thanks for working on this. It's very close to being ready to land :)

У уто, 26. 02 2013. у 19:43 +0000, Stevan Radaković пише:
> === modified file 'license_protected_downloads/views.py'

> @@ -479,3 +480,13 @@
> path = result[1]
>
> return HttpResponse(json.dumps(RenderTextFiles.find_and_render(path)))
> +
> +
> +def get_remote_static(request):
> +

Please provide a docstring explaining what this does. And replace this blank line with it.

> + name = request.GET.get("name")
> + if name not in settings.SUPPORTED_REMOTE_STATIC_FILES:
> + raise Http404

Please add a message to this exception: it's always a good practice to
do that.

> +
> + data = urllib2.urlopen(settings.SUPPORTED_REMOTE_STATIC_FILES[name])
> + return HttpResponse(data)
>
> === modified file 'settings.py'
> --- settings.py 2012-11-29 09:36:04 +0000
> +++ settings.py 2013-02-26 19:42:24 +0000
> @@ -191,3 +191,9 @@
> 'Building From Source',
> 'Firmware Update',
> 'RTSM']
> +
> +SUPPORTED_REMOTE_STATIC_FILES = {
> + "linarofamily.js": "http://www.linaro.org/remote/js/linarofamily.js",
> + "init.css": "http://www.linaro.org/remote/css/init.css",
> + "remote.css": "http://www.linaro.org/remote/css/remote.css",
> + }
>
> === modified file 'templates/header.html'
> --- templates/header.html 2013-02-21 08:48:00 +0000
> +++ templates/header.html 2013-02-26 19:42:24 +0000
> @@ -5,9 +5,9 @@
> {% endif %}
> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
> <title>Linaro Snapshots</title>
> - <link href="//www.linaro.org/remote/css/init.css" rel="stylesheet" type="text/css" >
> - <link href="//www.linaro.org/remote/css/remote.css" rel="stylesheet" type="text/css" >
> - <script language="javascript" type="text/javascript" src="//www.linaro.org/remote/js/linarofamily.js"></script>
> + <link href="/get-remote-static?name=init.css" rel="stylesheet" type="text/css" >
> + <link href="/get-remote-static?name=remote.css" rel="stylesheet" type="text/css" >
> + <script language="javascript" type="text/javascript" src="/get-remote-static?name=linarofamily.js"></script>

This seems to break the line length limit: please fix this by breaking
into multiple lines where needed to meet our 79-character maximum line
length.

>
> <script type="text/javascript" src="/js/jquery-1.7.2.js"></script>
>
> === modified file 'urls.py'
> --- urls.py 2012-10-22 12:48:48 +0000
> +++ urls.py 2013-02-26 19:42:24 +0000
> @@ -22,6 +22,10 @@
> url(r'^css/(?P<path>.*)$', 'django.views.static.serve',
> {'document_root': settings.CSS_PATH}),
>
> + url(r'^get-remote-static',
> + 'license_protected_downloads.views.get_remote_static',
> + name='get_remote_static'),
> +
> # The license page...
> url(r'^license$',
> 'license_protected_downloads.views.show_license',

Btw, any reason there're no tests for this? Seems pretty simple to test
the get_remote_static() view (you might want to override settings in a
test though). It would be good to test the "negative" case as well: the
fact that it throws a 404 when something that is not supported is
requested.

  review needs-fixing

Cheers,
Danilo