Repository: Clinical-Genomics/patientMatcher

Issue:
delete patient should delete its matches as well
Should work from the cli and from DELETE requests

Discussion:

Patch:
diff --git a/README.md b/README.md
index de4b514..3529f9d 100644
--- a/README.md
+++ b/README.md
@@ -2,7 +2,7 @@
 [![Build Status](https://travis-ci.com/northwestwitch/patientMatcher.svg?branch=master)](https://travis-ci.com/northwestwitch/patientMatcher) [![Coverage Status](https://coveralls.io/repos/github/Clinical-Genomics/patientMatcher/badge.svg?branch=master&kill_cache=1)](https://coveralls.io/github/Clinical-Genomics/patientMatcher?branch=master)
 
 ## Prerequisites
-To use this server you'll need to have a working instance of MongoDB. from the mongo shell you can create a database and an authenticated user to handle connections using this syntax:
+To use this server you'll need to have a working instance of **MongoDB**. from the mongo shell you can create a database and an authenticated user to handle connections using this syntax:
 
 ```bash
 use pmatcher
@@ -14,6 +14,11 @@ db.createUser(
    }
 )
 ```
+After setting up the restricted access to the server you'll just have to launch the mongo demon using authentication:
+```bash
+mongod --auth --dbpath path_to_database_data
+```
+
 
 ## Installation
 Clone the repository from github using this command:
@@ -61,6 +66,7 @@ Options:
   -contact TEXT           An email address
 ```
 
+
 ### Adding a client to the database
 In order to save patients into patientMatcher you need to create at least one authorized client.
 Use the following command to insert a client object in the database:
@@ -77,6 +83,7 @@ Options:
 POST request aimed at adding or modifying a patient in patientMatcher **should be using a token** from a client present in the database.
 
 
+
 ### Removing a patient from the database.
 You can remove a patient using the command line interface by invoking this command and providing **either its ID or its label** (or both actually):
 
@@ -88,6 +95,7 @@ Options:
 -label TEXT  label of the patient to be removed from database
 ```
 
+
 ## Server endpoints
 
 - **/patient/add**
@@ -104,13 +112,13 @@ curl -X POST \
     "genomicFeatures":[{"gene":{"id":"EFTUD2"}}]
   }}' localhost:9020/patient/add
 ```
-
 To update the data of a patient already submitted to the server you can use the same command and add a patient with the same ID.
 
 The action of adding or updating a patient in the server will trigger an **external search of similar patients from connected nodes**.
 If there are no connected nodes in the database or you are uploading demo data no search will be performed on other nodes.
 
 
+
 - **patient/delete/<patient_id>**
 You can delete a patient from the database by sending a **DELETE** request with its ID to the server. Example:
 ```bash
@@ -118,6 +126,10 @@ curl -X DELETE \
   -H 'X-Auth-Token: custom_token' \
   localhost:9020/patient/delete/patient_id
 ```
+Please note that when a patient is deleted all its match results will be also deleted from the database. This is valid for **matches where the patient was used as the query patient** in searches performed on other nodes or the internal patientMatcher database (internal search).
+Matching results where the removed patient is instead listed among the matching results will be not removed from the database.
+
+
 
 - **/patient/view**
 Use this endpoint to **get** a list of all patients in the database. Example:
@@ -127,6 +139,7 @@ curl -X GET \
   localhost:9020/patient/view
 ```
 
+
 - **/match**
 **POST** a request with a query patient to patientMatcher and get a response with the patients in the server which are most similar to your query. Example:
 ```bash
@@ -142,6 +155,7 @@ curl -X POST \
   }}' localhost:9020/match
 ```
 
+
 - **/match/external/<patient_id>**
 Trigger a search in external nodes for patients similar to the one specified by the ID. Example:
 ```bash
@@ -150,6 +164,7 @@ curl -X POST \
   localhost:9020/match/external/patient_id
 ```
 
