diff --git a/alembic/versions/68a0ec57694e_add_active_column_to_licenses.py b/alembic/versions/68a0ec57694e_add_active_column_to_licenses.py new file mode 100644 index 00000000..5547828e --- /dev/null +++ b/alembic/versions/68a0ec57694e_add_active_column_to_licenses.py @@ -0,0 +1,29 @@ +"""Add active column to licenses + +Revision ID: 68a0ec57694e +Revises: 2b6f40ea2fb6 +Create Date: 2024-10-22 15:36:41.868909 + +""" + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = "68a0ec57694e" +down_revision = "2b6f40ea2fb6" +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.add_column("licenses", sa.Column("active", sa.Boolean(), nullable=False, server_default=sa.true())) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column("licenses", "active") + # ### end Alembic commands ### diff --git a/src/mavedb/models/license.py b/src/mavedb/models/license.py index a7096a09..65efa67c 100644 --- a/src/mavedb/models/license.py +++ b/src/mavedb/models/license.py @@ -1,6 +1,6 @@ from datetime import date -from sqlalchemy import Column, Date, Integer, String +from sqlalchemy import Boolean, Column, Date, Integer, String from mavedb.db.base import Base @@ -16,3 +16,4 @@ class License(Base): version = Column(String, nullable=True, unique=False) creation_date = Column(Date, nullable=False, default=date.today) modification_date = Column(Date, nullable=False, default=date.today, onupdate=date.today) + active = Column(Boolean, nullable=False) diff --git a/src/mavedb/routers/licenses.py b/src/mavedb/routers/licenses.py index 0bded44a..78b29aa1 100644 --- a/src/mavedb/routers/licenses.py +++ b/src/mavedb/routers/licenses.py @@ -23,6 +23,19 @@ def list_licenses( return items +@router.get("/active", status_code=200, response_model=List[license.ShortLicense], responses={404: {}}) +def list_active_licenses( + *, + db: Session = Depends(deps.get_db), +) -> Any: + """ + List active licenses. + """ + + items = db.query(License).where(License.active.is_(True)).order_by(License.short_name).all() + return items + + @router.get("/{item_id}", status_code=200, response_model=license.License, responses={404: {}}) def fetch_license( *, diff --git a/src/mavedb/routers/score_sets.py b/src/mavedb/routers/score_sets.py index d294d9c0..353ee1ab 100644 --- a/src/mavedb/routers/score_sets.py +++ b/src/mavedb/routers/score_sets.py @@ -348,6 +348,11 @@ async def create_score_set( if not license_: logger.info(msg="Failed to create score set; The requested license does not exist.", extra=logging_context()) raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="Unknown license") + elif not license_.active: + logger.info( + msg="Failed to create score set; The requested license is no longer active.", extra=logging_context() + ) + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="Invalid license") save_to_logging_context({"requested_superseded_score_set": item_create.superseded_score_set_urn}) if item_create.superseded_score_set_urn is not None: @@ -698,6 +703,14 @@ async def update_score_set( ) raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="Unknown license") + # Allow in-active licenses to be retained on update if they already exist on the item. + elif not license_.active and item.licence_id != item_update.license_id: + logger.info( + msg="Failed to update score set license; The requested license is no longer active.", + extra=logging_context(), + ) + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="Invalid license") + item.license = license_ item.doi_identifiers = [ diff --git a/src/mavedb/view_models/license.py b/src/mavedb/view_models/license.py index 9b870b5e..85d92a53 100644 --- a/src/mavedb/view_models/license.py +++ b/src/mavedb/view_models/license.py @@ -10,6 +10,7 @@ class LicenseBase(BaseModel): long_name: str short_name: str + active: bool link: Optional[str] version: Optional[str] diff --git a/tests/helpers/constants.py b/tests/helpers/constants.py index ce74e89f..c6c88269 100644 --- a/tests/helpers/constants.py +++ b/tests/helpers/constants.py @@ -14,6 +14,7 @@ VALID_GENE = "BRCA1" SAVED_PUBMED_PUBLICATION = { + "recordType": "PublicationIdentifier", "identifier": "20711194", "dbName": "PubMed", "title": "None", @@ -28,6 +29,7 @@ } SAVED_DOI_IDENTIFIER = { + "recordType": "DoiIdentifier", "identifier": TEST_CROSSREF_IDENTIFIER, "url": f"https://doi.org/{TEST_CROSSREF_IDENTIFIER}", "id": 1, @@ -51,6 +53,7 @@ } SAVED_CONTRIBUTOR = { + "recordType": "Contributor", "orcidId": TEST_USER["username"], "givenName": TEST_USER["first_name"], "familyName": TEST_USER["last_name"], @@ -80,6 +83,7 @@ } SAVED_EXTRA_CONTRIBUTOR = { + "recordType": "Contributor", "orcidId": EXTRA_USER["username"], "givenName": EXTRA_USER["first_name"], "familyName": EXTRA_USER["last_name"], @@ -347,14 +351,17 @@ "text": "Don't be evil.", "link": "localhost", "version": "1.0", + "active": True, } SAVED_SHORT_TEST_LICENSE = { + "recordType": "ShortLicense", "id": TEST_LICENSE["id"], "shortName": TEST_LICENSE["short_name"], "longName": TEST_LICENSE["long_name"], "link": TEST_LICENSE["link"], "version": TEST_LICENSE["version"], + "active": TEST_LICENSE["active"], } EXTRA_LICENSE = { @@ -364,14 +371,37 @@ "text": "Don't be tooooo evil.", "link": "localhost", "version": "1.0", + "active": True, } SAVED_SHORT_EXTRA_LICENSE = { + "recordType": "ShortLicense", "id": EXTRA_LICENSE["id"], "shortName": EXTRA_LICENSE["short_name"], "longName": EXTRA_LICENSE["long_name"], "link": EXTRA_LICENSE["link"], "version": EXTRA_LICENSE["version"], + "active": EXTRA_LICENSE["active"], +} + +TEST_INACTIVE_LICENSE = { + "id": 3, + "short_name": "Long", + "long_name": "Short", + "text": "Be evil.", + "link": "localhost", + "version": "1.0", + "active": False, +} + +SAVED_SHORT_INACTIVE_LICENSE = { + "recordType": "ShortLicense", + "id": TEST_INACTIVE_LICENSE["id"], + "shortName": TEST_INACTIVE_LICENSE["short_name"], + "longName": TEST_INACTIVE_LICENSE["long_name"], + "link": TEST_INACTIVE_LICENSE["link"], + "version": TEST_INACTIVE_LICENSE["version"], + "active": TEST_INACTIVE_LICENSE["active"], } TEST_SEQ_SCORESET = { diff --git a/tests/helpers/util.py b/tests/helpers/util.py index ee872004..108ec983 100644 --- a/tests/helpers/util.py +++ b/tests/helpers/util.py @@ -12,6 +12,7 @@ from mavedb.models.contributor import Contributor from mavedb.models.enums.processing_state import ProcessingState from mavedb.models.score_set import ScoreSet as ScoreSetDbModel +from mavedb.models.license import License from mavedb.models.user import User from mavedb.view_models.experiment import Experiment, ExperimentCreate from mavedb.view_models.score_set import ScoreSet, ScoreSetCreate @@ -52,6 +53,17 @@ def change_ownership(db, urn, model): db.commit() +def change_to_inactive_license(db, urn): + """Change the license of the score set with given urn to an inactive license.""" + item = db.query(ScoreSetDbModel).filter(ScoreSetDbModel.urn == urn).one_or_none() + assert item is not None + license = db.query(License).filter(License.active.is_(False)).first() + assert license is not None + item.license_id = license.id + db.add(item) + db.commit() + + def create_experiment(client, update=None): experiment_payload = deepcopy(TEST_MINIMAL_EXPERIMENT) if update is not None: diff --git a/tests/lib/conftest.py b/tests/lib/conftest.py index 94860c35..076dac4b 100644 --- a/tests/lib/conftest.py +++ b/tests/lib/conftest.py @@ -5,7 +5,14 @@ from mavedb.models.role import Role from mavedb.models.taxonomy import Taxonomy from mavedb.models.user import User -from tests.helpers.constants import ADMIN_USER, EXTRA_USER, TEST_LICENSE, TEST_TAXONOMY, TEST_USER +from tests.helpers.constants import ( + ADMIN_USER, + EXTRA_USER, + TEST_LICENSE, + TEST_INACTIVE_LICENSE, + TEST_TAXONOMY, + TEST_USER, +) @pytest.fixture @@ -20,4 +27,5 @@ def setup_lib_db(session): db.add(User(**ADMIN_USER, role_objs=[Role(name=UserRole.admin)])) db.add(Taxonomy(**TEST_TAXONOMY)) db.add(License(**TEST_LICENSE)) + db.add(License(**TEST_INACTIVE_LICENSE)) db.commit() diff --git a/tests/routers/conftest.py b/tests/routers/conftest.py index e0ef23ee..f16ff93b 100644 --- a/tests/routers/conftest.py +++ b/tests/routers/conftest.py @@ -19,6 +19,7 @@ TEST_CDOT_TRANSCRIPT, TEST_DB_KEYWORDS, TEST_LICENSE, + TEST_INACTIVE_LICENSE, EXTRA_LICENSE, TEST_TAXONOMY, TEST_USER, @@ -44,6 +45,7 @@ def setup_router_db(session): db.add(User(**ADMIN_USER, role_objs=[Role(name=UserRole.admin)])) db.add(Taxonomy(**TEST_TAXONOMY)) db.add(License(**TEST_LICENSE)) + db.add(License(**TEST_INACTIVE_LICENSE)) db.add(License(**EXTRA_LICENSE)) db.add(Contributor(**EXTRA_CONTRIBUTOR)) db.bulk_save_objects([ControlledKeyword(**keyword_obj) for keyword_obj in TEST_DB_KEYWORDS]) diff --git a/tests/routers/test_licenses.py b/tests/routers/test_licenses.py new file mode 100644 index 00000000..97c487a3 --- /dev/null +++ b/tests/routers/test_licenses.py @@ -0,0 +1,47 @@ +import pytest + +from tests.helpers.constants import TEST_LICENSE +from tests.helpers.dependency_overrider import DependencyOverrider + + +@pytest.mark.parametrize("user_overrides", [None, "anonymous_app_overrides", "admin_app_overrides"]) +def test_can_list_licenses_as_any_user_class(setup_router_db, client, user_overrides, request): + if user_overrides is not None: + dep_overrides = request.getfixturevalue(user_overrides) + with DependencyOverrider(dep_overrides): + response = client.get("/api/v1/licenses/") + else: + response = client.get("/api/v1/licenses/") + + assert response.status_code == 200 + response_value = response.json() + assert len(response_value) == 3 + + +@pytest.mark.parametrize("user_overrides", [None, "anonymous_app_overrides", "admin_app_overrides"]) +def test_can_list_active_licenses_as_any_user_class(setup_router_db, client, user_overrides, request): + if user_overrides is not None: + dep_overrides = request.getfixturevalue(user_overrides) + with DependencyOverrider(dep_overrides): + response = client.get("/api/v1/licenses/active") + else: + response = client.get("/api/v1/licenses/active") + + assert response.status_code == 200 + response_value = response.json() + assert len(response_value) == 2 + license_state = [_license["active"] for _license in response_value] + assert all(license_state) + + +def test_can_fetch_arbitrary_license(setup_router_db, client): + response = client.get("/api/v1/licenses/1") + + assert response.status_code == 200 + response_value = response.json() + response_value["text"] == TEST_LICENSE["text"] + + +def test_cannot_fetch_nonexistent_license(setup_router_db, client): + response = client.get("/api/v1/licenses/100") + assert response.status_code == 404 diff --git a/tests/routers/test_score_set.py b/tests/routers/test_score_set.py index b9a0f801..1b64683f 100644 --- a/tests/routers/test_score_set.py +++ b/tests/routers/test_score_set.py @@ -29,6 +29,7 @@ TEST_SAVED_SCORESET_RANGE, TEST_MINIMAL_ACC_SCORESET_RESPONSE, TEST_USER, + TEST_INACTIVE_LICENSE, SAVED_DOI_IDENTIFIER, SAVED_EXTRA_CONTRIBUTOR, SAVED_PUBMED_PUBLICATION, @@ -38,6 +39,7 @@ from tests.helpers.util import ( add_contributor, change_ownership, + change_to_inactive_license, create_experiment, create_seq_score_set, create_seq_score_set_with_variants, @@ -1525,3 +1527,33 @@ def test_contributor_can_add_score_set_to_others_public_experiment( score_set_post_payload["experimentUrn"] = published_score_set["experiment"]["urn"] response = client.post("/api/v1/score-sets/", json=score_set_post_payload) assert response.status_code == 200 + + +def test_cannot_create_score_set_with_inactive_license(session, client, setup_router_db): + experiment = create_experiment(client) + score_set_post_payload = deepcopy(TEST_MINIMAL_SEQ_SCORESET) + score_set_post_payload["experimentUrn"] = experiment["urn"] + score_set_post_payload["licenseId"] = TEST_INACTIVE_LICENSE["id"] + response = client.post("/api/v1/score-sets/", json=score_set_post_payload) + assert response.status_code == 400 + + +def test_cannot_modify_score_set_to_inactive_license(session, client, setup_router_db): + experiment = create_experiment(client) + score_set = create_seq_score_set(client, experiment["urn"]) + score_set_post_payload = score_set.copy() + score_set_post_payload.update({"licenseId": TEST_INACTIVE_LICENSE["id"], "urn": score_set["urn"]}) + response = client.put(f"/api/v1/score-sets/{score_set['urn']}", json=score_set_post_payload) + assert response.status_code == 400 + + +def test_can_modify_metadata_for_score_set_with_inactive_license(session, client, setup_router_db): + experiment = create_experiment(client) + score_set = create_seq_score_set(client, experiment["urn"]) + change_to_inactive_license(session, score_set["urn"]) + score_set_post_payload = score_set.copy() + score_set_post_payload.update({"title": "Update title", "urn": score_set["urn"]}) + response = client.put(f"/api/v1/score-sets/{score_set['urn']}", json=score_set_post_payload) + assert response.status_code == 200 + response_data = response.json() + assert ("title", response_data["title"]) == ("title", "Update title") diff --git a/tests/worker/conftest.py b/tests/worker/conftest.py index 3a2a4cf6..7d989005 100644 --- a/tests/worker/conftest.py +++ b/tests/worker/conftest.py @@ -6,7 +6,7 @@ from mavedb.models.license import License from mavedb.models.taxonomy import Taxonomy from mavedb.models.user import User -from tests.helpers.constants import EXTRA_USER, TEST_LICENSE, TEST_TAXONOMY, TEST_USER +from tests.helpers.constants import EXTRA_USER, TEST_LICENSE, TEST_INACTIVE_LICENSE, TEST_TAXONOMY, TEST_USER from tests.helpers.util import create_experiment, create_seq_score_set @@ -17,6 +17,7 @@ def setup_worker_db(session): db.add(User(**EXTRA_USER)) db.add(Taxonomy(**TEST_TAXONOMY)) db.add(License(**TEST_LICENSE)) + db.add(License(**TEST_INACTIVE_LICENSE)) db.commit()