Skip to content
Merged
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
137 changes: 137 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.exceptions import (QiitaDBOperationNotPermittedError,
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,136 @@ 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

try:
Copy link
Member

Choose a reason for hiding this comment

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

These 3 try/except don't make sense, can you change? Or perhaps I'm missing something?

Copy link
Member

Choose a reason for hiding this comment

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

BTW they are also missing tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

condensed

Copy link
Contributor

Choose a reason for hiding this comment

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

can_be_submitted_to_ebi is a boolean that tells you if you can query the next attribute or not. Please, use an if statement rather than an exception. try/except should be used when something unexpected may happen, not as a substitute of an if:

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

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

can_be_submitted_to_ebi = pd.can_be_submitted_to_ebi
ebi_run_accessions = pd.ebi_run_accessions
except QiitaDBOperationNotPermittedError:
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 tests 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.

Already in

can_be_submitted_to_ebi = False
ebi_run_accessions = None

try:
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 tests 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.

Already in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I don't see tests in which can_be_submitted_to_XXX gets true/false (only false are tested).

can_be_submitted_to_vamps = pd.can_be_submitted_to_vamps
is_submitted_to_vamps = pd.is_submitted_to_vamps
except QiitaDBOperationNotPermittedError:
can_be_submitted_to_vamps = False
is_submitted_to_vamps = 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_be_submitted_to_ebi,
'can_be_submitted_to_vamps': can_be_submitted_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 {'sandbox', 'awaiting_approval', 'private', 'public'}:
Copy link
Member

Choose a reason for hiding this comment

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

This if is not tested.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do not hardcode this values. Please, extract from DB if you want to check here.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I don't see why that list cant be hard coded, as the rest is hard coded below. It makes sense to have it hardcoded here because of that.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, below you're only checking for the 'private' status - no need for all of them hardocded.

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 all([qiita_config.require_approval, visibility == 'private',
Copy link
Contributor

Choose a reason for hiding this comment

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

The first three if statements have common checks. Please extract common checks and use nested ifs to avoid evaluating multiple times the same expression.

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

user.level == 'admin']):
pd.visibility = 'private'
status = 'success'
msg = 'Artifact visibility changed to private'
# Set the approval to private if approval not required
elif all([not qiita_config.require_approval,
visibility == 'private']):
pd.visibility = 'private'
status = 'success'
msg = 'Artifact visibility changed to private'
# Trying to set approval without admin privileges
elif visibility == 'private':
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
6 changes: 3 additions & 3 deletions qiita_pet/handlers/study_handlers/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@
from .vamps_handlers import VAMPSHandler
from .base import StudyIndexHandler, StudyBaseInfoAJAX, StudyDeleteAjax
from .prep_template import PrepTemplateGraphAJAX, PrepTemplateAJAX
from .artifact import ArtifactGraphAJAX
from .artifact import ArtifactGraphAJAX, ArtifactAdminAJAX, ArtifactAJAX
from .sample_template import SampleTemplateAJAX, SampleAJAX

__all__ = ['ListStudiesHandler', 'StudyApprovalList', 'ShareStudyAJAX',
'StudyEditHandler', 'CreateStudyAJAX', 'StudyDescriptionHandler',
'PreprocessingSummaryHandler', 'EBISubmitHandler',
'MetadataSummaryHandler', 'VAMPSHandler', 'SearchStudiesAJAX',
'PrepTemplateGraphAJAX', 'ArtifactGraphAJAX',
'PrepTemplateGraphAJAX', 'ArtifactGraphAJAX', 'ArtifactAdminAJAX',
'StudyIndexHandler', 'StudyBaseInfoAJAX', 'SampleTemplateAJAX',
'PrepTemplateAJAX', 'SampleAJAX', 'StudyDeleteAjax']
'PrepTemplateAJAX', 'SampleAJAX', 'StudyDeleteAjax', 'ArtifactAJAX']
Loading