+
 - **/patient/matches/<patient_id>**
 Return all matches (internal and external) with positive results for a patient specified by an ID. Example:
 ```bash
@@ -158,6 +173,7 @@ curl -X GET \
   localhost:9020/matches/patient_id
 ```
 
+
 ## Patient matching algorithm, used both for internal and external searches
 Each patient query submitted to the server triggers a matching algorithm which will search and return those patients on the server that are most similar to the queried one.
 Patient similarity is measured by the a **similarity score** that may span **from 0 (no matching) to 1 (exact matching)**.
diff --git a/patientMatcher/cli/add.py b/patientMatcher/cli/add.py
index de1867d..a779ac8 100644
--- a/patientMatcher/cli/add.py
+++ b/patientMatcher/cli/add.py
@@ -62,7 +62,7 @@ def client(id, token, url, contact=None):
 
 
 @add.command()
-@click.option('-monarch_phenotypes/-no_monarch_phenotypes', default=False)
+@click.option('-monarch_phenotypes/-no_monarch_phenotypes', default=False, help="Compute Monarch phenotypes")
 @with_appcontext
 def demodata(monarch_phenotypes):
     """Adds a set of 50 demo patients to database"""
diff --git a/patientMatcher/cli/remove.py b/patientMatcher/cli/remove.py
index 9f59826..257815b 100644
--- a/patientMatcher/cli/remove.py
+++ b/patientMatcher/cli/remove.py
@@ -14,13 +14,19 @@ def remove():
 @remove.command()
 @click.option('-id', type=click.STRING, nargs=1, required=False, help="ID of the patient to be removed from database")
 @click.option('-label', type=click.STRING, nargs=1, required=False, help="Label of the patient to be removed from database")
+@click.option('-remove_matches/-leave_matches', default=False, help="Remove or leave on db matches triggered by patient")
 @with_appcontext
-def patient(id, label):
+def patient(id, label, remove_matches):
     """Removing a patient from patientMatcher providing its ID"""
 
     if not id and not label:
         click.echo('Error: either ID and/or label should be provided to delete a patient.')
         raise click.Abort()
+
+    if remove_matches and not id:
+        click.echo('Please provide patient ID and not label to remove all its matches.')
+        raise click.Abort()
+
     query = {}
     if id:
         query['_id'] = id
@@ -29,3 +35,10 @@ def patient(id, label):
 
     n_removed = delete_by_query(query=query, mongo_db= current_app.db, mongo_collection='patients')
     click.echo('Number of patients removed from database:{}'.format(n_removed))
+
+    if remove_matches:
+        # this will remove ONLY matches where this patient was the query patient
+        # NOT those where patient was among the matching results
+        query = {'data.patient.id' : id}
+        n_removed = delete_by_query(query=query, mongo_db= current_app.db, mongo_collection='matches')
+        click.echo('Number of matches for this patient removed from database:{}'.format(n_removed))
diff --git a/patientMatcher/server/controllers.py b/patientMatcher/server/controllers.py
index 359f775..dd1449d 100644
--- a/patientMatcher/server/controllers.py
+++ b/patientMatcher/server/controllers.py
@@ -85,10 +85,17 @@ def bad_request(error_code):
 def delete_patient(database, patient_id):
     """Remove a patient by ID"""
     message = ''
+
+    # first delete all matches in database for this patient:
+    query = {'data.patient.id' : patient_id}
+    deleted = delete_by_query(query, database, 'matches')
+    LOG.info('deleted {} matche/s triggered by this patient'.format(deleted))
+
+
     query = {'_id' : patient_id}
     deleted = delete_by_query(query, database, 'patients')
     if deleted == 1:
-        message = 'Patient was successfully deleted from database'
+        message = 'Patient and its matches were successfully deleted from database'
     else:
         message = 'ERROR. Could not delete a patient with ID {} from database'.format(patient_id)
     return message


