Web API: file upload & proposal_pipeline support, tests, and some cleanup #301
Conversation
…ual Redis functionality yet anyway) don't rely on presence of Redis server.
…init__ of APITestCase because that seems to cause problems.
| ("in_progress", "in_progress"), | ||
| ("received", "received") | ||
| ) | ||
| job_status_values = [j[0] for j in job_status_pairs] |
There was a problem hiding this comment.
It doesn't look like job_status_pairs is ever used; if not, can we just make a tuple with the five strings? I'd argue a tuple is better than a list here due to being immutable.
There was a problem hiding this comment.
It should be a tuple; also I meant to use job_status_pairs in the arguments for one of the CharFields in that file. I've added those changes and will push them shortly.
| contents = models.BinaryField() | ||
| file = models.FileField(null=True) | ||
| filename = models.CharField(default=None, max_length=512, null=True) | ||
| hexhash = models.CharField(default=None, max_length=32, null=True) |
There was a problem hiding this comment.
Spit balling, but should this be the primary key?
There was a problem hiding this comment.
Quite possibly. I was looking at the hashing approach as speculative, but if we keep it, then this should probably the the primary key.
| url(r'^job/upload/(?P<hexhash>[a-z0-9]{32})(/)$', | ||
| views.FileUploadViewInstance.as_view()), | ||
| url(r'^job/upload(/)$', | ||
| views.FileUploadView.as_view()), |
There was a problem hiding this comment.
Minor, but what do you think about using a term like file or attachment in place of upload in these urls? Trying to think through what kind of API would be the most helpful
There was a problem hiding this comment.
I'm open to that, and in general view all these URLs as merely first-pass suggestions. Happy to change any/all as we find better options.
| if RegulationFile.objects.filter(hexhash=hexhash).count() > 0: | ||
| return Response(dict(error="File already present."), | ||
| status=status.HTTP_400_BAD_REQUEST) | ||
| else: |
There was a problem hiding this comment.
Minor style thing, but if we're going to if-else and one of the branches exits early, we probably want to make the other branch also exit. This means I'd recommend something like:
if something:
return
else:
# some other logic
returnor
if something:
return
# do some other logic
returnover
if something:
return
else:
# do some logic
return
| @@ -1 +1,7 @@ | |||
| from regparser.web.settings.base import * # noqa | |||
|
|
|||
| # You need to have an email server running locally for this to work. | |||
| # ``python -m smtpd -n -c DebuggingServer localhost:2525`` should be fine. | |||
There was a problem hiding this comment.
Very nice, I didn't know this existed
| try: | ||
| self.hashed_contents = md5(self.file_contents.encode( | ||
| "utf-8")).hexdigest() | ||
| except AttributeError: |
There was a problem hiding this comment.
Ideally self.hashed_contents would be consistently either a byte string or a unicode string. If we can't guarantee that for whatever reason, I believe Django has some safe string methods for consistently handling conversions across Python2 and 3
| """ | ||
| hostname = getattr(settings, "CANONICAL_HOSTNAME", "") | ||
| hostport = getattr(settings, "CANONICAL_PORT", "") | ||
| if hostport: | ||
| if hostport and hostport is not "80": |
There was a problem hiding this comment.
Would it make sense to do the same for tls + 443?
|
This makes tons of useful changes for us, great work! I'll go ahead and merge and we can keep fixing up later if desired |
No description provided.