Code review comment for lp:~dooferlad/linaro-license-protection/public_push_support

Hi James,

overall it looks good, a couple of suggestion follows.

On Wed, Jul 17, 2013 at 5:29 PM, James Tunnicliffe
<email address hidden> wrote:
>
> + def test_get_public_key_post_and_get_file(self):
> + response = self.client.get("http://testserver/api/request_key",
> + data={"key": settings.MASTER_API_KEY,
> + "public": ""})
> +
> + self.assertEqual(response.status_code, 200)
> + # Don't care what the key is, as long as it isn't blank
> + self.assertRegexpMatches(response.content, "\S+")
> + key = response.content
> +
> + # Now write a file so we can upload it
> + file_content = "test_get_key_post_and_get_file"
> + file_root = "/tmp"
> +
> + tmp_file_name = os.path.join(
> + file_root,
> + self.make_temporary_file(file_content))
> +
> + buildinfo_content = "\n".join([
> + "Format-Version: 0.1",
> + "Files-Pattern: *",
> + "Build-Name: test",
> + "License-Type: open"])
> + tmp_build_info = os.path.join(
> + file_root,
> + self.make_temporary_file(buildinfo_content))
> +
> + # Send the files
> + with open(tmp_file_name) as f:
> + response = self.client.post("http://testserver/pub/file_name",
> + data={"key": key, "file": f})
> + self.assertEqual(response.status_code, 200)
> +
> + with open(tmp_build_info) as f:
> + response = self.client.post("http://testserver/pub/BUILD-INFO.txt",
> + data={"key": key, "file": f})
> + self.assertEqual(response.status_code, 200)
> +
> + # Check the upload worked by reading the file back from its uploaded
> + # location
> + uploaded_file_path = os.path.join(settings.SERVED_PATHS[0],
> + "file_name")
> +
> + with open(uploaded_file_path) as f:
> + self.assertEqual(f.read(), file_content)
> +
> + # Test we can fetch the newly uploaded file
> + response = self.client.get("http://testserver/pub/file_name")
> + self.assertEqual(response.status_code, 200)
> +
> + # Delete the files generated by the test
> + shutil.rmtree(os.path.join(settings.SERVED_PATHS[0], "pub"))

Wouldn't it be better to have all the logic of this test method inside
a try-finally?
I'm concerned with an error that can happen, for whatever reason,
somewhere before the call to shutil.rmtree, leaving cruft on the file
system.

> === modified file 'license_protected_downloads/uploads.py'
>
>
> @@ -41,11 +44,14 @@
> APIKeyStore.objects.filter(key=request.POST["key"])):
> return HttpResponseServerError("Invalid key")
>
> + api_key = APIKeyStore.objects.filter(key=request.POST["key"])
> +

Does the call to APIKeyStore.objects.filter get cached in some way?
The problem is that the other solution I have in mind means to always
call APIKeyStore.objects.filter even if "key in request.POST" is
False.

--
Milo Casagrande | Automation Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs