Skip to content

download public prep data #3491

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lucaspatel merged 2 commits into from
Nov 21, 2025
+87 −35
Merged

download public prep data #3491

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5795a01
download public prep data
antgonza Nov 20, 2025
97e96cc
addressing @lucaspatel comments
antgonza Nov 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 48 additions & 1 deletion qiita_pet/handlers/download.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,7 @@ def get(self):
zip_fn = '%s_%s.zip' % (
fname, datetime.now().strftime('%m%d%y-%H%M%S'))
self._set_nginx_headers(zip_fn)
else:
elif study_id is not None:
study_id = int(study_id)
try:
study = Study(study_id)
Expand Down Expand Up @@ -612,6 +612,53 @@ def get(self):
'%m%d%y-%H%M%S'))

self._set_nginx_headers(zip_fn)
else:
prep_id = int(prep_id)
try:
prep = PrepTemplate(prep_id)
except QiitaDBUnknownIDError:
raise HTTPError(422, reason='Prep does not exist')
else:
public_raw_download = Study(prep.study_id).public_raw_download
if prep.status != 'public':
raise HTTPError(404, reason='Prep is not public. If this '
'is a mistake contact: %s' %
qiita_config.help_email)
elif data == 'raw' and not public_raw_download:
raise HTTPError(422, reason='No raw data access. If this '
'is a mistake contact: %s'
% qiita_config.help_email)
else:
Copy link
Collaborator

@lucaspatel lucaspatel Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is getting to be a lot of nested else. I'm sure the logic works but it makes it quite hard to read.

Copy link
Member Author

@antgonza antgonza Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# raw data
if data == 'raw':
artifacts = [prep.artifact]
# bioms
elif data == 'biom':
artifacts = [a for a in
prep.artifact.descendants.nodes()
if a.artifact_type == 'BIOM' and
a.visibility == 'public']
else:
raise HTTPError(
422,
reason='You can only download raw/biom from preps')
for a in artifacts:
if a.visibility != 'public' or a.has_human:
continue
to_download.extend(self._list_artifact_files_nginx(a))

if not to_download:
raise HTTPError(422, reason='Nothing to download. If '
Copy link
Collaborator

@lucaspatel lucaspatel Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this is your linter splitting the line up or something, but this seems unnecessary.

'this is a mistake contact: %s'
% qiita_config.help_email)
else:
self._write_nginx_file_list(to_download)

zip_fn = 'prep_%d_%s_%s.zip' % (
prep_id, data, datetime.now().strftime(
'%m%d%y-%H%M%S'))

self._set_nginx_headers(zip_fn)

self.finish()

Expand Down
20 changes: 20 additions & 0 deletions qiita_pet/test/test_download.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,26 @@ def test_download(self):
'mapping_files/5_mapping_file.txt')
self.assertRegex(response.body.decode('ascii'), exp)

# Now let's check download prep with no raw data access
response = self.get('/public_download/?data=raw&prep_id=1')
self.assertTrue(response.reason.startswith('No raw data access.'))

# Now success
Study(1).public_raw_download = True
response = self.get('/public_download/?data=raw&prep_id=1')
self.assertEqual(response.code, 200)
exp = ('- [0-9]* /protected/raw_data/1_s_G1_L001_sequences.fastq.gz '
'raw_data/1_s_G1_L001_sequences.fastq.gz\n- [0-9]* /protected'
'/raw_data/1_s_G1_L001_sequences_barcodes.fastq.gz raw_data/'
'1_s_G1_L001_sequences_barcodes.fastq.gz\n- [0-9]* /protected/'
'templates/1_prep_1_qiime_19700101-000000.txt mapping_files/'
'1_mapping_file.txt\n')
self.assertRegex(response.body.decode('ascii'), exp)

# for simplicity, let's just check respose.code
response = self.get('/public_download/?data=biom&prep_id=1')
self.assertEqual(response.code, 200)

Copy link
Collaborator

@lucaspatel lucaspatel Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Collaborator

@lucaspatel lucaspatel Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what if there are no artifacts available? 422 with "Nothing to download" is that somewhere?

Copy link
Member Author

@antgonza antgonza Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

def test_download_sample_information(self):
response = self.get('/public_artifact_download/')
self.assertEqual(response.code, 422)
Expand Down
Loading