download public prep data #3491
Conversation
lucaspatel
left a comment
lucaspatel
left a comment
There was a problem hiding this comment.
Some suggested improvements but otherwise fine.
| raise HTTPError(422, reason='No raw data access. If this ' | ||
| 'is a mistake contact: %s' | ||
| % qiita_config.help_email) | ||
| else: |
There was a problem hiding this comment.
This is getting to be a lot of nested else. I'm sure the logic works but it makes it quite hard to read.
There was a problem hiding this comment.
I agree with you, I think this class shows how it has grown over time, which makes it difficult and complex. Thus, decided to rewrite it. Note that I didn't change the tests as those are checking multiple options, parameters and errors.
qiita_pet/handlers/download.py
Outdated
| to_download.extend(self._list_artifact_files_nginx(a)) | ||
|
|
||
| if not to_download: | ||
| raise HTTPError(422, reason='Nothing to download. If ' |
There was a problem hiding this comment.
I'm guessing this is your linter splitting the line up or something, but this seems unnecessary.
| # for simplicity, let's just check respose.code | ||
| response = self.get('/public_download/?data=biom&prep_id=1') | ||
| self.assertEqual(response.code, 200) | ||
|
|
There was a problem hiding this comment.
May want to test or communicate intended behavior for the conditions you added in the main logic, namely:
if a.visibility != 'public' or a.has_human:
There was a problem hiding this comment.
And what if there are no artifacts available? 422 with "Nothing to download" is that somewhere?
There was a problem hiding this comment.
I completely agree, note that those tests are done for when a user uses study_id vs. prep_id. Thus, in stead of creating more tests, I "simplified" the code and added a lot of comments so a study_id test also covers the prep_id test, and viceversa - as they now share the same/main error checking blocks.
lucaspatel
left a comment
lucaspatel
left a comment
There was a problem hiding this comment.
Much improved, nice work
3 participants
By mistake this is not currently allowed, and it should be.