From d749e4a33944682a22586788e5fd06939cc2371c Mon Sep 17 00:00:00 2001 From: Natasha Singh Date: Thu, 16 Mar 2023 17:11:05 -0400 Subject: [PATCH 1/8] :sparkles: Add visibility_reason column to all tables --- dataservice/api/common/model.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/dataservice/api/common/model.py b/dataservice/api/common/model.py index 94df5b92b..38cc840b8 100644 --- a/dataservice/api/common/model.py +++ b/dataservice/api/common/model.py @@ -14,6 +14,12 @@ COMMON_ENUM = {"Not Reported", "Not Applicable", "Not Allowed To Collect", "Not Available", "Reported Unknown"} +VISIBILITY_REASON_DEFAULT = "unknown" +VISIBILITY_REASON_ENUM = { + "peddy_issue", "consent_issue", "ready_for_release", + VISIBILITY_REASON_DEFAULT +} + class KfId(types.TypeDecorator): """ @@ -203,3 +209,8 @@ class Base(IDMixin, TimestampMixin): nullable=False, server_default='true', doc='Flags visibility of data from the dataservice') + visibility_reason = db.Column( + db.Text(), + server_default=VISIBILITY_REASON_DEFAULT, + doc='Gives justification for the value in the visible column' + ) From fee41ec0140963bb345615e4b65daa866568f358 Mon Sep 17 00:00:00 2001 From: Natasha Singh Date: Thu, 16 Mar 2023 17:11:32 -0400 Subject: [PATCH 2/8] :recycle: Validate visibility_reason in base schema --- dataservice/api/common/model.py | 6 ++---- dataservice/api/common/schemas.py | 10 ++++++++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/dataservice/api/common/model.py b/dataservice/api/common/model.py index 38cc840b8..a9f1d0c67 100644 --- a/dataservice/api/common/model.py +++ b/dataservice/api/common/model.py @@ -14,10 +14,9 @@ COMMON_ENUM = {"Not Reported", "Not Applicable", "Not Allowed To Collect", "Not Available", "Reported Unknown"} -VISIBILITY_REASON_DEFAULT = "unknown" VISIBILITY_REASON_ENUM = { - "peddy_issue", "consent_issue", "ready_for_release", - VISIBILITY_REASON_DEFAULT + "Ready For Release", "Pre-Release", "Peddy Issue", "Consent Hold", + "File Quality Issue", "Other", "Unknown" } @@ -211,6 +210,5 @@ class Base(IDMixin, TimestampMixin): doc='Flags visibility of data from the dataservice') visibility_reason = db.Column( db.Text(), - server_default=VISIBILITY_REASON_DEFAULT, doc='Gives justification for the value in the visible column' ) diff --git a/dataservice/api/common/schemas.py b/dataservice/api/common/schemas.py index 0d1c51a36..846f17978 100644 --- a/dataservice/api/common/schemas.py +++ b/dataservice/api/common/schemas.py @@ -15,7 +15,9 @@ from flask_marshmallow import Schema from dataservice.api.common.pagination import Pagination, After from dataservice.api.common.validation import validate_kf_id +from dataservice.api.common.model import VISIBILITY_REASON_ENUM from dataservice.extensions import db + AVAILABILITY_ENUM = {'Immediate Download', 'Cold Storage'} @@ -55,6 +57,14 @@ def wrap_pre(self, data, many): def valid(self, value): validate_kf_id(self.Meta.model.__prefix__, value) + @validates('visibility_reason') + def validate_visibility_reason(self, value): + error_message = 'Not a valid choice. Must be one of: {}'.format( + ', '.join([str(item) for item in VISIBILITY_REASON_ENUM])) + + if value not in VISIBILITY_REASON_ENUM: + raise ValidationError(error_message) + @post_dump(pass_many=True) def wrap_envelope(self, data, many): """ From c762c4781d8250e2d8ba0b158b892ff0ea25b440 Mon Sep 17 00:00:00 2001 From: Natasha Singh Date: Thu, 16 Mar 2023 17:12:23 -0400 Subject: [PATCH 3/8] :white_check_mark: Test enumerated visibility_reason field --- tests/test_api.py | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/tests/test_api.py b/tests/test_api.py index 1bbea060d..b876dab16 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -9,6 +9,7 @@ ENTITY_PARAMS, _add_foreign_keys ) +from dataservice.api.common.model import VISIBILITY_REASON_ENUM class TestAPI: @@ -294,6 +295,48 @@ def test_bad_input(self, client, entities, endpoint, invalid_params, assert body['_status']['code'] == 400 assert 'could not {} '.format(action) in body['_status']['message'] + @pytest.mark.parametrize('reason', VISIBILITY_REASON_ENUM) + @pytest.mark.parametrize('endpoint', + [endpoint for endpoint in ENDPOINTS]) + def test_visibility_reason(self, client, entities, endpoint, reason): + """ Tests inputs to visibility_reason field """ + # Setup inputs + inputs = ENTITY_PARAMS['fields'][endpoint].copy() + model_cls = ENDPOINT_ENTITY_MAP.get(endpoint) + entity = entities.get(model_cls)[0] + _add_foreign_keys(inputs, entity) + + # Send request with bad value + kf_id = entity.kf_id + url = '{}/{}'.format(endpoint, kf_id) + + inputs = {"visibility_reason": reason} + resp = client.patch(url, data=json.dumps(inputs), + headers={'Content-Type': 'application/json'}) + + body = json.loads(resp.data.decode('utf-8')) + assert body['_status']['code'] == 200 + + @pytest.mark.parametrize('endpoint', + [endpoint for endpoint in ENDPOINTS]) + def test_bad_visibility_reason(self, client, entities, endpoint): + """ Tests bad inputs to visibility_reason field """ + # Setup inputs + inputs = ENTITY_PARAMS['fields'][endpoint].copy() + model_cls = ENDPOINT_ENTITY_MAP.get(endpoint) + entity = entities.get(model_cls)[0] + _add_foreign_keys(inputs, entity) + url = endpoint + + # Send request with bad value + inputs.update({"visibility_reason": "foobar"}) + resp = client.post(url, data=json.dumps(inputs), + headers={'Content-Type': 'application/json'}) + + body = json.loads(resp.data.decode('utf-8')) + assert body['_status']['code'] == 400 + assert 'Must be one of' in body['_status']['message'] + @pytest.mark.parametrize('method', ['POST']) @pytest.mark.parametrize('endpoint, field', [ From 9576669659c58aae15d1bb6e0bad883b9cae126a Mon Sep 17 00:00:00 2001 From: Natasha Singh Date: Mon, 10 Apr 2023 16:10:27 -0400 Subject: [PATCH 4/8] :sparkles: Add visibility_comment column for extra details --- dataservice/api/common/model.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dataservice/api/common/model.py b/dataservice/api/common/model.py index a9f1d0c67..d02965ac0 100644 --- a/dataservice/api/common/model.py +++ b/dataservice/api/common/model.py @@ -212,3 +212,7 @@ class Base(IDMixin, TimestampMixin): db.Text(), doc='Gives justification for the value in the visible column' ) + visibility_comment = db.Column( + db.Text(), + doc='Additional details for the visibility reason' + ) From 0b234f275d614d65db1e27cc9782b62bf14b2a11 Mon Sep 17 00:00:00 2001 From: Natasha Singh Date: Thu, 16 Mar 2023 17:11:59 -0400 Subject: [PATCH 5/8] :card_file_box: Add migration for visibility_reason --- migrations/versions/16e588cc6b85_.py | 155 +++++++++++++++++++++++++++ 1 file changed, 155 insertions(+) create mode 100644 migrations/versions/16e588cc6b85_.py diff --git a/migrations/versions/16e588cc6b85_.py b/migrations/versions/16e588cc6b85_.py new file mode 100644 index 000000000..c0334f972 --- /dev/null +++ b/migrations/versions/16e588cc6b85_.py @@ -0,0 +1,155 @@ +""" +Add visibility_reason and visibility_comment columns to capture +justification for visibility value + +Revision ID: 16e588cc6b85 +Revises: 475214ed802d +Create Date: 2023-03-16 13:15:59.929140 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '16e588cc6b85' +down_revision = '475214ed802d' +branch_labels = None +depends_on = None + + +def add_visibility_reason(): + # ### commands auto generated by Alembic - please adjust! ### + op.add_column('alias_group', sa.Column('visibility_reason', + sa.Text(), nullable=True)) + op.add_column('biospecimen', sa.Column('visibility_reason', + sa.Text(), nullable=True)) + op.add_column('biospecimen_diagnosis', sa.Column( + 'visibility_reason', sa.Text(), nullable=True)) + op.add_column('biospecimen_genomic_file', sa.Column( + 'visibility_reason', sa.Text(), nullable=True)) + op.add_column('cavatica_app', sa.Column('visibility_reason', + sa.Text(), nullable=True)) + op.add_column('diagnosis', sa.Column('visibility_reason', + sa.Text(), nullable=True)) + op.add_column('family', sa.Column('visibility_reason', + sa.Text(), nullable=True)) + op.add_column('family_relationship', sa.Column( + 'visibility_reason', sa.Text(), nullable=True)) + op.add_column('genomic_file', sa.Column('visibility_reason', + sa.Text(), nullable=True)) + op.add_column('investigator', sa.Column('visibility_reason', + sa.Text(), nullable=True)) + op.add_column('outcome', sa.Column('visibility_reason', + sa.Text(), nullable=True)) + op.add_column('participant', sa.Column('visibility_reason', + sa.Text(), nullable=True)) + op.add_column('phenotype', sa.Column('visibility_reason', + sa.Text(), nullable=True)) + op.add_column('read_group', sa.Column('visibility_reason', + sa.Text(), nullable=True)) + op.add_column('read_group_genomic_file', sa.Column( + 'visibility_reason', sa.Text(), nullable=True)) + op.add_column('sequencing_center', sa.Column( + 'visibility_reason', sa.Text(), nullable=True)) + op.add_column('sequencing_experiment', sa.Column( + 'visibility_reason', sa.Text(), nullable=True)) + op.add_column('sequencing_experiment_genomic_file', sa.Column( + 'visibility_reason', sa.Text(), nullable=True)) + op.add_column('study', sa.Column('visibility_reason', + sa.Text(), nullable=True)) + op.add_column('study_file', sa.Column('visibility_reason', + sa.Text(), nullable=True)) + op.add_column('task', sa.Column('visibility_reason', + sa.Text(), nullable=True)) + op.add_column('task_genomic_file', sa.Column( + 'visibility_reason', sa.Text(), nullable=True)) + # ### end Alembic commands ### + + +def remove_visibility_reason(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column('task_genomic_file', 'visibility_reason') + op.drop_column('task', 'visibility_reason') + op.drop_column('study_file', 'visibility_reason') + op.drop_column('study', 'visibility_reason') + op.drop_column('sequencing_experiment_genomic_file', 'visibility_reason') + op.drop_column('sequencing_experiment', 'visibility_reason') + op.drop_column('sequencing_center', 'visibility_reason') + op.drop_column('read_group_genomic_file', 'visibility_reason') + op.drop_column('read_group', 'visibility_reason') + op.drop_column('phenotype', 'visibility_reason') + op.drop_column('participant', 'visibility_reason') + op.drop_column('outcome', 'visibility_reason') + op.drop_column('investigator', 'visibility_reason') + op.drop_column('genomic_file', 'visibility_reason') + op.drop_column('family_relationship', 'visibility_reason') + op.drop_column('family', 'visibility_reason') + op.drop_column('diagnosis', 'visibility_reason') + op.drop_column('cavatica_app', 'visibility_reason') + op.drop_column('biospecimen_genomic_file', 'visibility_reason') + op.drop_column('biospecimen_diagnosis', 'visibility_reason') + op.drop_column('biospecimen', 'visibility_reason') + op.drop_column('alias_group', 'visibility_reason') + # ### end Alembic commands ### + +def add_visibility_comment(): + # ### commands auto generated by Alembic - please adjust! ### + op.add_column('alias_group', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('biospecimen', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('biospecimen_diagnosis', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('biospecimen_genomic_file', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('cavatica_app', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('diagnosis', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('family', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('family_relationship', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('genomic_file', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('investigator', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('outcome', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('participant', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('phenotype', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('read_group', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('read_group_genomic_file', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('sequencing_center', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('sequencing_experiment', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('sequencing_experiment_genomic_file', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('study', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('study_file', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('task', sa.Column('visibility_comment', sa.Text(), nullable=True)) + op.add_column('task_genomic_file', sa.Column('visibility_comment', sa.Text(), nullable=True)) + # ### end Alembic commands ### + + +def remove_visibility_comment(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column('task_genomic_file', 'visibility_comment') + op.drop_column('task', 'visibility_comment') + op.drop_column('study_file', 'visibility_comment') + op.drop_column('study', 'visibility_comment') + op.drop_column('sequencing_experiment_genomic_file', 'visibility_comment') + op.drop_column('sequencing_experiment', 'visibility_comment') + op.drop_column('sequencing_center', 'visibility_comment') + op.drop_column('read_group_genomic_file', 'visibility_comment') + op.drop_column('read_group', 'visibility_comment') + op.drop_column('phenotype', 'visibility_comment') + op.drop_column('participant', 'visibility_comment') + op.drop_column('outcome', 'visibility_comment') + op.drop_column('investigator', 'visibility_comment') + op.drop_column('genomic_file', 'visibility_comment') + op.drop_column('family_relationship', 'visibility_comment') + op.drop_column('family', 'visibility_comment') + op.drop_column('diagnosis', 'visibility_comment') + op.drop_column('cavatica_app', 'visibility_comment') + op.drop_column('biospecimen_genomic_file', 'visibility_comment') + op.drop_column('biospecimen_diagnosis', 'visibility_comment') + op.drop_column('biospecimen', 'visibility_comment') + op.drop_column('alias_group', 'visibility_comment') + # ### end Alembic commands ### + +def upgrade(): + add_visibility_reason() + add_visibility_comment() + +def downgrade(): + remove_visibility_reason() + remove_visibility_comment() From e5e32fee6521688b1c512f6cacadc6030967f21e Mon Sep 17 00:00:00 2001 From: Natasha Singh Date: Tue, 11 Apr 2023 10:35:30 -0400 Subject: [PATCH 6/8] :bug: Validate vis reason only if not null --- dataservice/api/common/schemas.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/dataservice/api/common/schemas.py b/dataservice/api/common/schemas.py index 846f17978..1485d1ee3 100644 --- a/dataservice/api/common/schemas.py +++ b/dataservice/api/common/schemas.py @@ -59,11 +59,11 @@ def valid(self, value): @validates('visibility_reason') def validate_visibility_reason(self, value): - error_message = 'Not a valid choice. Must be one of: {}'.format( - ', '.join([str(item) for item in VISIBILITY_REASON_ENUM])) - - if value not in VISIBILITY_REASON_ENUM: - raise ValidationError(error_message) + if value and (value not in VISIBILITY_REASON_ENUM): + raise ValidationError( + 'Not a valid choice. Must be one of: {}'.format( + ', '.join([str(item) for item in VISIBILITY_REASON_ENUM])) + ) @post_dump(pass_many=True) def wrap_envelope(self, data, many): From c9d236180eb4f868b91f34fa3b5c15f3afb1b771 Mon Sep 17 00:00:00 2001 From: Natasha Singh Date: Tue, 11 Apr 2023 14:41:35 -0400 Subject: [PATCH 7/8] :recycle: File Quality Issue -> Sequencing Quality Issue --- dataservice/api/common/model.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dataservice/api/common/model.py b/dataservice/api/common/model.py index d02965ac0..69abe439d 100644 --- a/dataservice/api/common/model.py +++ b/dataservice/api/common/model.py @@ -16,7 +16,7 @@ VISIBILITY_REASON_ENUM = { "Ready For Release", "Pre-Release", "Peddy Issue", "Consent Hold", - "File Quality Issue", "Other", "Unknown" + "Sequencing Quality Issue", "Other", "Unknown" } From da64bad2f49d42e6d89096577e2db63dbb679e6e Mon Sep 17 00:00:00 2001 From: Natasha Singh Date: Tue, 11 Apr 2023 14:54:32 -0400 Subject: [PATCH 8/8] :recycle: Replace Peddy Issue with Sample Issue --- dataservice/api/common/model.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dataservice/api/common/model.py b/dataservice/api/common/model.py index 69abe439d..ceacfc981 100644 --- a/dataservice/api/common/model.py +++ b/dataservice/api/common/model.py @@ -15,8 +15,8 @@ "Not Available", "Reported Unknown"} VISIBILITY_REASON_ENUM = { - "Ready For Release", "Pre-Release", "Peddy Issue", "Consent Hold", - "Sequencing Quality Issue", "Other", "Unknown" + "Ready For Release", "Pre-Release", "Sample Issue", + "Consent Hold", "Sequencing Quality Issue", "Other", "Unknown" }