On 22 July 2013 16:04, Milo Casagrande <email address hidden> wrote:
> 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.

Good point. Will clean that up.

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

You couldn't make the call - if request.POST["key"] didn't exist, it
would raise an exception. If you had no filter for the key value, the
filter would match everything, which would be a bad thing.

--
James Tunnicliffe