Test patch:
diff --git a/tests/backend/test_backend_patient.py b/tests/backend/test_backend_patient.py
index 6f3db57..2130770 100644
--- a/tests/backend/test_backend_patient.py
+++ b/tests/backend/test_backend_patient.py
@@ -26,7 +26,7 @@ def test_load_demo_patients(demo_data_path, database):
     assert len(inserted_ids) == 0
 
 
-def test_backend_remove_patient(json_patients, database):
+def test_backend_remove_patient(json_patients, database, match_obs):
     """ Test adding 2 test patients and then removing them using label or ID """
 
     # test conversion to format required for the database:
diff --git a/tests/cli/test_commands.py b/tests/cli/test_commands.py
index 7dd2d2c..d1f6b35 100644
--- a/tests/cli/test_commands.py
+++ b/tests/cli/test_commands.py
@@ -69,15 +69,15 @@ def test_cli_add_demo_data(database):
     assert database['patients'].find().count() == 50
 
 
-def test_cli_remove_patient(database, json_patients):
+def test_cli_remove_patient(database, json_patients, match_obs):
     app.db = database
     runner = app.test_cli_runner()
 
     # add a test patient to database
     test_patient = json_patients[0]
-    test_patient['_id'] = 'test_id'
+    test_patient['_id'] = 'P0000079'
     inserted_id = app.db['patients'].insert_one(test_patient).inserted_id
-    assert inserted_id == 'test_id'
+    assert inserted_id == 'P0000079'
 
     # there is now 1 patient in database
     assert database['patients'].find().count() == 1
@@ -86,9 +86,29 @@ def test_cli_remove_patient(database, json_patients):
     result =  runner.invoke(cli, ['remove', 'patient', '-id', '', '-label', ''])
     assert 'Error' in result.output
 
+    # Add mock patient matches objects to database
+    database['matches'].insert_many(match_obs)
+    # There should be 2 matches in database for this patient:
+    assert database['matches'].find( {'data.patient.id' : inserted_id }).count() == 2
+
     # involke cli command to remove the patient by id and label
-    result =  runner.invoke(cli, ['remove', 'patient', '-id', inserted_id, '-label', 'Patient number 1'])
+    result =  runner.invoke(cli, ['remove', 'patient', '-id', inserted_id, '-label', 'Patient number 1', '-leave_matches'])
     assert result.exit_code == 0
 
     # check that the patient was removed from database
     assert database['patients'].find().count() == 0
+
+    # But matches are still there
+    assert database['matches'].find( {'data.patient.id' : inserted_id }).count() == 2
+
+    # Run remove patient command with option to remove matches but without patient ID
+    result =  runner.invoke(cli, ['remove', 'patient', '-label', 'Patient number 1', '-remove_matches'])
+    # And make sure that it doesn't work
+    assert 'Please provide patient ID and not label to remove all its matches.' in result.output
+
+    # Test now the proper command to remove patient matches:
+    result =  runner.invoke(cli, ['remove', 'patient', '-id', inserted_id, '-remove_matches'])
+    assert result.exit_code == 0
+
+    # And make sure that patient removal removed its matchings
+    assert database['matches'].find( {'data.patient.id' : inserted_id }).count() == 0
diff --git a/tests/conftest.py b/tests/conftest.py
index fc67761..a054a15 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -64,7 +64,7 @@ def match_obs():
             'has_matches' : True,
             'data' : {
                 'patient' : {
-                    'id' : 'test_patient'
+                    'id' : 'P0000079'
                 }
             },
             'results' : [
@@ -78,7 +78,7 @@ def match_obs():
             'has_matches' : False,
             'data' : {
                 'patient' : {
-                    'id' : 'test_patient'
+                    'id' : 'P0000079'
                 }
             },
             'results' : [],
@@ -93,7 +93,7 @@ def match_obs():
                 }
             },
             'results' : [
-                {'patient' : { 'id' : 'test_patient'}},
+                {'patient' : { 'id' : 'P0000079'}},
             ],
             'match_type' : 'internal'
         },
