Skip to content

fixes #2245 #2350

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
merged 3 commits into from
Oct 19, 2017
Merged

fixes #2245 #2350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9c356f5
fixes #2245
josenavas Oct 13, 2017
195e0fc
Addressing @antgonza's comments
josenavas Oct 16, 2017
9d40d94
Fixing test
josenavas Oct 16, 2017
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
14 changes: 8 additions & 6 deletions qiita_db/analysis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,15 +429,17 @@ def mapping_file(self):

Returns
-------
str or None
full filepath to the mapping file or None if not generated
int or None
The filepath id of the analysis mapping file or None
if not generated
"""
fp = [fp for _, fp, fp_type in qdb.util.retrieve_filepaths(
"analysis_filepath", "analysis_id", self._id)
if fp_type == 'plain_text']
fp = [fp_id
for fp_id, _, fp_type in qdb.util.retrieve_filepaths(
"analysis_filepath", "analysis_id", self._id)
if fp_type == 'plain_text']
Copy link
Contributor

Choose a reason for hiding this comment

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

It's accurate that a mapping file is "plain_text" but should the system be able to differentiate those types with, say, biom summary? Out of context for this PR but seemed unexpected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point only the mapping file is stored in the table "analysis_filepath". The BIOM-related files (and others) are specifically stored with the artifact, not directly to the analysis.

Copy link
Contributor

Choose a reason for hiding this comment

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

"At this point" suggests this could lead to bugs in the future if that changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the data model changes in the future then other sections beside this one will need to be updated - so I would say that it would depend on the nature of the data model changes.

Copy link
Member

Choose a reason for hiding this comment

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

Discussing offline, @wasade and I agreed that perhaps the solution of this specific kind of conflicts will be to implement something like #2361


if fp:
# returning the actual path vs. an array
# returning the actual filepath id vs. an array
return fp[0]
else:
return None
Expand Down
3 changes: 2 additions & 1 deletion qiita_db/handlers/analysis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ def get(self, analysis_id):
"""
with qdb.sql_connection.TRN:
a = _get_analysis(analysis_id)
mf_fp = a.mapping_file
mf_fp = qdb.util.get_filepath_information(
a.mapping_file)['fullpath']
response = None
if mf_fp is not None:
df = qdb.metadata_template.util.load_template_to_dataframe(
Expand Down
24 changes: 18 additions & 6 deletions qiita_db/test/test_analysis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,9 @@ def test_set_pmid(self):
def test_retrieve_mapping_file(self):
exp = join(self.fp, "1_analysis_mapping.txt")
obs = self.analysis.mapping_file
self.assertEqual(obs, exp)
self.assertIsNotNone(obs)
self.assertEqual(
qdb.util.get_filepath_information(obs)['fullpath'], exp)
self.assertTrue(exists(exp))

def test_retrieve_tgz(self):
Expand Down Expand Up @@ -370,7 +372,8 @@ def test_build_mapping_file(self):

npt.assert_warns(qdb.exceptions.QiitaDBWarning,
analysis._build_mapping_file, samples)
obs = analysis.mapping_file
obs = qdb.util.get_filepath_information(
analysis.mapping_file)['fullpath']

exp = self.get_fp("%s_analysis_mapping.txt" % analysis.id)
self.assertEqual(obs, exp)
Expand All @@ -388,8 +391,10 @@ def test_build_mapping_file_duplicated_samples_no_merge(self):
npt.assert_warns(qdb.exceptions.QiitaDBWarning,
analysis._build_mapping_file, samples, True)

mapping_fp = qdb.util.get_filepath_information(
analysis.mapping_file)['fullpath']
obs = qdb.metadata_template.util.load_template_to_dataframe(
analysis.mapping_file, index='#SampleID')
mapping_fp, index='#SampleID')
exp = qdb.metadata_template.util.load_template_to_dataframe(
self.duplicated_samples_not_merged, index='#SampleID')

Expand All @@ -405,8 +410,10 @@ def test_build_mapping_file_duplicated_samples_merge(self):
3: ['1.SKB8.640193', '1.SKD8.640184', '1.SKB7.640196']}
npt.assert_warns(qdb.exceptions.QiitaDBWarning,
analysis._build_mapping_file, samples)
mapping_fp = qdb.util.get_filepath_information(
analysis.mapping_file)['fullpath']
obs = qdb.metadata_template.util.load_template_to_dataframe(
analysis.mapping_file, index='#SampleID')
mapping_fp, index='#SampleID')
exp = qdb.metadata_template.util.load_template_to_dataframe(
self.map_exp_fp, index='#SampleID')
assert_frame_equal(obs, exp)
Expand Down Expand Up @@ -460,8 +467,10 @@ def test_build_files(self):
# testing that the generated files have the same sample ids
biom_fp = biom_tables[0][1]
biom_ids = load_table(biom_fp).ids(axis='sample')
mapping_fp = qdb.util.get_filepath_information(
analysis.mapping_file)['fullpath']
mf_ids = qdb.metadata_template.util.load_template_to_dataframe(
analysis.mapping_file, index='#SampleID').index
mapping_fp, index='#SampleID').index

self.assertItemsEqual(biom_ids, mf_ids)

Expand Down Expand Up @@ -493,8 +502,11 @@ def test_build_files_merge_duplicated_sample_ids(self):
biom_ids = []
for _, fp in biom_tables:
biom_ids.extend(load_table(fp).ids(axis='sample'))

mapping_fp = qdb.util.get_filepath_information(
new.mapping_file)['fullpath']
mf_ids = qdb.metadata_template.util.load_template_to_dataframe(
new.mapping_file, index='#SampleID').index
mapping_fp, index='#SampleID').index

self.assertItemsEqual(biom_ids, mf_ids)

Expand Down
1 change: 1 addition & 0 deletions qiita_pet/handlers/analysis_handlers/base_handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ def analysis_description_handler_get_request(analysis_id, user):
return {'analysis_name': analysis.name,
'analysis_id': analysis.id,
'analysis_description': analysis.description,
'analysis_mapping_id': analysis.mapping_file,
'alert_type': alert_type,
'alert_msg': alert_msg}

Expand Down
3 changes: 3 additions & 0 deletions qiita_pet/handlers/analysis_handlers/tests/test_base_handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ def test_analysis_description_handler_get_request(self):
exp = {'analysis_name': 'SomeAnalysis',
'analysis_id': 1,
'analysis_description': 'A test analysis',
'analysis_mapping_id': 16,
'alert_type': 'info',
'alert_msg': ''}
self.assertEqual(obs, exp)
Expand All @@ -42,6 +43,7 @@ def test_analysis_description_handler_get_request(self):
exp = {'analysis_name': 'SomeAnalysis',
'analysis_id': 1,
'analysis_description': 'A test analysis',
'analysis_mapping_id': 16,
'alert_type': 'info',
'alert_msg': 'An artifact is being deleted from this analysis'}
self.assertEqual(obs, exp)
Expand All @@ -54,6 +56,7 @@ def test_analysis_description_handler_get_request(self):
exp = {'analysis_name': 'SomeAnalysis',
'analysis_id': 1,
'analysis_description': 'A test analysis',
'analysis_mapping_id': 16,
'alert_type': 'danger',
'alert_msg': 'Error deleting artifact'}
self.assertEqual(obs, exp)
Expand Down
5 changes: 4 additions & 1 deletion qiita_pet/templates/analysis_description.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,10 @@
<div class="col">
<h2>
{{analysis_name}} - ID {{analysis_id}}
<a class="btn btn-info glyphicon glyphicon-share" data-toggle="modal" data-target="#share-analysis-modal-view" onclick="modify_sharing({{analysis_id}});"></a>
<a class="btn btn-info" data-toggle="modal" data-target="#share-analysis-modal-view" onclick="modify_sharing({{analysis_id}});"><span class=" glyphicon glyphicon-share"></span></a>
{% if analysis_mapping_id is not None %}
<a class="btn btn-default" href="{% raw qiita_config.portal_dir %}/download/{{analysis_mapping_id}}"><span class="glyphicon glyphicon-download-alt"></span> Mapping file</a>
{% end %}
</h2>
<h3>{{analysis_description}}</h3>
Shared with: <span id="shared_html_{{analysis_id}}"></span>
Expand Down