From 53da9f3634f18b2461cc689add01021303e23112 Mon Sep 17 00:00:00 2001 From: pbugni Date: Thu, 4 Jan 2024 18:35:39 -0800 Subject: [PATCH 1/9] IRONN-225 EMPRO month 12 expired vs. not available (#4354) fix for https://movember.atlassian.net/browse/IRONN-225 EMPRO qb_status expired status is ambiguous - can mean "not yet available" or "expired". Added comparison with consent to correct in adherence report. --- portal/migrations/versions/66368e673005_.py | 63 ++++++++++++++++++ portal/models/questionnaire_response.py | 12 +++- portal/models/reporting.py | 33 ++++++---- portal/timeout_lock.py | 2 +- portal/views/patient.py | 72 +++++++++++++++++---- 5 files changed, 156 insertions(+), 26 deletions(-) create mode 100644 portal/migrations/versions/66368e673005_.py diff --git a/portal/migrations/versions/66368e673005_.py b/portal/migrations/versions/66368e673005_.py new file mode 100644 index 000000000..fb788ac1b --- /dev/null +++ b/portal/migrations/versions/66368e673005_.py @@ -0,0 +1,63 @@ +"""IRONN-225 update adherence data for expired EMPRO users + +Revision ID: 66368e673005 +Revises: d1f3ed8d16ef +Create Date: 2023-12-11 16:56:10.427854 + +""" +from alembic import op +from datetime import datetime +import sqlalchemy as sa +from sqlalchemy.orm import sessionmaker + + +# revision identifiers, used by Alembic. +revision = '66368e673005' +down_revision = 'd1f3ed8d16ef' + +Session = sessionmaker() + + +def upgrade(): + # IRONN-225 noted expired EMPRO users adherence data showed + # `not yet available`. Code corrected, need to force renewal + # for those affected. + + bind = op.get_bind() + session = Session(bind=bind) + + now = datetime.utcnow().strftime("%Y-%m-%d %H:%M:%S") + patient_ids = [] + # get list of non-deleted users with a 12th month expiration + # that has already passed. (12 = baseline + zero-index 10) + for patient_id in session.execute( + "SELECT DISTINCT(user_id) FROM qb_timeline JOIN users" + " ON users.id = user_id WHERE deleted_id IS NULL" + " AND research_study_id = 1 AND qb_iteration = 10" + f" AND status = 'expired' AND at < '{now}'"): + patient_ids.append(patient_id[0]) + + # purge their respective rows from adherence cache, IFF status + # shows IRONN-225 symptom. + rs_visit = "1:Month 12" + for patient_id in patient_ids: + status = session.execute( + "SELECT data->>'status' FROM adherence_data WHERE" + f" patient_id = {patient_id} AND" + f" rs_id_visit = '{rs_visit}'" + ).first() + if status and status[0] != "Not Yet Available": + continue + + # purge the user's EMPRO adherence rows to force refresh + session.execute( + "DELETE FROM adherence_data WHERE" + f" patient_id = {patient_id} AND" + f" rs_id_visit like '1:%'" + ) + + +def downgrade(): + # No reasonable downgrade + pass + diff --git a/portal/models/questionnaire_response.py b/portal/models/questionnaire_response.py index 208413581..3ca4981a8 100644 --- a/portal/models/questionnaire_response.py +++ b/portal/models/questionnaire_response.py @@ -842,7 +842,7 @@ def required_qs(self, qb_id): def aggregate_responses( instrument_ids, current_user, research_study_id, patch_dstu2=False, - ignore_qb_requirement=False, celery_task=None): + ignore_qb_requirement=False, celery_task=None, patient_ids=None): """Build a bundle of QuestionnaireResponses :param instrument_ids: list of instrument_ids to restrict results to @@ -852,13 +852,19 @@ def aggregate_responses( :param patch_dstu2: set to make bundle DSTU2 compliant :param ignore_qb_requirement: set to include all questionnaire responses :param celery_task: if defined, send occasional progress updates + :param patient_ids: if defined, limit result set to given patient list + NB: research_study_id not used to filter / restrict query set, but rather + for lookup of visit name. Use instrument_ids to restrict query set. """ from .qb_timeline import qb_status_visit_name # avoid cycle # Gather up the patient IDs for whom current user has 'view' permission user_ids = patients_query( - current_user, include_test_role=False).with_entities(User.id) + current_user, + include_test_role=False, + filter_by_ids=patient_ids, + ).with_entities(User.id) annotated_questionnaire_responses = [] questionnaire_responses = QuestionnaireResponse.query.filter( @@ -920,7 +926,7 @@ def aggregate_responses( 'resource': document, # Todo: return URL to individual QuestionnaireResponse resource 'fullUrl': url_for( - '.assessment', + 'assessment_engine_api.assessment', patient_id=subject.id, _external=True, ), diff --git a/portal/models/reporting.py b/portal/models/reporting.py index af9ea6e40..2b29b17ec 100644 --- a/portal/models/reporting.py +++ b/portal/models/reporting.py @@ -22,7 +22,7 @@ from .overall_status import OverallStatus from .questionnaire_response import aggregate_responses from .qb_status import QB_Status -from .qb_timeline import qb_status_visit_name +from .qb_timeline import QBT, qb_status_visit_name from .questionnaire_bank import visit_name from .questionnaire_response import ( QNR_results, @@ -152,10 +152,20 @@ def empro_row_detail(row, ts_reporting): # build up data until we find valid cache for patient's history status = str(qb_stats.overall_status) row = patient_data(patient) + row["status"] = status if status == "Expired" and research_study_id == EMPRO_RS_ID: - row["status"] = "Not Yet Available" - else: - row["status"] = status + # Expired status ambiguous for EMPRO study. + # - If the last available questionnaire in the study is present in + # the user's timeline and the expired date has passed, it is + # legitimately "Expired". + # - Otherwise, due to complex business rules around delayed + # start/availability mark as "Not Yet Available" + exp_row = QBT.query.filter(QBT.research_study_id == EMPRO_RS_ID).filter( + QBT.user_id == patient.id).filter( + QBT.status == 'expired').filter( + QBT.qb_iteration == 10).first() # baseline is 1, 11 iterations base 0 + if not exp_row or exp_row.at > as_of_date: + row["status"] = "Not Yet Available" if last_viable: general_row_detail(row, patient, last_viable) @@ -164,8 +174,8 @@ def empro_row_detail(row, ts_reporting): ts_reporting = TriggerStatesReporting(patient_id=patient.id) empro_row_detail(row, ts_reporting) - # latest is only valid for a week, unless the user withdrew - valid_for = 500 if row['status'] in ('Expired', 'Withdrawn') else 7 + # latest is only valid for a day, unless the user withdrew + valid_for = 30 if row['status'] in ('Expired', 'Withdrawn') else 1 AdherenceData.persist( patient_id=patient.id, rs_id_visit=rs_visit, @@ -177,9 +187,10 @@ def empro_row_detail(row, ts_reporting): for qbd, status in qb_stats.older_qbds(last_viable): rs_visit = AdherenceData.rs_visit_string( research_study_id, visit_name(qbd)) - # once we find cached_data, the rest of the user's history is good + # once we find cached_data, the rest of the user's history is likely + # good, but best to verify nothing is stale if AdherenceData.fetch(patient_id=patient.id, rs_id_visit=rs_visit): - break + continue historic = row.copy() historic['status'] = status @@ -190,7 +201,7 @@ def empro_row_detail(row, ts_reporting): AdherenceData.persist( patient_id=patient.id, rs_id_visit=rs_visit, - valid_for_days=500, + valid_for_days=30, data=historic) added_rows += 1 @@ -223,7 +234,7 @@ def empro_row_detail(row, ts_reporting): AdherenceData.persist( patient_id=patient.id, rs_id_visit=rs_visit, - valid_for_days=500, + valid_for_days=30, data=indef) added_rows += 1 @@ -261,7 +272,7 @@ def cache_adherence_data( as_of_date = datetime.utcnow() # Purge any rows that have or will soon expire - valid = (as_of_date + timedelta(days=1)) + valid = (as_of_date + timedelta(hours=1)) AdherenceData.query.filter(AdherenceData.valid_till < valid).delete() db.session.commit() diff --git a/portal/timeout_lock.py b/portal/timeout_lock.py index 75f91a6a3..715f7f2bf 100644 --- a/portal/timeout_lock.py +++ b/portal/timeout_lock.py @@ -106,7 +106,7 @@ def guarded_task_launch(task, **kwargs): class CacheModeration(object): """Redis key implementation to prevent same key from excessive updates""" - def __init__(self, key, timeout=3600): + def __init__(self, key, timeout=300): self.key = key self.timeout = timeout self.redis = redis.StrictRedis.from_url( diff --git a/portal/views/patient.py b/portal/views/patient.py index 09a39ce89..ac307f323 100644 --- a/portal/views/patient.py +++ b/portal/views/patient.py @@ -30,7 +30,12 @@ from ..models.questionnaire_bank import QuestionnaireBank, trigger_date from ..models.questionnaire_response import QuestionnaireResponse from ..models.reference import Reference -from ..models.research_study import ResearchStudy +from ..models.reporting import single_patient_adherence_data +from ..models.research_study import ( + EMPRO_RS_ID, + ResearchStudy, + research_study_id_from_questionnaire +) from ..models.role import ROLE from ..models.user import User, current_user, get_user from ..timeout_lock import ADHERENCE_DATA_KEY, CacheModeration @@ -313,6 +318,7 @@ def patient_timeline(patient_id): :param research_study_id: set to alternative research study ID - default 0 :param trace: set 'true' to view detailed logs generated, works best in concert with purge + :param only: set to filter all results but top level attribute given """ from ..date_tools import FHIR_datetime, RelativeDelta @@ -323,6 +329,7 @@ def patient_timeline(patient_id): from ..models.qbd import QBD from ..models.qb_status import QB_Status from ..models.questionnaire_bank import visit_name + from ..models.questionnaire_response import aggregate_responses from ..models.research_protocol import ResearchProtocol from ..trace import dump_trace, establish_trace @@ -341,6 +348,7 @@ def patient_timeline(patient_id): # questionnaire_response : qb relationships and remove cache lock # on adherence data. if purge == 'all': + # remove adherence cache key to allow fresh run cache_moderation = CacheModeration(key=ADHERENCE_DATA_KEY.format( patient_id=patient_id, research_study_id=research_study_id)) @@ -455,18 +463,60 @@ def get_recur_id(qnr): status['indefinite status'] = indef_status adherence_data = sorted_adherence_data(patient_id, research_study_id) + if not adherence_data: + # immediately following a cache purge, adherence data is gone and + # needs to be recreated. + now = datetime.utcnow() + single_patient_adherence_data( + user, as_of_date=now, research_study_id=EMPRO_RS_ID) + adherence_data = sorted_adherence_data(patient_id, research_study_id) + + qnr_responses = aggregate_responses( + instrument_ids=None, + current_user=current_user(), + research_study_id=research_study_id, + patch_dstu2=True, + ignore_qb_requirement=True, + patient_ids=[patient_id] + ) + # filter qnr data to a manageable result data set + qnr_data = [] + for row in qnr_responses['entry']: + i = {} + d = row['resource'] + i['questionnaire'] = d['questionnaire']['reference'].split('/')[-1] + + # qnr_responses return all. filter to requested research_study + study_id = research_study_id_from_questionnaire(i['questionnaire']) + if study_id != research_study_id: + continue + i['auth_method'] = d['encounter']['auth_method'] + i['encounter_period'] = d['encounter']['period'] + i['document_authored'] = d['authored'] + i['ae_session'] = d['identifier']['value'] + i['status'] = d['status'] + i['org'] = d['subject']['careProvider'][0]['display'] + i['visit'] = d['timepoint'] + qnr_data.append(i) + + kwargs = { + "rps": rps, + "status": status, + "posted": posted, + "timeline": results, + "adherence_data": adherence_data, + "qnr_data": qnr_data + } if trace: - return jsonify( - rps=rps, - status=status, - posted=posted, - timeline=results, - adherence_data=adherence_data, - trace=dump_trace("END time line lookup")) - return jsonify( - rps=rps, status=status, posted=posted, timeline=results, - adherence_data=adherence_data) + kwargs["trace"] = dump_trace("END time line lookup") + + only = request.args.get('only', False) + if only: + if only not in kwargs: + raise ValueError(f"{only} not in {kwargs.keys()}") + return jsonify(only, kwargs[only]) + return jsonify(**kwargs) @patient_api.route('/api/patient//timewarp/') From d7b7989746ff72cba83adc7e2f9ccb3a3505478f Mon Sep 17 00:00:00 2001 From: pbugni Date: Mon, 22 Jan 2024 17:37:42 -0800 Subject: [PATCH 2/9] Add doc for `-f` (force) flag to deploy-docker.sh (#4355) after the 2nd time of needing to look this up, figured it was worth documenting... --- bin/deploy-docker.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/bin/deploy-docker.sh b/bin/deploy-docker.sh index 65c5e0d72..43bbddf30 100755 --- a/bin/deploy-docker.sh +++ b/bin/deploy-docker.sh @@ -11,6 +11,7 @@ Usage: $cmdname [-h] [-b] [-n] -h Show this help message -b Backup current database before attempting update + -f Ignore recent activity check before attempting restart -n Do not pull docker images prior to starting Docker deployment script From d42b2713ae6330af453940f63f48838fe71c522a Mon Sep 17 00:00:00 2001 From: Amy Chen Date: Mon, 5 Feb 2024 08:39:15 -0800 Subject: [PATCH 3/9] TN-3271 : add main study registry link email variable (#4359) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit part of https://movember.atlassian.net/browse/TN-3271 Add main study registry link as email variable that can be referenced in the Liferay email items. Note: once this is approved and merged, I will add reference to this variable in relevant Life email items example screenshot when testing: ![Screenshot 2024-02-01 at 12 58 57 PM](https://github.com/uwcirg/truenth-portal/assets/12942714/1bc0a5bb-67ca-4df9-9cff-3180021853cc) --------- Co-authored-by: Amy Chen --- portal/models/communication.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/portal/models/communication.py b/portal/models/communication.py index c37753b7c..034d74f70 100644 --- a/portal/models/communication.py +++ b/portal/models/communication.py @@ -115,6 +115,13 @@ def _lookup_assessment_link(): '{label}'.format( ae_link=access_link(next_step='present_needed'), label=label)) + def _lookup_main_study_registry_link(): + label = _('Learn more about the IRONMAN registry') + registry_link = 'https://ironmanregistry.org/' + return ( + '{label}'.format( + registry_link=registry_link, label=label)) + def _lookup_clinic_name(): if user.organizations: return _(user.organizations[0].name) From 3a11e96723d6369e48470b1e972157ed39e431d6 Mon Sep 17 00:00:00 2001 From: pbugni Date: Fri, 9 Feb 2024 12:16:38 -0800 Subject: [PATCH 4/9] logger.error on empro message sans email (#4362) Generate a reasonable error if EMPRO patient can't receive email at time of generation. (shouldn't ever happen in production, but makes a mess of the logs in dev/testing) --- portal/models/message.py | 5 +++++ portal/trigger_states/empro_states.py | 10 +++++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/portal/models/message.py b/portal/models/message.py index 5a2180a59..48ac3cd7c 100644 --- a/portal/models/message.py +++ b/portal/models/message.py @@ -116,6 +116,11 @@ def recipients(self): def recipients(self, value): """Set recipients_id if a user is found w/ matching email""" + if value is None: + self._recipients = None + self.recipient_id = None + return + # As the schema only tracks a single recipient_id, capture abuse; # don't allow comma in recipients till schema can capture if ',' in value: diff --git a/portal/trigger_states/empro_states.py b/portal/trigger_states/empro_states.py index 70e490711..a2c73e52f 100644 --- a/portal/trigger_states/empro_states.py +++ b/portal/trigger_states/empro_states.py @@ -292,9 +292,13 @@ def process_processed(ts): patient = User.query.get(ts.user_id) # Patient always gets mail - pending_emails.append(( - patient_email(patient, soft_triggers, hard_triggers), - "patient thank you")) + if patient.email_ready(): + pending_emails.append(( + patient_email(patient, soft_triggers, hard_triggers), + "patient thank you")) + else: + current_app.logger.error( + f"EMPRO Patient({patient.id}) w/o email! Can't send message") if hard_triggers: triggers['action_state'] = 'required' From 03a4bfce6e8ccde02a4dfbec237e977201430934 Mon Sep 17 00:00:00 2001 From: Justin McReynolds Date: Mon, 4 Mar 2024 16:45:41 -0800 Subject: [PATCH 5/9] TN-3264 Add options to irondemog_v3 (#4364) TN-3264 Adds 6 new options to irondemog_v3.26 (ethnic group) --- portal/config/eproms/Questionnaire.json | 36 +++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/portal/config/eproms/Questionnaire.json b/portal/config/eproms/Questionnaire.json index 9a1cc079d..2ace521d2 100644 --- a/portal/config/eproms/Questionnaire.json +++ b/portal/config/eproms/Questionnaire.json @@ -5851,6 +5851,42 @@ "display": "Other", "code": "irondemog_v3.26.8" } + }, + { + "valueCoding": { + "display": "African", + "code": "irondemog_v3.26.9" + } + }, + { + "valueCoding": { + "display": "Black", + "code": "irondemog_v3.26.10" + } + }, + { + "valueCoding": { + "display": "Coloured", + "code": "irondemog_v3.26.11" + } + }, + { + "valueCoding": { + "display": "Indian", + "code": "irondemog_v3.26.12" + } + }, + { + "valueCoding": { + "display": "White / Caucasian", + "code": "irondemog_v3.26.13" + } + }, + { + "valueCoding": { + "display": "Other", + "code": "irondemog_v3.26.14" + } } ] }, From d9bb3a0656af2ecf908621eff172f4318d90222c Mon Sep 17 00:00:00 2001 From: Justin McReynolds Date: Tue, 5 Mar 2024 16:14:33 -0800 Subject: [PATCH 6/9] IRONN-236 V5 for Lister & Clatterbridge (#4360) https://movember.atlassian.net/browse/IRONN-236 --- portal/config/eproms/Organization.json | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/portal/config/eproms/Organization.json b/portal/config/eproms/Organization.json index f60757572..2a1998ceb 100644 --- a/portal/config/eproms/Organization.json +++ b/portal/config/eproms/Organization.json @@ -3189,7 +3189,8 @@ }, { "research_protocols": [ - {"name": "IRONMAN v3"} + {"name": "IRONMAN v3", "retired_as_of": "2024-02-12T12:00:00Z"}, + {"name": "IRONMAN v5"} ], "url": "http://us.truenth.org/identity-codes/research-protocol" } @@ -3217,7 +3218,8 @@ }, { "research_protocols": [ - {"name": "IRONMAN v3"} + {"name": "IRONMAN v3", "retired_as_of": "2024-02-12T12:00:00Z"}, + {"name": "IRONMAN v5"} ], "url": "http://us.truenth.org/identity-codes/research-protocol" } From 1ff18a7f2993019c7efd04df376f94c8371bffca Mon Sep 17 00:00:00 2001 From: pbugni Date: Tue, 5 Mar 2024 16:22:23 -0800 Subject: [PATCH 7/9] user consent withdrawal date regression (#4361) reverts bug introduced by https://github.com/uwcirg/truenth-portal/pull/4343 , which incorrectly started using the oldest (not most recent) user_consent as last valid consent prior to withdrawal. includes migration to clean up problem cases. required careful re-assessment of consent/withdrawal dates for a number of patients found to have qb_timeline revisions after applying patch above. see https://movember.atlassian.net/browse/IRONN-210 for more detail. NB: #4363 merges into this branch, to be done prior to merging this PR. --------- Co-authored-by: Justin McReynolds Co-authored-by: Ivan Cvitkovic --- manage.py | 161 +------------- portal/config/config.py | 5 +- portal/factories/redis.py | 4 + portal/migrations/versions/3c871e710277_.py | 225 ++++++++++++++++++++ portal/migrations/versions/66368e673005_.py | 12 +- portal/migrations/versions/edb52362d013_.py | 144 +++++++++++++ portal/models/adherence_data.py | 24 ++- portal/models/qb_status.py | 5 + portal/models/qb_timeline.py | 109 ++++------ portal/models/questionnaire_response.py | 73 +++++++ portal/models/reporting.py | 89 ++++++-- portal/models/user.py | 2 +- portal/models/user_consent.py | 13 +- portal/tasks.py | 14 +- portal/timeout_lock.py | 8 +- portal/views/healthcheck.py | 8 +- portal/views/patient.py | 21 +- portal/views/user.py | 14 +- tests/test_consent.py | 6 +- tests/test_healthcheck.py | 24 +-- tests/test_intervention.py | 1 + tests/test_qb_timeline.py | 10 +- tests/test_reporting.py | 41 +++- 23 files changed, 718 insertions(+), 295 deletions(-) create mode 100644 portal/factories/redis.py create mode 100644 portal/migrations/versions/3c871e710277_.py create mode 100644 portal/migrations/versions/edb52362d013_.py diff --git a/manage.py b/manage.py index 52b38ba67..13cadc19d 100644 --- a/manage.py +++ b/manage.py @@ -44,7 +44,11 @@ QuestionnaireBank, add_static_questionnaire_bank, ) -from portal.models.questionnaire_response import QuestionnaireResponse +from portal.models.questionnaire_response import ( + QuestionnaireResponse, + capture_patient_state, + present_before_after_state, +) from portal.models.relationship import add_static_relationships from portal.models.research_study import ( BASE_RS_ID, @@ -611,94 +615,6 @@ def update_qnr(qnr_id, link_id, actor, noop, replacement): click.echo(message) -@click.option('--subject_id', type=int, multiple=True, help="Subject user ID", required=True) -@click.option( - '--actor', - default="__system__", - required=False, - help='email address of user taking this action, for audit trail' -) -@app.cli.command() -def remove_post_withdrawn_qnrs(subject_id, actor): - """Remove QNRs posted beyond subject's withdrawal date""" - from sqlalchemy.types import DateTime - from portal.cache import cache - from portal.models.questionnaire_bank import trigger_date - - rs_id = 0 # only base study till need arises - acting_user = get_actor(actor, require_admin=True) - - for subject_id in subject_id: - # Confirm user has withdrawn - subject = get_target(id=subject_id) - study_id = subject.external_study_id - - # Make sure we're not working w/ stale timeline data - QuestionnaireResponse.purge_qb_relationship( - subject_id=subject_id, - research_study_id=rs_id, - acting_user_id=acting_user.id) - cache.delete_memoized(trigger_date) - update_users_QBT( - subject_id, - research_study_id=rs_id, - invalidate_existing=True) - - deceased_date = None if not subject.deceased else subject.deceased.timestamp - withdrawn_visit = QBT.withdrawn_qbd(subject_id, rs_id) - if not withdrawn_visit: - raise ValueError("Only applicable to withdrawn users") - - # Obtain all QNRs submitted beyond withdrawal date - query = QuestionnaireResponse.query.filter( - QuestionnaireResponse.document["authored"].astext.cast(DateTime) > - withdrawn_visit.relative_start - ).filter( - QuestionnaireResponse.subject_id == subject_id).with_entities( - QuestionnaireResponse.id, - QuestionnaireResponse.questionnaire_bank_id, - QuestionnaireResponse.qb_iteration, - QuestionnaireResponse.document["questionnaire"]["reference"]. - label("instrument"), - QuestionnaireResponse.document["authored"]. - label("authored") - ).order_by(QuestionnaireResponse.document["authored"]) - - for qnr in query: - # match format in bug report for easy diff - sub_padding = " "*(11 - len(str(subject_id))) - stdy_padding = " "*(12 - len(study_id)) - out = ( - f"{sub_padding}{subject_id} | " - f"{study_id}{stdy_padding}| " - f"{withdrawn_visit.relative_start.strftime('%Y-%m-%d %H:%M:%S.%f')[:-3]} | " - f"{qnr.authored} | ") - - # do not include any belonging to the last active visit, unless - # they came in after deceased date - if ( - qnr.questionnaire_bank_id == withdrawn_visit.qb_id and - qnr.qb_iteration == withdrawn_visit.iteration and - (not deceased_date or FHIR_datetime.parse( - qnr.authored) < deceased_date)): - print(f"{out}keep") - continue - if "irondemog" in qnr.instrument: - print(f"{out}keep (indefinite)") - continue - print(f"{out}delete") - db.session.delete(QuestionnaireResponse.query.get(qnr.id)) - auditable_event( - message=( - "deleted questionnaire response submitted beyond " - "withdrawal visit as per request by PCCTC"), - context="assessment", - user_id=acting_user.id, - subject_id=subject_id) - db.session.commit() - return - - @click.option('--src_id', type=int, help="Source Patient ID (WILL BE DELETED!)") @click.option('--tgt_id', type=int, help="Target Patient ID") @click.option( @@ -804,71 +720,6 @@ def merge_users(src_id, tgt_id, actor): print(message) -def capture_patient_state(patient_id): - """Call to capture QBT and QNR state for patient, used for before/after""" - qnrs = QuestionnaireResponse.qnr_state(patient_id) - tl = QBT.timeline_state(patient_id) - return {'qnrs': qnrs, 'timeline': tl} - - -def present_before_after_state(user_id, external_study_id, before_state): - from portal.dict_tools import dict_compare - after_qnrs = QuestionnaireResponse.qnr_state(user_id) - after_timeline = QBT.timeline_state(user_id) - qnrs_lost_reference = [] - - def visit_from_timeline(qb_name, qb_iteration, timeline_results): - """timeline results have computed visit name - quick lookup""" - for visit, name, iteration in timeline_results.values(): - if qb_name == name and qb_iteration == iteration: - return visit - raise ValueError(f"no visit found for {qb_name}, {qb_iteration}") - - # Compare results - added_q, removed_q, modified_q, same = dict_compare( - after_qnrs, before_state['qnrs']) - assert not added_q - assert not removed_q - - added_t, removed_t, modified_t, same = dict_compare( - after_timeline, before_state['timeline']) - - if any((added_t, removed_t, modified_t, modified_q)): - print(f"\nPatient {user_id} ({external_study_id}):") - if modified_q: - print("\tModified QNRs (old, new)") - for mod in sorted(modified_q): - print(f"\t\t{mod} {modified_q[mod][1]} ==>" - f" {modified_q[mod][0]}") - # If the QNR previously had a reference QB and since - # lost it, capture for reporting. - if ( - modified_q[mod][1][0] != "none of the above" and - modified_q[mod][0][0] == "none of the above"): - visit_name = visit_from_timeline( - modified_q[mod][1][0], - modified_q[mod][1][1], - before_state["timeline"]) - qnrs_lost_reference.append((visit_name, modified_q[mod][1][2])) - if added_t: - print("\tAdditional timeline rows:") - for item in sorted(added_t): - print(f"\t\t{item} {after_timeline[item]}") - if removed_t: - print("\tRemoved timeline rows:") - for item in sorted(removed_t): - print( - f"\t\t{item} " - f"{before_state['timeline'][item]}") - if modified_t: - print(f"\tModified timeline rows: (old, new)") - for item in sorted(modified_t): - print(f"\t\t{item}") - print(f"\t\t\t{modified_t[item][1]} ==> {modified_t[item][0]}") - - return after_qnrs, after_timeline, qnrs_lost_reference - - @app.cli.command() @click.option( '--correct_overlaps', '-c', default=False, is_flag=True, @@ -989,7 +840,7 @@ def preview_site_update(org_id, retired): ) update_users_QBT( patient.id, research_study_id=0, invalidate_existing=True) - after_qnrs, after_timeline, qnrs_lost_reference = present_before_after_state( + after_qnrs, after_timeline, qnrs_lost_reference, _ = present_before_after_state( patient.id, patient.external_study_id, patient_state[patient.id]) total_qnrs += len(patient_state[patient.id]['qnrs']) total_qbs_completed_b4 += len( diff --git a/portal/config/config.py b/portal/config/config.py index 0631e4795..d3fecfc07 100644 --- a/portal/config/config.py +++ b/portal/config/config.py @@ -1,8 +1,7 @@ """Configuration""" import os -import redis - +from portal.factories.redis import create_redis from portal.models.role import ROLE SITE_CFG = 'site.cfg' @@ -152,7 +151,7 @@ class BaseConfig(object): REDIS_URL ) - SESSION_REDIS = redis.from_url(SESSION_REDIS_URL) + SESSION_REDIS = create_redis(SESSION_REDIS_URL) UPDATE_PATIENT_TASK_BATCH_SIZE = int( os.environ.get('UPDATE_PATIENT_TASK_BATCH_SIZE', 16) diff --git a/portal/factories/redis.py b/portal/factories/redis.py new file mode 100644 index 000000000..d5debfb57 --- /dev/null +++ b/portal/factories/redis.py @@ -0,0 +1,4 @@ +import redis + +def create_redis(url): + return redis.Redis.from_url(url) diff --git a/portal/migrations/versions/3c871e710277_.py b/portal/migrations/versions/3c871e710277_.py new file mode 100644 index 000000000..85aed5bdf --- /dev/null +++ b/portal/migrations/versions/3c871e710277_.py @@ -0,0 +1,225 @@ +"""Correct user_consent regression issues raised by PR #4343 + +Revision ID: 3c871e710277 +Revises: edb52362d013 +Create Date: 2024-01-25 20:04:48.109980 + +""" +from alembic import op +from sqlalchemy.orm import sessionmaker +from sqlalchemy.sql.functions import func + +from portal.cache import cache +from portal.models.adherence_data import AdherenceData +from portal.models.research_study import BASE_RS_ID, EMPRO_RS_ID +from portal.models.qb_timeline import QBT, update_users_QBT +from portal.models.questionnaire_bank import trigger_date +from portal.models.questionnaire_response import ( + QuestionnaireResponse, + capture_patient_state, + present_before_after_state, +) +from portal.models.user import User +from portal.models.user_consent import UserConsent, consent_withdrawal_dates +from portal.timeout_lock import ADHERENCE_DATA_KEY, CacheModeration + +Session = sessionmaker() + + +# revision identifiers, used by Alembic. +revision = '3c871e710277' +down_revision = 'edb52362d013' + + +# csv values direct from attachment in #IRONN-210, used to verify +verified_user_consent_dates = ( + { + 101: ("13-Dec-17", "13-Dec-17"), + 1073: ("16-Nov-18", "19-Sep-23"), + 1113: ("24-Oct-18", "27-Oct-21"), + 1186: ("19-Dec-18", "19-Dec-18"), + 1229: ("14-Jan-19", "10-Jan-24"), + 145: ("11-Jan-18", "17-Oct-18"), + 1524: ("12-Mar-19", "28-Oct-21"), + 1608: ("2-Apr-19", "7-Jun-21"), + 164: ("8-Jan-18", "7-Mar-18"), + 184: ("2-Feb-18", "2-May-19"), + 2049: ("4-Jul-19", "1-Jun-22"), + 209: ("22-Feb-18", "14-Dec-20"), + 224: ("28-Feb-18", "9-Mar-18"), + 2425: ("18-Sep-19", "26-May-21"), + 2547: ("25-Sep-19", "4-Aug-21"), + 2748: ("19-Nov-19", "22-Oct-22"), + 2845: ("23-Aug-19", "23-Sep-21"), + 2911: ("27-Nov-19", "9-Sep-23"), + 310: ("12-Apr-18", "16-Aug-18"), + 3251: ("16-Mar-20", "19-Jan-22"), + 3256: ("19-Mar-20", "5-May-22"), + 3427: ("26-May-20", "2-Sep-22"), + 3430: ("16-Jun-20", "15-May-21"), + 3455: ("4-Jun-20", "7-May-21"), + 3826: ("11-Nov-20", "30-Nov-20"), + 4316: ("19-Apr-21", "27-Apr-22"), + 4806: ("17-Feb-22", "13-Oct-22"), + 482: ("8-Aug-17", "28-Jul-20"), + 4861: ("28-Sep-21", "27-Feb-22"), + 4868: ("3-Mar-22", "18-Aug-22"), + 5004: ("5-Oct-21", "24-Sep-23"), + 5336: ("31-Jan-22", "7-Nov-23"), + 5852: ("5-Jul-22", "15-Apr-23"), + 5853: ("5-Jul-22", "20-Apr-23"), + 5959: ("26-Jul-22", "17-Aug-22"), + 6204: ("17-Sep-22", "25-Oct-23"), + 6218: ("27-Sep-22", "29-Oct-23"), + 641: ("7-Aug-18", "29-Dec-20"), + 653: ("9-Jul-18", "10-Sep-18"), + 6686: ("29-Jan-23", "12-Jun-23"), + # 719: ("29-May-18", "27-Aug-18"), as per story, leave alone + # 723: ("16-May-18", "25-Aug-23"), as per story, leave alone + 774: ("24-Oct-17", "9-Nov-17"), + 833: ("12-Sep-18", "11-Sep-23"), + 892: ("18-Sep-18", "5-Jan-20"), + 98: ("13-Dec-17", "22-Mar-18"), + 986: ("6-Sep-18", "22-Jun-23"), + 987: ("26-Jul-18", "14-Oct-19"), + }, + { + 563: ("10-Nov-22", "16-Dec-22"), + 3591: ("1-Oct-22", "1-Oct-23"), + 5596: ("12-Jul-22", "12-Oct-22"), + 5747: ("6-Jun-22", "10-Jun-23"), + 5849: ("5-Jul-22", "12-Oct-22"), + 6026: ("4-Nov-22", "4-Nov-23"), + } +) + + +def upgrade(): + """Correct UserConsents for any negatively affected patients + + Prior to the release of 23.10.12.1, moving withdrawal dates wasn't + allowed. This made lookups for the last valid user_consent *prior* + to the withdrawal date, reliable, as user_consents land in the table + in an ordered fashion, and the most recently deleted prior to + withdrawal would have been in use. + + The implementation of IRONN-210 enabled moving of withdrawal dates, + and incorrectly assumed it would be necessary to allow lookups of + the previous valid consent, to just work further back in the stack + of user_consents. That would only be correct on the few tested, + where the user didn't have multiple user_consents on file prior to + withdrawal. + + To enable moving withdrawal dates, user_consents now allow multiple + of status "suspended", with the most recent by id taking precedence. + To determine the valid consent in use prior to withdrawal, look back + by insertion order (id) for the first deleted user consent prior to + status "suspended". + + With code changes in place, migration must simply locate any potential + consent changes since the error was introduced and recalculate timeline + """ + # turns out, we have no reliable mechanism to determine which patients + # may have been affected, as the acceptance date on withdrawn was simply + # changed on the already withdrawn user_consent, and no audit of the + # modification was recorded. need to try a recalc and find persist + # any changes for any users with a suspended user_consent and more + # than two (the original valid consent plus the suspended one) on + # any given research study. + bind = op.get_bind() + session = Session(bind=bind) + + for study_id in (BASE_RS_ID, EMPRO_RS_ID): + # due to changes in adherence report for withdrawn users + # this query is now simply any withdrawn patient who isn't + # deleted from the system. + subquery = session.query(User.id).filter( + User.deleted_id.is_(None)).subquery() + query = session.query(UserConsent.user_id.distinct()).filter( + UserConsent.research_study_id == study_id).filter( + UserConsent.status == "suspended").filter( + UserConsent.user_id.in_(subquery)) + + delay_timeline_updates_till_after_migration = True + slow_report_details = False + delete_adh_ids = [] + for row in query: + patient_id = row[0] + if patient_id in (719, 1186, 1305): + # special cases best left alone + continue + user = User.query.get(patient_id) + consent_date, withdrawal_date = consent_withdrawal_dates( + user, study_id) + if withdrawal_date is None: + # scenario happens with a withdrawn patient re-start + # i.e. as withdrawal was entered in error. + # no change needed in this situation + continue + + if slow_report_details: + # report if dates don't match spreadsheet in IRONN-210 + cd_str = '{dt.day}-{dt:%b}-{dt:%y}'.format(dt=consent_date) + wd_str = '{dt.day}-{dt:%b}-{dt:%y}'.format(dt=withdrawal_date) + try: + match = verified_user_consent_dates[study_id][patient_id] + if (cd_str, wd_str) != match: + print(f"user_id {patient_id} \t {cd_str} \t {wd_str}") + print(" vs expected:") + print(f"\t\t {match[0]} \t {match[1]}") + except KeyError: + # user found to not see timeline change + pass + + # fake an adherence cache run to avoid unnecessary and more + # important, to prevent from locking out a subsequent update + # needed after recognizing a real change below + adherence_cache_moderation = CacheModeration(key=ADHERENCE_DATA_KEY.format( + patient_id=patient_id, + research_study_id=study_id)) + adherence_cache_moderation.run_now() + + b4_state = capture_patient_state(patient_id) + update_users_QBT( + patient_id, + research_study_id=study_id, + invalidate_existing=True) + _, _, _, any_changes = present_before_after_state( + patient_id, study_id, b4_state) + if not any_changes: + continue + + print(f"{patient_id} changed, purge old adherence data and relationships") + adherence_cache_moderation.reset() + + QuestionnaireResponse.purge_qb_relationship( + subject_id=patient_id, + research_study_id=study_id, + acting_user_id=patient_id) + cache.delete_memoized(trigger_date) + + if delay_timeline_updates_till_after_migration: + session.query(QBT).filter(QBT.user_id == patient_id).filter( + QBT.research_study_id == study_id).delete() + adh_ids = session.query(AdherenceData.id).filter( + AdherenceData.patient_id == patient_id).filter( + AdherenceData.rs_id_visit.like(f"{study_id}:%") + ) + for ad_id in adh_ids: + delete_adh_ids.append(ad_id) + else: + update_users_QBT( + patient_id, + research_study_id=study_id, + invalidate_existing=True) + + # SQL alchemy can't combine `like` expression with delete op. + for ad_id in delete_adh_ids: + # yes this should be possible in a single stmt, + # not a loop, but no dice + session.query(AdherenceData).filter( + AdherenceData.id == ad_id).delete() + +def downgrade(): + """no downgrade available""" + pass diff --git a/portal/migrations/versions/66368e673005_.py b/portal/migrations/versions/66368e673005_.py index fb788ac1b..cd021f96a 100644 --- a/portal/migrations/versions/66368e673005_.py +++ b/portal/migrations/versions/66368e673005_.py @@ -7,9 +7,11 @@ """ from alembic import op from datetime import datetime -import sqlalchemy as sa from sqlalchemy.orm import sessionmaker +from portal.models.user import User +from portal.models.user_consent import consent_withdrawal_dates + # revision identifiers, used by Alembic. revision = '66368e673005' @@ -49,6 +51,14 @@ def upgrade(): if status and status[0] != "Not Yet Available": continue + # if the patient is withdrawn, skip over, will get picked + # up in migration 3c871e710277, going out in same release + patient = User.query.get(patient_id) + _, withdrawal_date = consent_withdrawal_dates( + patient, 1) + if withdrawal_date: + continue + # purge the user's EMPRO adherence rows to force refresh session.execute( "DELETE FROM adherence_data WHERE" diff --git a/portal/migrations/versions/edb52362d013_.py b/portal/migrations/versions/edb52362d013_.py new file mode 100644 index 000000000..6dced10ea --- /dev/null +++ b/portal/migrations/versions/edb52362d013_.py @@ -0,0 +1,144 @@ +"""Remove/correct bogus user_consents as per IRONN-210 + +Revision ID: edb52362d013 +Revises: 66368e673005 +Create Date: 2024-01-11 16:23:34.961937 + +""" +from alembic import op +from datetime import datetime +from flask import current_app +from sqlalchemy.orm import sessionmaker + +from portal.models.user_consent import UserConsent + +# revision identifiers, used by Alembic. +revision = 'edb52362d013' +down_revision = '66368e673005' + +Session = sessionmaker() + + +def user_consent_manual_cleanup(session): + # turned into a detailed situation, of obtaining original dates from MR + # and correcting a number of bogus rows in the user_consent table. + # This hand curated list comes from attachments in + # https://movember.atlassian.net/browse/IRONN-210 + # run these first, then confirm everything looks clean. + now = datetime.utcnow() + version = current_app.config.metadata['version'] + + admin_id = session.execute( + "SELECT id FROM users WHERE email = '__system__'" + ).next()[0] + + def audit_insert(subject_id, user_consent_id, acceptance_date=None): + msg = f"remove bogus user_consent {user_consent_id} per IRONN-210" + if acceptance_date: + msg = f"corrected user_consent {user_consent_id} to {acceptance_date} per IRONN-210" + print(msg) + insert = ( + "INSERT INTO AUDIT" + " (user_id, subject_id, context, timestamp, version, comment)" + " VALUES" + f"({admin_id}, {subject_id}, 'consent'," + f" '{now}', '{version}', '{msg}')") + session.execute(insert) + + def delete_user_consent(user_id, user_consent_id): + return UserConsent.query.filter( + UserConsent.id == user_consent_id).filter( + UserConsent.user_id == user_id).delete() + + def update_user_consent(user_id, user_consent_id, acceptance_date): + uc = UserConsent.query.filter( + UserConsent.id == user_consent_id).filter( + UserConsent.user_id == user_id).first() + if uc: + uc.acceptance_date = acceptance_date + return True + + bogus_values = ( + {'user_id': 101, 'user_consent_id': 219}, + {'user_id': 145, 'user_consent_id': 1238}, + {'user_id': 164, 'user_consent_id': 218}, + {'user_id': 224, 'user_consent_id': 211}, + {'user_id': 310, 'user_consent_id': 1198}, + {'user_id': 310, 'user_consent_id': 1199}, + {'user_id': 310, 'user_consent_id': 1200}, + {'user_id': 4316, 'user_consent_id': 5033}, + {'user_id': 4316, 'user_consent_id': 5032}, + {'user_id': 98, 'user_consent_id': 339}, + {'user_id': 774, 'user_consent_id': 897}, + {'user_id': 723, 'user_consent_id': 551}, + {'user_id': 653, 'user_consent_id': 820}, + {'user_id': 563, 'user_consent_id': 5896}, + {'user_id': 6686, 'user_consent_id': 6117}, + ) + + correct_values = ( + {'user_id': 986, 'user_consent_id': 7434, 'acceptance_date': '2023/06/22 18:00:00'}, + ) + for row in correct_values: + if update_user_consent( + user_id=row['user_id'], + user_consent_id=row['user_consent_id'], + acceptance_date=row['acceptance_date']): + audit_insert( + subject_id=row['user_id'], + user_consent_id=row['user_consent_id'], + acceptance_date=row['acceptance_date']) + session.commit() + + for row in bogus_values: + if delete_user_consent( + user_id=row['user_id'], + user_consent_id=row['user_consent_id']): + audit_insert( + subject_id=row['user_id'], + user_consent_id=row['user_consent_id']) + session.commit() + + +def upgrade(): + """Correct UserConsents for any negatively affected patients + + Prior to the release of 23.10.12.1, moving withdrawal dates wasn't + allowed. This made lookups for the last valid user_consent *prior* + to the withdrawal date, reliable, as user_consents land in the table + in an ordered fashion, and the most recently deleted prior to + withdrawal would have been in use. + + The implementation of IRONN-210 enabled moving of withdrawal dates, + and incorrectly assumed it would be necessary to allow lookups of + the previous valid consent, to just work further back in the stack + of user_consents. That would only be correct on the few tested, + where the user didn't have multiple user_consents on file prior to + withdrawal. + + To enable moving withdrawal dates, user_consents now allow multiple + of status "suspended", with the most recent by id taking precedence. + To determine the valid consent in use prior to withdrawal, look back + by insertion order (id) for the first deleted user consent prior to + status "suspended". + + With code changes in place, migration must simply locate any potential + consent changes since the error was introduced and recalculate timeline + """ + # turns out, we have no reliable mechanism to determine which patients + # may have been affected, as the acceptance date on withdrawn was simply + # changed on the already withdrawn user_consent, and no audit of the + # modification was recorded. need to try a recalc and find persist + # any changes for any users with a suspended user_consent and more + # than two (the original valid consent plus the suspended one) on + # any given research study. + bind = op.get_bind() + session = Session(bind=bind) + + user_consent_manual_cleanup(session) + session.commit() + + +def downgrade(): + """no downgrade available""" + pass diff --git a/portal/models/adherence_data.py b/portal/models/adherence_data.py index a633d2770..f70473656 100644 --- a/portal/models/adherence_data.py +++ b/portal/models/adherence_data.py @@ -2,9 +2,10 @@ from datetime import datetime, timedelta from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy import UniqueConstraint +import re from ..database import db - +withdrawn = " post-withdrawn" class AdherenceData(db.Model): """ Cached adherence report data @@ -35,16 +36,20 @@ class AdherenceData(db.Model): 'patient_id', 'rs_id_visit', name='_adherence_unique_patient_visit'),) @staticmethod - def rs_visit_string(rs_id, visit_string): + def rs_visit_string(rs_id, visit_string, post_withdrawn=False): """trivial helper to build rs_id_visit string into desired format""" assert isinstance(rs_id, int) assert visit_string + if post_withdrawn: + visit_string += withdrawn return f"{rs_id}:{visit_string}" def rs_visit_parse(self): """break parts of rs_id and visit_string out of rs_id_visit field""" rs_id, visit_string = self.rs_id_visit.split(':') assert visit_string + if visit_string.endswith(withdrawn): + visit_string = visit_string[:-(len(withdrawn))] return int(rs_id), visit_string @staticmethod @@ -92,15 +97,24 @@ def sort_by_visit_key(d): :returns: list of values sorted by keys """ + pattern = re.compile(f"Month ([0-9]+)({withdrawn})?") def sort_key(key): if key == 'Baseline': return 0, 0 + elif key == f"Baseline{withdrawn}": + return 0, 1 elif key == 'Indefinite': return 2, 0 + elif key == f'Indefinite{withdrawn}': + return 2, 1 else: - month, num = key.split(" ") - assert month == "Month" - return 1, int(num) + match = pattern.match(key) + if not match.groups(): + raise ValueError(f"couldn't parse key {key}") + month_num = int(match.groups()[0]) + if match.groups()[1]: + month_num += 100 + return 1, month_num sorted_keys = sorted(d.keys(), key=sort_key) sorted_values = [d[key] for key in sorted_keys] diff --git a/portal/models/qb_status.py b/portal/models/qb_status.py index 83711bb62..26422a0ea 100644 --- a/portal/models/qb_status.py +++ b/portal/models/qb_status.py @@ -77,6 +77,11 @@ def _sync_timeline(self): # locate current qb - last found with start <= self.as_of_date cur_index, cur_qbd = None, None for i, qbd in zip(range(len(self.__ordered_qbs)), self.__ordered_qbs): + if self._withdrawal_date and ( + qbd.relative_start > self._withdrawal_date): + # as we now keep timeline data beyond withdrawal, break + # out if the requested date is beyond withdrawal + break if qbd.relative_start <= self.as_of_date: cur_index = i cur_qbd = qbd diff --git a/portal/models/qb_timeline.py b/portal/models/qb_timeline.py index 08f34e6de..aee9822af 100644 --- a/portal/models/qb_timeline.py +++ b/portal/models/qb_timeline.py @@ -4,7 +4,6 @@ from dateutil.relativedelta import relativedelta from flask import current_app -import redis from redis.exceptions import ConnectionError from sqlalchemy.types import Enum as SQLA_Enum from werkzeug.exceptions import BadRequest @@ -13,13 +12,9 @@ from ..cache import cache, TWO_HOURS from ..database import db from ..date_tools import FHIR_datetime, RelativeDelta -from ..factories.celery import create_celery +from ..factories.redis import create_redis from ..set_tools import left_center_right -from ..timeout_lock import ( - ADHERENCE_DATA_KEY, - CacheModeration, - TimeoutLock, -) +from ..timeout_lock import ADHERENCE_DATA_KEY, CacheModeration, TimeoutLock from ..trace import trace from .adherence_data import AdherenceData from .overall_status import OverallStatus @@ -95,6 +90,8 @@ def timeline_state(user_id): results = dict() for i in tl: qb = QuestionnaireBank.query.get(i.qb_id) + if qb is None: + continue recur_id = qb.recurs[0].id if qb.recurs else None vn = visit_name(QBD( relative_start=None, @@ -105,25 +102,6 @@ def timeline_state(user_id): vn, name_map[i.qb_id], i.qb_iteration] return results - @staticmethod - def withdrawn_qbd(user_id, research_study_id): - """Returns active QBD at time of user's withdrawal if applicable - - :returns: a QBD representing the visit active at point of withdrawal - from given study, using `relative_start` to hold date-time of - withdrawal; or None if n/a - """ - qbt = QBT.query.filter(QBT.user_id == user_id).filter( - QBT.research_study_id == research_study_id).filter( - QBT.status == OverallStatus.withdrawn).first() - if not qbt: - return None - return QBD( - relative_start=qbt.at, - iteration=qbt.qb_iteration, - recur_id=qbt.qb_recur_id, - qb_id=qbt.qb_id) - class AtOrderedList(list): """Specialize ``list`` to maintain insertion order and ``at`` attribute @@ -289,6 +267,8 @@ def calc_and_adjust_start(user, research_study_id, qbd, initial_trigger): return qbd.relative_start delta = users_trigger - initial_trigger + # this case should no longer be possible; raise the alarm + raise RuntimeError("found initial trigger to differ by: %s", str(delta)) current_app.logger.debug("calc_and_adjust_start delta: %s", str(delta)) return qbd.relative_start + delta @@ -604,7 +584,7 @@ def ordered_qbs(user, research_study_id, classification=None): This does NOT include the indefinite classification unless requested, as it plays by a different set of rules. - :param user: the user to lookup + :param user: the user to look up :param research_study_id: the research study being processed :param classification: set to ``indefinite`` for that special handling :returns: QBD for each (QB, iteration, recur) @@ -723,11 +703,6 @@ def ordered_qbs(user, research_study_id, classification=None): if transition_now: rp_flyweight.transition() - # done if user withdrew before QB starts - if withdrawal_date and withdrawal_date < rp_flyweight.cur_start: - trace("withdrawn as of {}".format(withdrawal_date)) - break - rp_flyweight.adjust_start() yield rp_flyweight.cur_qbd @@ -786,6 +761,17 @@ def invalidate_users_QBT(user_id, research_study_id): for ad in adh_data: db.session.delete(ad) + if not current_app.config.get("TESTING", False): + # clear the timeout lock as well, since we need a refresh + # after deletion of the adherence data + # otherwise, we experience a deadlock situation where tables can't be dropped + # between test runs, as postgres believes a deadlock condition exists + cache_moderation = CacheModeration(key=ADHERENCE_DATA_KEY.format( + patient_id=user_id, + research_study_id=research_study_id)) + cache_moderation.reset() + + # args have to match order and values - no wild carding avail as_of = QB_StatusCacheKey().current() if research_study_id != 'all': @@ -886,6 +872,7 @@ def update_users_QBT(user_id, research_study_id, invalidate_existing=False): def attempt_update(user_id, research_study_id, invalidate_existing): """Updates user's QBT or raises if lock is unattainable""" from .qb_status import patient_research_study_status + from ..tasks import LOW_PRIORITY, cache_single_patient_adherence_data # acquire a multiprocessing lock to prevent multiple requests # from duplicating rows during this slow process @@ -933,7 +920,7 @@ def attempt_update(user_id, research_study_id, invalidate_existing): trace(f"user determined ineligible for {research_study_id}") return - # Create time line for user, from initial trigger date + # Create time-line for user, from initial trigger date qb_generator = ordered_qbs(user, research_study_id) user_qnrs = QNR_results(user, research_study_id) @@ -979,7 +966,7 @@ def attempt_update(user_id, research_study_id, invalidate_existing): # QBs - one needing to be removed (say the old # month 36) in favor of the skipped new (say # month 33), and the last legit old one (say - # month 30) needing it's endpoint adjusted + # month 30) needing its endpoint adjusted # further below. remove_qb_id = pending_qbts[i].qb_id remove_iteration = pending_qbts[i].qb_iteration @@ -1054,7 +1041,7 @@ def attempt_update(user_id, research_study_id, invalidate_existing): "Problematic qbd: %s", user_id, str(qbd)) continue - # Must double check overlap; may no longer be true, if + # Must double-check overlap; may no longer be true, if # last_posted_index was one before... if pending_qbts[last_posted_index].at > start: # For questionnaires with common instrument names that @@ -1170,25 +1157,27 @@ def attempt_update(user_id, research_study_id, invalidate_existing): pending_qbts.append(QBT( at=expired_date, status='expired', **kwargs)) - # If user withdrew from study - remove any rows post withdrawal + # If user withdrew from study, add a row marking the withdrawal + # to the user's timeline, at the proper sequence. num_stored = 0 _, withdrawal_date = consent_withdrawal_dates( user, research_study_id=research_study_id) if withdrawal_date: trace("withdrawn as of {}".format(withdrawal_date)) - store_rows = [ - qbt for qbt in pending_qbts if qbt.at < withdrawal_date] - if store_rows: - # To satisfy the `Withdrawn sanity check` in qb_status - # the withdrawn row needs to match the last valid qb - kwargs['qb_id'] = store_rows[-1].qb_id - kwargs['qb_iteration'] = store_rows[-1].qb_iteration - kwargs['qb_recur_id'] = store_rows[-1].qb_recur_id - - store_rows.append(QBT( - at=withdrawal_date, - status='withdrawn', - **kwargs)) + j = 0 + for qbt in pending_qbts: + if qbt.at > withdrawal_date: + break + j += 1 + if j > 0: + # include visit in withdrawn for qb_status functionality + kwargs['qb_id'] = pending_qbts[j-1].qb_id + kwargs['qb_iteration'] = pending_qbts[j-1].qb_iteration + kwargs['qb_recur_id'] = pending_qbts[j-1].qb_recur_id + store_rows = ( + pending_qbts[0:j] + + [QBT(at=withdrawal_date, status='withdrawn', **kwargs)] + + pending_qbts[j:]) check_for_overlaps(store_rows) db.session.add_all(store_rows) num_stored = len(store_rows) @@ -1204,19 +1193,12 @@ def attempt_update(user_id, research_study_id, invalidate_existing): db.session.commit() # With fresh calculation of a user's timeline, queue update of - # user's adherence data as celery job, avoiding recursive issues - # if this call happens to be part of an already running update - cache_moderation = CacheModeration(key=ADHERENCE_DATA_KEY.format( - patient_id=user_id, - research_study_id=research_study_id)) - if not cache_moderation.run_recently(): - kwargs = { - 'patient_id': user_id, - 'research_study_id': research_study_id} - celery = create_celery(current_app) - celery.send_task( - 'portal.tasks.cache_adherence_data_task', - kwargs=kwargs) + # user's adherence data as celery job + kwargs = { + 'patient_id': user_id, + 'research_study_id': research_study_id} + cache_single_patient_adherence_data.apply_async( + kwargs=kwargs, queue=LOW_PRIORITY, retry=False) success = False for attempt in range(1, 6): @@ -1262,8 +1244,7 @@ def __init__(self): # Lookup the configured expiration of the matching cache # container ("DOGPILE_CACHE_REGIONS" -> "assessment_cache_region") if self.redis is None: - self.redis = redis.StrictRedis.from_url( - current_app.config['REDIS_URL']) + self.redis = create_redis(current_app.config['REDIS_URL']) regions = current_app.config['DOGPILE_CACHE_REGIONS'] for region_name, duration in regions: if region_name == self.region_name: diff --git a/portal/models/questionnaire_response.py b/portal/models/questionnaire_response.py index 3ca4981a8..0353ffa6f 100644 --- a/portal/models/questionnaire_response.py +++ b/portal/models/questionnaire_response.py @@ -1217,3 +1217,76 @@ def first_last_like_qnr(qnr): continue last = q return initial, last + + +def capture_patient_state(patient_id): + """Call to capture QBT and QNR state for patient, used for before/after""" + from .qb_timeline import QBT + qnrs = QuestionnaireResponse.qnr_state(patient_id) + tl = QBT.timeline_state(patient_id) + return {'qnrs': qnrs, 'timeline': tl} + + +def present_before_after_state(user_id, external_study_id, before_state): + from .qb_timeline import QBT + from ..dict_tools import dict_compare + after_qnrs = QuestionnaireResponse.qnr_state(user_id) + after_timeline = QBT.timeline_state(user_id) + qnrs_lost_reference = [] + any_change_noted = False + + def visit_from_timeline(qb_name, qb_iteration, timeline_results): + """timeline results have computed visit name - quick lookup""" + for visit, name, iteration in timeline_results.values(): + if qb_name == name and qb_iteration == iteration: + return visit + raise ValueError(f"no visit found for {qb_name}, {qb_iteration}") + + # Compare results + added_q, removed_q, modified_q, same = dict_compare( + after_qnrs, before_state['qnrs']) + assert not added_q + assert not removed_q + + added_t, removed_t, modified_t, same = dict_compare( + after_timeline, before_state['timeline']) + + if any((added_t, removed_t, modified_t, modified_q)): + any_change_noted = True + print(f"\nPatient {user_id} ({external_study_id}):") + if modified_q: + any_change_noted = True + print("\tModified QNRs (old, new)") + for mod in sorted(modified_q): + print(f"\t\t{mod} {modified_q[mod][1]} ==>" + f" {modified_q[mod][0]}") + # If the QNR previously had a reference QB and since + # lost it, capture for reporting. + if ( + modified_q[mod][1][0] != "none of the above" and + modified_q[mod][0][0] == "none of the above"): + visit_name = visit_from_timeline( + modified_q[mod][1][0], + modified_q[mod][1][1], + before_state["timeline"]) + qnrs_lost_reference.append((visit_name, modified_q[mod][1][2])) + if added_t: + any_change_noted = True + print("\tAdditional timeline rows:") + for item in sorted(added_t): + print(f"\t\t{item} {after_timeline[item]}") + if removed_t: + any_change_noted = True + print("\tRemoved timeline rows:") + for item in sorted(removed_t): + print( + f"\t\t{item} " + f"{before_state['timeline'][item]}") + if modified_t: + any_change_noted = True + print(f"\tModified timeline rows: (old, new)") + for item in sorted(modified_t): + print(f"\t\t{item}") + print(f"\t\t\t{modified_t[item][1]} ==> {modified_t[item][0]}") + + return after_qnrs, after_timeline, qnrs_lost_reference, any_change_noted diff --git a/portal/models/reporting.py b/portal/models/reporting.py index 2b29b17ec..6292f383e 100644 --- a/portal/models/reporting.py +++ b/portal/models/reporting.py @@ -35,7 +35,7 @@ from .user_consent import consent_withdrawal_dates -def single_patient_adherence_data(patient, as_of_date, research_study_id): +def single_patient_adherence_data(patient_id, research_study_id): """Update any missing (from cache) adherence data for patient NB: all changes are side effects, persisted in adherence_data table. @@ -48,8 +48,9 @@ def single_patient_adherence_data(patient, as_of_date, research_study_id): :returns: number of added rows """ + as_of_date = datetime.utcnow() cache_moderation = CacheModeration(key=ADHERENCE_DATA_KEY.format( - patient_id=patient.id, + patient_id=patient_id, research_study_id=research_study_id)) if cache_moderation.run_recently(): return 0 @@ -86,19 +87,22 @@ def patient_data(patient): def general_row_detail(row, patient, qbd): """Add general (either study) data for given (patient, qbd)""" # purge values that may have previous row data set and aren't certain - for key in "completion_date", "oow_completion_date", "entry_method": + for key in "oow_completion_date", "entry_method", "visit": row.pop(key, None) row['qb'] = qbd.questionnaire_bank.name - row['visit'] = visit_name(qbd) - if row['status'] == 'Completed': - row['completion_date'] = report_format( - qbd.completed_date(patient.id)) or "" - row['oow_completion_date'] = report_format( - qbd.oow_completed_date(patient.id)) or "" + + # if withdrawn, include a row with that and little more if row['status'] == 'Withdrawn': - # visit unreliable when withdrawn - clear - row['visit'] = '' + # use date of withdrawal for "completion date" + _, withdrawal_date = consent_withdrawal_dates( + user=patient, research_study_id=research_study_id) + row['completion_date'] = report_format(withdrawal_date) + return + row['visit'] = visit_name(qbd) + row['completion_date'] = ( + report_format(qbd.completed_date(patient.id)) + if row['status'] == 'Completed' else '') entry_method = QNR_results( patient, research_study_id=research_study_id, @@ -109,6 +113,9 @@ def general_row_detail(row, patient, qbd): def empro_row_detail(row, ts_reporting): """Add EMPRO specifics""" + if not ts_reporting: + return + # Rename column header for EMPRO if 'completion_date' in row: row['EMPRO_questionnaire_completion_date'] = ( @@ -135,6 +142,7 @@ def empro_row_detail(row, ts_reporting): row['content_domains_accessed'] = ', '.join(da) if da else "" added_rows = 0 + patient = User.query.get(patient_id) qb_stats = QB_Status( user=patient, research_study_id=research_study_id, @@ -167,12 +175,12 @@ def empro_row_detail(row, ts_reporting): if not exp_row or exp_row.at > as_of_date: row["status"] = "Not Yet Available" + ts_reporting = ( + TriggerStatesReporting(patient_id=patient.id) + if research_study_id == EMPRO_RS_ID else None) if last_viable: general_row_detail(row, patient, last_viable) - if research_study_id == EMPRO_RS_ID: - # Initialize trigger states reporting for patient - ts_reporting = TriggerStatesReporting(patient_id=patient.id) - empro_row_detail(row, ts_reporting) + empro_row_detail(row, ts_reporting) # latest is only valid for a day, unless the user withdrew valid_for = 30 if row['status'] in ('Expired', 'Withdrawn') else 1 @@ -183,6 +191,43 @@ def empro_row_detail(row, ts_reporting): data=row) added_rows += 1 + # if the last row was withdrawn, add any completed visits beyond + # date of withdrawal + if row["status"] == 'Withdrawn': + withdrawal_date = ( + row['completion_date'] if 'completion_date' in row + else row['EMPRO_questionnaire_completion_date']) + missing_qbts = [] + completed_after_withdrawn = QBT.query.filter( + QBT.at > withdrawal_date).filter( + QBT.status == OverallStatus.completed).filter( + QBT.research_study_id == research_study_id).filter( + QBT.user_id == patient.id).order_by(QBT.at) + for qbt in completed_after_withdrawn: + missing_qbts.append((qbt.at, qbt.qbd())) + + # one more special case! the withdrawn visit was completed + # but BEFORE the user withdrew. the qb_status accurately sees + # the visit as withdrawn, and wrote that to the last row, but + # failed to write out the completed status first. + pre_wd_visit_cd = last_viable.completed_date(patient.id) + if pre_wd_visit_cd and not [ + x for x, y in missing_qbts if x == pre_wd_visit_cd]: + missing_qbts.append((pre_wd_visit_cd, last_viable)) + + for at, qbd in missing_qbts: + row['status'] = 'Completed' # overwrite withdrawn state + general_row_detail(row, patient, qbd) + empro_row_detail(row, ts_reporting) + rs_visit = AdherenceData.rs_visit_string( + research_study_id, row['visit'], post_withdrawn=True) + AdherenceData.persist( + patient_id=patient.id, + rs_id_visit=rs_visit, + valid_for_days=30, + data=row) + added_rows += 1 + # as we require a full history, continue to add rows for each previous for qbd, status in qb_stats.older_qbds(last_viable): rs_visit = AdherenceData.rs_visit_string( @@ -195,9 +240,7 @@ def empro_row_detail(row, ts_reporting): historic = row.copy() historic['status'] = status general_row_detail(historic, patient, qbd) - - if research_study_id == EMPRO_RS_ID: - empro_row_detail(historic, ts_reporting) + empro_row_detail(historic, ts_reporting) AdherenceData.persist( patient_id=patient.id, rs_id_visit=rs_visit, @@ -267,6 +310,7 @@ def cache_adherence_data( if limit was hit """ + from ..tasks import cache_single_patient_adherence_data # For building cache, use system account; skip privilege checks acting_user = User.query.filter_by(email='__system__').one() as_of_date = datetime.utcnow() @@ -295,11 +339,16 @@ def patient_generator(): added_rows = 0 for patient in patient_generator(): - if added_rows > limit: + if limit and added_rows > limit: current_app.logger.info( "pre-mature exit caching adherence data having hit limit") break - single_patient_adherence_data(patient, as_of_date, research_study_id) + # queue patient's adherence cache refresh as a separate job + kwargs = { + 'patient_id': patient.id, + 'research_study_id': research_study_id} + cache_single_patient_adherence_data.apply_async( + kwargs=kwargs, retry=False) return {'added': added_rows, 'limit_hit': limit and added_rows > limit} diff --git a/portal/models/user.py b/portal/models/user.py index b7b073d1f..b39de7116 100644 --- a/portal/models/user.py +++ b/portal/models/user.py @@ -323,7 +323,7 @@ class User(db.Model, UserMixin): cascade='delete') _consents = db.relationship( 'UserConsent', lazy='joined', cascade='delete', - order_by="desc(UserConsent.acceptance_date)") + order_by="desc(UserConsent.id)") indigenous = db.relationship(Coding, lazy='dynamic', secondary="user_indigenous") encounters = db.relationship('Encounter', cascade='delete') diff --git a/portal/models/user_consent.py b/portal/models/user_consent.py index 7b3df880c..c326e6bcb 100644 --- a/portal/models/user_consent.py +++ b/portal/models/user_consent.py @@ -165,7 +165,7 @@ def latest_consent(user, research_study_id): if no match is located """ - # consents are ordered desc(acceptance_date) + # consents are ordered desc(id), i.e. most recent action first for consent in user.valid_consents: if consent.research_study_id != research_study_id: continue @@ -197,7 +197,10 @@ def consent_withdrawal_dates(user, research_study_id): return consent.acceptance_date, withdrawal_date # Look for withdrawn case. If found, also look up the previous - # consent date (prior to withdrawal) + # consent date (prior to withdrawal). As withdrawal dates can + # be moved, continue to look back beyond all `suspended` until + # one of status deleted is found, as that would be the last one + # valid prior to withdrawal. prior_acceptance = None for consent in user.all_consents: @@ -205,13 +208,15 @@ def consent_withdrawal_dates(user, research_study_id): continue if not withdrawal_date and ( consent.status == 'suspended' and not consent.deleted_id): + # the first or most recent withdrawal takes precedence. withdrawal_date = consent.acceptance_date if prior_acceptance: raise ValueError( "don't expect prior acceptance before withdrawal date") if consent.status == 'deleted' and withdrawal_date: + # the first deleted prior to any number of `suspended` is + # taken to be the most recent legit consent prior to withdrawal prior_acceptance = consent.acceptance_date - # situation where consent date was changed before withdrawal - # requires we continue to look and use last found (don't break!) + break return prior_acceptance, withdrawal_date diff --git a/portal/tasks.py b/portal/tasks.py index f53a9b20d..be2317f5b 100644 --- a/portal/tasks.py +++ b/portal/tasks.py @@ -14,13 +14,13 @@ from celery.utils.log import get_task_logger from flask import current_app -import redis from requests import Request, Session from requests.exceptions import RequestException from sqlalchemy import and_ from .database import db from .factories.app import create_app +from .factories.redis import create_redis from .factories.celery import create_celery from .models.communication import Communication from .models.communication_request import queue_outstanding_messages @@ -32,6 +32,7 @@ cache_adherence_data, generate_and_send_summaries, research_report, + single_patient_adherence_data, ) from .models.research_study import ResearchStudy from .models.role import ROLE, Role @@ -113,9 +114,16 @@ def info(): queue=LOW_PRIORITY) @scheduled_task def cache_adherence_data_task(**kwargs): + """Queues up all patients needing a cache refresh""" return cache_adherence_data(**kwargs) +@celery.task(queue=LOW_PRIORITY, ignore_results=True) +def cache_single_patient_adherence_data(**kwargs): + """Populates adherence data for a single patient""" + return single_patient_adherence_data(**kwargs) + + @celery.task(bind=True, track_started=True, queue=LOW_PRIORITY) def adherence_report_task(self, **kwargs): current_app.logger.debug("launch adherence report task: %s", self.request.id) @@ -393,7 +401,7 @@ def token_watchdog(**kwargs): def celery_beat_health_check(**kwargs): """Refreshes self-expiring redis value for /healthcheck of celerybeat""" - rs = redis.StrictRedis.from_url(current_app.config['REDIS_URL']) + rs = create_redis(current_app.config['REDIS_URL']) return rs.setex( name='last_celery_beat_ping', time=current_app.config['LAST_CELERY_BEAT_PING_EXPIRATION_TIME'], @@ -406,7 +414,7 @@ def celery_beat_health_check(**kwargs): def celery_beat_health_check_low_priority_queue(**kwargs): """Refreshes self-expiring redis value for /healthcheck of celerybeat""" - rs = redis.StrictRedis.from_url(current_app.config['REDIS_URL']) + rs = create_redis(current_app.config['REDIS_URL']) return rs.setex( name='last_celery_beat_ping_low_priority_queue', time=10*current_app.config['LAST_CELERY_BEAT_PING_EXPIRATION_TIME'], diff --git a/portal/timeout_lock.py b/portal/timeout_lock.py index b33782d88..21ab8af39 100644 --- a/portal/timeout_lock.py +++ b/portal/timeout_lock.py @@ -1,8 +1,8 @@ import time from flask import current_app -import redis +from .factories.redis import create_redis class LockTimeout(BaseException): """Exception raised when wait for TimeoutLock exceeds timeout""" @@ -31,8 +31,7 @@ def __init__(self, key, expires=60, timeout=10): self.key = key self.timeout = timeout self.expires = expires - self.redis = redis.StrictRedis.from_url( - current_app.config['REDIS_URL']) + self.redis = create_redis(current_app.config['REDIS_URL']) def __enter__(self): timeout = self.timeout @@ -105,8 +104,7 @@ class CacheModeration(object): def __init__(self, key, timeout=300): self.key = key self.timeout = timeout - self.redis = redis.StrictRedis.from_url( - current_app.config['REDIS_URL']) + self.redis = create_redis(current_app.config['REDIS_URL']) def run_recently(self): """if key has value in redis (i.e. didn't expire) return value""" diff --git a/portal/views/healthcheck.py b/portal/views/healthcheck.py index af96c5db3..be84061a8 100644 --- a/portal/views/healthcheck.py +++ b/portal/views/healthcheck.py @@ -3,12 +3,12 @@ from celery.exceptions import TimeoutError from celery.result import AsyncResult from flask import Blueprint, current_app -import redis from redis.exceptions import ConnectionError from sqlalchemy import text from ..database import db from ..factories.celery import create_celery +from ..factories.redis import create_redis HEALTHCHECK_FAILURE_STATUS_CODE = 200 @@ -23,7 +23,7 @@ def celery_beat_ping(): This allows us to monitor whether celery beat tasks are running """ try: - rs = redis.StrictRedis.from_url(current_app.config['REDIS_URL']) + rs = create_redis(current_app.config['REDIS_URL']) rs.setex( name='last_celery_beat_ping', time=current_app.config['LAST_CELERY_BEAT_PING_EXPIRATION_TIME'], @@ -64,7 +64,7 @@ def celery_available(): def celery_beat_available(): """Determines whether celery beat is available""" try: - rs = redis.from_url(current_app.config['REDIS_URL']) + rs = create_redis(current_app.config['REDIS_URL']) # Celery beat feeds scheduled jobs (a la cron) to the respective # job queues (standard and low priority). As a monitor, a job @@ -109,7 +109,7 @@ def redis_available(): # is available. Otherwise we assume # it's not available try: - rs = redis.from_url(current_app.config["REDIS_URL"]) + rs = create_redis(current_app.config["REDIS_URL"]) rs.ping() return True, 'Redis is available.' except Exception as e: diff --git a/portal/views/patient.py b/portal/views/patient.py index ac307f323..7ef1e9c16 100644 --- a/portal/views/patient.py +++ b/portal/views/patient.py @@ -30,14 +30,13 @@ from ..models.questionnaire_bank import QuestionnaireBank, trigger_date from ..models.questionnaire_response import QuestionnaireResponse from ..models.reference import Reference -from ..models.reporting import single_patient_adherence_data from ..models.research_study import ( - EMPRO_RS_ID, ResearchStudy, research_study_id_from_questionnaire ) from ..models.role import ROLE from ..models.user import User, current_user, get_user +from ..models.user_consent import consent_withdrawal_dates from ..timeout_lock import ADHERENCE_DATA_KEY, CacheModeration from .crossdomain import crossdomain from .demographics import demographics @@ -331,6 +330,7 @@ def patient_timeline(patient_id): from ..models.questionnaire_bank import visit_name from ..models.questionnaire_response import aggregate_responses from ..models.research_protocol import ResearchProtocol + from ..tasks import cache_single_patient_adherence_data from ..trace import dump_trace, establish_trace user = get_user(patient_id, permission='view') @@ -466,9 +466,11 @@ def get_recur_id(qnr): if not adherence_data: # immediately following a cache purge, adherence data is gone and # needs to be recreated. - now = datetime.utcnow() - single_patient_adherence_data( - user, as_of_date=now, research_study_id=EMPRO_RS_ID) + kwargs = { + "patient_id": user.id, + "research_study_id": research_study_id, + } + cache_single_patient_adherence_data(**kwargs) adherence_data = sorted_adherence_data(patient_id, research_study_id) qnr_responses = aggregate_responses( @@ -494,13 +496,20 @@ def get_recur_id(qnr): i['auth_method'] = d['encounter']['auth_method'] i['encounter_period'] = d['encounter']['period'] i['document_authored'] = d['authored'] - i['ae_session'] = d['identifier']['value'] + try: + i['ae_session'] = d['identifier']['value'] + except KeyError: + # happens with sub-study follow up, skip ae_session + pass i['status'] = d['status'] i['org'] = d['subject']['careProvider'][0]['display'] i['visit'] = d['timepoint'] qnr_data.append(i) + consent_date, withdrawal_date = consent_withdrawal_dates(user, research_study_id) + consents = {"consent_date": consent_date, "withdrawal_date": withdrawal_date} kwargs = { + "consents": consents, "rps": rps, "status": status, "posted": posted, diff --git a/portal/views/user.py b/portal/views/user.py index 45e0b7ccd..5acce26ea 100644 --- a/portal/views/user.py +++ b/portal/views/user.py @@ -765,7 +765,9 @@ def withdraw_user_consent(user_id): Used to withdraw a consent agreements between a user and an organization. If a consent exists for the given user/org, the consent will be marked deleted, and a matching consent (with new status/option values) will be - created in its place. + created in its place. If the user was already withdrawn, a new row will + be created also with status `suspended`, the prior will retain its + `suspended` status and marked deleted. NB Invalid to request a withdrawal date prior to current consent. @@ -891,8 +893,18 @@ def withdraw_consent( if acceptance_date > datetime.utcnow() + timedelta(days=1): raise ValueError( "Can't suspend with acceptance date in the future") + prior_withdrawal_date = wc.acceptance_date wc.acceptance_date = acceptance_date db.session.commit() + # Audit the change + auditable_event( + f"Consent agreement {wc.id} updated from {prior_withdrawal_date} " + f"to {acceptance_date}", + user_id=current_user().id, + subject_id=user.id, + context="consent" + ) + # As withdrawal time just changed, force recalculation invalidate_users_QBT( user_id=user.id, research_study_id=research_study_id) diff --git a/tests/test_consent.py b/tests/test_consent.py index da0c4b8dd..77000eba8 100644 --- a/tests/test_consent.py +++ b/tests/test_consent.py @@ -109,11 +109,11 @@ def test_consent_order(self): '/api/user/{}/consent'.format(TEST_USER_ID)) assert response.status_code == 200 assert len(response.json['consent_agreements']) == 3 - # should be ordered by acceptance date, descending: (uc3, uc1, uc2) + # regardless of age, most recent action takes precedence uc1, uc2, uc3 = map(db.session.merge, (uc1, uc2, uc3)) assert response.json['consent_agreements'][0] == uc3.as_json() - assert response.json['consent_agreements'][1] == uc1.as_json() - assert response.json['consent_agreements'][2] == uc2.as_json() + assert response.json['consent_agreements'][1] == uc2.as_json() + assert response.json['consent_agreements'][2] == uc1.as_json() def test_post_user_consent(self): self.shallow_org_tree() diff --git a/tests/test_healthcheck.py b/tests/test_healthcheck.py index 7ace20aa4..66bc368c6 100644 --- a/tests/test_healthcheck.py +++ b/tests/test_healthcheck.py @@ -29,21 +29,21 @@ def test_celery_available_fails_when_celery_ping_fails( results = celery_available() assert results[0] is False - @patch('portal.views.healthcheck.redis') + @patch('portal.views.healthcheck.create_redis') def test_celery_beat_available_fails_when_redis_var_none( self, - redis_mock + create_redis_mock ): - redis_mock.from_url.return_value.get.return_value = None + create_redis_mock.return_value.get.return_value = None results = celery_beat_available() assert results[0] is False - @patch('portal.views.healthcheck.redis') + @patch('portal.views.healthcheck.create_redis') def test_celery_beat_available_succeeds_when_redis_var_set( self, - redis_mock + create_redis_mock ): - redis_mock.from_url.return_value.get.return_value = \ + create_redis_mock.return_value.get.return_value = \ str(datetime.now()) results = celery_beat_available() assert results[0] is True @@ -68,21 +68,21 @@ def test_postgresql_available_fails_when_query_exception( results = postgresql_available() assert results[0] is False - @patch('portal.views.healthcheck.redis') + @patch('portal.views.healthcheck.create_redis') def test_redis_available_succeeds_when_ping_successful( self, - redis_mock + create_redis_mock ): - redis_mock.from_url.return_value.ping.return_value = True + create_redis_mock.return_value.ping.return_value = True results = redis_available() assert results[0] is True - @patch('portal.views.healthcheck.redis') + @patch('portal.views.healthcheck.create_redis') def test_redis_available_fails_when_ping_throws_exception( self, - redis_mock + create_redis_mock ): - redis_mock.from_url.return_value.ping.side_effect = \ + create_redis_mock.return_value.ping.side_effect = \ redis.ConnectionError() results = redis_available() assert results[0] is False diff --git a/tests/test_intervention.py b/tests/test_intervention.py index 69efd1070..a29fd3214 100644 --- a/tests/test_intervention.py +++ b/tests/test_intervention.py @@ -555,6 +555,7 @@ def test_in_role(initialize_static, test_user): assert sm.quick_access_check(user) +@pytest.mark.skip("no longer supporting moving initial trigger dates") def test_card_html_update( client, initialize_static, initialized_patient_logged_in): """Confirm assessment status state affects AE card on /home view""" diff --git a/tests/test_qb_timeline.py b/tests/test_qb_timeline.py index 19e0b9293..2aba026f5 100644 --- a/tests/test_qb_timeline.py +++ b/tests/test_qb_timeline.py @@ -309,7 +309,7 @@ def test_qb_post_consent_change(self): assert qbstatus.overall_status == OverallStatus.completed def test_withdrawn(self): - # qbs should halt beyond withdrawal + # check qb_status post withdrawal crv = self.setup_org_qbs() crv_id = crv.id # consent 17 months in past @@ -334,8 +334,9 @@ def test_withdrawn(self): for n in (3, 6, 9, 15): assert visit_name(next(gen)) == 'Month {}'.format(n) - with pytest.raises(StopIteration): - next(gen) + # current should be withdrawn, subsequent avail in case + # post withdrawn results come in + assert visit_name(next(gen)) == 'Month 18' # Confirm withdrawn user can still access "current" # as needed for reporting @@ -343,9 +344,10 @@ def test_withdrawn(self): qb_stats = QB_Status( user=user, research_study_id=0, - as_of_date=now) + as_of_date=now+relativedelta(days=1)) current = qb_stats.current_qbd(even_if_withdrawn=True) assert current + assert qb_stats.overall_status == OverallStatus.withdrawn def test_change_midstream_rp(self): back7, nowish = associative_backdate( diff --git a/tests/test_reporting.py b/tests/test_reporting.py index a81c39ff5..0d1675272 100644 --- a/tests/test_reporting.py +++ b/tests/test_reporting.py @@ -3,6 +3,7 @@ from datetime import datetime from dateutil.relativedelta import relativedelta from flask_webtest import SessionScope +from time import sleep from portal.cache import cache from portal.extensions import db @@ -150,16 +151,40 @@ def test_adherence_sort(self): "completion_date ": "19 - Jun - 2023 07: 42:46 ", "oow_completion_date": "" }, + "Month 12 post-withdrawn": { + "qb": "CRV Baseline v2", + "site": "CRV", + "visit": "Month 12", + "status": "Completed", + "consent": "20 - May - 2023 07: 42:46 ", + "completion_date ": "25 - Jun - 2023 00:00:00 ", + "country ": None, + "user_id ": 3, + "study_id": "study user 3", + "site_code": ""}, "Month 12": { "qb": "CRV Baseline v2", "site": "CRV", "visit": "Month 12", - "status": "Overdue", + "status": "Withdrawn", + "completion_date ": "22 - Jun - 2023 00:00:00 ", "consent": "20 - May - 2023 07: 42:46 ", "country ": None, "user_id ": 3, "study_id": "study user 3", "site_code": ""}, + "Baseline post-withdrawn": { + "qb": "CRV Baseline v2", + "site": "CRV", + "visit": "Baseline", + "status": "Completed", + "completion_date ": "22 - Jun - 2023 00:00:00 ", + "consent": "19 - Jun - 2023 07: 42:46", + "country ": None, + "user_id ": 2, + "study_id": "study user 2", + "site_code": "" + }, "Baseline": { "qb": "CRV Baseline v2", "site": "CRV", @@ -173,10 +198,17 @@ def test_adherence_sort(self): }, } results = sort_by_visit_key(sort_me) - assert len(results) == 3 + assert len(results) == 5 assert results[0]["visit"] == "Baseline" - assert results[1]["visit"] == "Month 3" - assert results[2]["visit"] == "Month 12" + assert results[0]["status"] == "Due" + assert results[1]["visit"] == "Baseline" + assert results[1]["status"] == "Completed" + assert results[2]["visit"] == "Month 3" + assert results[2]["status"] == "Completed" + assert results[3]["visit"] == "Month 12" + assert results[3]["status"] == "Withdrawn" + assert results[4]["visit"] == "Month 12" + assert results[4]["status"] == "Completed" def populate_adherence_cache(self, test_users): """helper method to bring current test user state into adherence cache""" @@ -298,6 +330,7 @@ def test_results(self): self.consent_with_org(org_id=org_id) self.login() self.populate_adherence_cache(test_users=(user2, user3, user4)) + sleep(5) # as adherence jobs run independently, give em time response = self.results_from_async_call( "/api/report/questionnaire_status", timeout=10) From 8c597efa50c1a949b0f2e3caca5e58553ea511d6 Mon Sep 17 00:00:00 2001 From: pbugni Date: Wed, 6 Mar 2024 14:04:25 -0800 Subject: [PATCH 8/9] confirm staff have valid email before attempt to send (#4366) found additional cases where test system may be attempting to send EMPRO alert emails to user's w/o a valid email address (staff). added detailed logging where failure is occurring, as well. --- portal/models/message.py | 4 ++++ portal/trigger_states/empro_messages.py | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/portal/models/message.py b/portal/models/message.py index 48ac3cd7c..36209ca25 100644 --- a/portal/models/message.py +++ b/portal/models/message.py @@ -157,6 +157,10 @@ def send_message(self, cc_address=None): NB the cc isn't persisted with the rest of the record. """ + if not self.recipients: + current_app.logger.error( + "can't email w/o recipients. failed to send " + f"'{self.subject}' to user {self.recipient_id}") message = Message( subject=self.subject, extra_headers=extra_headers(), diff --git a/portal/trigger_states/empro_messages.py b/portal/trigger_states/empro_messages.py index 65dcf26c6..0ad1c5015 100644 --- a/portal/trigger_states/empro_messages.py +++ b/portal/trigger_states/empro_messages.py @@ -37,6 +37,7 @@ def patient_email(patient, soft_triggers, hard_triggers): mr = MailResource( app_text(name), locale_code=patient.locale_code, variables=args) em = EmailMessage( + recipient_id=patient.id, recipients=patient.email, sender=current_app.config['MAIL_DEFAULT_SENDER'], subject=mr.subject, @@ -112,11 +113,15 @@ def staff_emails(patient, hard_triggers, initial_notification): } emails = [] for staff in staff_list: + if not staff.email_ready(): + current_app.logger.error(f"can't email staff {staff} without email") + continue mr = MailResource( app_text(app_text_name), locale_code=staff.locale_code, variables=args) emails.append(EmailMessage( + recipient_id=staff.id, recipients=staff.email, sender=current_app.config['MAIL_DEFAULT_SENDER'], subject=mr.subject, From fb2491461c6b8f36deb1d59a11cda05aea36f4e1 Mon Sep 17 00:00:00 2001 From: pbugni Date: Thu, 7 Mar 2024 08:18:35 -0800 Subject: [PATCH 9/9] log missing cur_qbd situation (#4367) the following was recently found on elk: ``` File "/opt/venvs/portal/lib/python3.9/site-packages/portal/models/qb_status.py", line 102, in _sync_timeline if cur_index > 0: TypeError: '>' not supported between instances of 'NoneType' and 'int' ``` pencil tracing the code, can't think of a logical explanation for the combination of variables that would cause this. modified code to stop raising exceptions, be more robust and log an error if this happens again with more detail. --- portal/models/qb_status.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/portal/models/qb_status.py b/portal/models/qb_status.py index 26422a0ea..c6cf1e774 100644 --- a/portal/models/qb_status.py +++ b/portal/models/qb_status.py @@ -89,22 +89,24 @@ def _sync_timeline(self): break # w/o a cur, probably hasn't started, set expired and leave - if not cur_qbd and ( - self.__ordered_qbs[0].relative_start > self.as_of_date): - trace( - "no current QBD (too early); first qb doesn't start till" - " {} vs as_of {}".format( - self.__ordered_qbs[0].relative_start, self.as_of_date)) + if not cur_qbd: + if self.__ordered_qbs[0].relative_start > self.as_of_date: + trace( + "no current QBD (too early); first qb doesn't start till" + " {} vs as_of {}".format( + self.__ordered_qbs[0].relative_start, self.as_of_date)) + else: + current_app.logger.error(f"patient {self.user.id} w/o cur_qbd??") self._overall_status = OverallStatus.expired self.next_qbd = self.__ordered_qbs[0] return - if cur_index > 0: + if cur_index and cur_index > 0: self.prev_qbd = self.__ordered_qbs[cur_index-1] else: self.prev_qbd = None - if cur_index < len(self.__ordered_qbs) - 1: + if cur_index and cur_index < len(self.__ordered_qbs) - 1: self.next_qbd = self.__ordered_qbs[cur_index+1] else: self.next_qbd = None