diff --git a/tests/server/test_server_responses.py b/tests/server/test_server_responses.py
index be552af..9abfd57 100644
--- a/tests/server/test_server_responses.py
+++ b/tests/server/test_server_responses.py
@@ -96,7 +96,7 @@ def test_patient_view(database, test_client):
     assert auth_response.status_code == 200
 
 
-def test_delete_patient(database, demo_data_path, test_client):
+def test_delete_patient(database, demo_data_path, test_client, match_obs):
     """Test deleting a patient from database by sending a DELETE request"""
 
     app.db = database
@@ -106,7 +106,7 @@ def test_delete_patient(database, demo_data_path, test_client):
 
     # 50 cases present on patients collection
     assert database['patients'].find().count() == 50
-    delete_id = inserted_ids[0]
+    delete_id = 'P0000079'
 
     # try to delete patient without auth token:
     response = app.test_client().delete(''.join(['patient/delete/', delete_id]))
@@ -123,6 +123,13 @@ def test_delete_patient(database, demo_data_path, test_client):
     # but server returns error
     assert data == 'ERROR. Could not delete a patient with ID not_a_valid_ID from database'
 
+    assert database['matches'].find().count() == 0 # no matches in database
+    # insert into database some mock matching objects
+    database['matches'].insert_many(match_obs)
+
+    # patient "delete_id" should have two associated matches in database
+    assert database['matches'].find({'data.patient.id' : delete_id}).count() == 2
+
     # Send valid patient ID and valid token
     response = app.test_client().delete(''.join(['patient/delete/', delete_id]), headers = auth_headers(ok_token))
     assert response.status_code == 200
@@ -130,6 +137,10 @@ def test_delete_patient(database, demo_data_path, test_client):
     # make sure that the patient was removed from database
     assert database['patients'].find().count() == 49
 
+    # make sure that patient matches are also gone
+    assert database['matches'].find().count() == 1
+
+
 
 def test_patient_matches(database, match_obs, test_client):
     """testing the endpoint that retrieves the matchings by patient ID"""
@@ -147,7 +158,7 @@ def test_patient_matches(database, match_obs, test_client):
 
     # test endpoint to get matches by ID
     # test by sending a non-authorized request
-    response = app.test_client().get('matches/test_patient')
+    response = app.test_client().get('matches/P0000079')
     # response gives a 401 code (not authorized)
     assert response.status_code == 401
 
@@ -160,7 +171,7 @@ def test_patient_matches(database, match_obs, test_client):
     assert data == 'Could not find any matches in database for patient ID unknown_patient'
 
     # Try with authenticates request and valid patient
-    response = app.test_client().get('matches/test_patient', headers = auth_headers(ok_token))
+    response = app.test_client().get('matches/P0000079', headers = auth_headers(ok_token))
     # response gives success
     assert response.status_code == 200
     data = json.loads(response.data)
@@ -168,15 +179,15 @@ def test_patient_matches(database, match_obs, test_client):
     assert len(data['results']) == 2 # 2 matches returned because endpoint returns only matches with results
 
     # Test that there are actually 3 matches by calling directly the function returning matches
-    matches = patient_matches(database=database, patient_id='test_patient', type=None, with_results=False)
+    matches = patient_matches(database=database, patient_id='P0000079', type=None, with_results=False)
     assert len(matches) == 3
 
     # Call the same function to get only external matches
-    matches = patient_matches(database=database, patient_id='test_patient', type='external', with_results=False)
+    matches = patient_matches(database=database, patient_id='P0000079', type='external', with_results=False)
     assert len(matches) == 1
 
     # Call the same function to get only external matches
-    matches = patient_matches(database=database, patient_id='test_patient', type='internal', with_results=False)
+    matches = patient_matches(database=database, patient_id='P0000079', type='internal', with_results=False)
     assert len(matches) == 2