Skip to content
Merged
14 changes: 14 additions & 0 deletions qiita_db/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
convert_from_id
convert_to_id
get_environmental_packages
get_visibilities
purge_filepaths
move_filepaths_to_upload_folder
move_upload_files_to_trash
Expand Down Expand Up @@ -979,6 +980,19 @@ def get_environmental_packages():
return qdb.sql_connection.TRN.execute_fetchindex()


def get_visibilities():
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing tests

"""Get the list of available visibilities for artifacts

Returns
-------
list of str
The available visibilities
"""
with qdb.sql_connection.TRN:
qdb.sql_connection.TRN.add("SELECT visibility FROM qiita.visibility")
return qdb.sql_connection.TRN.execute_fetchindex()


def get_timeseries_types():
"""Get the list of available timeseries types

Expand Down
6 changes: 4 additions & 2 deletions qiita_pet/handlers/api_proxy/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
ena_ontology_get_req, prep_template_samples_get_req)
from .studies import (
data_types_get_req, study_get_req, study_prep_get_req, study_delete_req)
from .artifact import artifact_graph_get_req
from .artifact import (artifact_graph_get_req, artifact_get_req,
artifact_status_put_req, artifact_delete_req)

__all__ = ['prep_template_summary_get_req', 'sample_template_post_req',
'sample_template_put_req', 'data_types_get_req',
Expand All @@ -33,7 +34,8 @@
'prep_template_summary_get_req', 'prep_template_post_req',
'prep_template_put_req', 'prep_template_delete_req',
'prep_template_graph_get_req', 'prep_template_filepaths_get_req',
'artifact_get_graph', 'prep_template_get_req', 'study_delete_req',
'artifact_get_req', 'artifact_status_put_req',
'artifact_delete_req', 'prep_template_get_req', 'study_delete_req',
'study_prep_get_req', 'sample_template_get_req',
'artifact_graph_get_req', 'ena_ontology_get_req',
'sample_template_meta_cats_get_req',
Expand Down
129 changes: 129 additions & 0 deletions qiita_pet/handlers/api_proxy/artifact.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@
# The full license is in the file LICENSE, distributed with this software.
# -----------------------------------------------------------------------------
from qiita_db.artifact import Artifact
from qiita_db.user import User
from qiita_db.util import get_visibilities
from qiita_db.exceptions import QiitaDBArtifactDeletionError
from qiita_core.qiita_settings import qiita_config
from qiita_pet.handlers.api_proxy.util import check_access


Expand Down Expand Up @@ -53,3 +57,128 @@ def artifact_graph_get_req(artifact_id, direction, user_id):
'node_labels': node_labels,
'status': 'success',
'message': ''}


def artifact_get_req(artifact_id, user_id):
"""Get information about the artifact

Parameters
----------
artifact_id : int
Artifact being acted on
user_id : str
The user requesting the action

Returns
-------
dict
information about the artifact
"""
pd = Artifact(int(artifact_id))
access_error = check_access(pd.study.id, user_id)
if access_error:
return access_error

can_submit_to_ebi = pd.can_be_submitted_to_ebi
ebi_run_accessions = pd.ebi_run_accessions if can_submit_to_ebi else None

can_submit_to_vamps = pd.can_be_submitted_to_vamps
is_submitted_to_vamps = pd.is_submitted_to_vamps if can_submit_to_vamps \
else False

return {
'timestamp': pd.timestamp,
'processing_parameters': pd.processing_parameters,
'visibility': pd.visibility,
'artifact_type': pd.artifact_type,
'data_type': pd.data_type,
'can_be_submitted_to_ebi': can_submit_to_ebi,
'can_be_submitted_to_vamps': can_submit_to_vamps,
'is_submitted_to_vamps': is_submitted_to_vamps,
'filepaths': pd.filepaths,
'parents': [a.id for a in pd.parents],
'prep_templates': [p.id for p in pd.prep_templates],
'ebi_run_accessions': ebi_run_accessions,
'study': pd.study.id}


def artifact_delete_req(artifact_id, user_id):
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test for this?

Copy link
Member

Choose a reason for hiding this comment

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

Can you add doc strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like there were more merge issues then I thought, since they were both there before. Let me dig on those.

"""Deletes the artifact

Parameters
----------
artifact_id : int
Artifact being acted on
user_id : str
The user requesting the action

