Merge lp://qastaging/~stevanr/linaro-license-protection/bug-1085007 into lp://qastaging/~linaro-automation/linaro-license-protection/trunk

Proposed by Stevan Radaković
Status: Merged
Approved by: Данило Шеган
Approved revision: 174
Merged at revision: 171
Proposed branch: lp://qastaging/~stevanr/linaro-license-protection/bug-1085007
Merge into: lp://qastaging/~linaro-automation/linaro-license-protection/trunk
Diff against target: 197 lines (+105/-4)
6 files modified
license_protected_downloads/tests/helpers.py (+43/-0)
license_protected_downloads/tests/test_views.py (+29/-1)
license_protected_downloads/views.py (+17/-0)
settings.py (+6/-0)
templates/header.html (+6/-3)
urls.py (+4/-0)
To merge this branch: bzr merge lp://qastaging/~stevanr/linaro-license-protection/bug-1085007
Reviewer Review Type Date Requested Status
Данило Шеган (community) Approve
Review via email: mp+150646@code.qastaging.launchpad.net

Description of the change

Additional fix for bug 1085007.
When certificates for www.linaro.org are fixed, this code will become obsolete and may be removed.

To post a comment you must log in.
Revision history for this message
Данило Шеган (danilo) wrote :
Download full text (3.2 KiB)

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 wh...

Read more...

review: Needs Fixing
171. By Stevan Radaković

Add tests, docstrings and fix pep8 in html.

172. By Stevan Radaković

Add test server for urllib testing.

173. By Stevan Radaković

Add test server helper from danilos branch.

174. By Stevan Radaković

Add test for non-existing file. Add exception handling for HTTPError.

Revision history for this message
Данило Шеган (danilo) wrote :

Thanks for the improvements. Please extend the comment in get_remote_static to say how we should switch the error to Http404 once we start sending the email to infrastructure-errors.

review: Approve
175. By Stevan Radaković

Add additional comment for re-raising error.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches