diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b44e98f..1a53e1fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ - `email` and `additionalContacts` in API reference json schema (#380) ### Changed - Updated versions of external actions and MongoDB in GitHub workflows (#374) +- Allow updating contact's email and href separately using the command line (#381) ### Fixed - Fixed typo on conda installation docs (#373) diff --git a/patientMatcher/cli/add.py b/patientMatcher/cli/add.py index 69d1bafa..cce3fb09 100644 --- a/patientMatcher/cli/add.py +++ b/patientMatcher/cli/add.py @@ -4,6 +4,7 @@ import click from flask.cli import current_app, with_appcontext + from patientMatcher.resources import path_to_benchmark_patients from patientMatcher.utils.add import add_node, load_demo_patients from patientMatcher.utils.delete import drop_all_collections diff --git a/patientMatcher/cli/commands.py b/patientMatcher/cli/commands.py index cd99610f..f5e54f91 100644 --- a/patientMatcher/cli/commands.py +++ b/patientMatcher/cli/commands.py @@ -6,6 +6,7 @@ from flask import Flask, current_app from flask.cli import FlaskGroup, routes_command, run_command, with_appcontext from flask_mail import Message + from patientMatcher import __version__ from patientMatcher.server import create_app diff --git a/patientMatcher/cli/remove.py b/patientMatcher/cli/remove.py index 7be55c6c..e2908de3 100644 --- a/patientMatcher/cli/remove.py +++ b/patientMatcher/cli/remove.py @@ -3,6 +3,7 @@ import click from flask.cli import current_app, with_appcontext + from patientMatcher.utils.delete import delete_by_query diff --git a/patientMatcher/cli/update.py b/patientMatcher/cli/update.py index 28e7a02c..e49ae719 100644 --- a/patientMatcher/cli/update.py +++ b/patientMatcher/cli/update.py @@ -4,11 +4,13 @@ import click from flask.cli import current_app, with_appcontext + from patientMatcher.parse.patient import EMAIL_REGEX, href_validate from patientMatcher.utils.patient import patients from patientMatcher.utils.update import update_resources LOG = logging.getLogger(__name__) +HREF_FIELD = "contact.href" @click.group() @@ -19,54 +21,69 @@ def update(): @update.command() @with_appcontext -@click.option( - "-o", "--old-href", type=click.STRING, nargs=1, required=True, help="Old contact href" -) -@click.option("-h", "--href", type=click.STRING, nargs=1, required=True, help="New contact href") -@click.option("-n", "--name", type=click.STRING, nargs=1, required=True, help="New contact name") -@click.option( - "--institution", type=click.STRING, nargs=1, required=False, help="New contact institution" -) -def contact(old_href, href, name, institution): - """Update contact person for a group of patients""" - - # If new contact is a simple email, add "mailto" schema - if bool(EMAIL_REGEX.match(href)) is True and "mailto:" not in href: - href = ":".join(["mailto", href]) - - if href_validate(href) is False: - LOG.error( - "Provided href does not have a valid schema. Provide either a URL (http://.., https://..) or an email address (mailto:..)" +@click.option("--href", type=click.STRING, required=False, help="Contact's href") +@click.option("--email", type=click.STRING, required=False, help="Contact's email") +@click.option("--new-href", type=click.STRING, required=False, help="New href") +@click.option("--new-email", type=click.STRING, required=False, help="New email") +@click.option("--new-name", type=click.STRING, required=False, help="New name") +@click.option("--new-institution", type=click.STRING, required=False, help="New institution") +def contact(href, email, new_href, new_email, new_name, new_institution): + """Update contact person for a group of patients.""" + + # Validate exactly one identifier + if bool(href) == bool(email): + raise click.UsageError("You must provide EITHER --href or --email") + + # Validate at least one field to update + if not any([new_href, new_email, new_name, new_institution]): + click.echo( + "Provide at least a field you wish to update: " + "--new-href / --new-email / --new-name / --new-institution" ) return - database = current_app.db - query = {"contact.href": {"$regex": old_href}} + # Build query + query = {HREF_FIELD: {"$regex": href}} if href else {"contact.email": email} - # Retrieving all patients matching the given old_href - old_contact_patients = patients(database=database, match_query=query) - # Retriving unique contacts for the above patients - match_contacts = list(old_contact_patients.distinct("contact.href")) + database = current_app.db + matching_patients = patients(database=database, match_query=query) + matching_contacts = list(matching_patients.distinct(HREF_FIELD)) - if len(match_contacts) == 0: - click.echo(f"No patients found with contact URI '{old_href}'") + if not matching_contacts: + click.echo(f"No patients found with query '{query}'") return - if len(match_contacts) > 1: + if len(matching_contacts) > 1: click.echo( - f"Your search for contact url '{old_href}' is returning more than one patients' contact: {match_contacts}.\nPlease restrict your search by typing a different href." + f"Your search for contact query '{query}' is returning more than one patients' contact.\n" + "Please restrict your search by typing a different href/email." ) return - # Search is returning only one contact, it's OK to use it for updating patients - matches = list(old_contact_patients) - new_contact = dict(href=href, name=name) - if institution: - new_contact["institution"] = institution + # Build update dict + set_options = {} + if new_href: + if EMAIL_REGEX.match(new_href) and not new_href.startswith("mailto:"): + new_href = f"mailto:{new_href}" + if not href_validate(new_href): + LOG.error( + "Provided href does not have a valid schema. Provide either a URL (http://.., https://..) or an email address (mailto:..)" + ) + return + set_options[HREF_FIELD] = new_href + if new_email: + set_options["contact.email"] = new_email + if new_name: + set_options["contact.name"] = new_name + if new_institution: + set_options["contact.institution"] = new_institution + + # Confirm and update + patient_count = len(list(matching_patients)) if click.confirm( - f"{len(matches)} patients with the old contact href '{matches[0]['contact']['href']}' will be updated with contact info:{new_contact}. Confirm?", + f"{patient_count} patients will be updated with contact info: {set_options}. Confirm?", abort=True, ): - result = database.patients.update_many(query, {"$set": {"contact": new_contact}}) + result = database.patients.update_many(query, {"$set": set_options}) click.echo(f"Contact information was updated for {result.modified_count} patients.") diff --git a/patientMatcher/match/handler.py b/patientMatcher/match/handler.py index 6d7c2689..5ed2b3fe 100644 --- a/patientMatcher/match/handler.py +++ b/patientMatcher/match/handler.py @@ -5,10 +5,11 @@ import logging import requests +from werkzeug.datastructures import Headers + from patientMatcher.match.genotype_matcher import match as genomatch from patientMatcher.match.phenotype_matcher import match as phenomatch from patientMatcher.parse.patient import json_patient -from werkzeug.datastructures import Headers LOG = logging.getLogger(__name__) diff --git a/patientMatcher/parse/patient.py b/patientMatcher/parse/patient.py index 9052e548..0593f0c9 100644 --- a/patientMatcher/parse/patient.py +++ b/patientMatcher/parse/patient.py @@ -7,6 +7,7 @@ from urllib.parse import urlparse from jsonschema import FormatChecker, RefResolver, validate + from patientMatcher.utils.gene import ensembl_to_symbol, entrez_to_symbol, symbol_to_ensembl from patientMatcher.utils.variant import liftover diff --git a/patientMatcher/server/__init__.py b/patientMatcher/server/__init__.py index 177e7166..0b4a4fb4 100644 --- a/patientMatcher/server/__init__.py +++ b/patientMatcher/server/__init__.py @@ -8,11 +8,7 @@ from flask import Flask from flask_mail import Mail from pymongo import MongoClient -from pymongo.errors import ( - ConnectionFailure, - OperationFailure, - ServerSelectionTimeoutError, -) +from pymongo.errors import ConnectionFailure, OperationFailure, ServerSelectionTimeoutError from patientMatcher.resources import path_to_hpo_terms, path_to_phenotype_annotations from patientMatcher.utils.notify import TlsSMTPHandler, admins_email_format diff --git a/patientMatcher/server/controllers.py b/patientMatcher/server/controllers.py index ced963f2..f0c0a6c0 100644 --- a/patientMatcher/server/controllers.py +++ b/patientMatcher/server/controllers.py @@ -3,6 +3,7 @@ from flask import current_app, jsonify from jsonschema import ValidationError + from patientMatcher.__version__ import __version__ from patientMatcher.auth.auth import authorize from patientMatcher.constants import STATUS_CODES diff --git a/patientMatcher/server/views.py b/patientMatcher/server/views.py index 3c53dbe0..a24dc4ae 100644 --- a/patientMatcher/server/views.py +++ b/patientMatcher/server/views.py @@ -6,6 +6,7 @@ from bson import json_util from flask import Blueprint, current_app, jsonify, render_template, request, send_from_directory from flask_negotiate import consumes, produces + from patientMatcher.auth.auth import authorize from patientMatcher.match.handler import internal_matcher, patient_matches from patientMatcher.utils.add import backend_add_patient diff --git a/patientMatcher/utils/add.py b/patientMatcher/utils/add.py index 145cce64..43d8b042 100644 --- a/patientMatcher/utils/add.py +++ b/patientMatcher/utils/add.py @@ -5,9 +5,10 @@ import sys import enlighten +from pymongo import MongoClient + from patientMatcher.match.handler import external_matcher from patientMatcher.parse.patient import mme_patient -from pymongo import MongoClient LOG = logging.getLogger(__name__) diff --git a/patientMatcher/utils/notify.py b/patientMatcher/utils/notify.py index d90f227f..b081b055 100644 --- a/patientMatcher/utils/notify.py +++ b/patientMatcher/utils/notify.py @@ -4,6 +4,7 @@ from logging.handlers import SMTPHandler from flask_mail import Message + from patientMatcher.__version__ import __version__ LOG = logging.getLogger(__name__) diff --git a/patientMatcher/utils/patient.py b/patientMatcher/utils/patient.py index 454ce34a..866d58b6 100644 --- a/patientMatcher/utils/patient.py +++ b/patientMatcher/utils/patient.py @@ -50,10 +50,7 @@ def patients(database, ids=None, match_query=None): if ids: # if only specified patients should be returned query["_id"] = {"$in": ids} elif match_query: - LOG.info("Return patients matching query {}".format(match_query)) query = match_query - else: - LOG.info("Return all patients in database") results = database["patients"].find(query) return results diff --git a/patientMatcher/utils/update.py b/patientMatcher/utils/update.py index 2a4a2534..74ca7fc3 100644 --- a/patientMatcher/utils/update.py +++ b/patientMatcher/utils/update.py @@ -3,6 +3,7 @@ import requests from clint.textui import progress + from patientMatcher.constants import PHENOTYPE_TERMS LOG = logging.getLogger(__name__) diff --git a/tests/backend/test_backend_patient.py b/tests/backend/test_backend_patient.py index d1c58245..a8e13b6c 100644 --- a/tests/backend/test_backend_patient.py +++ b/tests/backend/test_backend_patient.py @@ -3,6 +3,7 @@ import os import pytest + from patientMatcher.parse.patient import mme_patient from patientMatcher.utils.add import backend_add_patient, load_demo_patients from patientMatcher.utils.delete import delete_by_query diff --git a/tests/cli/test_commands.py b/tests/cli/test_commands.py index d7abda9e..5d258206 100644 --- a/tests/cli/test_commands.py +++ b/tests/cli/test_commands.py @@ -1,6 +1,7 @@ #!/usr/bin/env python3 # -*- coding: utf-8 -*- from flask_mail import Message + from patientMatcher.cli.commands import cli diff --git a/tests/cli/test_update.py b/tests/cli/test_update.py index 66e039d4..8cfa3c32 100644 --- a/tests/cli/test_update.py +++ b/tests/cli/test_update.py @@ -1,8 +1,11 @@ import responses + from patientMatcher.cli.commands import cli from patientMatcher.constants import PHENOTYPE_TERMS CONTACT_HREF = "contact.href" +NEW_HREF = "https://test.com" +NEW_EMAIL = "new.email@mail.com" NEW_NAME = "New Name" TEST_INST = "Test Institution" @@ -34,115 +37,175 @@ def test_update_resources(mock_app): assert result.exit_code == 0 -def test_update_contact(mock_app, gpx4_patients): - """Test the command to bulk-update patients contact""" +def test_update_contact_success(mock_app, gpx4_patients, monkeypatch): + """Test updating a contact successfully""" runner = mock_app.test_cli_runner() patients_collection = mock_app.db.patients - # GIVEN a database with some patients + # GIVEN a database with patients patients_collection.insert_many(gpx4_patients) - test_patients = patients_collection.find() - # Sharing a contact information - contacts = test_patients.distinct(CONTACT_HREF) + contacts = list(patients_collection.find().distinct(CONTACT_HREF)) assert len(contacts) == 1 - # WHEN their contact info is updated using the cli - new_href = "new.contact@mail.com" + # Mock click.confirm to always return True + monkeypatch.setattr("click.confirm", lambda *args, **kwargs: True) + + # WHEN updating contact info result = runner.invoke( cli, [ "update", "contact", - "--old-href", - contacts[0], "--href", - new_href, - "--name", + contacts[0], + "--new-href", + NEW_HREF, + "--new-email", + NEW_EMAIL, + "--new-name", NEW_NAME, - "--institution", + "--new-institution", TEST_INST, ], - input="y", ) + + # THEN the update should succeed assert result.exit_code == 0 + assert "Contact information was updated" in result.output + + updated_patient = list(patients_collection.find({CONTACT_HREF: NEW_HREF})) + assert len(updated_patient) > 0 + assert updated_patient[0]["contact"]["href"] == NEW_HREF + assert updated_patient[0]["contact"]["email"] == NEW_EMAIL + assert updated_patient[0]["contact"]["name"] == NEW_NAME + assert updated_patient[0]["contact"]["institution"] == TEST_INST - # THEN the config info should be updated - updated_patient = patients_collection.find({CONTACT_HREF: ":".join(["mailto", new_href])}) - assert len(list(updated_patient)) > 0 + +def test_update_contact_validation(mock_app, gpx4_patients): + """Test validations for contact CLI""" + + runner = mock_app.test_cli_runner() + patients_collection = mock_app.db.patients + + patients_collection.insert_many(gpx4_patients) + contacts = list(patients_collection.find().distinct(CONTACT_HREF)) + assert len(contacts) == 1 + + # WHEN no update fields are provided + result_no_update = runner.invoke( + cli, + ["update", "contact", "--href", contacts[0]], + ) + assert result_no_update.exit_code == 0 + assert "Provide at least a field you wish to update" in result_no_update.output + + # WHEN both --href and --email are provided + result_invalid = runner.invoke( + cli, + [ + "update", + "contact", + "--href", + contacts[0], + "--email", + "some@email.com", + "--new-name", + "Name", + ], + ) + assert result_invalid.exit_code != 0 + assert "You must provide EITHER --href or --email" in result_invalid.output def test_update_contact_no_href_match(mock_app, gpx4_patients): - """Test the command to bulk-update patients contact when old contact href is not matching any patients""" + """ + Test the command when the provided old contact href does not match any patients. + The command should not update anything and should output a message. + """ runner = mock_app.test_cli_runner() patients_collection = mock_app.db.patients # GIVEN a database with some patients patients_collection.insert_many(gpx4_patients) - test_patients = patients_collection.find() - # Sharing a contact information - contacts = test_patients.distinct(CONTACT_HREF) + contacts = list(patients_collection.find().distinct(CONTACT_HREF)) assert len(contacts) == 1 - old_contact_href = contacts[0] + existing_href = contacts[0] - # GIVEN a contact href without matches in the patients documents - wrong_href = "some_href" - assert wrong_href not in old_contact_href + # GIVEN a contact href that doesn't exist in the database + wrong_href = "nonexistent_href" + assert wrong_href not in existing_href - # WHEN their contact info is updated using the cli + # WHEN attempting to update using the CLI new_href = "new.contact@mail.com" result = runner.invoke( cli, [ "update", "contact", - "--old-href", - wrong_href, "--href", + wrong_href, + "--new-href", new_href, - "--name", + "--new-name", NEW_NAME, - "--institution", + "--new-institution", TEST_INST, ], + input="y", # in case the CLI asks for confirmation ) + + # THEN the CLI should succeed but print a message about no patients found assert result.exit_code == 0 + assert "No patients found with query" in result.output - # THEN no patients contact should be updated - assert patients_collection.find_one({CONTACT_HREF: ":".join(["mailto", new_href])}) is None + # AND no patient contact should be updated + updated_patient = patients_collection.find_one({CONTACT_HREF: f"mailto:{new_href}"}) + assert updated_patient is None def test_update_contact_multiple_href_match(mock_app, gpx4_patients): - """Test the command to bulk-update patients contact when old contact href is matching more than one patient contact""" + """ + Test the command when the old contact href matches multiple patients. + The CLI should print a warning and not update any patient contact. + """ runner = mock_app.test_cli_runner() patients_collection = mock_app.db.patients - assert len(gpx4_patients) == 2 - # GIVEN a database with 2 patients with sligthly different contact href + assert len(gpx4_patients) >= 2 + + # GIVEN a database with 2 patients sharing similar href patterns gpx4_patients[0]["contact"]["href"] = "test_1@mail.com" - gpx4_patients[0]["contact"]["href"] = "test_2@mail.com" + gpx4_patients[1]["contact"]["href"] = "test_2@mail.com" patients_collection.insert_many(gpx4_patients) - # WHEN their contact info is updated using the cli but the search for the old href returns multiple contacts - old_href = "test_" + # WHEN attempting to update with a href pattern that matches multiple contacts + old_href_pattern = "test_" # this will match both test_1 and test_2 new_href = "test_3@mail.com" + result = runner.invoke( cli, [ "update", "contact", - "--old-href", - old_href, "--href", + old_href_pattern, + "--new-href", new_href, - "--name", + "--new-name", NEW_NAME, - "--institution", + "--new-institution", TEST_INST, ], + input="y", ) - # THEN no patients contact should be updated - assert patients_collection.find_one({CONTACT_HREF: ":".join(["mailto", new_href])}) is None + # THEN the CLI should print a warning about multiple matches + assert result.exit_code == 0 + assert "returning more than one patients' contact" in result.output + + # AND no patient should be updated + updated_patient = patients_collection.find_one({CONTACT_HREF: f"mailto:{new_href}"}) + assert updated_patient is None diff --git a/tests/conftest.py b/tests/conftest.py index 78869331..5d65b011 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4,6 +4,7 @@ import mongomock import pytest import responses + from patientMatcher.resources import path_to_benchmark_patients from patientMatcher.server import create_app diff --git a/tests/match/test_GT_matching.py b/tests/match/test_GT_matching.py index 6c8cd302..cb24b7bb 100644 --- a/tests/match/test_GT_matching.py +++ b/tests/match/test_GT_matching.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- import responses + from patientMatcher.match.genotype_matcher import match from patientMatcher.parse.patient import mme_patient diff --git a/tests/match/test_matching_handler.py b/tests/match/test_matching_handler.py index f2fdfa94..1541d27a 100644 --- a/tests/match/test_matching_handler.py +++ b/tests/match/test_matching_handler.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- import requests import responses + from patientMatcher.match.handler import external_matcher, internal_matcher from patientMatcher.parse.patient import mme_patient from patientMatcher.utils.add import backend_add_patient diff --git a/tests/server/test_server_responses.py b/tests/server/test_server_responses.py index 02bc3500..098ab9a1 100644 --- a/tests/server/test_server_responses.py +++ b/tests/server/test_server_responses.py @@ -2,6 +2,7 @@ import json import responses + from patientMatcher.__version__ import __version__ from patientMatcher.match.handler import patient_matches from patientMatcher.parse.patient import mme_patient diff --git a/tests/utils/test_ensembl_rest_api.py b/tests/utils/test_ensembl_rest_api.py index 0fe31d0d..4bf9d9ad 100644 --- a/tests/utils/test_ensembl_rest_api.py +++ b/tests/utils/test_ensembl_rest_api.py @@ -1,9 +1,10 @@ # -*- coding: UTF-8 -*- import pytest import responses -from patientMatcher.utils import ensembl_rest_client as ensembl_api from requests.exceptions import MissingSchema +from patientMatcher.utils import ensembl_rest_client as ensembl_api + @responses.activate def test_except_on_invalid_response(): diff --git a/tests/utils/test_gene.py b/tests/utils/test_gene.py index 7fc8761b..007be56e 100644 --- a/tests/utils/test_gene.py +++ b/tests/utils/test_gene.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- import responses + from patientMatcher.utils.ensembl_rest_client import requests from patientMatcher.utils.gene import ensembl_to_symbol, entrez_to_symbol, symbol_to_ensembl diff --git a/tests/utils/test_variant.py b/tests/utils/test_variant.py index 938c752b..2a844197 100644 --- a/tests/utils/test_variant.py +++ b/tests/utils/test_variant.py @@ -1,4 +1,5 @@ import responses + from patientMatcher.utils.variant import liftover