Returns
-------
dict
Status of action, in the form {'status': status, 'message': msg}
status: status of the action, either success or error
message: Human readable message for status
"""
pd = Artifact(int(artifact_id))
access_error = check_access(pd.study.id, user_id)
if access_error:
return access_error
try:
Artifact.delete(int(artifact_id))
except QiitaDBArtifactDeletionError as e:
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test for the except?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return {'status': 'error',
'message': str(e)}
return {'status': 'success',
'message': ''}


def artifact_status_put_req(artifact_id, user_id, visibility):
"""Set the status of the artifact given

Parameters
----------
artifact_id : int
Artifact being acted on
user_id : str
The user requesting the action
visibility : {'sandbox', 'awaiting_approval', 'private', 'public'}
What to change the visibility to

Returns
-------
dict
Status of action, in the form {'status': status, 'message': msg}
status: status of the action, either success or error
message: Human readable message for status
"""
if visibility not in get_visibilities():
return {'status': 'error',
'message': 'Unknown visiblity value: %s' % visibility}

pd = Artifact(int(artifact_id))
access_error = check_access(pd.study.id, user_id)
if access_error:
return access_error
user = User(str(user_id))
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity - why the specific cast to str in here? it looks unnecessary as you've already said before that everything is passed as a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was casting explicitly to int for the other objects so I just followed the same pattern and did an explicit cast here as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the reasoning doesn't apply here but it is ok - no need to change.

# Set the approval to private if needs approval and admin
if visibility == 'private':
if all([qiita_config.require_approval, user.level == 'admin']):
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to create a list and use all. Also, if you rearrange the ifs, you can save some expression checks:

if not qiita_config.require_approval:
    # Set the visibility to private b/c approval is not required
elif user.level == 'admin':
    # Set the visibility to private b/c approval is required and level is admin
else:
    # Error b/c there are no privileges

Also, the msg variable and status are set tu success multiple times. Move that code before the main if statement and only change in case of error.

pd.visibility = 'private'
status = 'success'
msg = 'Artifact visibility changed to private'
# Set the approval to private if approval not required
elif not qiita_config.require_approval:
pd.visibility = 'private'
status = 'success'
msg = 'Artifact visibility changed to private'
# Trying to set approval without admin privileges
else:
status = 'error'
msg = 'User does not have permissions to approve change'
else:
pd.visibility = visibility
status = 'success'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is redundant code form lines 165/166 (as I noticed in my previous comment).

Move the setting of status and msg outside the if in line 164 and remove lines 165 and 166. The output is the same and if we ever need to change the message there is a single line in the code that we need to change.

Copy link
Contributor

Choose a reason for hiding this comment

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

@squirrelo ETA on this? Having your PR in will reduce my developing time b/c I will be able to connect the interface and my work in the same PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

On Feb 12, 2016, at 10:54, Jose Navas notifications@github.com wrote:

In qiita_pet/handlers/api_proxy/artifact.py #1593 (comment):

  • Set the approval to private if needs approval and admin

  • if visibility == 'private':
  •    status = 'success'
    
  •    msg = 'Artifact visibility changed to private'
    
  •    if not qiita_config.require_approval:
    
  •        pd.visibility = 'private'
    
  •    # Set the approval to private if approval not required
    
  •    elif user.level == 'admin':
    
  •        pd.visibility = 'private'
    
  •    # Trying to set approval without admin privileges
    
  •    else:
    
  •        status = 'error'
    
  •        msg = 'User does not have permissions to approve change'
    
  • else:
  •    pd.visibility = visibility
    
  •    status = 'success'
    
    @squirrelo https://github.com/squirrelo ETA on this? Having your PR in will reduce my developing time b/c I will be able to connect the interface and my work in the same PR.


Reply to this email directly or view it on GitHub https://github.com/biocore/qiita/pull/1593/files#r52781956.

msg = 'Artifact visibility changed to %s' % visibility

return {'status': status,
'message': msg}
111 changes: 101 additions & 10 deletions qiita_pet/handlers/api_proxy/tests/test_artifact.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,103 @@
# The full license is in the file LICENSE, distributed with this software.
# -----------------------------------------------------------------------------
from unittest import TestCase, main
from datetime import datetime
from os.path import join

from qiita_pet.handlers.api_proxy.artifact import artifact_graph_get_req
from qiita_core.util import qiita_test_checker
from qiita_core.qiita_settings import qiita_config
from qiita_db.artifact import Artifact
from qiita_db.exceptions import QiitaDBUnknownIDError
from qiita_pet.handlers.api_proxy.artifact import (
artifact_get_req, artifact_status_put_req, artifact_graph_get_req,
artifact_delete_req)


@qiita_test_checker()
class TestArtifactAPI(TestCase):
def tearDown(self):
Artifact(1).visibility = 'private'

def test_artifact_get_req(self):
obs = artifact_get_req(1, 'test@foo.bar')
exp = {'is_submitted_to_vamps': False,
'data_type': '18S',
'can_be_submitted_to_vamps': False,
'can_be_submitted_to_ebi': False,
'timestamp': datetime(2012, 10, 1, 9, 30, 27),
'prep_templates': [1],
'visibility': 'private',
'study': 1,
'processing_parameters': None,
'ebi_run_accessions': None,
'parents': [],
'filepaths': [
(1, join(qiita_config.base_data_dir,
'raw_data/1_s_G1_L001_sequences.fastq.gz'),
'raw_forward_seqs'),
(2, join(qiita_config.base_data_dir,
'raw_data/1_s_G1_L001_sequences_barcodes.'
'fastq.gz'),
'raw_barcodes')],
'artifact_type': 'FASTQ'}
self.assertEqual(obs, exp)

def test_artifact_get_req_no_access(self):
obs = artifact_get_req(1, 'demo@microbio.me')
exp = {'status': 'error',
'message': 'User does not have access to study'}
self.assertEqual(obs, exp)

def test_artifact_delete_req(self):
obs = artifact_delete_req(3, 'test@foo.bar')
exp = {'status': 'success', 'message': ''}
self.assertEqual(obs, exp)

with self.assertRaises(QiitaDBUnknownIDError):
Artifact(3)

def test_artifact_delete_req_error(self):
obs = artifact_delete_req(1, 'test@foo.bar')
exp = {'status': 'error',
'message': 'Cannot delete artifact 1: it has children: 2, 3'}
self.assertEqual(obs, exp)

def test_artifact_delete_req_no_access(self):
obs = artifact_delete_req(3, 'demo@microbio.me')
exp = {'status': 'error',
'message': 'User does not have access to study'}
self.assertEqual(obs, exp)

def test_artifact_status_put_req(self):
obs = artifact_status_put_req(1, 'test@foo.bar', 'sandbox')
exp = {'status': 'success',
'message': 'Artifact visibility changed to sandbox'}
self.assertEqual(obs, exp)

def test_artifact_status_put_req_private(self):
obs = artifact_status_put_req(1, 'admin@foo.bar', 'private')
exp = {'status': 'success',
'message': 'Artifact visibility changed to private'}
self.assertEqual(obs, exp)

def test_artifact_status_put_req_private_bad_permissions(self):
obs = artifact_status_put_req(1, 'test@foo.bar', 'private')
exp = {'status': 'error',
'message': 'User does not have permissions to approve change'}
self.assertEqual(obs, exp)

def test_artifact_status_put_req_no_access(self):
obs = artifact_status_put_req(1, 'demo@microbio.me', 'sandbox')
exp = {'status': 'error',
'message': 'User does not have access to study'}
self.assertEqual(obs, exp)

def test_artifact_status_put_req_unknown_status(self):
obs = artifact_status_put_req(1, 'test@foo.bar', 'BADSTAT')
exp = {'status': 'error',
'message': 'Unknown visiblity value: BADSTAT'}
self.assertEqual(obs, exp)

def test_artifact_graph_get_req_ancestors(self):
obs = artifact_graph_get_req(1, 'ancestors', 'test@foo.bar')
exp = {'status': 'success',
Expand All @@ -23,25 +115,24 @@ def test_artifact_graph_get_req_descendants(self):
obs = artifact_graph_get_req(1, 'descendants', 'test@foo.bar')
exp = {'status': 'success',
'message': '',
'edge_list': [(1, 3), (1, 2), (2, 4)],
'node_labels': [(1, 'Raw data 1 - FASTQ'),
(3, 'Demultiplexed 2 - Demultiplexed'),
(2, 'Demultiplexed 1 - Demultiplexed'),
(4, 'BIOM - BIOM')]}
self.assertEqual(obs, exp)

def test_artifact_graph_get_req_bad(self):
obs = artifact_graph_get_req(1, 'UNKNOWN', 'test@foo.bar')
exp = {'status': 'error',
'message': 'Unknown directon UNKNOWN'}
(4, 'BIOM - BIOM')],
'edge_list': [(1, 3), (1, 2), (2, 4)]}
self.assertEqual(obs, exp)

def test_artifact_graph_get_req_no_access(self):
obs = artifact_graph_get_req(1, 'descendants', 'demo@microbio.me')
obs = artifact_graph_get_req(1, 'ancestors', 'demo@microbio.me')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably something weird in the merge conflict resolution.

exp = {'status': 'error',
'message': 'User does not have access to study'}
self.assertEqual(obs, exp)

def test_artifact_graph_get_req_bad_direction(self):
obs = artifact_graph_get_req(1, 'WRONG', 'test@foo.bar')
exp = {'status': 'error', 'message': 'Unknown directon WRONG'}
self.assertEqual(obs, exp)


if __name__ == "__main__":
main()
41 changes: 20 additions & 21 deletions qiita_pet/handlers/api_proxy/tests/test_sample_template.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
from os.path import join, exists

from qiita_core.util import qiita_test_checker
from qiita_core.qiita_settings import qiita_config
import qiita_db as qdb
from qiita_pet.handlers.api_proxy.sample_template import (
sample_template_summary_get_req, sample_template_post_req,
Expand All @@ -21,34 +20,34 @@

@qiita_test_checker()
class TestSampleAPI(TestCase):
info = {
"timeseries_type_id": 1,
"metadata_complete": True,
"mixs_compliant": True,
"number_samples_collected": 25,
"number_samples_promised": 28,
"study_alias": "FCM",
"study_description": "DESC",
"study_abstract": "ABS",
"emp_person_id": qdb.study.StudyPerson(2),
"principal_investigator_id": qdb.study.StudyPerson(3),
"lab_person_id": qdb.study.StudyPerson(1)
def setUp(self):
info = {
"timeseries_type_id": 1,
"metadata_complete": True,
"mixs_compliant": True,
"number_samples_collected": 25,
"number_samples_promised": 28,
"study_alias": "FCM",
"study_description": "DESC",
"study_abstract": "ABS",
"emp_person_id": qdb.study.StudyPerson(2),
"principal_investigator_id": qdb.study.StudyPerson(3),
"lab_person_id": qdb.study.StudyPerson(1)
}

new_study = qdb.study.Study.create(
qdb.user.User('test@foo.bar'), "Some New Study", [1],
info)
self.new_study = qdb.study.Study.create(
qdb.user.User('test@foo.bar'), "Some New Study", [1],
info)

def setUp(self):
fp = join(qiita_config.base_data_dir, 'uploads',
str(self.new_study.id), 'uploaded_file.txt')
base_dir = qdb.util.get_mountpoint('uploads')[0][1]
fp = join(base_dir, str(self.new_study.id), 'uploaded_file.txt')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it will simplify the handling of the tests if you don't use this test (note code duplication here and in tearDown).
My recommendation would be that you actually create a new file using mkstemp (note that you can indicate the directory in which you want it) and just remove it in the tearDown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no code duplication. One creates a file for the new study upload folder, the other replaces the file for the existing study.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it means that the new file created for the new study is not deleted in the tearDown?

Copy link
Contributor

Choose a reason for hiding this comment

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

@squirrelo ping about this one

if not exists(fp):
with open(fp, 'w') as f:
f.write('')

def tearDown(self):
fp = join(qiita_config.base_data_dir, 'uploads', '1',
'uploaded_file.txt')
base_dir = qdb.util.get_mountpoint('uploads')[0][1]
fp = join(base_dir, '1', 'uploaded_file.txt')
if not exists(fp):
with open(fp, 'w') as f:
f.write('')
Expand Down
2 changes: 1 addition & 1 deletion qiita_pet/handlers/api_proxy/tests/test_studies.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
data_types_get_req, study_get_req, study_prep_get_req, study_delete_req)


@qiita_test_checker
@qiita_test_checker()
class TestStudyAPI(TestCase):
def test_data_types_get_req(self):
obs = data_types_get_req()
Expand Down
Loading