From 556dfaa2792c25422d79fe4fcdaebcd25ac26eb7 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Wed, 19 Aug 2026 10:18:42 -0500 Subject: [PATCH 01/27] Initial implementation of transaction record corrections --- .../lambdas/python/common/cc_common/config.py | 4 + .../cc_common/data_model/data_client.py | 42 +++++ .../data_model/transaction_client.py | 64 +++++++ .../lambdas/python/common/tests/__init__.py | 1 + .../python/common/tests/function/__init__.py | 12 ++ .../test_data_client_ssn_correction.py | 139 +++++++++++++++ .../test_transaction_client.py | 166 ++++++++++++++++++ .../unit/test_data_model/test_data_client.py | 66 +++++++ .../python/provider-data-v1/tests/__init__.py | 2 + .../tests/function/__init__.py | 25 +++ .../function/test_handlers/test_ingest.py | 34 +++- .../compact-connect/stacks/ingest_stack.py | 11 +- .../transaction_history_table.py | 16 ++ 13 files changed, 579 insertions(+), 3 deletions(-) diff --git a/backend/compact-connect/lambdas/python/common/cc_common/config.py b/backend/compact-connect/lambdas/python/common/cc_common/config.py index 4c46a62a19..74f627deb8 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/config.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/config.py @@ -304,6 +304,10 @@ def transaction_history_table_name(self): def transaction_history_table(self): return boto3.resource('dynamodb').Table(self.transaction_history_table_name) + @property + def transaction_history_transaction_id_gsi_name(self): + return os.environ['TRANSACTION_HISTORY_TRANSACTION_ID_GSI_NAME'] + @property def rate_limiting_table_name(self): return os.environ['RATE_LIMITING_TABLE_NAME'] diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py index 14914cb162..9ecfb86672 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py @@ -3221,6 +3221,19 @@ def migrate_provider_for_ssn_correction( new_provider_id=new_provider_id, ) + # Re-point the practitioner's payment transactions at the new provider id, for the same replay-safety + # reason as the document move above: the commit below is the point at which the idempotency guard + # flips, so a failure before it retries the whole migration (and these writes are idempotent), while + # anything left until after it would never be retried at all. This runs on partial migrations too - + # the privileges purchased against the corrected license move in both cases. + payment_transaction_ids = self._collect_transaction_ids(records_to_move) + if payment_transaction_ids: + self.config.transaction_client.update_licensee_id_for_transactions( + compact=compact, + transaction_ids=payment_transaction_ids, + new_licensee_id=new_provider_id, + ) + all_transaction_items = [ *create_transaction_items, *delete_transaction_items, @@ -3255,6 +3268,35 @@ def migrate_provider_for_ssn_correction( ), ) + @staticmethod + def _collect_transaction_ids(records: list[CCDataClass]) -> set[str]: + """ + Collect every payment transaction id referenced by the privilege records being migrated. + + A privilege record carries the transaction id of its most recent purchase or renewal; the earlier + ones survive only on the privilege update history records, in `previous` (and, for a renewal, in + `updatedValues`). Taking the union of all three covers the practitioner's full purchase history for + the privileges that are moving. + """ + transaction_ids = set() + for record in records: + if record.type == ProviderRecordType.PRIVILEGE: + # compactTransactionId is optional on the privilege record, so this reads through the dict + # rather than the data class property, which raises when the field is absent + transaction_id = record.to_dict().get('compactTransactionId') + if transaction_id: + transaction_ids.add(transaction_id) + elif record.type == ProviderRecordType.PRIVILEGE_UPDATE: + transaction_ids.update( + transaction_id + for transaction_id in ( + record.previous.get('compactTransactionId'), + record.updatedValues.get('compactTransactionId'), + ) + if transaction_id + ) + return transaction_ids + @staticmethod def _provider_record_key(record: CCDataClass) -> dict[str, str]: """Get the current pk/sk of a record, as regenerated by its schema.""" diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py index a77f8340c3..ba74fb56a0 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py @@ -7,6 +7,9 @@ from cc_common.data_model.schema.transaction.record import UnsettledTransactionRecordSchema AUTHORIZE_DOT_NET_CLIENT_TYPE = 'authorize.net' +# The record type of a settled transaction, as registered by TransactionRecordSchema. Distinguishes +# settled transactions from the 'unsettled_transaction' records that share their compact/transactionId. +SETTLED_TRANSACTION_RECORD_TYPE = 'transaction' class TransactionClient: @@ -282,6 +285,67 @@ def add_privilege_information_to_transactions( return transactions + def update_licensee_id_for_transactions( + self, *, compact: str, transaction_ids: set[str], new_licensee_id: str + ) -> int: + """ + Re-point the licenseeId of the given transactions at a new provider id. + + Used by the SSN-correction migration: a transaction records the provider id that was in place when + the privilege was purchased, and the transaction report resolves practitioner names from that field. + Without this, every transaction settled before a correction reports the practitioner as UNKNOWN. + + Only settled 'transaction' records are written, since unsettled transactions do not have a licenseeId field. + Once it does settle, the settlement processing workflow resolves the licensee id from the privilege record, + which by then lives under the new provider id. + + Writes are idempotent: a record already carrying the new licensee id is left alone, so a replay of a + retried migration is a no-op. + + :param compact: The compact name + :param transaction_ids: The transaction ids to re-point + :param new_licensee_id: The provider id the transactions now belong to + :return: The number of transaction records updated + """ + updated_count = 0 + for transaction_id in sorted(transaction_ids): + response = self.config.transaction_history_table.query( + IndexName=self.config.transaction_history_transaction_id_gsi_name, + KeyConditionExpression=Key('transactionId').eq(transaction_id) & Key('compact').eq(compact), + ) + settled_records = [ + item for item in response.get('Items', []) if item.get('type') == SETTLED_TRANSACTION_RECORD_TYPE + ] + if not settled_records: + logger.warning( + 'No settled transaction record found for transaction id; skipping licensee id update', + compact=compact, + transaction_id=transaction_id, + ) + continue + + for record in settled_records: + if record.get('licenseeId') == new_licensee_id: + # already updated by an earlier attempt at this migration + continue + self.config.transaction_history_table.update_item( + Key={'pk': record['pk'], 'sk': record['sk']}, + UpdateExpression='SET licenseeId = :licenseeId, dateOfUpdate = :dateOfUpdate', + ExpressionAttributeValues={ + ':licenseeId': new_licensee_id, + ':dateOfUpdate': self.config.current_standard_datetime.isoformat(), + }, + ) + updated_count += 1 + + logger.info( + 'Updated licensee id on transaction records', + compact=compact, + requested_transaction_ids=len(transaction_ids), + updated_records=updated_count, + ) + return updated_count + def store_unsettled_transaction(self, compact: str, transaction_id: str, transaction_date: str) -> None: """ Store an unsettled transaction record in DynamoDB. diff --git a/backend/compact-connect/lambdas/python/common/tests/__init__.py b/backend/compact-connect/lambdas/python/common/tests/__init__.py index 40cdb444cd..26be46b5a4 100644 --- a/backend/compact-connect/lambdas/python/common/tests/__init__.py +++ b/backend/compact-connect/lambdas/python/common/tests/__init__.py @@ -24,6 +24,7 @@ def setUpClass(cls): 'COMPACT_CONFIGURATION_TABLE_NAME': 'compact-configuration-table', 'EMAIL_NOTIFICATION_SERVICE_LAMBDA_NAME': 'email-notification-service', 'TRANSACTION_HISTORY_TABLE_NAME': 'transaction-history-table', + 'TRANSACTION_HISTORY_TRANSACTION_ID_GSI_NAME': 'transactionIdGSI', 'ENVIRONMENT_NAME': 'test', 'PROV_FAM_GIV_MID_INDEX_NAME': 'providerFamGivMid', 'FAM_GIV_INDEX_NAME': 'famGiv', diff --git a/backend/compact-connect/lambdas/python/common/tests/function/__init__.py b/backend/compact-connect/lambdas/python/common/tests/function/__init__.py index 1eb9573971..0e0444216d 100644 --- a/backend/compact-connect/lambdas/python/common/tests/function/__init__.py +++ b/backend/compact-connect/lambdas/python/common/tests/function/__init__.py @@ -208,9 +208,21 @@ def create_transaction_history_table(self): AttributeDefinitions=[ {'AttributeName': 'pk', 'AttributeType': 'S'}, {'AttributeName': 'sk', 'AttributeType': 'S'}, + {'AttributeName': 'transactionId', 'AttributeType': 'S'}, + {'AttributeName': 'compact', 'AttributeType': 'S'}, ], TableName=os.environ['TRANSACTION_HISTORY_TABLE_NAME'], BillingMode='PAY_PER_REQUEST', + GlobalSecondaryIndexes=[ + { + 'IndexName': os.environ['TRANSACTION_HISTORY_TRANSACTION_ID_GSI_NAME'], + 'KeySchema': [ + {'AttributeName': 'transactionId', 'KeyType': 'HASH'}, + {'AttributeName': 'compact', 'KeyType': 'RANGE'}, + ], + 'Projection': {'ProjectionType': 'ALL'}, + }, + ], ) def create_license_preprocessing_queue(self): diff --git a/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py b/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py index c10f721223..a6e34aa75f 100644 --- a/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py +++ b/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py @@ -17,6 +17,9 @@ # aslp compact license type that is not the default 'speech-language pathologist' OTHER_LICENSE_TYPE = 'audiologist' OTHER_LICENSE_TYPE_ABBREVIATION = 'aud' +# payment transaction ids other than the generator default, which the default privilege carries +OTHER_LICENSE_TRANSACTION_ID = '9876543210' +RENEWAL_TRANSACTION_ID = '5555555555' @mock_aws @@ -72,6 +75,25 @@ def _put_full_old_provider_records(self): ) self.test_data_generator.put_default_military_affiliation_in_provider_table() self.test_data_generator.put_default_provider_update_record_in_provider_table() + # the settled transaction the default privilege was purchased with, recorded against the old provider + self._store_transaction(DEFAULT_COMPACT_TRANSACTION_ID, licensee_id=DEFAULT_PROVIDER_ID) + + def _store_transaction(self, transaction_id: str, licensee_id: str): + self.config.transaction_client.store_transactions( + transactions=[ + self.test_data_generator.generate_default_transaction( + {'transactionId': transaction_id, 'licenseeId': licensee_id} + ) + ] + ) + + def _get_transaction_licensee_ids(self) -> dict[str, str]: + """Map every settled transaction in the history table to the provider id it is attributed to.""" + return { + record['transactionId']: record['licenseeId'] + for record in self._transaction_history_table.scan()['Items'] + if record['type'] == 'transaction' + } def test_full_migration_moves_all_records_and_empties_old_partition(self): self._put_full_old_provider_records() @@ -758,3 +780,120 @@ def _fail_on_target_license_delete(**kwargs): self.assertTrue(result.full_migration) self.assertEqual(DEFAULT_REGISTERED_EMAIL_ADDRESS, result.old_provider_registered_email) self.assertEqual([], self._get_all_records_for_provider(DEFAULT_PROVIDER_ID)) + + def test_full_migration_repoints_the_transaction_to_the_new_provider(self): + """The transaction a migrated privilege was purchased with must follow the practitioner, so the + transaction report can still resolve their name after the old provider partition is gone. + """ + self._put_full_old_provider_records() + + result = self._migrate() + + self.assertTrue(result.full_migration) + self.assertEqual({DEFAULT_COMPACT_TRANSACTION_ID: NEW_PROVIDER_ID}, self._get_transaction_licensee_ids()) + + def test_partial_migration_repoints_only_the_corrected_licenses_transactions(self): + """A transaction belonging to a privilege that stays with the old provider must keep pointing at the + old provider id - it is still correct for that purchase. + """ + self._put_full_old_provider_records() + # a second license of another type, whose privilege was purchased with its own transaction + self.test_data_generator.put_default_license_record_in_provider_table({'licenseType': OTHER_LICENSE_TYPE}) + self.test_data_generator.put_default_privilege_record_in_provider_table( + {'licenseType': OTHER_LICENSE_TYPE, 'compactTransactionId': OTHER_LICENSE_TRANSACTION_ID} + ) + self._store_transaction(OTHER_LICENSE_TRANSACTION_ID, licensee_id=DEFAULT_PROVIDER_ID) + + result = self._migrate() + + self.assertFalse(result.full_migration) + self.assertEqual( + { + DEFAULT_COMPACT_TRANSACTION_ID: NEW_PROVIDER_ID, + OTHER_LICENSE_TRANSACTION_ID: DEFAULT_PROVIDER_ID, + }, + self._get_transaction_licensee_ids(), + ) + + def test_repoints_a_transaction_reachable_only_through_privilege_update_history(self): + """A renewed privilege carries only its most recent transaction id; the earlier purchase survives on + the privilege update record, and its transaction must be re-pointed too. + """ + self._put_full_old_provider_records() + # renew the privilege: it now carries a newer transaction id, while the update record's `previous` + # still references the original purchase + self.test_data_generator.put_default_privilege_record_in_provider_table( + {'compactTransactionId': RENEWAL_TRANSACTION_ID} + ) + self._store_transaction(RENEWAL_TRANSACTION_ID, licensee_id=DEFAULT_PROVIDER_ID) + + self._migrate() + + self.assertEqual( + { + DEFAULT_COMPACT_TRANSACTION_ID: NEW_PROVIDER_ID, + RENEWAL_TRANSACTION_ID: NEW_PROVIDER_ID, + }, + self._get_transaction_licensee_ids(), + ) + + def test_transactions_are_repointed_before_the_migration_commits(self): + """The transaction update must happen before the commit that flips the idempotency guard, so a + failure retries the whole migration rather than stranding the transactions. + """ + self._put_full_old_provider_records() + + with patch.object( + self.config.dynamodb_client, + 'transact_write_items', + side_effect=RuntimeError('simulated failure committing the migration'), + ): + with self.assertRaises(Exception): # noqa: B017 the migration wraps the failure + self._migrate() + + # the migration did not commit, but the transaction was already re-pointed + self.assertEqual(1, len(self._get_records_of_type(DEFAULT_PROVIDER_ID, 'license'))) + self.assertEqual({DEFAULT_COMPACT_TRANSACTION_ID: NEW_PROVIDER_ID}, self._get_transaction_licensee_ids()) + + # the replay completes the migration and leaves the transaction correctly attributed + result = self._migrate() + + self.assertTrue(result.migration_performed) + self.assertEqual({DEFAULT_COMPACT_TRANSACTION_ID: NEW_PROVIDER_ID}, self._get_transaction_licensee_ids()) + + def test_migration_with_no_privileges_makes_no_transaction_history_calls(self): + self.test_data_generator.put_default_provider_record_in_provider_table() + self.test_data_generator.put_default_license_record_in_provider_table() + + with patch('cc_common.config._Config.transaction_client') as mock_transaction_client: + result = self._migrate() + + self.assertTrue(result.migration_performed) + mock_transaction_client.update_licensee_id_for_transactions.assert_not_called() + + def test_unsettled_transaction_does_not_fail_the_migration(self): + """A privilege purchased but not yet settled has no transaction record to re-point. That is an + expected state, not an error - the ingest handler alarms on any ERROR log line. + """ + self.test_data_generator.put_default_provider_record_in_provider_table() + self.test_data_generator.put_default_license_record_in_provider_table() + self.test_data_generator.put_default_privilege_record_in_provider_table() + self.config.transaction_client.store_unsettled_transaction( + compact=DEFAULT_COMPACT, + transaction_id=DEFAULT_COMPACT_TRANSACTION_ID, + transaction_date='2024-11-08T20:00:00+00:00', + ) + + with patch('cc_common.data_model.transaction_client.logger') as mock_logger: + result = self._migrate() + + self.assertTrue(result.migration_performed) + self.assertEqual({}, self._get_transaction_licensee_ids()) + mock_logger.warning.assert_called_once() + mock_logger.error.assert_not_called() + # the unsettled record must not have gained a licenseeId + unsettled_records = [ + record for record in self._transaction_history_table.scan()['Items'] if record['type'] != 'transaction' + ] + self.assertEqual(1, len(unsettled_records)) + self.assertNotIn('licenseeId', unsettled_records[0]) diff --git a/backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py b/backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py index e72dd93a6a..0d32375e85 100644 --- a/backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py +++ b/backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py @@ -430,3 +430,169 @@ def test_get_most_recent_transaction_for_compact_raises_when_no_transactions_fou third_call = mock_table.query.call_args_list[2] third_condition_values = third_call.kwargs['KeyConditionExpression']._values # noqa: SLF001 self.assertIn(f'COMPACT#{compact}#TRANSACTIONS#MONTH#2023-12', third_condition_values) + + +STORED_AT = datetime.fromisoformat('2024-11-08T23:59:59+00:00') +UPDATED_AT = datetime.fromisoformat('2024-11-09T12:00:00+00:00') +NEW_LICENSEE_ID = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' + + +@mock_aws +@patch('cc_common.config._Config.current_standard_datetime', STORED_AT) +class TestUpdateLicenseeIdForTransactions(TstFunction): + """Tests for re-pointing a transaction's licenseeId after an SSN-correction migration.""" + + def _store_transaction(self, transaction_id: str, compact: str = 'aslp', licensee_id: str | None = None): + self.config.transaction_client.store_transactions( + transactions=[ + self.test_data_generator.generate_default_transaction( + { + 'transactionId': transaction_id, + 'compact': compact, + **({'licenseeId': licensee_id} if licensee_id else {}), + } + ) + ] + ) + + def _get_all_records(self) -> list[dict]: + return self._transaction_history_table.scan()['Items'] + + def _get_settled_record(self, transaction_id: str) -> dict: + records = [ + record + for record in self._get_all_records() + if record['type'] == 'transaction' and record['transactionId'] == transaction_id + ] + self.assertEqual(1, len(records), f'Expected exactly one settled record for {transaction_id}') + return records[0] + + def _update(self, transaction_ids: set[str], compact: str = 'aslp', new_licensee_id: str = NEW_LICENSEE_ID): + with patch('cc_common.config._Config.current_standard_datetime', UPDATED_AT): + return self.config.transaction_client.update_licensee_id_for_transactions( + compact=compact, + transaction_ids=transaction_ids, + new_licensee_id=new_licensee_id, + ) + + def test_transaction_id_index_returns_stored_transaction(self): + """The transaction id index locates a stored transaction by its id and compact.""" + from boto3.dynamodb.conditions import Key + + self._store_transaction('123') + + response = self.config.transaction_history_table.query( + IndexName=self.config.transaction_history_transaction_id_gsi_name, + KeyConditionExpression=Key('transactionId').eq('123') & Key('compact').eq('aslp'), + ) + + self.assertEqual(1, len(response['Items'])) + self.assertEqual('123', response['Items'][0]['transactionId']) + + def test_rewrites_licensee_id_and_date_of_update(self): + self._store_transaction('123') + + updated_count = self._update({'123'}) + + self.assertEqual(1, updated_count) + record = self._get_settled_record('123') + self.assertEqual(NEW_LICENSEE_ID, record['licenseeId']) + self.assertEqual(UPDATED_AT.isoformat(), record['dateOfUpdate']) + + def test_leaves_every_other_attribute_untouched(self): + self._store_transaction('123') + original_record = self._get_settled_record('123') + + self._update({'123'}) + + expected_record = { + **original_record, + 'licenseeId': NEW_LICENSEE_ID, + 'dateOfUpdate': UPDATED_AT.isoformat(), + } + self.assertEqual(expected_record, self._get_settled_record('123')) + + def test_updates_several_transactions_in_one_call(self): + self._store_transaction('123') + self._store_transaction('456') + self._store_transaction('789') + + updated_count = self._update({'123', '456', '789'}) + + self.assertEqual(3, updated_count) + for transaction_id in ('123', '456', '789'): + self.assertEqual(NEW_LICENSEE_ID, self._get_settled_record(transaction_id)['licenseeId']) + + def test_does_not_touch_a_transaction_in_another_compact(self): + self._store_transaction('123', compact='aslp') + self._store_transaction('123', compact='octp') + octp_licensee_id = [record for record in self._get_all_records() if record['compact'] == 'octp'][0][ + 'licenseeId' + ] + + updated_count = self._update({'123'}, compact='aslp') + + self.assertEqual(1, updated_count) + records_by_compact = {record['compact']: record for record in self._get_all_records()} + self.assertEqual(NEW_LICENSEE_ID, records_by_compact['aslp']['licenseeId']) + self.assertEqual(octp_licensee_id, records_by_compact['octp']['licenseeId']) + + def test_does_not_touch_the_unsettled_record_for_the_same_transaction_id(self): + self._store_transaction('123') + self.config.transaction_client.store_unsettled_transaction( + compact='aslp', transaction_id='123', transaction_date=STORED_AT.isoformat() + ) + unsettled_record = next( + record for record in self._get_all_records() if record['type'] == 'unsettled_transaction' + ) + + updated_count = self._update({'123'}) + + self.assertEqual(1, updated_count) + self.assertEqual(NEW_LICENSEE_ID, self._get_settled_record('123')['licenseeId']) + # the unsettled record has no licenseeId at all, and must not gain one + self.assertEqual( + unsettled_record, + next(record for record in self._get_all_records() if record['type'] == 'unsettled_transaction'), + ) + + def test_unmatched_transaction_id_warns_and_does_not_block_the_others(self): + self._store_transaction('123') + + with patch('cc_common.data_model.transaction_client.logger') as mock_logger: + updated_count = self._update({'123', 'not-settled-yet'}) + + self.assertEqual(1, updated_count) + self.assertEqual(NEW_LICENSEE_ID, self._get_settled_record('123')['licenseeId']) + mock_logger.warning.assert_called_once() + # an unsettled purchase is an expected state, not an error - the ingest handler alarms on ERROR logs + mock_logger.error.assert_not_called() + + def test_second_call_is_a_no_op(self): + self._store_transaction('123') + self._update({'123'}) + + with patch( + 'cc_common.config._Config.current_standard_datetime', datetime.fromisoformat('2024-11-10T12:00:00+00:00') + ): + updated_count = self.config.transaction_client.update_licensee_id_for_transactions( + compact='aslp', + transaction_ids={'123'}, + new_licensee_id=NEW_LICENSEE_ID, + ) + + self.assertEqual(0, updated_count) + # the record was not rewritten, so its dateOfUpdate still reflects the first update + self.assertEqual(UPDATED_AT.isoformat(), self._get_settled_record('123')['dateOfUpdate']) + + def test_empty_transaction_ids_makes_no_calls(self): + with patch('cc_common.config._Config.transaction_history_table') as mock_table: + updated_count = self.config.transaction_client.update_licensee_id_for_transactions( + compact='aslp', + transaction_ids=set(), + new_licensee_id=NEW_LICENSEE_ID, + ) + + self.assertEqual(0, updated_count) + mock_table.query.assert_not_called() + mock_table.update_item.assert_not_called() diff --git a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py index dcf3b91d29..8927500780 100644 --- a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py +++ b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py @@ -1,3 +1,4 @@ +from datetime import date from unittest.mock import MagicMock from botocore.exceptions import ClientError @@ -45,3 +46,68 @@ def test_get_provider_not_found(self): # Verify it raises CCNotFoundException with self.assertRaises(CCNotFoundException): self.client.get_provider(compact='aslp', provider_id='test_id', detail=True, consistent_read=False) + + +class TestCollectTransactionIds(TstLambdas): + """ + Tests for the collection of payment transaction ids from the privilege records an SSN-correction + migration is moving. These are the transactions whose licenseeId has to follow the practitioner. + """ + + def setUp(self): + from cc_common.data_model.data_client import DataClient + + self.collect = DataClient._collect_transaction_ids # noqa: SLF001 protected-access + + def test_collects_the_privilege_records_transaction_id(self): + privilege = self.test_data_generator.generate_default_privilege({'compactTransactionId': 'tx-current'}) + + self.assertEqual({'tx-current'}, self.collect([privilege])) + + def test_privilege_without_a_transaction_id_contributes_nothing(self): + """compactTransactionId is optional on load, so a record read from the table may not carry one.""" + from cc_common.data_model.schema.privilege import PrivilegeData + + database_record = self.test_data_generator.generate_default_privilege().serialize_to_database_record() + database_record.pop('compactTransactionId') + privilege_without_transaction_id = PrivilegeData.from_database_record(database_record) + + self.assertEqual(set(), self.collect([privilege_without_transaction_id])) + + def test_collects_both_transaction_ids_from_an_update_record(self): + privilege_update = self.test_data_generator.generate_default_privilege_update( + value_overrides={'updatedValues': {'compactTransactionId': 'tx-new'}}, + previous_privilege=self.test_data_generator.generate_default_privilege({'compactTransactionId': 'tx-old'}), + ) + + self.assertEqual({'tx-old', 'tx-new'}, self.collect([privilege_update])) + + def test_update_record_without_an_updated_transaction_id_contributes_only_the_previous_one(self): + privilege_update = self.test_data_generator.generate_default_privilege_update( + value_overrides={'updatedValues': {'dateOfExpiration': date.fromisoformat('2030-01-01')}}, + previous_privilege=self.test_data_generator.generate_default_privilege({'compactTransactionId': 'tx-old'}), + ) + + self.assertEqual({'tx-old'}, self.collect([privilege_update])) + + def test_ignores_records_that_are_not_privileges(self): + records = [ + self.test_data_generator.generate_default_license(), + self.test_data_generator.generate_default_license_update(), + self.test_data_generator.generate_default_adverse_action(), + self.test_data_generator.generate_default_investigation(), + self.test_data_generator.generate_default_military_affiliation(), + self.test_data_generator.generate_default_provider(), + self.test_data_generator.generate_default_provider_update(), + ] + + self.assertEqual(set(), self.collect(records)) + + def test_shared_transaction_id_is_collected_once(self): + privilege = self.test_data_generator.generate_default_privilege({'compactTransactionId': 'tx-shared'}) + privilege_update = self.test_data_generator.generate_default_privilege_update( + value_overrides={'updatedValues': {'compactTransactionId': 'tx-shared'}}, + previous_privilege=privilege, + ) + + self.assertEqual({'tx-shared'}, self.collect([privilege, privilege_update])) diff --git a/backend/compact-connect/lambdas/python/provider-data-v1/tests/__init__.py b/backend/compact-connect/lambdas/python/provider-data-v1/tests/__init__.py index 8dac50a09b..51c84dd011 100644 --- a/backend/compact-connect/lambdas/python/provider-data-v1/tests/__init__.py +++ b/backend/compact-connect/lambdas/python/provider-data-v1/tests/__init__.py @@ -21,6 +21,8 @@ def setUpClass(cls): 'RATE_LIMITING_TABLE_NAME': 'rate-limiting-table', 'SSN_TABLE_NAME': 'ssn-table', 'COMPACT_CONFIGURATION_TABLE_NAME': 'compact-configuration-table', + 'TRANSACTION_HISTORY_TABLE_NAME': 'transaction-history-table', + 'TRANSACTION_HISTORY_TRANSACTION_ID_GSI_NAME': 'transactionIdGSI', 'ENVIRONMENT_NAME': 'test', 'PROV_FAM_GIV_MID_INDEX_NAME': 'providerFamGivMid', 'FAM_GIV_INDEX_NAME': 'famGiv', diff --git a/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/__init__.py b/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/__init__.py index 46e87c7bbe..ae9a3c7764 100644 --- a/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/__init__.py +++ b/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/__init__.py @@ -55,6 +55,7 @@ def build_resources(self): self.create_ssn_table() self.create_rate_limiting_table() self.create_compact_configuration_table() + self.create_transaction_history_table() self.create_license_preprocessing_queue() self.create_staff_user_pool() @@ -241,6 +242,29 @@ def create_license_preprocessing_queue(self): self._license_preprocessing_queue = boto3.resource('sqs').create_queue(QueueName='workflow-queue') os.environ['LICENSE_PREPROCESSING_QUEUE_URL'] = self._license_preprocessing_queue.url + def create_transaction_history_table(self): + self._transaction_history_table = boto3.resource('dynamodb').create_table( + KeySchema=[{'AttributeName': 'pk', 'KeyType': 'HASH'}, {'AttributeName': 'sk', 'KeyType': 'RANGE'}], + AttributeDefinitions=[ + {'AttributeName': 'pk', 'AttributeType': 'S'}, + {'AttributeName': 'sk', 'AttributeType': 'S'}, + {'AttributeName': 'transactionId', 'AttributeType': 'S'}, + {'AttributeName': 'compact', 'AttributeType': 'S'}, + ], + TableName=os.environ['TRANSACTION_HISTORY_TABLE_NAME'], + BillingMode='PAY_PER_REQUEST', + GlobalSecondaryIndexes=[ + { + 'IndexName': os.environ['TRANSACTION_HISTORY_TRANSACTION_ID_GSI_NAME'], + 'KeySchema': [ + {'AttributeName': 'transactionId', 'KeyType': 'HASH'}, + {'AttributeName': 'compact', 'KeyType': 'RANGE'}, + ], + 'Projection': {'ProjectionType': 'ALL'}, + }, + ], + ) + def delete_resources(self): self._bucket.objects.delete() self._bucket.delete() @@ -248,6 +272,7 @@ def delete_resources(self): self._staff_users_table.delete() self._ssn_table.delete() self._compact_configuration_table.delete() + self._transaction_history_table.delete() self._rate_limiting_table.delete() self._license_preprocessing_queue.delete() boto3.client('events').delete_event_bus(Name=os.environ['EVENT_BUS_NAME']) diff --git a/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py b/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py index bec638cf33..92f1e5484c 100644 --- a/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py +++ b/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py @@ -4,7 +4,7 @@ from aws_lambda_powertools.metrics import MetricUnit from cc_common.data_model.update_tier_enum import UpdateTierEnum -from common_test.test_constants import DEFAULT_PROVIDER_ID +from common_test.test_constants import DEFAULT_COMPACT_TRANSACTION_ID, DEFAULT_PROVIDER_ID from moto import mock_aws from .. import TstFunction @@ -1012,6 +1012,38 @@ def _get_api_response_snapshot_for_provider(self, provider_id: str) -> dict: ) return json.loads(json.dumps(provider_user_records.generate_api_response_object(), cls=ResponseEncoder)) + def _store_transaction(self, transaction_id: str, licensee_id: str): + self.config.transaction_client.store_transactions( + transactions=[ + self.test_data_generator.generate_default_transaction( + {'transactionId': transaction_id, 'licenseeId': licensee_id} + ) + ] + ) + + def _get_transaction_licensee_ids(self) -> dict[str, str]: + return { + record['transactionId']: record['licenseeId'] + for record in self._transaction_history_table.scan()['Items'] + if record['type'] == 'transaction' + } + + def test_migration_repoints_transaction_records_at_the_new_provider_id(self): + """The practitioner's settled transactions must follow them to the corrected provider id, so the + transaction report can still resolve their name once the old provider partition is gone. + """ + self._put_old_provider_records() + # the transaction the default privilege was purchased with, recorded against the old provider id + self._store_transaction(DEFAULT_COMPACT_TRANSACTION_ID, licensee_id=self.OLD_PROVIDER_ID) + + resp = self._run_ingest_with_previous_provider_id() + + self.assertEqual({'batchItemFailures': []}, resp) + self.assertEqual( + {DEFAULT_COMPACT_TRANSACTION_ID: self.NEW_PROVIDER_ID}, + self._get_transaction_licensee_ids(), + ) + def test_full_migration_moves_records_under_new_provider_id(self): old_provider_record_items = self._put_old_provider_records() self._create_old_cognito_user() diff --git a/backend/compact-connect/stacks/ingest_stack.py b/backend/compact-connect/stacks/ingest_stack.py index 854e17abb5..8d233981f6 100644 --- a/backend/compact-connect/stacks/ingest_stack.py +++ b/backend/compact-connect/stacks/ingest_stack.py @@ -57,6 +57,10 @@ def _add_v1_ingest_chain( 'EMAIL_NOTIFICATION_SERVICE_LAMBDA_NAME': ( persistent_stack.email_notification_service_lambda.function_name ), + 'TRANSACTION_HISTORY_TABLE_NAME': persistent_stack.transaction_history_table.table_name, + 'TRANSACTION_HISTORY_TRANSACTION_ID_GSI_NAME': ( + persistent_stack.transaction_history_table.transaction_id_gsi_name + ), **self.common_env_vars, }, alarm_topic=persistent_stack.alarm_topic, @@ -72,6 +76,9 @@ def _add_v1_ingest_chain( persistent_stack.provider_users_bucket.grant_read_write(ingest_handler) persistent_stack.provider_users_bucket.grant_delete(ingest_handler) persistent_stack.email_notification_service_lambda.grant_invoke(ingest_handler) + # The SSN-correction migration also re-points the licenseeId of the practitioner's payment + # transactions at their new provider id, so the transaction report can still resolve their name + persistent_stack.transaction_history_table.grant_read_write_data(ingest_handler) NagSuppressions.add_resource_suppressions_by_path( Stack.of(ingest_handler.role), @@ -81,8 +88,8 @@ def _add_v1_ingest_chain( 'id': 'AwsSolutions-IAM5', 'reason': """ This policy contains wild-carded actions and resources but they are scoped to the - specific actions, KMS key, Table, user pool, bucket, and lambda that this handler - specifically needs access to. + specific actions, KMS key, Tables (including the transaction history table's indexes), + user pool, bucket, and lambda that this handler specifically needs access to. """, }, ], diff --git a/backend/compact-connect/stacks/persistent_stack/transaction_history_table.py b/backend/compact-connect/stacks/persistent_stack/transaction_history_table.py index e80f5bc6b1..42c6cfe0ab 100644 --- a/backend/compact-connect/stacks/persistent_stack/transaction_history_table.py +++ b/backend/compact-connect/stacks/persistent_stack/transaction_history_table.py @@ -5,6 +5,7 @@ AttributeType, BillingMode, PointInTimeRecoverySpecification, + ProjectionType, Table, TableEncryption, ) @@ -44,6 +45,21 @@ def __init__( **kwargs, ) + self.transaction_id_gsi_name = 'transactionIdGSI' + + # Looks a transaction up by the id the payment processor assigned it, which the base table's + # month / settlement-time keying cannot do. The SSN-correction migration uses this to re-point a + # transaction's licenseeId at the practitioner's new provider id. The full projection leaves the + # index usable as the general by-id entry point to this table, since transaction records are not + # searchable anywhere else in the system. The compact sort key keeps an id from ever resolving + # across compacts. + self.add_global_secondary_index( + index_name=self.transaction_id_gsi_name, + partition_key=Attribute(name='transactionId', type=AttributeType.STRING), + sort_key=Attribute(name='compact', type=AttributeType.STRING), + projection_type=ProjectionType.ALL, + ) + # Set up backup plan backup_enabled = environment_context['backup_enabled'] if backup_enabled and backup_infrastructure_stack is not None: From 01e64cec28db9ffcf79ea1f8d4da7177b9e24214 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Wed, 19 Aug 2026 10:36:17 -0500 Subject: [PATCH 02/27] Add logs/simplify update logic --- .../cc_common/data_model/data_client.py | 15 +++++++++ .../data_model/transaction_client.py | 33 +++++++++++++------ .../test_transaction_client.py | 14 ++------ .../unit/test_data_model/test_data_client.py | 23 ++++++++++--- 4 files changed, 60 insertions(+), 25 deletions(-) diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py index 9ecfb86672..cb95d0cd92 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py @@ -3233,6 +3233,8 @@ def migrate_provider_for_ssn_correction( transaction_ids=payment_transaction_ids, new_licensee_id=new_provider_id, ) + else: + logger.info('No payment transactions are associated with the migrated records') all_transaction_items = [ *create_transaction_items, @@ -3286,6 +3288,19 @@ def _collect_transaction_ids(records: list[CCDataClass]) -> set[str]: transaction_id = record.to_dict().get('compactTransactionId') if transaction_id: transaction_ids.add(transaction_id) + else: + # Every privilege is written with the id of the transaction it was purchased with, so a + # privilege without one is a data defect. Its transaction cannot be re-pointed here, and + # will keep reporting the practitioner under the old provider id until it is corrected + # by hand, so this is surfaced as an error for someone to act on. + logger.error( + 'Migrated privilege record has no compactTransactionId; its transaction cannot be ' + 're-pointed to the new provider id', + compact=record.compact, + provider_id=record.providerId, + jurisdiction=record.jurisdiction, + license_type=record.licenseType, + ) elif record.type == ProviderRecordType.PRIVILEGE_UPDATE: transaction_ids.update( transaction_id diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py index ba74fb56a0..8030ef16e5 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/transaction_client.py @@ -1,6 +1,6 @@ from datetime import UTC, datetime, timedelta -from boto3.dynamodb.conditions import Key +from boto3.dynamodb.conditions import Attr, Key from cc_common.config import _Config, logger from cc_common.data_model.schema.transaction import TransactionData @@ -312,10 +312,11 @@ def update_licensee_id_for_transactions( response = self.config.transaction_history_table.query( IndexName=self.config.transaction_history_transaction_id_gsi_name, KeyConditionExpression=Key('transactionId').eq(transaction_id) & Key('compact').eq(compact), + # the index is not sparse: the unsettled record written at purchase time carries the same + # transaction id and compact, and has no licenseeId to update + FilterExpression=Attr('type').eq(SETTLED_TRANSACTION_RECORD_TYPE), ) - settled_records = [ - item for item in response.get('Items', []) if item.get('type') == SETTLED_TRANSACTION_RECORD_TYPE - ] + settled_records = response.get('Items', []) if not settled_records: logger.warning( 'No settled transaction record found for transaction id; skipping licensee id update', @@ -326,15 +327,27 @@ def update_licensee_id_for_transactions( for record in settled_records: if record.get('licenseeId') == new_licensee_id: - # already updated by an earlier attempt at this migration + logger.info( + 'Transaction is already attributed to the new provider id; skipping update', + compact=compact, + transaction_id=transaction_id, + licensee_id=new_licensee_id, + ) continue + logger.info( + 'Re-pointing transaction to the new provider id', + compact=compact, + transaction_id=transaction_id, + previous_licensee_id=record.get('licenseeId'), + new_licensee_id=new_licensee_id, + ) + # Only the licenseeId is written. The transaction itself did not change, only which provider + # record it is attributed to, so dateOfUpdate, which tracks when the settlement data was written, + # is left as it was. self.config.transaction_history_table.update_item( Key={'pk': record['pk'], 'sk': record['sk']}, - UpdateExpression='SET licenseeId = :licenseeId, dateOfUpdate = :dateOfUpdate', - ExpressionAttributeValues={ - ':licenseeId': new_licensee_id, - ':dateOfUpdate': self.config.current_standard_datetime.isoformat(), - }, + UpdateExpression='SET licenseeId = :licenseeId', + ExpressionAttributeValues={':licenseeId': new_licensee_id}, ) updated_count += 1 diff --git a/backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py b/backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py index 0d32375e85..14645af5c2 100644 --- a/backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py +++ b/backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py @@ -489,15 +489,13 @@ def test_transaction_id_index_returns_stored_transaction(self): self.assertEqual(1, len(response['Items'])) self.assertEqual('123', response['Items'][0]['transactionId']) - def test_rewrites_licensee_id_and_date_of_update(self): + def test_rewrites_licensee_id(self): self._store_transaction('123') updated_count = self._update({'123'}) self.assertEqual(1, updated_count) - record = self._get_settled_record('123') - self.assertEqual(NEW_LICENSEE_ID, record['licenseeId']) - self.assertEqual(UPDATED_AT.isoformat(), record['dateOfUpdate']) + self.assertEqual(NEW_LICENSEE_ID, self._get_settled_record('123')['licenseeId']) def test_leaves_every_other_attribute_untouched(self): self._store_transaction('123') @@ -505,11 +503,7 @@ def test_leaves_every_other_attribute_untouched(self): self._update({'123'}) - expected_record = { - **original_record, - 'licenseeId': NEW_LICENSEE_ID, - 'dateOfUpdate': UPDATED_AT.isoformat(), - } + expected_record = {**original_record, 'licenseeId': NEW_LICENSEE_ID} self.assertEqual(expected_record, self._get_settled_record('123')) def test_updates_several_transactions_in_one_call(self): @@ -582,8 +576,6 @@ def test_second_call_is_a_no_op(self): ) self.assertEqual(0, updated_count) - # the record was not rewritten, so its dateOfUpdate still reflects the first update - self.assertEqual(UPDATED_AT.isoformat(), self._get_settled_record('123')['dateOfUpdate']) def test_empty_transaction_ids_makes_no_calls(self): with patch('cc_common.config._Config.transaction_history_table') as mock_table: diff --git a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py index 8927500780..44328adeee 100644 --- a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py +++ b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py @@ -1,5 +1,5 @@ from datetime import date -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch from botocore.exceptions import ClientError from cc_common.exceptions import CCNotFoundException @@ -64,15 +64,30 @@ def test_collects_the_privilege_records_transaction_id(self): self.assertEqual({'tx-current'}, self.collect([privilege])) - def test_privilege_without_a_transaction_id_contributes_nothing(self): - """compactTransactionId is optional on load, so a record read from the table may not carry one.""" + def test_privilege_without_a_transaction_id_does_not_raise_exception_and_logs_an_error(self): + """compactTransactionId is optional on load, so a record read from the table may not carry one. + Its transaction cannot be re-pointed, which leaves the practitioner reporting as UNKNOWN until + someone corrects it by hand - so it is surfaced as an error rather than passed over quietly. + """ from cc_common.data_model.schema.privilege import PrivilegeData database_record = self.test_data_generator.generate_default_privilege().serialize_to_database_record() database_record.pop('compactTransactionId') privilege_without_transaction_id = PrivilegeData.from_database_record(database_record) - self.assertEqual(set(), self.collect([privilege_without_transaction_id])) + with patch('cc_common.data_model.data_client.logger') as mock_logger: + collected = self.collect([privilege_without_transaction_id]) + + self.assertEqual(set(), collected) + # the message states what is wrong, and the context identifies which privilege needs correcting + mock_logger.error.assert_called_once_with( + 'Migrated privilege record has no compactTransactionId; its transaction cannot be ' + 're-pointed to the new provider id', + compact=privilege_without_transaction_id.compact, + provider_id=privilege_without_transaction_id.providerId, + jurisdiction=privilege_without_transaction_id.jurisdiction, + license_type=privilege_without_transaction_id.licenseType, + ) def test_collects_both_transaction_ids_from_an_update_record(self): privilege_update = self.test_data_generator.generate_default_privilege_update( From 96ef15033dfab43bc55d4d7f5ecfa6a67f060ac9 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Wed, 19 Aug 2026 11:00:03 -0500 Subject: [PATCH 03/27] simplify logic to collect transaction ids from previous snapshots --- .../cc_common/data_model/data_client.py | 21 ++++++------------- .../unit/test_data_model/test_data_client.py | 14 ++++--------- 2 files changed, 10 insertions(+), 25 deletions(-) diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py index cb95d0cd92..f7b4420440 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py @@ -3276,9 +3276,9 @@ def _collect_transaction_ids(records: list[CCDataClass]) -> set[str]: Collect every payment transaction id referenced by the privilege records being migrated. A privilege record carries the transaction id of its most recent purchase or renewal; the earlier - ones survive only on the privilege update history records, in `previous` (and, for a renewal, in - `updatedValues`). Taking the union of all three covers the practitioner's full purchase history for - the privileges that are moving. + ones survive only on the privilege update history records, each of which snapshots the privilege as + it was before that update. The current record plus those snapshots cover the practitioner's full + purchase history for the privileges that are moving. """ transaction_ids = set() for record in records: @@ -3289,10 +3289,6 @@ def _collect_transaction_ids(records: list[CCDataClass]) -> set[str]: if transaction_id: transaction_ids.add(transaction_id) else: - # Every privilege is written with the id of the transaction it was purchased with, so a - # privilege without one is a data defect. Its transaction cannot be re-pointed here, and - # will keep reporting the practitioner under the old provider id until it is corrected - # by hand, so this is surfaced as an error for someone to act on. logger.error( 'Migrated privilege record has no compactTransactionId; its transaction cannot be ' 're-pointed to the new provider id', @@ -3302,14 +3298,9 @@ def _collect_transaction_ids(records: list[CCDataClass]) -> set[str]: license_type=record.licenseType, ) elif record.type == ProviderRecordType.PRIVILEGE_UPDATE: - transaction_ids.update( - transaction_id - for transaction_id in ( - record.previous.get('compactTransactionId'), - record.updatedValues.get('compactTransactionId'), - ) - if transaction_id - ) + # Using the `previous` snapshot we can collect all transaction ids from previous renewals and + # walk back to the transaction id of the original purchase. + transaction_ids.add(record.previous['compactTransactionId']) return transaction_ids @staticmethod diff --git a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py index 44328adeee..984d006998 100644 --- a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py +++ b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py @@ -1,4 +1,3 @@ -from datetime import date from unittest.mock import MagicMock, patch from botocore.exceptions import ClientError @@ -89,20 +88,15 @@ def test_privilege_without_a_transaction_id_does_not_raise_exception_and_logs_an license_type=privilege_without_transaction_id.licenseType, ) - def test_collects_both_transaction_ids_from_an_update_record(self): + def test_collects_the_previous_transaction_id_from_an_update_record(self): + """Only the `previous` snapshot is read. A renewal's own transaction id is written to the privilege + record in the same db transaction, so it is collected from there rather than from `updatedValues`. + """ privilege_update = self.test_data_generator.generate_default_privilege_update( value_overrides={'updatedValues': {'compactTransactionId': 'tx-new'}}, previous_privilege=self.test_data_generator.generate_default_privilege({'compactTransactionId': 'tx-old'}), ) - self.assertEqual({'tx-old', 'tx-new'}, self.collect([privilege_update])) - - def test_update_record_without_an_updated_transaction_id_contributes_only_the_previous_one(self): - privilege_update = self.test_data_generator.generate_default_privilege_update( - value_overrides={'updatedValues': {'dateOfExpiration': date.fromisoformat('2030-01-01')}}, - previous_privilege=self.test_data_generator.generate_default_privilege({'compactTransactionId': 'tx-old'}), - ) - self.assertEqual({'tx-old'}, self.collect([privilege_update])) def test_ignores_records_that_are_not_privileges(self): From 26757b828674e24f2ab9cd100caf6ce2c146987d Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Wed, 19 Aug 2026 11:35:01 -0500 Subject: [PATCH 04/27] refactor to extract common test logic into test data generator --- .../common/common_test/test_data_generator.py | 15 +++++++++++++++ .../test_data_client_ssn_correction.py | 19 ++++++++----------- .../test_transaction_client.py | 14 +++----------- .../function/test_handlers/test_ingest.py | 13 +++---------- .../python/purchases/tests/__init__.py | 1 + .../purchases/tests/function/__init__.py | 12 ++++++++++++ 6 files changed, 42 insertions(+), 32 deletions(-) diff --git a/backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py b/backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py index 55fdf3d3d9..2f87a7f31d 100644 --- a/backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py +++ b/backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py @@ -774,6 +774,21 @@ def generate_default_transaction(value_overrides: dict | None = None): return TransactionData.create_new(default_transaction) + @staticmethod + def put_default_transaction_in_transaction_history_table(value_overrides: dict | None = None): + """ + Creates a default settled transaction record and stores it in the transaction history table. + + :param value_overrides: Optional dictionary to override default values + :return: The TransactionData instance that was stored + """ + from cc_common.config import config + + transaction = TestDataGenerator.generate_default_transaction(value_overrides) + config.transaction_history_table.put_item(Item=transaction.serialize_to_database_record()) + + return transaction + @staticmethod def put_compact_active_member_jurisdictions( compact: str = DEFAULT_COMPACT, postal_abbreviations: list[str] = None diff --git a/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py b/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py index a6e34aa75f..513b257f5e 100644 --- a/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py +++ b/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py @@ -76,15 +76,8 @@ def _put_full_old_provider_records(self): self.test_data_generator.put_default_military_affiliation_in_provider_table() self.test_data_generator.put_default_provider_update_record_in_provider_table() # the settled transaction the default privilege was purchased with, recorded against the old provider - self._store_transaction(DEFAULT_COMPACT_TRANSACTION_ID, licensee_id=DEFAULT_PROVIDER_ID) - - def _store_transaction(self, transaction_id: str, licensee_id: str): - self.config.transaction_client.store_transactions( - transactions=[ - self.test_data_generator.generate_default_transaction( - {'transactionId': transaction_id, 'licenseeId': licensee_id} - ) - ] + self.test_data_generator.put_default_transaction_in_transaction_history_table( + {'transactionId': DEFAULT_COMPACT_TRANSACTION_ID, 'licenseeId': DEFAULT_PROVIDER_ID} ) def _get_transaction_licensee_ids(self) -> dict[str, str]: @@ -802,7 +795,9 @@ def test_partial_migration_repoints_only_the_corrected_licenses_transactions(sel self.test_data_generator.put_default_privilege_record_in_provider_table( {'licenseType': OTHER_LICENSE_TYPE, 'compactTransactionId': OTHER_LICENSE_TRANSACTION_ID} ) - self._store_transaction(OTHER_LICENSE_TRANSACTION_ID, licensee_id=DEFAULT_PROVIDER_ID) + self.test_data_generator.put_default_transaction_in_transaction_history_table( + {'transactionId': OTHER_LICENSE_TRANSACTION_ID, 'licenseeId': DEFAULT_PROVIDER_ID} + ) result = self._migrate() @@ -825,7 +820,9 @@ def test_repoints_a_transaction_reachable_only_through_privilege_update_history( self.test_data_generator.put_default_privilege_record_in_provider_table( {'compactTransactionId': RENEWAL_TRANSACTION_ID} ) - self._store_transaction(RENEWAL_TRANSACTION_ID, licensee_id=DEFAULT_PROVIDER_ID) + self.test_data_generator.put_default_transaction_in_transaction_history_table( + {'transactionId': RENEWAL_TRANSACTION_ID, 'licenseeId': DEFAULT_PROVIDER_ID} + ) self._migrate() diff --git a/backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py b/backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py index 14645af5c2..9a667752b3 100644 --- a/backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py +++ b/backend/compact-connect/lambdas/python/common/tests/function/test_data_model/test_transaction_client.py @@ -442,17 +442,9 @@ def test_get_most_recent_transaction_for_compact_raises_when_no_transactions_fou class TestUpdateLicenseeIdForTransactions(TstFunction): """Tests for re-pointing a transaction's licenseeId after an SSN-correction migration.""" - def _store_transaction(self, transaction_id: str, compact: str = 'aslp', licensee_id: str | None = None): - self.config.transaction_client.store_transactions( - transactions=[ - self.test_data_generator.generate_default_transaction( - { - 'transactionId': transaction_id, - 'compact': compact, - **({'licenseeId': licensee_id} if licensee_id else {}), - } - ) - ] + def _store_transaction(self, transaction_id: str, compact: str = 'aslp'): + return self.test_data_generator.put_default_transaction_in_transaction_history_table( + {'transactionId': transaction_id, 'compact': compact} ) def _get_all_records(self) -> list[dict]: diff --git a/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py b/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py index 92f1e5484c..690b2d0435 100644 --- a/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py +++ b/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py @@ -1012,15 +1012,6 @@ def _get_api_response_snapshot_for_provider(self, provider_id: str) -> dict: ) return json.loads(json.dumps(provider_user_records.generate_api_response_object(), cls=ResponseEncoder)) - def _store_transaction(self, transaction_id: str, licensee_id: str): - self.config.transaction_client.store_transactions( - transactions=[ - self.test_data_generator.generate_default_transaction( - {'transactionId': transaction_id, 'licenseeId': licensee_id} - ) - ] - ) - def _get_transaction_licensee_ids(self) -> dict[str, str]: return { record['transactionId']: record['licenseeId'] @@ -1034,7 +1025,9 @@ def test_migration_repoints_transaction_records_at_the_new_provider_id(self): """ self._put_old_provider_records() # the transaction the default privilege was purchased with, recorded against the old provider id - self._store_transaction(DEFAULT_COMPACT_TRANSACTION_ID, licensee_id=self.OLD_PROVIDER_ID) + self.test_data_generator.put_default_transaction_in_transaction_history_table( + {'transactionId': DEFAULT_COMPACT_TRANSACTION_ID, 'licenseeId': self.OLD_PROVIDER_ID} + ) resp = self._run_ingest_with_previous_provider_id() diff --git a/backend/compact-connect/lambdas/python/purchases/tests/__init__.py b/backend/compact-connect/lambdas/python/purchases/tests/__init__.py index edfeff132c..d4e47fbf7d 100644 --- a/backend/compact-connect/lambdas/python/purchases/tests/__init__.py +++ b/backend/compact-connect/lambdas/python/purchases/tests/__init__.py @@ -17,6 +17,7 @@ def setUpClass(cls): 'AWS_DEFAULT_REGION': 'us-east-1', 'COMPACT_CONFIGURATION_TABLE_NAME': 'compact-configuration-table', 'TRANSACTION_HISTORY_TABLE_NAME': 'transaction-history-table', + 'TRANSACTION_HISTORY_TRANSACTION_ID_GSI_NAME': 'transactionIdGSI', 'TRANSACTION_REPORTS_BUCKET_NAME': 'transaction-report-bucket', 'EMAIL_NOTIFICATION_SERVICE_LAMBDA_NAME': 'email-notification-service', 'COMPACTS': '["aslp", "octp", "coun"]', diff --git a/backend/compact-connect/lambdas/python/purchases/tests/function/__init__.py b/backend/compact-connect/lambdas/python/purchases/tests/function/__init__.py index 3d5527fb68..1603d11df8 100644 --- a/backend/compact-connect/lambdas/python/purchases/tests/function/__init__.py +++ b/backend/compact-connect/lambdas/python/purchases/tests/function/__init__.py @@ -57,10 +57,22 @@ def create_transaction_history_table(self): AttributeDefinitions=[ {'AttributeName': 'pk', 'AttributeType': 'S'}, {'AttributeName': 'sk', 'AttributeType': 'S'}, + {'AttributeName': 'transactionId', 'AttributeType': 'S'}, + {'AttributeName': 'compact', 'AttributeType': 'S'}, ], TableName=os.environ['TRANSACTION_HISTORY_TABLE_NAME'], KeySchema=[{'AttributeName': 'pk', 'KeyType': 'HASH'}, {'AttributeName': 'sk', 'KeyType': 'RANGE'}], BillingMode='PAY_PER_REQUEST', + GlobalSecondaryIndexes=[ + { + 'IndexName': os.environ['TRANSACTION_HISTORY_TRANSACTION_ID_GSI_NAME'], + 'KeySchema': [ + {'AttributeName': 'transactionId', 'KeyType': 'HASH'}, + {'AttributeName': 'compact', 'KeyType': 'RANGE'}, + ], + 'Projection': {'ProjectionType': 'ALL'}, + }, + ], ) def create_provider_table(self): From d4cfb08f2aa1d569c9937b6f407fc5faf3e5aae3 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Wed, 19 Aug 2026 11:56:20 -0500 Subject: [PATCH 05/27] add comment about migration failure and retry case --- .../python/common/cc_common/data_model/data_client.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py index f7b4420440..3bf9620c89 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py @@ -3226,6 +3226,11 @@ def migrate_provider_for_ssn_correction( # flips, so a failure before it retries the whole migration (and these writes are idempotent), while # anything left until after it would never be retried at all. This runs on partial migrations too - # the privileges purchased against the corrected license move in both cases. + # + # The ordering accepts a transient inconsistency: if the commit below fails, these transactions + # already point at new_provider_id while the provider records are still under previous_provider_id, + # so a report generated in that window renders the practitioner as UNKNOWN - briefly, the same + # symptom this re-pointing exists to remove. The SQS retry closes it by completing the migration. payment_transaction_ids = self._collect_transaction_ids(records_to_move) if payment_transaction_ids: self.config.transaction_client.update_licensee_id_for_transactions( From e9e8ac209acc2a2da3bc8009b1b551d5f312bb92 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Wed, 19 Aug 2026 13:15:21 -0500 Subject: [PATCH 06/27] Add transaction licensee id check for SSN correction smoke tests --- backend/compact-connect/tests/smoke/config.py | 4 + .../tests/smoke/smoke_tests_env_example.json | 1 + .../tests/smoke/ssn_migration_smoke_tests.py | 104 +++++++++++++++++- 3 files changed, 106 insertions(+), 3 deletions(-) diff --git a/backend/compact-connect/tests/smoke/config.py b/backend/compact-connect/tests/smoke/config.py index 28f319b286..39fce0fc30 100644 --- a/backend/compact-connect/tests/smoke/config.py +++ b/backend/compact-connect/tests/smoke/config.py @@ -48,6 +48,10 @@ def provider_user_dynamodb_table(self): def data_events_dynamodb_table(self): return boto3.resource('dynamodb').Table(os.environ['CC_TEST_DATA_EVENT_DYNAMO_TABLE_NAME']) + @property + def transaction_history_dynamodb_table(self): + return boto3.resource('dynamodb').Table(os.environ['CC_TEST_TRANSACTION_HISTORY_DYNAMO_TABLE_NAME']) + @property def staff_users_dynamodb_table(self): return boto3.resource('dynamodb').Table(os.environ['CC_TEST_STAFF_USER_DYNAMO_TABLE_NAME']) diff --git a/backend/compact-connect/tests/smoke/smoke_tests_env_example.json b/backend/compact-connect/tests/smoke/smoke_tests_env_example.json index f7b6cec2ad..17a967e368 100644 --- a/backend/compact-connect/tests/smoke/smoke_tests_env_example.json +++ b/backend/compact-connect/tests/smoke/smoke_tests_env_example.json @@ -9,6 +9,7 @@ "CC_TEST_GET_PROVIDER_SSN_LAMBDA_NAME": "Sandbox-APIStack-LicenseApiv1compactscompactprovid-1234", "CC_TEST_SSN_DYNAMO_TABLE_NAME": "Sandbox-PersistentStack-SSNTable12345", "CC_TEST_DATA_EVENT_DYNAMO_TABLE_NAME": "Sandbox-PersistentStack-DataEventTable1234", + "CC_TEST_TRANSACTION_HISTORY_DYNAMO_TABLE_NAME": "Sandbox-PersistentStack-TransactionHistoryTable1234", "CC_TEST_STAFF_USER_DYNAMO_TABLE_NAME": "Sandbox-PersistentStack-StaffUserTable1234", "CC_TEST_COGNITO_STAFF_USER_POOL_ID": "us-east-1_12345", "CC_TEST_COGNITO_STAFF_USER_POOL_CLIENT_ID": "72612345", diff --git a/backend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py b/backend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py index d2e35a5139..591a53cb4a 100644 --- a/backend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py +++ b/backend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py @@ -14,6 +14,8 @@ - A registered test provider user (CC_TEST_PROVIDER_USER_USERNAME) whose license records are stored under the SSN configured in the CC_TEST_PROVIDER_MOCK_SSN env var. - The CC_TEST_PROVIDER_USER_BUCKET_NAME env var set to the environment's provider users S3 bucket. +- The CC_TEST_TRANSACTION_HISTORY_DYNAMO_TABLE_NAME env var set to the environment's transaction history + table, so the transactions the provider's privileges were purchased with can be checked after migration. License uploads are performed against the State API (CC_TEST_STATE_API_BASE_URL) using a Cognito client-credentials app client, the same way state IT systems authenticate in production - see @@ -36,6 +38,7 @@ import boto3 import requests +from boto3.dynamodb.conditions import Attr, Key from botocore.exceptions import ClientError from config import config, logger from military_affiliation_smoke_tests import test_military_affiliation_upload @@ -57,6 +60,9 @@ # If you test provider is in a different compact, change this value TEST_COMPACT = 'coun' +# The transaction history table index that maps a payment transaction id back to its record. Fixed in +# the CDK (see stacks/persistent_stack/transaction_history_table.py), so it is not environment-specific. +TRANSACTION_ID_GSI_NAME = 'transactionIdGSI' # The corrected SSN the test provider is temporarily migrated to during the full migration roundtrip FULL_MIGRATION_CORRECTED_SSN = '999-99-8877' @@ -230,6 +236,87 @@ def _verify_all_records_migrated( print(f'Verified all {len(source_normalized)} migratable records now exist under provider {target_provider_id}') +def _get_privilege_transaction_ids(records: list[dict]) -> set[str]: + """Collect every payment transaction id referenced by a provider's privilege records. + + This mirrors the collection the migration itself performs: each privilege record carries the id of the + transaction it was most recently purchased or renewed with, and the earlier ones survive only on the + privilege update history records, which snapshot the privilege as it was before each update. + """ + transaction_ids = set() + for record in records: + if record['type'] == 'privilege' and record.get('compactTransactionId'): + transaction_ids.add(record['compactTransactionId']) + elif record['type'] == 'privilegeUpdate': + previous_transaction_id = record.get('previous', {}).get('compactTransactionId') + if previous_transaction_id: + transaction_ids.add(previous_transaction_id) + return transaction_ids + + +def _verify_transactions_attributed_to_provider(*, compact: str, provider_id: str, records: list[dict]): + """Verify every transaction the provider's privileges were purchased with is attributed to that provider. + + An SSN correction moves the privileges to a new provider id, and the migration re-points the licenseeId + on the matching transaction records so the weekly transaction report can still resolve the + practitioner's name afterwards. Without that, transactions settled before the correction report the + practitioner as UNKNOWN. + + A transaction id with no settled record in the transaction history table is reported but not treated as + a failure: the purchase may not have settled yet (settlement processing runs on a nightly schedule), and + once it does the settlement workflow resolves the licensee id from the privilege record, which by then + already lives under the new provider id. + """ + transaction_ids = _get_privilege_transaction_ids(records) + if not transaction_ids: + print( + f'Provider {provider_id} has no privileges carrying transaction ids; no transaction attribution to verify' + ) + return + + transaction_table = config.transaction_history_dynamodb_table + misattributed = {} + unsettled_transaction_ids = [] + verified_transaction_ids = [] + for transaction_id in sorted(transaction_ids): + query_response = transaction_table.query( + IndexName=TRANSACTION_ID_GSI_NAME, + KeyConditionExpression=Key('transactionId').eq(transaction_id) & Key('compact').eq(compact), + # the index also covers the unsettled record written at purchase time, which has no licenseeId + FilterExpression=Attr('type').eq('transaction'), + ) + settled_records = query_response.get('Items', []) + if not settled_records: + unsettled_transaction_ids.append(transaction_id) + continue + for settled_record in settled_records: + if settled_record.get('licenseeId') != provider_id: + misattributed[transaction_id] = settled_record.get('licenseeId') + else: + verified_transaction_ids.append(transaction_id) + + if misattributed: + raise SmokeTestFailureException( + f'The following transactions are not attributed to provider {provider_id} after migration ' + f'(transaction id -> licenseeId found): {misattributed}' + ) + + print( + f'Verified {len(verified_transaction_ids)} of {len(transaction_ids)} privilege transaction(s) are ' + f'attributed to provider {provider_id}' + ) + if unsettled_transaction_ids: + print( + f'No settled transaction record found for {unsettled_transaction_ids} - these purchases have ' + f'likely not settled yet in this environment, so there was nothing to re-point' + ) + if not verified_transaction_ids: + print( + "WARNING: none of this provider's privilege transactions had settled records to check, so the " + 'transaction attribution check did not verify anything in this run' + ) + + def _verify_license_ssn_last_four(*, records: list[dict], expected_ssn_last_four: str, license_type: str | None = None): """Verify every license record (optionally filtered to a single license type) carries the expected ssnLastFour. This is checked separately from _verify_all_records_migrated because ssnLastFour is @@ -552,9 +639,11 @@ def test_full_ssn_migration_roundtrip(): Step 1: Capture the test provider's baseline state (all DynamoDB records + all S3 objects). Step 2: Upload a fresh military affiliation document so there is a recent document to migrate. Step 3: Upload the provider's license with a corrected SSN and previousSSN set to their current mock SSN. - Step 4: Wait for the migration, then verify every record and S3 object moved to the new provider id. - Step 5: Migrate back to the original SSN (roundtrip) and verify everything returned to the original - provider id and the intermediate provider id was cleaned up. + Step 4: Wait for the migration, then verify every record and S3 object moved to the new provider id, + and that the transactions the provider's privileges were purchased with are attributed to it. + Step 5: Migrate back to the original SSN (roundtrip) and verify everything - records, documents, and + transaction attribution - returned to the original provider id and the intermediate provider id + was cleaned up. Step 6: Restore the test provider's Cognito account (deleted by the full migration) and registration fields so the shared test account remains usable. """ @@ -640,6 +729,11 @@ def test_full_ssn_migration_roundtrip(): _verify_all_s3_objects_migrated( source_objects=pre_migration_s3_objects, compact=compact, target_provider_id=migrated_provider_id ) + # the transactions the provider's privileges were purchased with must follow them to the new + # provider id, or the transaction report renders the practitioner as UNKNOWN + _verify_transactions_attributed_to_provider( + compact=compact, provider_id=migrated_provider_id, records=migrated_records + ) # Step 5: roundtrip back to the original SSN and verify everything returned home returned_provider_id = _migrate_test_provider_to_ssn( @@ -670,6 +764,10 @@ def test_full_ssn_migration_roundtrip(): _verify_all_s3_objects_migrated( source_objects=pre_migration_s3_objects, compact=compact, target_provider_id=original_provider_id ) + # the same transactions must now be attributed to the original provider id + _verify_transactions_attributed_to_provider( + compact=compact, provider_id=original_provider_id, records=returned_records + ) print('Roundtrip migration completed; all records and documents are back under the original provider id') finally: # Restore the shared test provider account no matter what state the test failed in: point the From 0224424f39c153bb39fb0f96e7ac043b0a341a33 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Wed, 19 Aug 2026 16:10:18 -0500 Subject: [PATCH 07/27] Ensure system owned fields are preserved with every license re-upload --- .../data_model/schema/license/record.py | 21 +++++ .../test_schema/test_license.py | 68 ++++++++++++++ .../provider-data-v1/handlers/ingest.py | 35 ++++--- .../function/test_handlers/test_ingest.py | 92 +++++++++++++++++++ .../data_model/schema/license/record.py | 21 +++++ .../test_schema/test_license.py | 68 ++++++++++++++ .../provider-data-v1/handlers/ingest.py | 27 ++++-- .../function/test_handlers/test_ingest.py | 66 +++++++++++++ .../data_model/schema/license/record.py | 43 +++++++++ .../test_schema/test_license.py | 68 ++++++++++++++ .../provider-data-v1/handlers/ingest.py | 28 ++++-- .../function/test_handlers/test_ingest.py | 66 +++++++++++++ 12 files changed, 576 insertions(+), 27 deletions(-) diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py index d61a0b8000..8ce453e1ce 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py @@ -34,6 +34,27 @@ from cc_common.data_model.schema.license.common import LicenseCommonSchema from cc_common.data_model.update_tier_enum import UpdateTierEnum +# Fields on a license record that CompactConnect owns rather than the uploading state. They are set by +# actions taken within the system (board encumbrances, investigations) or by the ingest process itself, +# and there is no way for a state to express them in a license upload. +# +# A license upload writes the whole record, so the uploading state is authoritative for every field it can +# send - omitting one removes it, which is intentional and recorded in the update record's removedValues. +# These fields are the exception: an upload cannot assert them, so it must not be able to retract them +# either. The ingest handler carries them forward from the existing record when re-uploading a license +# that already exists. +# +# Any future field of this kind must be added here, or a routine re-upload will silently drop it. Note +# that a field must also be declared on LicenseRecordSchema below to survive at all: this schema excludes +# undeclared attributes when a record is loaded. +SYSTEM_OWNED_LICENSE_FIELDS = frozenset( + { + 'encumberedStatus', + 'investigationStatus', + 'firstUploadDate', + } +) + @BaseRecordSchema.register_schema('license') class LicenseRecordSchema(BaseRecordSchema, LicenseCommonSchema): diff --git a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.py b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.py index 1b5705c87e..0e5881dca8 100644 --- a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.py +++ b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.py @@ -496,3 +496,71 @@ def test_license_status_corrected_to_inactive_at_expiration_utc_minus_four(self) result = LicenseGeneralResponseSchema().load(license_data) self.assertEqual('inactive', result['licenseStatus']) + + +class TestLicenseRecordFieldOwnership(TstLambdas): + """ + Guards the contract behind SYSTEM_OWNED_LICENSE_FIELDS. + + A license upload writes the whole record, so any field the upload cannot supply is dropped unless the + ingest handler carries it forward.This test fails if a new field is added to the license record without + deciding which side of that line it falls on. + """ + + # Regenerated from other fields by LicenseRecordSchema's pre_dump hooks on every write, and dropped + # from the loaded record by its post_load hook - so there is never anything to preserve. + GENERATED_ON_WRITE = { + 'pk', + 'sk', + 'licenseGSIPK', + 'licenseGSISK', + 'licenseUploadDateGSIPK', + 'licenseUploadDateGSISK', + } + # Stamped fresh by BaseRecordSchema on every write. + STAMPED_ON_WRITE = {'type', 'dateOfUpdate'} + # Calculated when a record is loaded and stripped again before it is written, so they are never stored. + CALCULATED_ON_LOAD = {'licenseStatus', 'compactEligibility'} + + def test_every_field_the_upload_cannot_supply_is_accounted_for(self): + from cc_common.data_model.schema.license.ingest import LicenseIngestSchema + from cc_common.data_model.schema.license.record import ( + SYSTEM_OWNED_LICENSE_FIELDS, + LicenseRecordSchema, + ) + + not_suppliable_by_upload = set(LicenseRecordSchema().fields) - set(LicenseIngestSchema().fields) + unaccounted_for = ( + not_suppliable_by_upload + - self.GENERATED_ON_WRITE + - self.STAMPED_ON_WRITE + - self.CALCULATED_ON_LOAD + - SYSTEM_OWNED_LICENSE_FIELDS + ) + + self.assertEqual( + set(), + unaccounted_for, + f'New license record field(s) {sorted(unaccounted_for)} cannot be supplied by a license upload, ' + 'so a routine re-upload will silently drop them. Decide which they are and add them to the right ' + 'place: SYSTEM_OWNED_LICENSE_FIELDS (in schema/license/record.py) if the system owns the value ' + 'and it must survive an upload, or one of the GENERATED_ON_WRITE / STAMPED_ON_WRITE / ' + 'CALCULATED_ON_LOAD sets in this test if the field is rebuilt or discarded on every write.', + ) + + def test_no_preserved_field_can_be_supplied_by_an_upload(self): + """A preserved field that a state can also send would be frozen at its first value forever, since + the carry-forward would overwrite whatever the upload provided. + """ + from cc_common.data_model.schema.license.ingest import LicenseIngestSchema + from cc_common.data_model.schema.license.record import SYSTEM_OWNED_LICENSE_FIELDS + + uploadable_but_preserved = SYSTEM_OWNED_LICENSE_FIELDS & set(LicenseIngestSchema().fields) + + self.assertEqual( + set(), + uploadable_but_preserved, + f'{sorted(uploadable_but_preserved)} can be supplied by a license upload, so preserving it would ' + 'pin it at its existing value and prevent a state from ever changing it. Remove it from ' + 'SYSTEM_OWNED_LICENSE_FIELDS.', + ) diff --git a/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py b/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py index 542f093946..3341531f23 100644 --- a/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py +++ b/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py @@ -9,7 +9,7 @@ from cc_common.data_model.schema.common import ActiveInactiveStatus, UpdateCategory from cc_common.data_model.schema.license import LicenseData from cc_common.data_model.schema.license.ingest import LicenseIngestSchema -from cc_common.data_model.schema.license.record import LicenseUpdateRecordSchema +from cc_common.data_model.schema.license.record import SYSTEM_OWNED_LICENSE_FIELDS, LicenseUpdateRecordSchema from cc_common.data_model.schema.provider import ProviderData from cc_common.event_batch_writer import EventBatchWriter from cc_common.exceptions import CCNotFoundException @@ -18,6 +18,16 @@ license_schema = LicenseIngestSchema() license_update_schema = LicenseUpdateRecordSchema() +# Keys on a license record that are not part of the state-supplied license data, so a difference in one is +# not a change to the license itself and does not belong in a licenseUpdate record: +# - dateOfUpdate is stamped on every write. +# - licenseStatus / compactEligibility are calculated when a record is loaded (see +# LicenseRecordSchema._calculate_statuses) and are stripped again before it is written, so they follow +# whatever else changed rather than being a change in their own right. +# - the system-owned fields are copied from the existing record onto the new one before this comparison +# runs, so they cannot differ here. +NON_LICENSE_DATA_KEYS = SYSTEM_OWNED_LICENSE_FIELDS | {'dateOfUpdate', 'licenseStatus', 'compactEligibility'} + # Custom metrics tracking how often states rely on the previousSSN last-resort correction feature, split by # whether the correction fully migrated the practitioner (old provider had no other licenses), only partially # migrated them (other licenses remained on the old provider id), or found nothing to migrate (spurious @@ -199,16 +209,20 @@ def ingest_license_message(message: dict): posted_license_record['licenseType'] ) if existing_license is not None: + # The write below replaces the whole license record, so any field the upload cannot carry + # would be dropped. Carry the system-owned fields (encumbrance / investigation status set + # by actions within CompactConnect, and the firstUploadDate behind the license upload date + # GSI) forward from the existing record. This happens before the update record is built, so + # the recorded history reflects what is actually written. + for field in SYSTEM_OWNED_LICENSE_FIELDS: + if existing_license.get(field) is not None: + posted_license_record[field] = existing_license[field] _process_license_update( existing_license=existing_license, new_license=posted_license_record, dynamo_transactions=dynamo_transactions, data_events=data_events, ) - # now grab the firstUploadDate from the existing record if available and put it in the posted_license - # for the license upload date GSI - if existing_license.get('firstUploadDate'): - posted_license_record['firstUploadDate'] = existing_license.get('firstUploadDate') else: # If this is the first time creating the license record, # set the firstUploadDate to the current time for license upload date GSI tracking @@ -274,18 +288,13 @@ def _process_license_update(*, existing_license: dict, new_license: dict, dynamo :param dict new_license: The newly-uploaded license record :param list dynamo_transactions: The dynamodb transaction array to append records to """ - # Remove fields that are calculated at runtime, not stored in the database - # uploadDate is metadata tracking when the license was first uploaded, not part of the license data - # firstUploadDate is metadata tracking when the license was first uploaded, not part of the license data - dynamic_keys = {'dateOfUpdate', 'status', 'uploadDate', 'firstUploadDate'} updated_values = { key: value for key, value in new_license.items() - if key not in dynamic_keys and (key not in existing_license.keys() or value != existing_license[key]) + if key not in NON_LICENSE_DATA_KEYS and (key not in existing_license.keys() or value != existing_license[key]) } - # If any fields are missing from the new license, we'll consider them removed - # Exclude dynamic keys from removed values since they're metadata, not part of the license data - removed_values = existing_license.keys() - new_license.keys() - dynamic_keys + # Any field the state sent before and left out of this upload is considered removed + removed_values = existing_license.keys() - new_license.keys() - NON_LICENSE_DATA_KEYS if not updated_values and not removed_values: logger.info('No changes detected for this license.') return diff --git a/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py b/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py index 690b2d0435..b40db2c977 100644 --- a/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py +++ b/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py @@ -531,6 +531,98 @@ def test_existing_provider_removed_email(self): self.assertEqual(1, len(provider_user_records._license_update_records)) # noqa SLF001 self.assertEqual(['emailAddress'], provider_user_records._license_update_records[0].removedValues) # noqa SLF001 + def _get_license_record(self, provider_id: str) -> dict: + from boto3.dynamodb.conditions import Key + + records = self.config.provider_table.query(KeyConditionExpression=Key('pk').eq(f'aslp#PROVIDER#{provider_id}'))[ + 'Items' + ] + return next(record for record in records if record['type'] == 'license') + + def _set_license_field(self, provider_id: str, field: str, value: str): + """Set a field on the stored license record that a state cannot send in an upload.""" + license_record = self._get_license_record(provider_id) + self.config.provider_table.update_item( + Key={'pk': license_record['pk'], 'sk': license_record['sk']}, + UpdateExpression='SET #field = :value', + ExpressionAttributeNames={'#field': field}, + ExpressionAttributeValues={':value': value}, + ) + + def test_existing_license_encumbered_status_survives_a_reupload(self): + """A state's upload cannot express an encumbrance, so a routine re-upload must not clear one. + + Dropping it would also flip the license's calculated status back to active and compact-eligible. + """ + from handlers.ingest import ingest_license_message + + provider_id = self._with_ingested_license() + # a board encumbrance applied after the original upload + self._set_license_field(provider_id, 'encumberedStatus', 'encumbered') + + with open('../common/tests/resources/ingest/event-bridge-message.json') as f: + message = json.load(f) + event = {'Records': [{'messageId': '123', 'body': json.dumps(message)}]} + self.assertEqual({'batchItemFailures': []}, ingest_license_message(event, self.mock_context)) + + self.assertEqual('encumbered', self._get_license_record(provider_id).get('encumberedStatus')) + + def test_existing_license_investigation_status_survives_a_reupload(self): + """As with encumbrances, an investigation is a CompactConnect action a state cannot send.""" + from handlers.ingest import ingest_license_message + + provider_id = self._with_ingested_license() + self._set_license_field(provider_id, 'investigationStatus', 'underInvestigation') + + with open('../common/tests/resources/ingest/event-bridge-message.json') as f: + message = json.load(f) + event = {'Records': [{'messageId': '123', 'body': json.dumps(message)}]} + self.assertEqual({'batchItemFailures': []}, ingest_license_message(event, self.mock_context)) + + self.assertEqual('underInvestigation', self._get_license_record(provider_id).get('investigationStatus')) + + def test_preserved_license_fields_are_not_reported_as_removed(self): + """The update history must reflect what actually happened: these fields were carried forward, not + removed, so they must not show up in the update record's removedValues. + """ + from handlers.ingest import ingest_license_message + + provider_id = self._with_ingested_license() + self._set_license_field(provider_id, 'encumberedStatus', 'encumbered') + self._set_license_field(provider_id, 'investigationStatus', 'underInvestigation') + + with open('../common/tests/resources/ingest/event-bridge-message.json') as f: + message = json.load(f) + event = {'Records': [{'messageId': '123', 'body': json.dumps(message)}]} + self.assertEqual({'batchItemFailures': []}, ingest_license_message(event, self.mock_context)) + + provider_user_records = self.config.data_client.get_provider_user_records( + compact='aslp', provider_id=provider_id, include_update_tier=UpdateTierEnum.TIER_THREE + ) + for update_record in provider_user_records._license_update_records: # noqa: SLF001 + self.assertNotIn('encumberedStatus', update_record.to_dict().get('removedValues', [])) + self.assertNotIn('investigationStatus', update_record.to_dict().get('removedValues', [])) + + def test_unchanged_reupload_of_a_preserved_license_creates_no_update_record(self): + """A re-upload that changes nothing must not leave an empty update record in the practitioner's + history. The calculated licenseStatus / compactEligibility differ between an encumbered record and + the freshly-loaded upload, but they are derived values rather than license data. + """ + from handlers.ingest import ingest_license_message + + provider_id = self._with_ingested_license() + self._set_license_field(provider_id, 'encumberedStatus', 'encumbered') + + with open('../common/tests/resources/ingest/event-bridge-message.json') as f: + message = json.load(f) + event = {'Records': [{'messageId': '123', 'body': json.dumps(message)}]} + self.assertEqual({'batchItemFailures': []}, ingest_license_message(event, self.mock_context)) + + provider_user_records = self.config.data_client.get_provider_user_records( + compact='aslp', provider_id=provider_id, include_update_tier=UpdateTierEnum.TIER_THREE + ) + self.assertEqual([], provider_user_records._license_update_records) # noqa: SLF001 + def test_existing_provider_added_email(self): from handlers.ingest import ingest_license_message diff --git a/backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/license/record.py b/backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/license/record.py index 3d2d1f2bfa..c9f1dd62e7 100644 --- a/backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/license/record.py +++ b/backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/license/record.py @@ -33,6 +33,27 @@ from cc_common.data_model.schema.license.common import LicenseCommonSchema from cc_common.data_model.update_tier_enum import UpdateTierEnum +# Fields on a license record that CompactConnect owns rather than the uploading state. They are set by +# actions taken within the system (board encumbrances, investigations) or by the ingest process itself, +# and there is no way for a state to express them in a license upload. +# +# A license upload writes the whole record, so the uploading state is authoritative for every field it can +# send - omitting one removes it, which is intentional and recorded in the update record's removedValues. +# These fields are the exception: an upload cannot assert them, so it must not be able to retract them +# either. The ingest handler carries them forward from the existing record when re-uploading a license +# that already exists. +# +# Any future field of this kind must be added here, or a routine re-upload will silently drop it. Note +# that a field must also be declared on LicenseRecordSchema below to survive at all: this schema excludes +# undeclared attributes when a record is loaded. +SYSTEM_OWNED_LICENSE_FIELDS = frozenset( + { + 'encumberedStatus', + 'investigationStatus', + 'firstUploadDate', + } +) + @BaseRecordSchema.register_schema('license') class LicenseRecordSchema(BaseRecordSchema, LicenseCommonSchema): diff --git a/backend/cosmetology-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.py b/backend/cosmetology-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.py index e9c00ca4ca..29788080d9 100644 --- a/backend/cosmetology-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.py +++ b/backend/cosmetology-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.py @@ -506,3 +506,71 @@ def test_license_status_corrected_to_inactive_at_expiration_utc_minus_four(self) result = LicenseGeneralResponseSchema().load(license_data) self.assertEqual('inactive', result['licenseStatus']) + + +class TestLicenseRecordFieldOwnership(TstLambdas): + """ + Guards the contract behind SYSTEM_OWNED_LICENSE_FIELDS. + + A license upload writes the whole record, so any field the upload cannot supply is dropped unless the + ingest handler carries it forward. This test fails if a new field is added to the license record without + deciding which side of that line it falls on. + """ + + # Regenerated from other fields by LicenseRecordSchema's pre_dump hooks on every write, and dropped + # from the loaded record by its post_load hook - so there is never anything to preserve. + GENERATED_ON_WRITE = { + 'pk', + 'sk', + 'licenseGSIPK', + 'licenseGSISK', + 'licenseUploadDateGSIPK', + 'licenseUploadDateGSISK', + } + # Stamped fresh by BaseRecordSchema on every write. + STAMPED_ON_WRITE = {'type', 'dateOfUpdate'} + # Calculated when a record is loaded and stripped again before it is written, so they are never stored. + CALCULATED_ON_LOAD = {'licenseStatus', 'compactEligibility'} + + def test_every_field_the_upload_cannot_supply_is_accounted_for(self): + from cc_common.data_model.schema.license.ingest import LicenseIngestSchema + from cc_common.data_model.schema.license.record import ( + SYSTEM_OWNED_LICENSE_FIELDS, + LicenseRecordSchema, + ) + + not_suppliable_by_upload = set(LicenseRecordSchema().fields) - set(LicenseIngestSchema().fields) + unaccounted_for = ( + not_suppliable_by_upload + - self.GENERATED_ON_WRITE + - self.STAMPED_ON_WRITE + - self.CALCULATED_ON_LOAD + - SYSTEM_OWNED_LICENSE_FIELDS + ) + + self.assertEqual( + set(), + unaccounted_for, + f'New license record field(s) {sorted(unaccounted_for)} cannot be supplied by a license upload, ' + 'so a routine re-upload will silently drop them. Decide which they are and add them to the right ' + 'place: SYSTEM_OWNED_LICENSE_FIELDS (in schema/license/record.py) if the system owns the value ' + 'and it must survive an upload, or one of the GENERATED_ON_WRITE / STAMPED_ON_WRITE / ' + 'CALCULATED_ON_LOAD sets in this test if the field is rebuilt or discarded on every write.', + ) + + def test_no_preserved_field_can_be_supplied_by_an_upload(self): + """A preserved field that a state can also send would be frozen at its first value forever, since + the carry-forward would overwrite whatever the upload provided. + """ + from cc_common.data_model.schema.license.ingest import LicenseIngestSchema + from cc_common.data_model.schema.license.record import SYSTEM_OWNED_LICENSE_FIELDS + + uploadable_but_preserved = SYSTEM_OWNED_LICENSE_FIELDS & set(LicenseIngestSchema().fields) + + self.assertEqual( + set(), + uploadable_but_preserved, + f'{sorted(uploadable_but_preserved)} can be supplied by a license upload, so preserving it would ' + 'pin it at its existing value and prevent a state from ever changing it. Remove it from ' + 'SYSTEM_OWNED_LICENSE_FIELDS.', + ) diff --git a/backend/cosmetology-app/lambdas/python/provider-data-v1/handlers/ingest.py b/backend/cosmetology-app/lambdas/python/provider-data-v1/handlers/ingest.py index 58e449f4c9..30b3e923b5 100644 --- a/backend/cosmetology-app/lambdas/python/provider-data-v1/handlers/ingest.py +++ b/backend/cosmetology-app/lambdas/python/provider-data-v1/handlers/ingest.py @@ -8,7 +8,7 @@ from cc_common.data_model.schema.common import ActiveInactiveStatus, UpdateCategory from cc_common.data_model.schema.license import LicenseData from cc_common.data_model.schema.license.ingest import LicenseIngestSchema -from cc_common.data_model.schema.license.record import LicenseUpdateRecordSchema +from cc_common.data_model.schema.license.record import SYSTEM_OWNED_LICENSE_FIELDS, LicenseUpdateRecordSchema from cc_common.data_model.schema.provider import ProviderData from cc_common.event_batch_writer import EventBatchWriter from cc_common.exceptions import CCNotFoundException @@ -17,6 +17,16 @@ license_schema = LicenseIngestSchema() license_update_schema = LicenseUpdateRecordSchema() +# Keys on a license record that are not part of the state-supplied license data, so a difference in one is +# not a change to the license itself and does not belong in a licenseUpdate record: +# - dateOfUpdate is stamped on every write. +# - licenseStatus / compactEligibility are calculated when a record is loaded (see +# LicenseRecordSchema._calculate_statuses) and are stripped again before it is written, so they follow +# whatever else changed rather than being a change in their own right. +# - the system-owned fields are copied from the existing record onto the new one before this comparison +# runs, so they cannot differ here. +NON_LICENSE_DATA_KEYS = SYSTEM_OWNED_LICENSE_FIELDS | {'dateOfUpdate', 'licenseStatus', 'compactEligibility'} + @sqs_handler def preprocess_license_ingest(message: dict): @@ -149,16 +159,20 @@ def ingest_license_message(message: dict): posted_license_record['licenseType'] ) if existing_license is not None: + # The write below replaces the whole license record, so any field the upload cannot carry + # would be dropped. Carry the system-owned fields (encumbrance / investigation status set + # by actions within CompactConnect, and the firstUploadDate behind the license upload date + # GSI) forward from the existing record. This happens before the update record is built, so + # the recorded history reflects what is actually written. + for field in SYSTEM_OWNED_LICENSE_FIELDS: + if existing_license.get(field) is not None: + posted_license_record[field] = existing_license[field] _process_license_update( existing_license=existing_license, new_license=posted_license_record, dynamo_transactions=dynamo_transactions, data_events=data_events, ) - # now grab the firstUploadDate from the existing record if available and put it in the posted_license - # for the license upload date GSI - if existing_license.get('firstUploadDate'): - posted_license_record['firstUploadDate'] = existing_license.get('firstUploadDate') else: # If this is the first time creating the license record, # set the firstUploadDate to the current time for license upload date GSI tracking @@ -247,11 +261,10 @@ def _process_license_update(*, existing_license: dict, new_license: dict, dynamo """ # Remove fields that are calculated at runtime, not stored in the database # uploadDate is metadata tracking when the license was first uploaded, not part of the license data - dynamic_keys = {'dateOfUpdate', 'status', 'uploadDate'} updated_values = { key: value for key, value in new_license.items() - if key not in dynamic_keys and (key not in existing_license.keys() or value != existing_license[key]) + if key not in NON_LICENSE_DATA_KEYS and (key not in existing_license.keys() or value != existing_license[key]) } # If any fields are missing from the new license, we'll consider them removed removed_values = existing_license.keys() - new_license.keys() diff --git a/backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py b/backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py index 784674ded3..dca1ab0e4a 100644 --- a/backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py +++ b/backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py @@ -58,6 +58,72 @@ def _get_provider_via_api(self, provider_id: str) -> dict: self.assertEqual(resp['statusCode'], 200) return json.loads(resp['body']) + def _get_license_record(self, provider_id: str) -> dict: + from boto3.dynamodb.conditions import Key + + records = self.config.provider_table.query(KeyConditionExpression=Key('pk').eq(f'cosm#PROVIDER#{provider_id}'))[ + 'Items' + ] + return next(record for record in records if record['type'] == 'license') + + def _set_license_field(self, provider_id: str, field: str, value: str): + """Set a field on the stored license record that a state cannot send in an upload.""" + license_record = self._get_license_record(provider_id) + self.config.provider_table.update_item( + Key={'pk': license_record['pk'], 'sk': license_record['sk']}, + UpdateExpression='SET #field = :value', + ExpressionAttributeNames={'#field': field}, + ExpressionAttributeValues={':value': value}, + ) + + def _reingest_default_license(self): + from handlers.ingest import ingest_license_message + + with open('../common/tests/resources/ingest/event-bridge-message.json') as f: + message = json.load(f) + event = {'Records': [{'messageId': '123', 'body': json.dumps(message)}]} + self.assertEqual({'batchItemFailures': []}, ingest_license_message(event, self.mock_context)) + + def test_existing_license_encumbered_status_survives_a_reupload(self): + """A state's upload cannot express an encumbrance, so a routine re-upload must not clear one. + + Dropping it would also flip the license's calculated status back to active and compact-eligible. + """ + provider_id = self._with_ingested_license() + # a board encumbrance applied after the original upload + self._set_license_field(provider_id, 'encumberedStatus', 'encumbered') + + self._reingest_default_license() + + self.assertEqual('encumbered', self._get_license_record(provider_id).get('encumberedStatus')) + + def test_existing_license_investigation_status_survives_a_reupload(self): + """As with encumbrances, an investigation is a CompactConnect action a state cannot send.""" + provider_id = self._with_ingested_license() + self._set_license_field(provider_id, 'investigationStatus', 'underInvestigation') + + self._reingest_default_license() + + self.assertEqual('underInvestigation', self._get_license_record(provider_id).get('investigationStatus')) + + def test_unchanged_reupload_of_a_preserved_license_creates_no_update_record(self): + """A re-upload that changes nothing must not leave an empty update record in the practitioner's + history. The calculated licenseStatus / compactEligibility differ between an encumbered record and + the freshly-loaded upload, but they are derived values rather than license data. + """ + from cc_common.data_model.update_tier_enum import UpdateTierEnum + + provider_id = self._with_ingested_license() + self._set_license_field(provider_id, 'encumberedStatus', 'encumbered') + self._set_license_field(provider_id, 'investigationStatus', 'underInvestigation') + + self._reingest_default_license() + + provider_user_records = self.config.data_client.get_provider_user_records( + compact='cosm', provider_id=provider_id, include_update_tier=UpdateTierEnum.TIER_THREE + ) + self.assertEqual([], provider_user_records._license_update_records) # noqa: SLF001 + def test_new_provider_ingest(self): from handlers.ingest import ingest_license_message diff --git a/backend/social-work-app/lambdas/python/common/cc_common/data_model/schema/license/record.py b/backend/social-work-app/lambdas/python/common/cc_common/data_model/schema/license/record.py index ad4c3ce190..794d0e07f0 100644 --- a/backend/social-work-app/lambdas/python/common/cc_common/data_model/schema/license/record.py +++ b/backend/social-work-app/lambdas/python/common/cc_common/data_model/schema/license/record.py @@ -36,6 +36,49 @@ from cc_common.data_model.schema.license.common import LicenseCommonSchema from cc_common.data_model.update_tier_enum import UpdateTierEnum +# Fields on a license record that CompactConnect owns rather than the uploading state. They are set by +# actions taken within the system (board encumbrances, investigations) or by the ingest process itself, +# and there is no way for a state to express them in a license upload. +# +# A license upload writes the whole record, so the uploading state is authoritative for every field it can +# send - omitting one removes it, which is intentional and recorded in the update record's removedValues. +# These fields are the exception: an upload cannot assert them, so it must not be able to retract them +# either. The ingest handler carries them forward from the existing record when re-uploading a license +# that already exists. +# +# Any future field of this kind must be added here, or a routine re-upload will silently drop it. Note +# that a field must also be declared on LicenseRecordSchema below to survive at all: this schema excludes +# undeclared attributes when a record is loaded. +SYSTEM_OWNED_LICENSE_FIELDS = frozenset( + { + 'encumberedStatus', + 'investigationStatus', + 'firstUploadDate', + } +) + + +# Fields on a license record that CompactConnect owns rather than the uploading state. They are set by +# actions taken within the system (board encumbrances, investigations) or by the ingest process itself, +# and there is no way for a state to express them in a license upload. +# +# A license upload writes the whole record, so the uploading state is authoritative for every field it can +# send - omitting one removes it, which is intentional and recorded in the update record's removedValues. +# These fields are the exception: an upload cannot assert them, so it must not be able to retract them +# either. The ingest handler carries them forward from the existing record when re-uploading a license +# that already exists. +# +# Any future field of this kind must be added here, or a routine re-upload will silently drop it. Note +# that a field must also be declared on LicenseRecordSchema below to survive at all: this schema excludes +# undeclared attributes when a record is loaded. +SYSTEM_OWNED_LICENSE_FIELDS = frozenset( + { + 'encumberedStatus', + 'investigationStatus', + 'firstUploadDate', + } +) + @BaseRecordSchema.register_schema('license') class LicenseRecordSchema(BaseRecordSchema, LicenseCommonSchema): diff --git a/backend/social-work-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.py b/backend/social-work-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.py index 096e1db027..a5857faa21 100644 --- a/backend/social-work-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.py +++ b/backend/social-work-app/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.py @@ -577,3 +577,71 @@ def test_license_status_corrected_to_inactive_at_expiration_utc_minus_four(self) result = LicenseGeneralResponseSchema().load(license_data) self.assertEqual('inactive', result['licenseStatus']) + + +class TestLicenseRecordFieldOwnership(TstLambdas): + """ + Guards the contract behind SYSTEM_OWNED_LICENSE_FIELDS. + + A license upload writes the whole record, so any field the upload cannot supply is dropped unless the + ingest handler carries it forward. This test fails if a new field is added to the license record without + deciding which side of that line it falls on. + """ + + # Regenerated from other fields by LicenseRecordSchema's pre_dump hooks on every write, and dropped + # from the loaded record by its post_load hook - so there is never anything to preserve. + GENERATED_ON_WRITE = { + 'pk', + 'sk', + 'licenseGSIPK', + 'licenseGSISK', + 'licenseUploadDateGSIPK', + 'licenseUploadDateGSISK', + } + # Stamped fresh by BaseRecordSchema on every write. + STAMPED_ON_WRITE = {'type', 'dateOfUpdate'} + # Calculated when a record is loaded and stripped again before it is written, so they are never stored. + CALCULATED_ON_LOAD = {'licenseStatus', 'compactEligibility'} + + def test_every_field_the_upload_cannot_supply_is_accounted_for(self): + from cc_common.data_model.schema.license.ingest import LicenseIngestSchema + from cc_common.data_model.schema.license.record import ( + SYSTEM_OWNED_LICENSE_FIELDS, + LicenseRecordSchema, + ) + + not_suppliable_by_upload = set(LicenseRecordSchema().fields) - set(LicenseIngestSchema().fields) + unaccounted_for = ( + not_suppliable_by_upload + - self.GENERATED_ON_WRITE + - self.STAMPED_ON_WRITE + - self.CALCULATED_ON_LOAD + - SYSTEM_OWNED_LICENSE_FIELDS + ) + + self.assertEqual( + set(), + unaccounted_for, + f'New license record field(s) {sorted(unaccounted_for)} cannot be supplied by a license upload, ' + 'so a routine re-upload will silently drop them. Decide which they are and add them to the right ' + 'place: SYSTEM_OWNED_LICENSE_FIELDS (in schema/license/record.py) if the system owns the value ' + 'and it must survive an upload, or one of the GENERATED_ON_WRITE / STAMPED_ON_WRITE / ' + 'CALCULATED_ON_LOAD sets in this test if the field is rebuilt or discarded on every write.', + ) + + def test_no_preserved_field_can_be_supplied_by_an_upload(self): + """A preserved field that a state can also send would be frozen at its first value forever, since + the carry-forward would overwrite whatever the upload provided. + """ + from cc_common.data_model.schema.license.ingest import LicenseIngestSchema + from cc_common.data_model.schema.license.record import SYSTEM_OWNED_LICENSE_FIELDS + + uploadable_but_preserved = SYSTEM_OWNED_LICENSE_FIELDS & set(LicenseIngestSchema().fields) + + self.assertEqual( + set(), + uploadable_but_preserved, + f'{sorted(uploadable_but_preserved)} can be supplied by a license upload, so preserving it would ' + 'pin it at its existing value and prevent a state from ever changing it. Remove it from ' + 'SYSTEM_OWNED_LICENSE_FIELDS.', + ) diff --git a/backend/social-work-app/lambdas/python/provider-data-v1/handlers/ingest.py b/backend/social-work-app/lambdas/python/provider-data-v1/handlers/ingest.py index 2b49a88057..eacc6f3e3e 100644 --- a/backend/social-work-app/lambdas/python/provider-data-v1/handlers/ingest.py +++ b/backend/social-work-app/lambdas/python/provider-data-v1/handlers/ingest.py @@ -16,7 +16,7 @@ ) from cc_common.data_model.schema.license import LicenseData from cc_common.data_model.schema.license.ingest import LicenseIngestSchema -from cc_common.data_model.schema.license.record import LicenseUpdateRecordSchema +from cc_common.data_model.schema.license.record import SYSTEM_OWNED_LICENSE_FIELDS, LicenseUpdateRecordSchema from cc_common.data_model.schema.provider import ProviderData from cc_common.data_model.schema.provider.record import ProviderUpdateRecordSchema from cc_common.event_batch_writer import EventBatchWriter @@ -26,6 +26,16 @@ license_schema = LicenseIngestSchema() license_update_schema = LicenseUpdateRecordSchema() + +# Keys on a license record that are not part of the state-supplied license data, so a difference in one is +# not a change to the license itself and does not belong in a licenseUpdate record: +# - dateOfUpdate is stamped on every write. +# - licenseStatus / compactEligibility are calculated when a record is loaded (see +# LicenseRecordSchema._calculate_statuses) and are stripped again before it is written, so they follow +# whatever else changed rather than being a change in their own right. +# - the system-owned fields are copied from the existing record onto the new one before this comparison +# runs, so they cannot differ here. +NON_LICENSE_DATA_KEYS = SYSTEM_OWNED_LICENSE_FIELDS | {'dateOfUpdate', 'licenseStatus', 'compactEligibility'} provider_update_schema = ProviderUpdateRecordSchema() # Fields tracked on the provider update "previous" snapshot (ProviderUpdatePreviousRecordSchema). @@ -204,16 +214,20 @@ def _matches_posted_license(license_record: LicenseData) -> bool: ) if existing_license_data is not None: existing_license = existing_license_data.to_dict() + # The write below replaces the whole license record, so any field the upload cannot carry + # would be dropped. Carry the system-owned fields (encumbrance / investigation status set + # by actions within CompactConnect, and the firstUploadDate behind the license upload date + # GSI) forward from the existing record. This happens before the update record is built, so + # the recorded history reflects what is actually written. + for field in SYSTEM_OWNED_LICENSE_FIELDS: + if existing_license.get(field) is not None: + posted_license_record[field] = existing_license[field] _process_license_update( existing_license=existing_license, new_license=posted_license_record, dynamo_transactions=dynamo_transactions, data_events=data_events, ) - # now grab the firstUploadDate from the existing record if available and put it in the posted_license - # for the license upload date GSI - if existing_license.get('firstUploadDate'): - posted_license_record['firstUploadDate'] = existing_license.get('firstUploadDate') else: logger.info('New license record detected') # If this is the first time creating the license record, @@ -448,6 +462,7 @@ def _check_for_missing_single_state_license_validation_error( ) ) + def _generate_cuid(compact: str) -> str: """ Generate a new Compact Unique Identifier (CUID) for a provider. @@ -623,11 +638,10 @@ def _process_license_update(*, existing_license: dict, new_license: dict, dynamo """ # Remove fields that are calculated at runtime, not stored in the database # uploadDate is metadata tracking when the license was first uploaded, not part of the license data - dynamic_keys = {'dateOfUpdate', 'status', 'uploadDate'} updated_values = { key: value for key, value in new_license.items() - if key not in dynamic_keys and (key not in existing_license.keys() or value != existing_license[key]) + if key not in NON_LICENSE_DATA_KEYS and (key not in existing_license.keys() or value != existing_license[key]) } # If any fields are missing from the new license, we'll consider them removed removed_values = existing_license.keys() - new_license.keys() diff --git a/backend/social-work-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py b/backend/social-work-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py index 22c5156d96..901be9eaaa 100644 --- a/backend/social-work-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py +++ b/backend/social-work-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py @@ -122,6 +122,72 @@ def _assert_provider_update_license_upload_name_change( self.assertEqual(expected_updated_given_name, provider_update.updatedValues['givenName']) self.assertNotIn('licenseJurisdiction', provider_update.updatedValues) + def _get_license_record(self, provider_id: str) -> dict: + from boto3.dynamodb.conditions import Key + + records = self.config.provider_table.query(KeyConditionExpression=Key('pk').eq(f'socw#PROVIDER#{provider_id}'))[ + 'Items' + ] + return next(record for record in records if record['type'] == 'license') + + def _set_license_field(self, provider_id: str, field: str, value: str): + """Set a field on the stored license record that a state cannot send in an upload.""" + license_record = self._get_license_record(provider_id) + self.config.provider_table.update_item( + Key={'pk': license_record['pk'], 'sk': license_record['sk']}, + UpdateExpression='SET #field = :value', + ExpressionAttributeNames={'#field': field}, + ExpressionAttributeValues={':value': value}, + ) + + def _reingest_default_license(self): + from handlers.ingest import ingest_license_message + + with open('../common/tests/resources/ingest/event-bridge-message.json') as f: + message = json.load(f) + event = {'Records': [{'messageId': '123', 'body': json.dumps(message)}]} + self.assertEqual({'batchItemFailures': []}, ingest_license_message(event, self.mock_context)) + + def test_existing_license_encumbered_status_survives_a_reupload(self): + """A state's upload cannot express an encumbrance, so a routine re-upload must not clear one. + + Dropping it would also flip the license's calculated status back to active and compact-eligible. + """ + provider_id = self._with_ingested_license() + # a board encumbrance applied after the original upload + self._set_license_field(provider_id, 'encumberedStatus', 'encumbered') + + self._reingest_default_license() + + self.assertEqual('encumbered', self._get_license_record(provider_id).get('encumberedStatus')) + + def test_existing_license_investigation_status_survives_a_reupload(self): + """As with encumbrances, an investigation is a CompactConnect action a state cannot send.""" + provider_id = self._with_ingested_license() + self._set_license_field(provider_id, 'investigationStatus', 'underInvestigation') + + self._reingest_default_license() + + self.assertEqual('underInvestigation', self._get_license_record(provider_id).get('investigationStatus')) + + def test_unchanged_reupload_of_a_preserved_license_creates_no_update_record(self): + """A re-upload that changes nothing must not leave an empty update record in the practitioner's + history. The calculated licenseStatus / compactEligibility differ between an encumbered record and + the freshly-loaded upload, but they are derived values rather than license data. + """ + from cc_common.data_model.update_tier_enum import UpdateTierEnum + + provider_id = self._with_ingested_license() + self._set_license_field(provider_id, 'encumberedStatus', 'encumbered') + self._set_license_field(provider_id, 'investigationStatus', 'underInvestigation') + + self._reingest_default_license() + + provider_user_records = self.config.data_client.get_provider_user_records( + compact='socw', provider_id=provider_id, include_update_tier=UpdateTierEnum.TIER_THREE + ) + self.assertEqual([], provider_user_records._license_update_records) # noqa: SLF001 + def test_new_provider_ingest(self): from handlers.ingest import ingest_license_message From 19f89d2e27649c0e9e11f99231ef6dc9e4a5a942 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Wed, 19 Aug 2026 22:01:23 -0500 Subject: [PATCH 08/27] Ensure encumbrance status is preserved on the provider record --- .../data_model/provider_record_util.py | 7 ++- .../data_model/schema/provider/record.py | 6 +++ .../tests/unit/test_provider_record_util.py | 43 +++++++++++++++++++ .../provider-data-v1/handlers/ingest.py | 7 +++ .../function/test_handlers/test_ingest.py | 39 +++++++++++++++-- .../data_model/provider_record_util.py | 7 ++- .../data_model/schema/provider/record.py | 6 +++ .../tests/unit/test_provider_record_util.py | 26 +++++++++++ .../provider-data-v1/handlers/ingest.py | 7 +++ .../function/test_handlers/test_ingest.py | 29 +++++++++++++ .../data_model/provider_record_util.py | 7 ++- .../data_model/schema/provider/record.py | 6 +++ .../tests/unit/test_provider_record_util.py | 26 +++++++++++ .../provider-data-v1/handlers/ingest.py | 7 +++ .../function/test_handlers/test_ingest.py | 29 +++++++++++++ 15 files changed, 246 insertions(+), 6 deletions(-) diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py index a4d3783cc5..0bde4b0b4d 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py @@ -25,6 +25,7 @@ from cc_common.data_model.schema.privilege import PrivilegeData, PrivilegeUpdateData from cc_common.data_model.schema.privilege.api import PrivilegeHistoryResponseSchema from cc_common.data_model.schema.provider import ProviderData, ProviderUpdateData +from cc_common.data_model.schema.provider.record import PROVIDER_AGGREGATE_FIELDS from cc_common.exceptions import CCInternalException, CCNotFoundException @@ -215,7 +216,11 @@ def populate_provider_record( } ) # else populate the current fields of the provider record first before updating with - # new values + # new values. + # The provider's aggregate fields are deliberately held back from the license overlay: they + # summarize every license and privilege the practitioner holds, so one license's value must not + # replace them. (On the create branch above there is no prior aggregate, so the license seeds it.) + license_record = {key: value for key, value in license_record.items() if key not in PROVIDER_AGGREGATE_FIELDS} return ProviderData.create_new( { # keep existing values from the current provider record diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py index a544b613d4..224a030d86 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py @@ -29,6 +29,12 @@ ) from cc_common.data_model.update_tier_enum import UpdateTierEnum +# Fields on the top-level provider record that aggregate across every license AND privilege the +# practitioner holds. They are maintained by the encumbrance flows, which set them when any record is +# encumbered and clear them only once nothing is. A single license's value must never be copied over +# them when an existing provider record is refreshed - see ProviderRecordUtility.populate_provider_record. +PROVIDER_AGGREGATE_FIELDS = frozenset({'encumberedStatus'}) + @BaseRecordSchema.register_schema('provider') class ProviderRecordSchema(BaseRecordSchema): diff --git a/backend/compact-connect/lambdas/python/common/tests/unit/test_provider_record_util.py b/backend/compact-connect/lambdas/python/common/tests/unit/test_provider_record_util.py index b8375414c0..216108cba4 100644 --- a/backend/compact-connect/lambdas/python/common/tests/unit/test_provider_record_util.py +++ b/backend/compact-connect/lambdas/python/common/tests/unit/test_provider_record_util.py @@ -1575,3 +1575,46 @@ def test_excludes_person_level_records(self): def test_returns_only_the_expected_records_and_nothing_else(self): self.assertEqual(self._keys(self.all_associated_records), self._keys(self.result)) + + +class TestPopulateProviderRecordAggregateFields(TstLambdas): + """ + The provider record's encumberedStatus aggregates every license AND privilege the practitioner holds, + and is maintained by the encumbrance flows. populate_provider_record overlays a single license onto the + provider record, so it must not carry that field across when refreshing an existing record - otherwise a + routine license upload (or registration, home-state change, or rollback) silently clears the aggregate. + """ + + def test_existing_provider_keeps_its_own_encumbered_status(self): + from cc_common.data_model.provider_record_util import ProviderRecordUtility + + # the provider is encumbered because of a privilege, while the license itself was lifted earlier + # and carries the residual 'unencumbered' + current_provider = self.test_data_generator.generate_default_provider({'encumberedStatus': 'encumbered'}) + license_record = self.test_data_generator.generate_default_license( + {'encumberedStatus': 'unencumbered'} + ).to_dict() + + provider_record = ProviderRecordUtility.populate_provider_record( + current_provider_record=current_provider, + license_record=license_record, + privilege_records=[], + ) + + self.assertEqual('encumbered', provider_record.to_dict()['encumberedStatus']) + + def test_new_provider_is_seeded_from_the_license(self): + """With no prior record there is no aggregate to protect, so an encumbered license must still + produce an encumbered provider - the SSN-correction migration builds a new provider this way. + """ + from cc_common.data_model.provider_record_util import ProviderRecordUtility + + license_record = self.test_data_generator.generate_default_license({'encumberedStatus': 'encumbered'}).to_dict() + + provider_record = ProviderRecordUtility.populate_provider_record( + current_provider_record=None, + license_record=license_record, + privilege_records=[], + ) + + self.assertEqual('encumbered', provider_record.to_dict()['encumberedStatus']) diff --git a/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py b/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py index 3341531f23..f68b5b0ccb 100644 --- a/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py +++ b/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py @@ -217,6 +217,13 @@ def ingest_license_message(message: dict): for field in SYSTEM_OWNED_LICENSE_FIELDS: if existing_license.get(field) is not None: posted_license_record[field] = existing_license[field] + # licenseStatus and compactEligibility were calculated when this record was loaded, + # before the encumbrance above was carried onto it. Round-trip through the schema so the + # derived values reflect it - find_best_license reads them when choosing which license + # represents the practitioner. + posted_license_record = license_record_schema.load( + json.loads(license_record_schema.dumps(deepcopy(posted_license_record))) + ) _process_license_update( existing_license=existing_license, new_license=posted_license_record, diff --git a/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py b/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py index b40db2c977..b2f79fcfbb 100644 --- a/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py +++ b/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py @@ -531,13 +531,23 @@ def test_existing_provider_removed_email(self): self.assertEqual(1, len(provider_user_records._license_update_records)) # noqa SLF001 self.assertEqual(['emailAddress'], provider_user_records._license_update_records[0].removedValues) # noqa SLF001 - def _get_license_record(self, provider_id: str) -> dict: + def _get_all_records(self, provider_id: str) -> list[dict]: from boto3.dynamodb.conditions import Key - records = self.config.provider_table.query(KeyConditionExpression=Key('pk').eq(f'aslp#PROVIDER#{provider_id}'))[ + return self.config.provider_table.query(KeyConditionExpression=Key('pk').eq(f'aslp#PROVIDER#{provider_id}'))[ 'Items' ] - return next(record for record in records if record['type'] == 'license') + + def _get_license_record(self, provider_id: str) -> dict: + return next(record for record in self._get_all_records(provider_id) if record['type'] == 'license') + + def _reingest_default_license(self): + from handlers.ingest import ingest_license_message + + with open('../common/tests/resources/ingest/event-bridge-message.json') as f: + message = json.load(f) + event = {'Records': [{'messageId': '123', 'body': json.dumps(message)}]} + self.assertEqual({'batchItemFailures': []}, ingest_license_message(event, self.mock_context)) def _set_license_field(self, provider_id: str, field: str, value: str): """Set a field on the stored license record that a state cannot send in an upload.""" @@ -623,6 +633,29 @@ def test_unchanged_reupload_of_a_preserved_license_creates_no_update_record(self ) self.assertEqual([], provider_user_records._license_update_records) # noqa: SLF001 + def test_reupload_does_not_overwrite_the_provider_encumbrance_aggregate(self): + """The provider record's encumberedStatus aggregates every license AND privilege, and is maintained + by the encumbrance flows. A license upload must not push one license's value onto it - a provider + encumbered because of a privilege would otherwise be cleared by a routine roster upload. + """ + provider_id = self._with_ingested_license() + # provider encumbered because of a privilege; the license itself was lifted earlier, so it carries + # the residual 'unencumbered' value + self._set_license_field(provider_id, 'encumberedStatus', 'unencumbered') + provider_record = next(record for record in self._get_all_records(provider_id) if record['type'] == 'provider') + self.config.provider_table.update_item( + Key={'pk': provider_record['pk'], 'sk': provider_record['sk']}, + UpdateExpression='SET encumberedStatus = :value', + ExpressionAttributeValues={':value': 'encumbered'}, + ) + + self._reingest_default_license() + + updated_provider_record = next( + record for record in self._get_all_records(provider_id) if record['type'] == 'provider' + ) + self.assertEqual('encumbered', updated_provider_record.get('encumberedStatus')) + def test_existing_provider_added_email(self): from handlers.ingest import ingest_license_message diff --git a/backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py b/backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py index 55b0d49b4c..f650402854 100644 --- a/backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py +++ b/backend/cosmetology-app/lambdas/python/common/cc_common/data_model/provider_record_util.py @@ -15,6 +15,7 @@ from cc_common.data_model.schema.investigation import InvestigationData from cc_common.data_model.schema.license import LicenseData, LicenseUpdateData from cc_common.data_model.schema.provider import ProviderData, ProviderUpdateData +from cc_common.data_model.schema.provider.record import PROVIDER_AGGREGATE_FIELDS from cc_common.exceptions import CCInternalException, CCNotFoundException @@ -124,7 +125,11 @@ def populate_provider_record(current_provider_record: ProviderData | None, licen } ) # else populate the current fields of the provider record first before updating with - # new values + # new values. + # The provider's aggregate fields are deliberately held back from the license overlay: they + # summarize every license and privilege the practitioner holds, so one license's value must not + # replace them. (On the create branch above there is no prior aggregate, so the license seeds it.) + license_record = {key: value for key, value in license_record.items() if key not in PROVIDER_AGGREGATE_FIELDS} return ProviderData.create_new( { # keep existing values from the current provider record diff --git a/backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/provider/record.py b/backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/provider/record.py index ec9860ca54..7008b80f91 100644 --- a/backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/provider/record.py +++ b/backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/provider/record.py @@ -24,6 +24,12 @@ ) from cc_common.data_model.update_tier_enum import UpdateTierEnum +# Fields on the top-level provider record that aggregate across every license AND privilege the +# practitioner holds. They are maintained by the encumbrance flows, which set them when any record is +# encumbered and clear them only once nothing is. A single license's value must never be copied over +# them when an existing provider record is refreshed - see ProviderRecordUtility.populate_provider_record. +PROVIDER_AGGREGATE_FIELDS = frozenset({'encumberedStatus'}) + @BaseRecordSchema.register_schema('provider') class ProviderRecordSchema(BaseRecordSchema): diff --git a/backend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.py b/backend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.py index def14f6311..65f7bc266d 100644 --- a/backend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.py +++ b/backend/cosmetology-app/lambdas/python/common/tests/unit/test_provider_record_util.py @@ -1293,3 +1293,29 @@ def test_no_licenses_returns_empty_list(self): docs = provider_user_records.generate_opensearch_documents() self.assertEqual([], docs) + + +class TestPopulateProviderRecordAggregateFields(TstLambdas): + """ + The provider record's encumberedStatus aggregates every license AND privilege the practitioner holds, + and is maintained by the encumbrance flows. populate_provider_record overlays a single license onto the + provider record, so it must not carry that field across when refreshing an existing record - otherwise a + routine license upload (or registration, home-state change, or rollback) silently clears the aggregate. + """ + + def test_existing_provider_keeps_its_own_encumbered_status(self): + from cc_common.data_model.provider_record_util import ProviderRecordUtility + + # the provider is encumbered because of a privilege, while the license itself was lifted earlier + # and carries the residual 'unencumbered' + current_provider = self.test_data_generator.generate_default_provider({'encumberedStatus': 'encumbered'}) + license_record = self.test_data_generator.generate_default_license( + {'encumberedStatus': 'unencumbered'} + ).to_dict() + + provider_record = ProviderRecordUtility.populate_provider_record( + current_provider_record=current_provider, + license_record=license_record, + ) + + self.assertEqual('encumbered', provider_record.to_dict()['encumberedStatus']) diff --git a/backend/cosmetology-app/lambdas/python/provider-data-v1/handlers/ingest.py b/backend/cosmetology-app/lambdas/python/provider-data-v1/handlers/ingest.py index 30b3e923b5..5f42454201 100644 --- a/backend/cosmetology-app/lambdas/python/provider-data-v1/handlers/ingest.py +++ b/backend/cosmetology-app/lambdas/python/provider-data-v1/handlers/ingest.py @@ -167,6 +167,13 @@ def ingest_license_message(message: dict): for field in SYSTEM_OWNED_LICENSE_FIELDS: if existing_license.get(field) is not None: posted_license_record[field] = existing_license[field] + # licenseStatus and compactEligibility were calculated when this record was loaded, + # before the encumbrance above was carried onto it. Round-trip through the schema so the + # derived values reflect it - find_best_license reads them when choosing which license + # represents the practitioner. + posted_license_record = license_record_schema.load( + json.loads(license_record_schema.dumps(deepcopy(posted_license_record))) + ) _process_license_update( existing_license=existing_license, new_license=posted_license_record, diff --git a/backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py b/backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py index dca1ab0e4a..d544e0e5c4 100644 --- a/backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py +++ b/backend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py @@ -84,6 +84,35 @@ def _reingest_default_license(self): event = {'Records': [{'messageId': '123', 'body': json.dumps(message)}]} self.assertEqual({'batchItemFailures': []}, ingest_license_message(event, self.mock_context)) + def test_reupload_does_not_overwrite_the_provider_encumbrance_aggregate(self): + """The provider record's encumberedStatus aggregates every license AND privilege, and is maintained + by the encumbrance flows. A license upload must not push one license's value onto it - a provider + encumbered because of a privilege would otherwise be cleared by a routine roster upload. + """ + from boto3.dynamodb.conditions import Key + + provider_id = self._with_ingested_license() + # provider encumbered because of a privilege; the license itself was lifted earlier, so it carries + # the residual 'unencumbered' value + self._set_license_field(provider_id, 'encumberedStatus', 'unencumbered') + + def provider_record(): + records = self.config.provider_table.query( + KeyConditionExpression=Key('pk').eq(f'cosm#PROVIDER#{provider_id}') + )['Items'] + return next(record for record in records if record['type'] == 'provider') + + record = provider_record() + self.config.provider_table.update_item( + Key={'pk': record['pk'], 'sk': record['sk']}, + UpdateExpression='SET encumberedStatus = :value', + ExpressionAttributeValues={':value': 'encumbered'}, + ) + + self._reingest_default_license() + + self.assertEqual('encumbered', provider_record().get('encumberedStatus')) + def test_existing_license_encumbered_status_survives_a_reupload(self): """A state's upload cannot express an encumbrance, so a routine re-upload must not clear one. diff --git a/backend/social-work-app/lambdas/python/common/cc_common/data_model/provider_record_util.py b/backend/social-work-app/lambdas/python/common/cc_common/data_model/provider_record_util.py index 28c123fc51..18c3bc5998 100644 --- a/backend/social-work-app/lambdas/python/common/cc_common/data_model/provider_record_util.py +++ b/backend/social-work-app/lambdas/python/common/cc_common/data_model/provider_record_util.py @@ -16,6 +16,7 @@ from cc_common.data_model.schema.investigation import InvestigationData from cc_common.data_model.schema.license import LicenseData, LicenseUpdateData from cc_common.data_model.schema.provider import ProviderData, ProviderUpdateData +from cc_common.data_model.schema.provider.record import PROVIDER_AGGREGATE_FIELDS from cc_common.exceptions import CCInternalException, CCNotFoundException from cc_common.license_recognition_util import LicenseRecognitionUtil @@ -181,7 +182,11 @@ def populate_provider_record( } ) # else populate the current fields of the provider record first before updating with - # new values + # new values. + # The provider's aggregate fields are deliberately held back from the license overlay: they + # summarize every license and privilege the practitioner holds, so one license's value must not + # replace them. (On the create branch above there is no prior aggregate, so the license seeds it.) + license_record = {key: value for key, value in license_record.items() if key not in PROVIDER_AGGREGATE_FIELDS} return ProviderData.create_new( { # keep existing values from the current provider record diff --git a/backend/social-work-app/lambdas/python/common/cc_common/data_model/schema/provider/record.py b/backend/social-work-app/lambdas/python/common/cc_common/data_model/schema/provider/record.py index 9033ee5fc1..ffd12645b3 100644 --- a/backend/social-work-app/lambdas/python/common/cc_common/data_model/schema/provider/record.py +++ b/backend/social-work-app/lambdas/python/common/cc_common/data_model/schema/provider/record.py @@ -26,6 +26,12 @@ ) from cc_common.data_model.update_tier_enum import UpdateTierEnum +# Fields on the top-level provider record that aggregate across every license AND privilege the +# practitioner holds. They are maintained by the encumbrance flows, which set them when any record is +# encumbered and clear them only once nothing is. A single license's value must never be copied over +# them when an existing provider record is refreshed - see ProviderRecordUtility.populate_provider_record. +PROVIDER_AGGREGATE_FIELDS = frozenset({'encumberedStatus'}) + @BaseRecordSchema.register_schema('provider') class ProviderRecordSchema(BaseRecordSchema): diff --git a/backend/social-work-app/lambdas/python/common/tests/unit/test_provider_record_util.py b/backend/social-work-app/lambdas/python/common/tests/unit/test_provider_record_util.py index db4d7aa704..c3363e1c53 100644 --- a/backend/social-work-app/lambdas/python/common/tests/unit/test_provider_record_util.py +++ b/backend/social-work-app/lambdas/python/common/tests/unit/test_provider_record_util.py @@ -2250,3 +2250,29 @@ def test_opensearch_documents_include_public_compact_identifier_on_every_documen self.assertEqual(2, len(docs)) for doc in docs: self.assertEqual('SWC-4548-1', doc['publicCompactIdentifier']) + + +class TestPopulateProviderRecordAggregateFields(TstLambdas): + """ + The provider record's encumberedStatus aggregates every license AND privilege the practitioner holds, + and is maintained by the encumbrance flows. populate_provider_record overlays a single license onto the + provider record, so it must not carry that field across when refreshing an existing record - otherwise a + routine license upload (or registration, home-state change, or rollback) silently clears the aggregate. + """ + + def test_existing_provider_keeps_its_own_encumbered_status(self): + from cc_common.data_model.provider_record_util import ProviderRecordUtility + + # the provider is encumbered because of a privilege, while the license itself was lifted earlier + # and carries the residual 'unencumbered' + current_provider = self.test_data_generator.generate_default_provider({'encumberedStatus': 'encumbered'}) + license_record = self.test_data_generator.generate_default_license( + {'encumberedStatus': 'unencumbered'} + ).to_dict() + + provider_record = ProviderRecordUtility.populate_provider_record( + current_provider_record=current_provider, + license_record=license_record, + ) + + self.assertEqual('encumbered', provider_record.to_dict()['encumberedStatus']) diff --git a/backend/social-work-app/lambdas/python/provider-data-v1/handlers/ingest.py b/backend/social-work-app/lambdas/python/provider-data-v1/handlers/ingest.py index eacc6f3e3e..04355d4c89 100644 --- a/backend/social-work-app/lambdas/python/provider-data-v1/handlers/ingest.py +++ b/backend/social-work-app/lambdas/python/provider-data-v1/handlers/ingest.py @@ -222,6 +222,13 @@ def _matches_posted_license(license_record: LicenseData) -> bool: for field in SYSTEM_OWNED_LICENSE_FIELDS: if existing_license.get(field) is not None: posted_license_record[field] = existing_license[field] + # licenseStatus and compactEligibility were calculated when this record was loaded, + # before the encumbrance above was carried onto it. Round-trip through the schema so the + # derived values reflect it - find_best_license reads them when choosing which license + # represents the practitioner. + posted_license_record = license_record_schema.load( + json.loads(license_record_schema.dumps(deepcopy(posted_license_record))) + ) _process_license_update( existing_license=existing_license, new_license=posted_license_record, diff --git a/backend/social-work-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py b/backend/social-work-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py index 901be9eaaa..fc8a734ee1 100644 --- a/backend/social-work-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py +++ b/backend/social-work-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py @@ -148,6 +148,35 @@ def _reingest_default_license(self): event = {'Records': [{'messageId': '123', 'body': json.dumps(message)}]} self.assertEqual({'batchItemFailures': []}, ingest_license_message(event, self.mock_context)) + def test_reupload_does_not_overwrite_the_provider_encumbrance_aggregate(self): + """The provider record's encumberedStatus aggregates every license AND privilege, and is maintained + by the encumbrance flows. A license upload must not push one license's value onto it - a provider + encumbered because of a privilege would otherwise be cleared by a routine roster upload. + """ + from boto3.dynamodb.conditions import Key + + provider_id = self._with_ingested_license() + # provider encumbered because of a privilege; the license itself was lifted earlier, so it carries + # the residual 'unencumbered' value + self._set_license_field(provider_id, 'encumberedStatus', 'unencumbered') + + def provider_record(): + records = self.config.provider_table.query( + KeyConditionExpression=Key('pk').eq(f'socw#PROVIDER#{provider_id}') + )['Items'] + return next(record for record in records if record['type'] == 'provider') + + record = provider_record() + self.config.provider_table.update_item( + Key={'pk': record['pk'], 'sk': record['sk']}, + UpdateExpression='SET encumberedStatus = :value', + ExpressionAttributeValues={':value': 'encumbered'}, + ) + + self._reingest_default_license() + + self.assertEqual('encumbered', provider_record().get('encumberedStatus')) + def test_existing_license_encumbered_status_survives_a_reupload(self): """A state's upload cannot express an encumbrance, so a routine re-upload must not clear one. From 972459ed72fdcfbf6ab70171128f79dd845c65d6 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Fri, 14 Aug 2026 14:38:56 -0500 Subject: [PATCH 09/27] update nanoid version for node check --- backend/compact-connect/lambdas/nodejs/yarn.lock | 6 +++--- backend/cosmetology-app/lambdas/nodejs/yarn.lock | 6 +++--- backend/social-work-app/lambdas/nodejs/yarn.lock | 6 +++--- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/backend/compact-connect/lambdas/nodejs/yarn.lock b/backend/compact-connect/lambdas/nodejs/yarn.lock index af62e8dc5e..7c2b9ec127 100644 --- a/backend/compact-connect/lambdas/nodejs/yarn.lock +++ b/backend/compact-connect/lambdas/nodejs/yarn.lock @@ -4674,9 +4674,9 @@ ms@^2.1.1, ms@^2.1.3: integrity sha512-6FlzubTLZG3J2a/NVCAleEhjzq5oxgHyaCU9yYXvcLsvoVaHJq/s5xXI6/XXP6tz7R9xAOtHnSO/tXtF3WRTlA== nanoid@^3.3.16: - version "3.3.16" - resolved "https://registry.yarnpkg.com/nanoid/-/nanoid-3.3.16.tgz#a04d8ec4b1f10009d2d533947aefe4293737816c" - integrity sha512-bzlKTyNJ7+LdGIIwy8ijFpIqEQIvafahV7eYykJ8Cvh42EdJeODoJ6gUJXpQJvej1BddH8OqTXZNE/KfbWAu8Q== + version "3.3.18" + resolved "https://registry.yarnpkg.com/nanoid/-/nanoid-3.3.18.tgz#f66a2de1199ffde0fcf21c8a5f13106b1c081913" + integrity sha512-DTg4MJbGMWkfi6VZFdNt2/caMbQy4Ou+Op/hJQvGEWcnVfoA1QA+xzRKAzw9jD6+GVOOeYr/mIcuDSdug6F6+w== napi-postinstall@^0.3.4: version "0.3.4" diff --git a/backend/cosmetology-app/lambdas/nodejs/yarn.lock b/backend/cosmetology-app/lambdas/nodejs/yarn.lock index af62e8dc5e..7c2b9ec127 100644 --- a/backend/cosmetology-app/lambdas/nodejs/yarn.lock +++ b/backend/cosmetology-app/lambdas/nodejs/yarn.lock @@ -4674,9 +4674,9 @@ ms@^2.1.1, ms@^2.1.3: integrity sha512-6FlzubTLZG3J2a/NVCAleEhjzq5oxgHyaCU9yYXvcLsvoVaHJq/s5xXI6/XXP6tz7R9xAOtHnSO/tXtF3WRTlA== nanoid@^3.3.16: - version "3.3.16" - resolved "https://registry.yarnpkg.com/nanoid/-/nanoid-3.3.16.tgz#a04d8ec4b1f10009d2d533947aefe4293737816c" - integrity sha512-bzlKTyNJ7+LdGIIwy8ijFpIqEQIvafahV7eYykJ8Cvh42EdJeODoJ6gUJXpQJvej1BddH8OqTXZNE/KfbWAu8Q== + version "3.3.18" + resolved "https://registry.yarnpkg.com/nanoid/-/nanoid-3.3.18.tgz#f66a2de1199ffde0fcf21c8a5f13106b1c081913" + integrity sha512-DTg4MJbGMWkfi6VZFdNt2/caMbQy4Ou+Op/hJQvGEWcnVfoA1QA+xzRKAzw9jD6+GVOOeYr/mIcuDSdug6F6+w== napi-postinstall@^0.3.4: version "0.3.4" diff --git a/backend/social-work-app/lambdas/nodejs/yarn.lock b/backend/social-work-app/lambdas/nodejs/yarn.lock index af62e8dc5e..7c2b9ec127 100644 --- a/backend/social-work-app/lambdas/nodejs/yarn.lock +++ b/backend/social-work-app/lambdas/nodejs/yarn.lock @@ -4674,9 +4674,9 @@ ms@^2.1.1, ms@^2.1.3: integrity sha512-6FlzubTLZG3J2a/NVCAleEhjzq5oxgHyaCU9yYXvcLsvoVaHJq/s5xXI6/XXP6tz7R9xAOtHnSO/tXtF3WRTlA== nanoid@^3.3.16: - version "3.3.16" - resolved "https://registry.yarnpkg.com/nanoid/-/nanoid-3.3.16.tgz#a04d8ec4b1f10009d2d533947aefe4293737816c" - integrity sha512-bzlKTyNJ7+LdGIIwy8ijFpIqEQIvafahV7eYykJ8Cvh42EdJeODoJ6gUJXpQJvej1BddH8OqTXZNE/KfbWAu8Q== + version "3.3.18" + resolved "https://registry.yarnpkg.com/nanoid/-/nanoid-3.3.18.tgz#f66a2de1199ffde0fcf21c8a5f13106b1c081913" + integrity sha512-DTg4MJbGMWkfi6VZFdNt2/caMbQy4Ou+Op/hJQvGEWcnVfoA1QA+xzRKAzw9jD6+GVOOeYr/mIcuDSdug6F6+w== napi-postinstall@^0.3.4: version "0.3.4" From db6689de58fd5ffae307cb4521949e01cd1ef282 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Wed, 19 Aug 2026 22:39:52 -0500 Subject: [PATCH 10/27] Updating existing top level record with encumbered and military status during migration --- .../cc_common/data_model/data_client.py | 156 ++++++++++++++-- .../data_model/schema/provider/record.py | 22 +++ .../test_data_client_ssn_correction.py | 170 ++++++++++++++++++ 3 files changed, 332 insertions(+), 16 deletions(-) diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py index 3bf9620c89..ceb4466dfe 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py @@ -43,6 +43,7 @@ from cc_common.data_model.schema.privilege import PrivilegeData, PrivilegeUpdateData from cc_common.data_model.schema.privilege.record import PrivilegeUpdateRecordSchema from cc_common.data_model.schema.provider import ProviderData, ProviderUpdateData +from cc_common.data_model.schema.provider.record import PROVIDER_ACCOUNT_STATE_FIELDS from cc_common.data_model.update_tier_enum import UpdateTierEnum from cc_common.exceptions import ( CCAmbiguousLicenseNumberException, @@ -3157,23 +3158,25 @@ def migrate_provider_for_ssn_correction( rekeyed_privileges.append(rekeyed_record) create_transaction_items.append(self._build_put_transaction_item(rekeyed_record)) - # Create a top-level provider record for the new provider only if it does not already have one; a - # pre-existing record is never modified. The existence check and this Put are not atomic with each - # other, so the Put is conditioned on the record still being absent: if a concurrent write (e.g. a + # Create a top-level provider record for the new provider only if it does not already have one. If the record + # does exist and we are performing a full migration, we perform an UPDATE with provider relevant fields such + # as military status. + # + # The existence check and this Put are not atomic with each other, so the Put is conditioned on the + # record still being absent: if a concurrent write (e.g. a # different migration into the same new provider id) creates one in between, this Put fails instead of # silently clobbering it, and the transaction raises for SQS retry to re-read the now-current state. - new_provider_record = self._build_new_provider_record_if_absent( + new_provider_record_item = self._build_new_provider_record_transaction_item( compact=compact, new_provider_id=new_provider_id, rekeyed_target_license=rekeyed_target_license, rekeyed_privileges=rekeyed_privileges, + old_provider_data=old_top_level_provider_data, + full_migration=full_migration, + migrated_records_are_encumbered=self._migrated_records_are_encumbered(records_to_move), ) - if new_provider_record is not None: - create_transaction_items.append( - self._build_put_transaction_item( - new_provider_record, condition={'ConditionExpression': 'attribute_not_exists(pk)'} - ) - ) + if new_provider_record_item is not None: + create_transaction_items.append(new_provider_record_item) # deletes: the moved records on the old provider, except the target license and the top-level provider # record (both handled in the final group). @@ -3275,6 +3278,20 @@ def migrate_provider_for_ssn_correction( ), ) + @staticmethod + def _migrated_records_are_encumbered(records_to_move: list[CCDataClass]) -> bool: + """ + Whether the records arriving under the new provider id carry an active encumbrance. + + Read from the adverse action records rather than the encumberedStatus flags on the license and + privilege records: the flags are a denormalized summary that a license re-upload has historically + been able to drop, while an adverse action with no effectiveLiftDate is the encumbrance itself. + """ + return any( + record.type == ProviderRecordType.ADVERSE_ACTION and record.effectiveLiftDate is None + for record in records_to_move + ) + @staticmethod def _collect_transaction_ids(records: list[CCDataClass]) -> set[str]: """ @@ -3547,27 +3564,134 @@ def _repopulate_provider_record_from_remaining_records( privilege_records=[privilege_data.to_dict() for privilege_data in remaining_privileges], ) - def _build_new_provider_record_if_absent( + def _build_new_provider_record_transaction_item( self, *, compact: str, new_provider_id: str, rekeyed_target_license: LicenseData, rekeyed_privileges: list[PrivilegeData], - ) -> ProviderData | None: + old_provider_data: ProviderData | None, + full_migration: bool, + migrated_records_are_encumbered: bool, + ) -> dict | None: """ Build a top-level provider record for the new provider from the migrated license/privileges, or return None if the new provider already has one (a pre-existing record is never modified). + + On a full migration the entire practitioner moves, including the person-level records behind their + military status, so the old provider record seeds the new one, carrying the provider-level fields + that no license can supply (military status and note, and the encumbrance aggregate). The corrected + license then overwrites everything it is authoritative for, including the provider id. Account + state is left behind: a full migration deletes the old Cognito user, so the new provider must start + unregistered, which includes its home jurisdiction selection, since registration sets that. + + A partial migration carries none of it, matching how the person-level records themselves are + handled: they stay with the old provider, so a military status on the new provider would have no + supporting affiliation records behind it. """ try: - self.get_provider_top_level_record(compact=compact, provider_id=new_provider_id) - return None + existing_new_provider_record = self.get_provider_top_level_record( + compact=compact, provider_id=new_provider_id + ) except CCNotFoundException: - return ProviderRecordUtility.populate_provider_record( - current_provider_record=None, + seed_record = None + if full_migration and old_provider_data is not None: + seed_record = ProviderData.create_new( + { + key: value + for key, value in old_provider_data.to_dict().items() + if key not in PROVIDER_ACCOUNT_STATE_FIELDS + } + ) + new_provider_record = ProviderRecordUtility.populate_provider_record( + current_provider_record=seed_record, license_record=rekeyed_target_license.to_dict(), privilege_records=[privilege_data.to_dict() for privilege_data in rekeyed_privileges], ) + if migrated_records_are_encumbered: + new_provider_record.update({'encumberedStatus': LicenseEncumberedStatusEnum.ENCUMBERED}) + return self._build_put_transaction_item( + new_provider_record, condition={'ConditionExpression': 'attribute_not_exists(pk)'} + ) + + return self._build_existing_provider_merge_item( + existing_new_provider_record=existing_new_provider_record, + old_provider_data=old_provider_data, + full_migration=full_migration, + migrated_records_are_encumbered=migrated_records_are_encumbered, + ) + + def _build_existing_provider_merge_item( + self, + *, + existing_new_provider_record: ProviderData, + old_provider_data: ProviderData | None, + full_migration: bool, + migrated_records_are_encumbered: bool, + ) -> dict | None: + """ + Merge the old provider's person-level fields into a provider record that already exists, or return + None when there is nothing to merge. + + A practitioner with two licenses is corrected in two uploads: the first is a partial migration that + creates the new provider record, the second a full migration that finds it already there and deletes + the old provider record. Without this, the military status the reviewer decided is lost at that + second step - it lives only on the top-level record and cannot be rebuilt from the migrated + affiliation records. + + Each field is written with `if_not_exists`, so this can only fill gaps: a provider who was audited in + their own right under the new provider id keeps their own status. The write is idempotent, so an SQS + replay of the migration re-applies it harmlessly. + + Person-level fields move only on a full migration, matching the records that back them: a partial + migration leaves the military affiliation records with the old provider, so a status carried without + them would have no supporting documentation behind it. The encumbrance flag is not person-level and + is not gated that way - it follows the records that actually moved, on either kind of migration, and + is only ever escalated. Clearing it stays the job of the encumbrance lift sweep, which is the only + code that can see every record the practitioner still holds. + """ + merge_values = {} + if full_migration and old_provider_data is not None: + merge_values = { + field: value + for field, value in ( + ('militaryStatus', old_provider_data.militaryStatus), + ('militaryStatusNote', old_provider_data.militaryStatusNote), + ) + if value is not None + } + if not merge_values and not migrated_records_are_encumbered: + return None + + record_key = self._provider_record_key(existing_new_provider_record) + # providerDateOfUpdate backs a GSI and is normally derived from dateOfUpdate when a record is + # dumped through its schema. This write bypasses the schema, so both are set together to keep the + # index consistent with the record. + now = config.current_standard_datetime.isoformat() + set_expressions = [f'{field} = if_not_exists({field}, :{field})' for field in merge_values] + expression_values = {f':{field}': {'S': value} for field, value in merge_values.items()} + if migrated_records_are_encumbered: + # written unconditionally rather than with if_not_exists: escalating to encumbered is always + # correct here, and a record already reading encumbered is unchanged by it + set_expressions.append('encumberedStatus = :encumberedStatus') + expression_values[':encumberedStatus'] = {'S': LicenseEncumberedStatusEnum.ENCUMBERED} + set_expressions.extend(['dateOfUpdate = :dateOfUpdate', 'providerDateOfUpdate = :dateOfUpdate']) + expression_values[':dateOfUpdate'] = {'S': now} + + logger.info( + 'Merging fields onto the existing new provider record', + person_level_fields=sorted(merge_values), + setting_encumbered=migrated_records_are_encumbered, + ) + return { + 'Update': { + 'TableName': self.config.provider_table_name, + 'Key': {'pk': {'S': record_key['pk']}, 'sk': {'S': record_key['sk']}}, + 'UpdateExpression': 'SET ' + ', '.join(set_expressions), + 'ExpressionAttributeValues': expression_values, + } + } def _execute_batched_transactions(self, transaction_items: list[dict]) -> None: """ diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py index 224a030d86..d42fec2434 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py @@ -35,6 +35,28 @@ # them when an existing provider record is refreshed - see ProviderRecordUtility.populate_provider_record. PROVIDER_AGGREGATE_FIELDS = frozenset({'encumberedStatus'}) +# Fields describing the practitioner's CompactConnect *account* rather than the practitioner themselves: +# their registration, an in-flight email change, and an in-flight account recovery. A full SSN-correction +# migration deletes the old Cognito user and requires the practitioner to register again under the +# corrected provider id, so none of this may follow them onto the new provider record. +# +# currentHomeJurisdiction belongs here rather than with the person-level fields: registration sets it +# alongside the registered email address (see DataClient registration flow), and until it is set the +# provider is calculated as compact-ineligible. Carrying it would leave a provider that cannot sign in +# looking registered and eligible to purchase. +PROVIDER_ACCOUNT_STATE_FIELDS = frozenset( + { + 'compactConnectRegisteredEmailAddress', + 'currentHomeJurisdiction', + 'pendingEmailAddress', + 'emailVerificationCode', + 'emailVerificationExpiry', + 'recoveryToken', + 'recoveryExpiry', + } +) + + @BaseRecordSchema.register_schema('provider') class ProviderRecordSchema(BaseRecordSchema): diff --git a/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py b/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py index 513b257f5e..e1c9e8ed43 100644 --- a/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py +++ b/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py @@ -187,6 +187,65 @@ def _put_records_associated_with_remaining_license(self): } ) + def test_full_migration_carries_person_level_fields_onto_the_new_provider_record(self): + """Military status describes the practitioner, not the license. On a full migration the whole + practitioner moves - including the military affiliation records behind that status - so the + provider-level fields those flows maintain must move with them. Account state stays behind: the old + Cognito user is deleted and the practitioner has to register again, which is also what re-sets their + home jurisdiction selection. + """ + self._put_full_old_provider_records() + self.test_data_generator.put_default_provider_record_in_provider_table( + { + 'militaryStatus': 'approved', + 'militaryStatusNote': 'verified against submitted documentation', + 'currentHomeJurisdiction': DEFAULT_LICENSE_JURISDICTION, + } + ) + + result = self._migrate() + + self.assertTrue(result.full_migration) + new_provider_record = next( + record for record in self._get_all_records_for_provider(NEW_PROVIDER_ID) if record['type'] == 'provider' + ) + # the record must belong to the new provider id, not the one it was copied from + self.assertEqual(NEW_PROVIDER_ID, new_provider_record['providerId']) + self.assertEqual(f'{DEFAULT_COMPACT}#PROVIDER#{NEW_PROVIDER_ID}', new_provider_record['pk']) + self.assertEqual('approved', new_provider_record.get('militaryStatus')) + self.assertEqual('verified against submitted documentation', new_provider_record.get('militaryStatusNote')) + # the old account is torn down by a full migration, so its registration state must not follow - + # including the home jurisdiction selection, which registration sets and which gates eligibility + self.assertNotIn('compactConnectRegisteredEmailAddress', new_provider_record) + # the schema defaults an unset selection to 'unknown', which is what keeps the provider + # compact-ineligible until they register again + self.assertEqual('unknown', new_provider_record.get('currentHomeJurisdiction')) + + def test_partial_migration_does_not_carry_person_level_fields_to_the_new_provider(self): + """The mirror of the full-migration case. A partial migration leaves the person-level records + (military affiliations, provider update history) with the old provider, so the new provider must not + claim a military status that has no affiliation records behind it. + """ + self._put_full_old_provider_records() + self._put_records_associated_with_remaining_license() + self.test_data_generator.put_default_provider_record_in_provider_table( + {'militaryStatus': 'approved', 'militaryStatusNote': 'verified against submitted documentation'} + ) + + result = self._migrate() + + self.assertFalse(result.full_migration) + new_provider_record = next( + record for record in self._get_all_records_for_provider(NEW_PROVIDER_ID) if record['type'] == 'provider' + ) + self.assertNotIn('militaryStatus', new_provider_record) + self.assertNotIn('militaryStatusNote', new_provider_record) + # the old provider keeps them, along with the affiliation records that support them + old_provider_record = next( + record for record in self._get_all_records_for_provider(DEFAULT_PROVIDER_ID) if record['type'] == 'provider' + ) + self.assertEqual('approved', old_provider_record.get('militaryStatus')) + def test_partial_migration_moves_only_records_associated_with_target_license(self): """A partial migration must move ONLY the corrected license and its dependent records: the privileges purchased against it, and the adverse action / investigation / update history records of the license @@ -395,6 +454,117 @@ def test_migration_leaves_new_provider_pre_existing_records_untouched(self): new_provider_record = self._get_records_of_type(NEW_PROVIDER_ID, 'provider')[0] self.assertEqual(pre_existing_provider.serialize_to_database_record(), new_provider_record) + def test_military_status_carries_onto_an_existing_new_provider_record(self): + """Correcting a two-license practitioner takes two uploads: the first is a partial migration that + creates the new provider record from the license alone, the second is a full migration that finds + that record already present. The practitioner's military status has to survive the sequence, because + the second step deletes the old provider record that holds it. + """ + self._put_full_old_provider_records() + self._put_records_associated_with_remaining_license() + self.test_data_generator.put_default_provider_record_in_provider_table( + {'militaryStatus': 'approved', 'militaryStatusNote': 'verified against submitted documentation'} + ) + + # first correction: a partial migration, which creates the new provider record with no military data + first_result = self._migrate() + + self.assertFalse(first_result.full_migration) + self.assertNotIn('militaryStatus', self._get_records_of_type(NEW_PROVIDER_ID, 'provider')[0]) + + # second correction: the remaining license, so a full migration into the now-existing provider record + second_result = self._migrate(license_type=OTHER_LICENSE_TYPE) + + self.assertTrue(second_result.full_migration) + new_provider_record = self._get_records_of_type(NEW_PROVIDER_ID, 'provider')[0] + self.assertEqual('approved', new_provider_record.get('militaryStatus')) + self.assertEqual('verified against submitted documentation', new_provider_record.get('militaryStatusNote')) + + def test_existing_new_provider_military_status_is_never_overwritten(self): + """The new provider id may already belong to a practitioner who has been audited in their own right. + Their status wins: the migration fills absent fields, it does not replace decided ones. + """ + self.test_data_generator.put_default_provider_record_in_provider_table( + { + 'providerId': NEW_PROVIDER_ID, + 'militaryStatus': 'declined', + 'militaryStatusNote': 'documentation expired', + } + ) + self.test_data_generator.put_default_provider_record_in_provider_table( + {'militaryStatus': 'approved', 'militaryStatusNote': 'verified against submitted documentation'} + ) + self.test_data_generator.put_default_license_record_in_provider_table() + + result = self._migrate() + + self.assertTrue(result.full_migration) + new_provider_record = self._get_records_of_type(NEW_PROVIDER_ID, 'provider')[0] + self.assertEqual('declined', new_provider_record.get('militaryStatus')) + self.assertEqual('documentation expired', new_provider_record.get('militaryStatusNote')) + + def test_existing_new_provider_record_untouched_when_there_is_no_military_status_to_carry(self): + """With nothing to merge, the pre-existing record must not be written to at all.""" + pre_existing_provider = self.test_data_generator.put_default_provider_record_in_provider_table( + {'providerId': NEW_PROVIDER_ID, 'licenseJurisdiction': 'ky', 'privilegeJurisdictions': set()} + ) + self.test_data_generator.put_default_provider_record_in_provider_table() + self.test_data_generator.put_default_license_record_in_provider_table() + + self._migrate() + + self.assertEqual( + pre_existing_provider.serialize_to_database_record(), + self._get_records_of_type(NEW_PROVIDER_ID, 'provider')[0], + ) + + def test_new_provider_record_is_encumbered_when_a_migrated_record_has_an_unlifted_adverse_action(self): + """The provider-level encumbrance flag gates privilege purchasing, and it is an aggregate the + migration has to establish for itself: the records arriving under the new provider id carry the + encumbrance, and no other flow will notice they moved. + """ + self._put_full_old_provider_records() + self._put_records_associated_with_remaining_license() + + result = self._migrate() + + # a partial migration, so the new provider record is created from scratch + self.assertFalse(result.full_migration) + new_provider_record = self._get_records_of_type(NEW_PROVIDER_ID, 'provider')[0] + self.assertEqual('encumbered', new_provider_record.get('encumberedStatus')) + + def test_existing_new_provider_record_is_encumbered_by_a_migrated_unlifted_adverse_action(self): + """Same aggregate, but merged onto a provider record that already exists - the second half of a + two-step correction, where the created-from-scratch path never runs. + """ + self.test_data_generator.put_default_provider_record_in_provider_table({'providerId': NEW_PROVIDER_ID}) + self._put_full_old_provider_records() + + result = self._migrate() + + self.assertTrue(result.full_migration) + new_provider_record = self._get_records_of_type(NEW_PROVIDER_ID, 'provider')[0] + self.assertEqual('encumbered', new_provider_record.get('encumberedStatus')) + + def test_lifted_adverse_actions_do_not_encumber_the_new_provider_record(self): + """A lifted encumbrance is not an encumbrance. Only unlifted adverse actions count.""" + self.test_data_generator.put_default_provider_record_in_provider_table() + self.test_data_generator.put_default_license_record_in_provider_table() + self.test_data_generator.put_default_privilege_record_in_provider_table() + self.test_data_generator.put_default_adverse_action_record_in_provider_table( + { + 'actionAgainst': 'privilege', + 'jurisdiction': DEFAULT_PRIVILEGE_JURISDICTION, + 'effectiveLiftDate': date.fromisoformat('2024-10-01'), + } + ) + + result = self._migrate() + + self.assertTrue(result.full_migration) + new_provider_record = self._get_records_of_type(NEW_PROVIDER_ID, 'provider')[0] + self.assertNotEqual('encumbered', new_provider_record.get('encumberedStatus')) + def test_migration_raises_when_new_provider_record_created_concurrently(self): """The absent-check for the new provider's top-level record and the Put that creates one are not atomic with each other. If a concurrent write creates that record in between, the Put must be From ddca8ca14db507f74b42b711134fb009f9a47888 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Wed, 19 Aug 2026 23:54:38 -0500 Subject: [PATCH 11/27] Add guard for field additions to provider schema --- .../cc_common/data_model/data_client.py | 15 +- .../data_model/schema/provider/record.py | 16 +- .../test_schema/test_provider.py | 76 +++++++ .../tests/smoke/ssn_migration_smoke_tests.py | 214 +++++++++++++++++- 4 files changed, 304 insertions(+), 17 deletions(-) diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py index ceb4466dfe..35729e7d19 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py @@ -43,7 +43,10 @@ from cc_common.data_model.schema.privilege import PrivilegeData, PrivilegeUpdateData from cc_common.data_model.schema.privilege.record import PrivilegeUpdateRecordSchema from cc_common.data_model.schema.provider import ProviderData, ProviderUpdateData -from cc_common.data_model.schema.provider.record import PROVIDER_ACCOUNT_STATE_FIELDS +from cc_common.data_model.schema.provider.record import ( + PROVIDER_ACCOUNT_STATE_FIELDS, + PROVIDER_PERSON_LEVEL_FIELDS, +) from cc_common.data_model.update_tier_enum import UpdateTierEnum from cc_common.exceptions import ( CCAmbiguousLicenseNumberException, @@ -3653,13 +3656,11 @@ def _build_existing_provider_merge_item( """ merge_values = {} if full_migration and old_provider_data is not None: + old_provider_fields = old_provider_data.to_dict() merge_values = { - field: value - for field, value in ( - ('militaryStatus', old_provider_data.militaryStatus), - ('militaryStatusNote', old_provider_data.militaryStatusNote), - ) - if value is not None + field: old_provider_fields[field] + for field in sorted(PROVIDER_PERSON_LEVEL_FIELDS) + if old_provider_fields.get(field) is not None } if not merge_values and not migrated_records_are_encumbered: return None diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py index d42fec2434..2901dd8b56 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py @@ -44,6 +44,21 @@ # alongside the registered email address (see DataClient registration flow), and until it is set the # provider is calculated as compact-ineligible. Carrying it would leave a provider that cannot sign in # looking registered and eligible to purchase. +# Fields describing the practitioner rather than their licenses or their account. They are maintained by +# flows that have nothing to do with license uploads - the military file upload and audit flows - and +# cannot be rebuilt from any record a migration moves, so a full SSN-correction migration has to carry +# them onto the corrected provider id explicitly. +# +# A partial migration does not: the records that back these stay with the old provider, so a status +# carried across would have no supporting documentation behind it. +PROVIDER_PERSON_LEVEL_FIELDS = frozenset( + { + 'militaryStatus', + 'militaryStatusNote', + } +) + + PROVIDER_ACCOUNT_STATE_FIELDS = frozenset( { 'compactConnectRegisteredEmailAddress', @@ -57,7 +72,6 @@ ) - @BaseRecordSchema.register_schema('provider') class ProviderRecordSchema(BaseRecordSchema): """ diff --git a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_provider.py b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_provider.py index 97a55e49d9..0acbb4d2b2 100644 --- a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_provider.py +++ b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_provider.py @@ -119,3 +119,79 @@ def test_prov_date_of_update_matches_new_date_of_update(self): self.assertEqual(new_date_of_update.isoformat(), dumped_record['dateOfUpdate']) # If 1 and 2 happened out of order, `providerDateOfUpdate` will be incorrect self.assertEqual(new_date_of_update.isoformat(), dumped_record['providerDateOfUpdate']) + + +class TestProviderRecordFieldOwnership(TstLambdas): + """ + Guards the classification behind the SSN-correction migration's handling of the top-level provider + record. + + That record is rebuilt rather than moved, from the corrected license plus whatever the migration + decides to carry across. Anything the license cannot supply and nobody classified is therefore dropped + silently - which is how the practitioner's military audit status and note went missing through a + correction. This test fails if a new provider field is added without deciding which of those it is. + """ + + # Populated by ProviderRecordSchema's pre_dump hooks from other fields on the record. + GENERATED_ON_WRITE = {'birthMonthDay', 'providerFamGivMid', 'providerDateOfUpdate'} + # Derived by ProviderRecordUtility.populate_provider_record from the practitioner's own records, so + # they are rebuilt rather than carried. + DERIVED_FROM_RECORDS = {'licenseJurisdiction', 'privilegeJurisdictions'} + + def test_every_provider_field_the_license_cannot_supply_is_classified(self): + from cc_common.data_model.schema.license.record import LicenseRecordSchema + from cc_common.data_model.schema.provider.record import ( + PROVIDER_ACCOUNT_STATE_FIELDS, + PROVIDER_PERSON_LEVEL_FIELDS, + ProviderRecordSchema, + ) + + not_suppliable_by_the_license = set(ProviderRecordSchema().fields) - set(LicenseRecordSchema().fields) + unclassified = ( + not_suppliable_by_the_license + - self.GENERATED_ON_WRITE + - self.DERIVED_FROM_RECORDS + - PROVIDER_ACCOUNT_STATE_FIELDS + - PROVIDER_PERSON_LEVEL_FIELDS + ) + + self.assertEqual( + set(), + unclassified, + f'New provider record field(s) {sorted(unclassified)} cannot be supplied by the license that a ' + 'migration rebuilds the provider record from, so an SSN correction will silently drop them. ' + 'Decide which they are and add them to the right place: PROVIDER_PERSON_LEVEL_FIELDS (in ' + 'schema/provider/record.py) if they describe the practitioner and must follow them to the ' + 'corrected provider id, PROVIDER_ACCOUNT_STATE_FIELDS if they belong to the CompactConnect ' + 'account the migration tears down, or one of the GENERATED_ON_WRITE / DERIVED_FROM_RECORDS sets ' + 'in this test if the value is rebuilt on every write.', + ) + + def test_person_level_and_account_state_fields_are_disjoint(self): + """A field cannot both follow the practitioner and stay with the account they are leaving.""" + from cc_common.data_model.schema.provider.record import ( + PROVIDER_ACCOUNT_STATE_FIELDS, + PROVIDER_PERSON_LEVEL_FIELDS, + ) + + self.assertEqual(set(), PROVIDER_PERSON_LEVEL_FIELDS & PROVIDER_ACCOUNT_STATE_FIELDS) + + def test_classified_fields_all_exist_on_the_record(self): + """A classification naming a field the schema does not have is dead weight, and usually a rename + that was only half applied. + """ + from cc_common.data_model.schema.provider.record import ( + PROVIDER_ACCOUNT_STATE_FIELDS, + PROVIDER_PERSON_LEVEL_FIELDS, + ProviderRecordSchema, + ) + + provider_fields = set(ProviderRecordSchema().fields) + classified = ( + PROVIDER_ACCOUNT_STATE_FIELDS + | PROVIDER_PERSON_LEVEL_FIELDS + | self.GENERATED_ON_WRITE + | self.DERIVED_FROM_RECORDS + ) + + self.assertEqual(set(), classified - provider_fields) diff --git a/backend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py b/backend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py index 591a53cb4a..1f8f2b76a9 100644 --- a/backend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py +++ b/backend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py @@ -107,6 +107,20 @@ # normalized (not dropped) so that provider-id-derived fields still participate in the comparison. _VOLATILE_RECORD_FIELDS = ('pk', 'sk', 'dateOfUpdate', 'providerDateOfUpdate', 'ssnLastFour') +# Fields on the top-level provider record that a FULL migration is expected to drop, because they describe +# the practitioner's CompactConnect account rather than the practitioner: the old Cognito user is deleted +# and they must register again under the corrected provider id, which is what re-sets these. Everything +# else on the record has to survive the migration unchanged - that is what this list makes checkable. +_PROVIDER_ACCOUNT_STATE_FIELDS = ( + 'compactConnectRegisteredEmailAddress', + 'currentHomeJurisdiction', + 'pendingEmailAddress', + 'emailVerificationCode', + 'emailVerificationExpiry', + 'recoveryToken', + 'recoveryExpiry', +) + _MIGRATION_WAIT_SECONDS = 900 _POLL_INTERVAL_SECONDS = 30 @@ -221,21 +235,86 @@ def _normalized_migratable_records(records: list[dict], provider_id: str) -> dic return normalized +def _describe_record_differences(source_canonical: str, target_canonical: str) -> str: + """Describe field-by-field how two canonicalized records differ, for a readable failure message.""" + source_fields = json.loads(source_canonical) + target_fields = json.loads(target_canonical) + differences = [] + for field in sorted(set(source_fields) | set(target_fields)): + before = source_fields.get(field, '') + after = target_fields.get(field, '') + if before != after: + differences.append(f'{field}: before={before!r} after={after!r}') + return '; '.join(differences) + + def _verify_all_records_migrated( *, source_records: list[dict], source_provider_id: str, target_records: list[dict], target_provider_id: str ): - """Verify every migratable record captured from the source provider now exists under the target provider.""" + """Verify every migratable record captured from the source provider now exists under the target provider, + field for field. + """ source_normalized = _normalized_migratable_records(source_records, source_provider_id) - target_normalized = set(_normalized_migratable_records(target_records, target_provider_id).values()) + target_normalized = _normalized_migratable_records(target_records, target_provider_id) + + problems = [] + for label, source_canonical in source_normalized.items(): + target_canonical = target_normalized.get(label) + if target_canonical is None: + problems.append(f'{label}: no matching record under provider {target_provider_id}') + elif target_canonical != source_canonical: + problems.append(f'{label}: {_describe_record_differences(source_canonical, target_canonical)}') - missing_records = [label for label, canonical in source_normalized.items() if canonical not in target_normalized] - if missing_records: + if problems: raise SmokeTestFailureException( - f'The following records were not migrated to provider {target_provider_id}: {missing_records}' + f'The following records did not survive migration to provider {target_provider_id} intact:\n ' + + '\n '.join(problems) ) print(f'Verified all {len(source_normalized)} migratable records now exist under provider {target_provider_id}') +def _verify_provider_record_migrated( + *, source_records: list[dict], source_provider_id: str, target_records: list[dict], target_provider_id: str +): + """Verify the top-level provider record survived a full migration intact. + + The top-level record is rebuilt rather than moved, so it is excluded from the record-for-record + comparison above - which is exactly why fields have been able to disappear from it unnoticed (the + practitioner's military audit status and note, for instance). Everything on it has to come through a + full migration unchanged apart from the provider id, the corrected ssnLastFour, and the account state + the practitioner re-establishes when they register again. + """ + source_provider_record = next(record for record in source_records if record['type'] == 'provider') + target_provider_record = next((record for record in target_records if record['type'] == 'provider'), None) + if target_provider_record is None: + raise SmokeTestFailureException(f'No top-level provider record was created for {target_provider_id}') + + ignored_fields = (*_VOLATILE_RECORD_FIELDS, *_PROVIDER_ACCOUNT_STATE_FIELDS) + + def _canonical(record: dict, provider_id: str) -> str: + scrubbed = {key: value for key, value in record.items() if key not in ignored_fields} + return json.dumps(scrubbed, sort_keys=True, default=str).replace(provider_id, '') + + source_canonical = _canonical(source_provider_record, source_provider_id) + target_canonical = _canonical(target_provider_record, target_provider_id) + if source_canonical != target_canonical: + raise SmokeTestFailureException( + f'The top-level provider record did not survive migration to {target_provider_id} intact: ' + f'{_describe_record_differences(source_canonical, target_canonical)}' + ) + + # the account state is expected to be gone, and that expectation is worth asserting rather than + # merely ignoring: a full migration deletes the Cognito user, so a record that still looked registered + # would be its own bug + still_registered = [field for field in _PROVIDER_ACCOUNT_STATE_FIELDS if field in target_provider_record] + if still_registered: + raise SmokeTestFailureException( + f'The migrated provider record for {target_provider_id} still carries account state that a full ' + f'migration should have left behind: {still_registered}' + ) + print(f'Verified the top-level provider record migrated intact to provider {target_provider_id}') + + def _get_privilege_transaction_ids(records: list[dict]) -> set[str]: """Collect every payment transaction id referenced by a provider's privilege records. @@ -317,6 +396,67 @@ def _verify_transactions_attributed_to_provider(*, compact: str, provider_id: st ) +def _set_provider_military_status(compact: str, provider_id: str, status: str, note: str): + """Stamp a military audit result onto a provider record. + + The mock practitioner these partial-migration tests build has no military documentation of their own, + so the state a partial migration has to leave alone is written here directly. militaryStatus and + militaryStatusNote live only on the top-level provider record and are set by the military file upload + and audit flows; nothing about them is tied to Cognito, so setting them straight on the record is a + faithful stand-in for an audited practitioner. + """ + dynamo_table = get_provider_user_dynamodb_table() + dynamo_table.update_item( + Key={'pk': f'{compact}#PROVIDER#{provider_id}', 'sk': f'{compact}#PROVIDER'}, + UpdateExpression='SET militaryStatus = :status, militaryStatusNote = :note', + ExpressionAttributeValues={':status': status, ':note': note}, + ) + print(f'Set militaryStatus={status} on provider {provider_id} for the partial migration test') + + +def _verify_records_left_behind_are_untouched( + *, source_records: list[dict], target_records: list[dict], migrated_license_type: str +): + """Verify a partial migration changed nothing belonging to the license that stayed. + + This is the property that separates a partial migration from a full one: only the corrected license's + records may be touched. Everything else under the old provider id has to come out byte for byte + identical - same provider id, same values, same dateOfUpdate, because those records should not have + been written at all. + + The top-level provider record is excluded: a partial migration deliberately rebuilds it from the + licenses that remain, so it is expected to change. It is checked separately. + """ + + def _keyed(records: list[dict]) -> dict[str, dict]: + return { + f'{record["type"]}: {record["sk"]}': record + for record in records + if record['type'] != 'provider' and record.get('licenseType') != migrated_license_type + } + + expected_records = _keyed(source_records) + actual_records = _keyed(target_records) + + problems = [] + for label, expected_record in expected_records.items(): + actual_record = actual_records.get(label) + if actual_record is None: + problems.append(f'{label}: no longer present under the old provider id') + elif actual_record != expected_record: + differences = _describe_record_differences( + json.dumps(expected_record, sort_keys=True, default=str), + json.dumps(actual_record, sort_keys=True, default=str), + ) + problems.append(f'{label}: {differences}') + if problems: + raise SmokeTestFailureException( + 'A partial migration modified records belonging to the license that stayed behind:\n ' + + '\n '.join(problems) + ) + print(f'Verified all {len(expected_records)} record(s) for the remaining license were left untouched') + + def _verify_license_ssn_last_four(*, records: list[dict], expected_ssn_last_four: str, license_type: str | None = None): """Verify every license record (optionally filtered to a single license type) carries the expected ssnLastFour. This is checked separately from _verify_all_records_migrated because ssnLastFour is @@ -639,8 +779,9 @@ def test_full_ssn_migration_roundtrip(): Step 1: Capture the test provider's baseline state (all DynamoDB records + all S3 objects). Step 2: Upload a fresh military affiliation document so there is a recent document to migrate. Step 3: Upload the provider's license with a corrected SSN and previousSSN set to their current mock SSN. - Step 4: Wait for the migration, then verify every record and S3 object moved to the new provider id, - and that the transactions the provider's privileges were purchased with are attributed to it. + Step 4: Wait for the migration, then verify every record and S3 object moved to the new provider id - + including the top-level provider record, field for field - and that the transactions the + provider's privileges were purchased with are attributed to it. Step 5: Migrate back to the original SSN (roundtrip) and verify everything - records, documents, and transaction attribution - returned to the original provider id and the intermediate provider id was cleaned up. @@ -716,6 +857,12 @@ def test_full_ssn_migration_roundtrip(): target_records=migrated_records, target_provider_id=migrated_provider_id, ) + _verify_provider_record_migrated( + source_records=pre_migration_records, + source_provider_id=original_provider_id, + target_records=migrated_records, + target_provider_id=migrated_provider_id, + ) if not any( record['type'] == 'providerUpdate' and record.get('updateType') == 'ssnCorrection' for record in migrated_records @@ -758,6 +905,12 @@ def test_full_ssn_migration_roundtrip(): target_records=returned_records, target_provider_id=original_provider_id, ) + _verify_provider_record_migrated( + source_records=pre_migration_records, + source_provider_id=original_provider_id, + target_records=returned_records, + target_provider_id=original_provider_id, + ) _verify_license_ssn_last_four( records=returned_records, expected_ssn_last_four=config.test_provider_mock_ssn[-4:] ) @@ -829,8 +982,9 @@ def test_partial_ssn_migration(): Step 1: Upload OT + OTA licenses under the same mock SSN and wait for the provider to be created. Step 2: Re-upload the OT license with a corrected SSN and previousSSN set to the original mock SSN. - Step 3: Verify the OT license now lives under a new provider id with its own top-level provider record, - while the OTA license and provider record remain under the old provider id. + Step 3: Verify the OT license now lives under a new provider id with its own top-level provider record + and arrived intact, while the OTA license and provider record remain under the old provider id + completely untouched - including the person-level state a partial migration must not move. Step 4: Clean up all DynamoDB records for both provider ids. """ test_staff_user_sub = create_test_staff_user( @@ -871,6 +1025,14 @@ def test_partial_ssn_migration(): ), ) + # Give the practitioner an audited military status, so there is person-level provider state for the + # migration to preserve, then snapshot everything before the correction + _set_provider_military_status( + PARTIAL_MIGRATION_COMPACT, old_provider_id, 'approved', 'verified for the partial migration test' + ) + pre_migration_records = _get_provider_dynamo_records(PARTIAL_MIGRATION_COMPACT, old_provider_id) + print(f'Captured {len(pre_migration_records)} pre-migration records under provider {old_provider_id}') + # Step 2: correct the SSN on the OT license only _upload_license_records( client_headers, @@ -909,6 +1071,21 @@ def _find_new_provider_id(): expected_ssn_last_four=PARTIAL_MIGRATION_ORIGINAL_SSN[-4:], license_type=OTA_LICENSE_TYPE, ) + # nothing belonging to the license that stayed may have been touched + _verify_records_left_behind_are_untouched( + source_records=pre_migration_records, + target_records=old_provider_records, + migrated_license_type=OT_LICENSE_TYPE, + ) + # a partial migration leaves the practitioner in place on the old provider id: their person-level + # state stays with the records that back it, rather than following the corrected license + old_provider_record = next(record for record in old_provider_records if record['type'] == 'provider') + if old_provider_record.get('militaryStatus') != 'approved': + raise SmokeTestFailureException( + f'The old provider record lost its military status during a partial migration: ' + f'militaryStatus={old_provider_record.get("militaryStatus")!r}, ' + f'militaryStatusNote={old_provider_record.get("militaryStatusNote")!r}' + ) print(f'Verified the OTA license and provider record remain under old provider {old_provider_id}') new_provider_records = _get_provider_dynamo_records(PARTIAL_MIGRATION_COMPACT, new_provider_id) @@ -926,6 +1103,25 @@ def _find_new_provider_id(): expected_ssn_last_four=PARTIAL_MIGRATION_CORRECTED_SSN[-4:], license_type=OT_LICENSE_TYPE, ) + # the military affiliation records stay with the old provider on a partial migration, so a status + # carried across would have no supporting documentation behind it. This is a deliberate omission - + # assert it, so that changing it later is a decision rather than an accident + new_provider_record = next(record for record in new_provider_records if record['type'] == 'provider') + inherited_person_level_fields = [ + field for field in ('militaryStatus', 'militaryStatusNote') if field in new_provider_record + ] + if inherited_person_level_fields: + raise SmokeTestFailureException( + f'The new provider record inherited person-level fields that a partial migration should ' + f'leave with the old provider: {inherited_person_level_fields}' + ) + # and the OT license's records must have arrived intact, not just present + _verify_all_records_migrated( + source_records=[record for record in pre_migration_records if record.get('licenseType') == OT_LICENSE_TYPE], + source_provider_id=old_provider_id, + target_records=new_provider_records, + target_provider_id=new_provider_id, + ) print(f'Verified the OT license and a new provider record exist under new provider {new_provider_id}') print('Partial migration smoke test passed.') finally: From b478936f9bfa70cc4c2a0ed2c8adf431d3f16cdc Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Wed, 19 Aug 2026 23:54:49 -0500 Subject: [PATCH 12/27] refactor --- .../python/common/cc_common/data_model/data_client.py | 8 +++----- .../common/tests/unit/test_data_model/test_data_client.py | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py index 35729e7d19..8718709a0d 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py @@ -3237,7 +3237,7 @@ def migrate_provider_for_ssn_correction( # already point at new_provider_id while the provider records are still under previous_provider_id, # so a report generated in that window renders the practitioner as UNKNOWN - briefly, the same # symptom this re-pointing exists to remove. The SQS retry closes it by completing the migration. - payment_transaction_ids = self._collect_transaction_ids(records_to_move) + payment_transaction_ids = self._collect_payment_transaction_ids(records_to_move) if payment_transaction_ids: self.config.transaction_client.update_licensee_id_for_transactions( compact=compact, @@ -3275,9 +3275,7 @@ def migrate_provider_for_ssn_correction( migration_performed=True, full_migration=full_migration, old_provider_registered_email=( - old_top_level_provider_data.to_dict().get('compactConnectRegisteredEmailAddress') - if full_migration - else None + old_top_level_provider_data.compactConnectRegisteredEmailAddress if full_migration else None ), ) @@ -3296,7 +3294,7 @@ def _migrated_records_are_encumbered(records_to_move: list[CCDataClass]) -> bool ) @staticmethod - def _collect_transaction_ids(records: list[CCDataClass]) -> set[str]: + def _collect_payment_transaction_ids(records: list[CCDataClass]) -> set[str]: """ Collect every payment transaction id referenced by the privilege records being migrated. diff --git a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py index 984d006998..0204266436 100644 --- a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py +++ b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py @@ -56,7 +56,7 @@ class TestCollectTransactionIds(TstLambdas): def setUp(self): from cc_common.data_model.data_client import DataClient - self.collect = DataClient._collect_transaction_ids # noqa: SLF001 protected-access + self.collect = DataClient._collect_payment_transaction_ids # noqa: SLF001 protected-access def test_collects_the_privilege_records_transaction_id(self): privilege = self.test_data_generator.generate_default_privilege({'compactTransactionId': 'tx-current'}) From 51e48663cb96e7a05eec00cda1b35992d0becaa6 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Thu, 20 Aug 2026 00:34:22 -0500 Subject: [PATCH 13/27] Enhance ssn migration smoke tests to check for top level provider fields --- .../tests/smoke/ssn_migration_smoke_tests.py | 134 ++++++++++++++---- 1 file changed, 109 insertions(+), 25 deletions(-) diff --git a/backend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py b/backend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py index 1f8f2b76a9..475adb74b4 100644 --- a/backend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py +++ b/backend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py @@ -121,6 +121,12 @@ 'recoveryExpiry', ) +# Most account-state fields simply disappear from a migrated record. currentHomeJurisdiction does not: its +# schema field carries a load_default, so it is filled in whenever a record is loaded and written back on +# every dump. What a migration resets is its *value*, and 'unknown' is the assertion that matters - that is +# the value which keeps the migrated provider compact-ineligible until the practitioner registers again. +_PROVIDER_ACCOUNT_STATE_RESET_VALUES = {'currentHomeJurisdiction': 'unknown'} + _MIGRATION_WAIT_SECONDS = 900 _POLL_INTERVAL_SECONDS = 30 @@ -211,7 +217,24 @@ def _get_provider_s3_objects(compact: str, provider_id: str) -> dict[str, bytes] return objects -def _normalized_migratable_records(records: list[dict], provider_id: str) -> dict[str, str]: +def _stable_record_key(record: dict, provider_id: str) -> str: + """A record identity that survives a migration. + + Every sort key this system writes is stable across a migration except for update records, whose final + segment is a hash over the record's `previous` snapshot - and that snapshot carries the provider id, so + re-keying a record always changes it (see ChangeHashMixin.hash_changes). Dropping that one segment + leaves the scope and the createDate, neither of which a migration touches. + + The hash exists to separate updates made within the same second for the same scope, so this key is not + guaranteed unique. Callers group by it rather than assuming one record per key. + """ + sort_key = record['sk'].replace(provider_id, '') + if '#UPDATE#' in sort_key: + sort_key = sort_key.rsplit('/', 1)[0] + return f'{record["type"]}: {sort_key}' + + +def _normalized_migratable_records(records: list[dict], provider_id: str) -> dict[str, list[str]]: """Canonicalize a provider's records for comparison across provider ids. Each record is serialized with its provider id replaced by a placeholder (which also normalizes @@ -230,8 +253,7 @@ def _normalized_migratable_records(records: list[dict], provider_id: str) -> dic continue scrubbed = {key: value for key, value in record.items() if key not in _VOLATILE_RECORD_FIELDS} canonical = json.dumps(scrubbed, sort_keys=True, default=str).replace(provider_id, '') - # key by something readable for failure messages - normalized[f'{record["type"]}: {record["sk"].replace(provider_id, "")}'] = canonical + normalized.setdefault(_stable_record_key(record, provider_id), []).append(canonical) return normalized @@ -253,24 +275,34 @@ def _verify_all_records_migrated( ): """Verify every migratable record captured from the source provider now exists under the target provider, field for field. + + Records are paired on the stable part of their sort key (see _stable_record_key) and compared on + content, so a record that changed is reported as a field-level diff rather than as a missing record. + Keys are grouped because an update record's key is not guaranteed unique. """ source_normalized = _normalized_migratable_records(source_records, source_provider_id) target_normalized = _normalized_migratable_records(target_records, target_provider_id) problems = [] - for label, source_canonical in source_normalized.items(): - target_canonical = target_normalized.get(label) - if target_canonical is None: - problems.append(f'{label}: no matching record under provider {target_provider_id}') - elif target_canonical != source_canonical: - problems.append(f'{label}: {_describe_record_differences(source_canonical, target_canonical)}') + source_record_count = 0 + for record_key, source_canonicals in source_normalized.items(): + unmatched_target_canonicals = list(target_normalized.get(record_key, [])) + for source_canonical in source_canonicals: + source_record_count += 1 + if source_canonical in unmatched_target_canonicals: + unmatched_target_canonicals.remove(source_canonical) + elif unmatched_target_canonicals: + differences = _describe_record_differences(source_canonical, unmatched_target_canonicals.pop(0)) + problems.append(f'{record_key}: {differences}') + else: + problems.append(f'{record_key}: no matching record under provider {target_provider_id}') if problems: raise SmokeTestFailureException( f'The following records did not survive migration to provider {target_provider_id} intact:\n ' + '\n '.join(problems) ) - print(f'Verified all {len(source_normalized)} migratable records now exist under provider {target_provider_id}') + print(f'Verified all {source_record_count} migratable records now exist under provider {target_provider_id}') def _verify_provider_record_migrated( @@ -303,14 +335,22 @@ def _canonical(record: dict, provider_id: str) -> str: f'{_describe_record_differences(source_canonical, target_canonical)}' ) - # the account state is expected to be gone, and that expectation is worth asserting rather than - # merely ignoring: a full migration deletes the Cognito user, so a record that still looked registered - # would be its own bug - still_registered = [field for field in _PROVIDER_ACCOUNT_STATE_FIELDS if field in target_provider_record] - if still_registered: + # the account state is expected to be reset, and that expectation is worth asserting rather than merely + # ignoring: a full migration deletes the Cognito user, so a record that still looked registered would be + # its own bug + carried_over = [] + for field in _PROVIDER_ACCOUNT_STATE_FIELDS: + expected_reset_value = _PROVIDER_ACCOUNT_STATE_RESET_VALUES.get(field) + actual_value = target_provider_record.get(field, '') + if expected_reset_value is None: + if field in target_provider_record: + carried_over.append(f'{field}={actual_value!r}, expected it to be dropped') + elif actual_value != expected_reset_value: + carried_over.append(f'{field}={actual_value!r}, expected {expected_reset_value!r}') + if carried_over: raise SmokeTestFailureException( f'The migrated provider record for {target_provider_id} still carries account state that a full ' - f'migration should have left behind: {still_registered}' + f'migration should have reset: {carried_over}' ) print(f'Verified the top-level provider record migrated intact to provider {target_provider_id}') @@ -430,7 +470,7 @@ def _verify_records_left_behind_are_untouched( def _keyed(records: list[dict]) -> dict[str, dict]: return { - f'{record["type"]}: {record["sk"]}': record + _stable_record_key(record, ''): record for record in records if record['type'] != 'provider' and record.get('licenseType') != migrated_license_type } @@ -608,6 +648,24 @@ def _find_new_provider_id(): return new_provider_id +def _expected_registration_values(provider_record: dict) -> dict: + """The registration values the shared test provider account is expected to carry. + + These cannot be read back off a record that has just been migrated: a migration deliberately strips + account state, so a migrated record has none to give. Restoring from one restores nothing and leaves the + provider unregistered - which then makes the next full migration skip the Cognito deletion that the + roundtrip exists to exercise, silently weakening the test. + + Both values are knowable without a baseline: the registered email address is the Cognito username by + definition, and the home jurisdiction is the jurisdiction of the license the provider record was built + from (which is what registration sets it to). + """ + return { + 'compactConnectRegisteredEmailAddress': config.test_provider_user_username, + 'currentHomeJurisdiction': provider_record['licenseJurisdiction'], + } + + def _restore_test_provider_account(compact: str, provider_id: str, baseline_provider_record: dict): """Restore the shared test provider account after a full migration deleted its Cognito user. @@ -666,9 +724,17 @@ def _restore_test_provider_account(compact: str, provider_id: str, baseline_prov # over, if they are not already present - independent of whether the Cognito user needed recreating provider_key = {'pk': f'{compact}#PROVIDER#{provider_id}', 'sk': f'{compact}#PROVIDER'} current_provider_record = get_provider_user_dynamodb_table().get_item(Key=provider_key).get('Item', {}) + restorable_fields = ('compactConnectRegisteredEmailAddress', 'currentHomeJurisdiction') + missing_from_baseline = [field for field in restorable_fields if field not in baseline_provider_record] + if missing_from_baseline: + # a baseline taken from an already-migrated record cannot restore what the migration stripped + print( + f'WARNING: no baseline value for {missing_from_baseline}, so provider record {provider_id} will ' + f'be left unregistered for those fields' + ) registration_fields = { field: baseline_provider_record[field] - for field in ('compactConnectRegisteredEmailAddress', 'currentHomeJurisdiction') + for field in restorable_fields if field in baseline_provider_record and current_provider_record.get(field) != baseline_provider_record[field] } if registration_fields: @@ -765,7 +831,12 @@ def _recover_stranded_test_provider(): for record in _get_provider_dynamo_records(TEST_COMPACT, config.test_provider_original_provider_id) if record['type'] == 'provider' ) - _restore_test_provider_account(TEST_COMPACT, config.test_provider_original_provider_id, recovered_provider_record) + # the recovered record has just been through a migration, so it carries no registration state of its own + _restore_test_provider_account( + TEST_COMPACT, + config.test_provider_original_provider_id, + _expected_registration_values(recovered_provider_record), + ) print('Recovery complete: records and Cognito account are back under the original provider id.') @@ -785,8 +856,9 @@ def test_full_ssn_migration_roundtrip(): Step 5: Migrate back to the original SSN (roundtrip) and verify everything - records, documents, and transaction attribution - returned to the original provider id and the intermediate provider id was cleaned up. - Step 6: Restore the test provider's Cognito account (deleted by the full migration) and registration - fields so the shared test account remains usable. + Step 6: On success, restore the test provider's Cognito account (deleted by the full migration) and + registration fields so the shared test account remains usable. On failure the account is left + alone, so the next run's recovery step can repair it. """ _recover_stranded_test_provider() @@ -922,11 +994,23 @@ def test_full_ssn_migration_roundtrip(): compact=compact, provider_id=original_provider_id, records=returned_records ) print('Roundtrip migration completed; all records and documents are back under the original provider id') - finally: - # Restore the shared test provider account no matter what state the test failed in: point the - # Cognito user at whichever provider id currently holds the provider's records (last_known_provider_id, - # tracked above - see its comment for why this is reliable without an extra lookup here). + + # Step 6: restore the shared test provider account. This runs only on success. + # A full migration deletes the Cognito user; recreating it after a failure points it at whichever + # provider id the records happened to reach, and 'custom:providerId' is immutable, so the next run + # cannot repair it. Leaving the account deleted is the state _recover_stranded_test_provider is + # built to detect and repair, which makes a re-run self-service. _restore_test_provider_account(compact, last_known_provider_id, baseline_provider_record) + except Exception: + print( + f'Test failed with the provider records under provider id {last_known_provider_id}. The Cognito ' + f'account has been left as-is rather than repointed; re-running will detect and recover this ' + f'state (supply {last_known_provider_id} when prompted). If the account still exists but is ' + f'bound to a different provider id, delete the Cognito user {config.test_provider_user_username} ' + f'first, since custom:providerId cannot be changed.' + ) + raise + finally: delete_test_staff_user(TEST_STAFF_USER_EMAIL, user_sub=test_staff_user_sub, compact=compact) delete_test_app_client(test_app_client_id) From 737dc202341e911449e3c29029d7f2858638d662 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Thu, 20 Aug 2026 00:40:47 -0500 Subject: [PATCH 14/27] Feedback - improve test assertions --- .../function/test_handlers/test_ingest.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py b/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py index b2f79fcfbb..021deae33f 100644 --- a/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py +++ b/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py @@ -534,9 +534,9 @@ def test_existing_provider_removed_email(self): def _get_all_records(self, provider_id: str) -> list[dict]: from boto3.dynamodb.conditions import Key - return self.config.provider_table.query(KeyConditionExpression=Key('pk').eq(f'aslp#PROVIDER#{provider_id}'))[ - 'Items' - ] + return self.config.provider_table.query( + KeyConditionExpression=Key('pk').eq(f'aslp#PROVIDER#{provider_id}'), ConsistentRead=True + )['Items'] def _get_license_record(self, provider_id: str) -> dict: return next(record for record in self._get_all_records(provider_id) if record['type'] == 'license') @@ -594,6 +594,11 @@ def test_existing_license_investigation_status_survives_a_reupload(self): def test_preserved_license_fields_are_not_reported_as_removed(self): """The update history must reflect what actually happened: these fields were carried forward, not removed, so they must not show up in the update record's removedValues. + + The re-upload also changes an upload-owned field (phoneNumber), so this exercises an update record + that actually gets created - an unchanged re-upload creates none at all (see + test_unchanged_reupload_of_a_preserved_license_creates_no_update_record), which would make the + removedValues assertions below vacuously true. """ from handlers.ingest import ingest_license_message @@ -603,15 +608,17 @@ def test_preserved_license_fields_are_not_reported_as_removed(self): with open('../common/tests/resources/ingest/event-bridge-message.json') as f: message = json.load(f) + message['detail']['phoneNumber'] = '+13213214322' event = {'Records': [{'messageId': '123', 'body': json.dumps(message)}]} self.assertEqual({'batchItemFailures': []}, ingest_license_message(event, self.mock_context)) provider_user_records = self.config.data_client.get_provider_user_records( compact='aslp', provider_id=provider_id, include_update_tier=UpdateTierEnum.TIER_THREE ) - for update_record in provider_user_records._license_update_records: # noqa: SLF001 - self.assertNotIn('encumberedStatus', update_record.to_dict().get('removedValues', [])) - self.assertNotIn('investigationStatus', update_record.to_dict().get('removedValues', [])) + self.assertEqual(1, len(provider_user_records._license_update_records)) # noqa: SLF001 + update_record = provider_user_records._license_update_records[0] # noqa: SLF001 + self.assertNotIn('encumberedStatus', update_record.to_dict().get('removedValues', [])) + self.assertNotIn('investigationStatus', update_record.to_dict().get('removedValues', [])) def test_unchanged_reupload_of_a_preserved_license_creates_no_update_record(self): """A re-upload that changes nothing must not leave an empty update record in the practitioner's From b8f9df2fc07cbc73479451c23bd04c3958b2b0c4 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Thu, 20 Aug 2026 00:52:38 -0500 Subject: [PATCH 15/27] Feedback - add condition expression for update and improve smoke test logs --- .../cc_common/data_model/data_client.py | 1 + .../test_data_client_ssn_correction.py | 28 +++++++++++++++++++ .../tests/smoke/ssn_migration_smoke_tests.py | 5 ++-- 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py index 8718709a0d..6ac6d1fd1c 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py @@ -3689,6 +3689,7 @@ def _build_existing_provider_merge_item( 'Key': {'pk': {'S': record_key['pk']}, 'sk': {'S': record_key['sk']}}, 'UpdateExpression': 'SET ' + ', '.join(set_expressions), 'ExpressionAttributeValues': expression_values, + 'ConditionExpression': 'attribute_exists(pk)', } } diff --git a/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py b/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py index e1c9e8ed43..e4ccc59e75 100644 --- a/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py +++ b/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py @@ -600,6 +600,34 @@ def _stale_absent_check_for_new_provider(*, compact, provider_id): self.assertEqual(1, len(new_provider_records)) self.assertEqual(pre_existing_provider.serialize_to_database_record(), new_provider_records[0]) + def test_migration_raises_when_new_provider_record_deleted_concurrently(self): + """The mirror of the create-branch race above: the existence check confirms the new provider's + top-level record is present, but the merge Update that follows is not atomic with that check. If a + concurrent write deletes the record in between, the merge must be conditioned on the record still + being present - otherwise DynamoDB's default UpdateItem behavior creates a new item from just the + SET clause, leaving a malformed provider record (missing providerId, compact, and everything else a + real record needs) rather than failing loudly for a retry to fix. + """ + self.test_data_generator.put_default_provider_record_in_provider_table({'providerId': NEW_PROVIDER_ID}) + self._put_full_old_provider_records() + + real_transact_write_items = self.config.dynamodb_client.transact_write_items + + def _delete_new_provider_record_then_commit(**kwargs): + self.config.provider_table.delete_item( + Key={'pk': f'{DEFAULT_COMPACT}#PROVIDER#{NEW_PROVIDER_ID}', 'sk': f'{DEFAULT_COMPACT}#PROVIDER'} + ) + return real_transact_write_items(**kwargs) + + with patch.object( + self.config.dynamodb_client, 'transact_write_items', side_effect=_delete_new_provider_record_then_commit + ): + with self.assertRaises(CCInternalException): + self._migrate() + + # the conditioned Update failed, so no stub record was recreated + self.assertEqual([], self._get_records_of_type(NEW_PROVIDER_ID, 'provider')) + def test_no_op_when_old_provider_has_no_matching_license(self): self.test_data_generator.put_default_provider_record_in_provider_table() self.test_data_generator.put_default_license_record_in_provider_table({'licenseType': OTHER_LICENSE_TYPE}) diff --git a/backend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py b/backend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py index 475adb74b4..fb26200b9a 100644 --- a/backend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py +++ b/backend/compact-connect/tests/smoke/ssn_migration_smoke_tests.py @@ -455,7 +455,7 @@ def _set_provider_military_status(compact: str, provider_id: str, status: str, n def _verify_records_left_behind_are_untouched( - *, source_records: list[dict], target_records: list[dict], migrated_license_type: str + *, source_records: list[dict], target_records: list[dict], provider_id: str, migrated_license_type: str ): """Verify a partial migration changed nothing belonging to the license that stayed. @@ -470,7 +470,7 @@ def _verify_records_left_behind_are_untouched( def _keyed(records: list[dict]) -> dict[str, dict]: return { - _stable_record_key(record, ''): record + _stable_record_key(record, provider_id): record for record in records if record['type'] != 'provider' and record.get('licenseType') != migrated_license_type } @@ -1159,6 +1159,7 @@ def _find_new_provider_id(): _verify_records_left_behind_are_untouched( source_records=pre_migration_records, target_records=old_provider_records, + provider_id=old_provider_id, migrated_license_type=OT_LICENSE_TYPE, ) # a partial migration leaves the practitioner in place on the old provider id: their person-level From 11e67037310b1e5d46a40554042e1ff3d40e4577 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Thu, 20 Aug 2026 09:49:08 -0500 Subject: [PATCH 16/27] Pass investigationStatus forward on privilege renewal --- .../cc_common/data_model/data_client.py | 15 ++- .../data_model/schema/privilege/record.py | 18 ++++ .../common/tests/function/test_data_client.py | 94 +++++++++++++++++++ .../test_schema/test_privilege.py | 93 ++++++++++++++++++ 4 files changed, 219 insertions(+), 1 deletion(-) diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py index 6ac6d1fd1c..ddd8d8b5e3 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py @@ -41,7 +41,10 @@ ) from cc_common.data_model.schema.military_affiliation.record import MilitaryAffiliationRecordSchema from cc_common.data_model.schema.privilege import PrivilegeData, PrivilegeUpdateData -from cc_common.data_model.schema.privilege.record import PrivilegeUpdateRecordSchema +from cc_common.data_model.schema.privilege.record import ( + SYSTEM_OWNED_PRIVILEGE_FIELDS, + PrivilegeUpdateRecordSchema, +) from cc_common.data_model.schema.provider import ProviderData, ProviderUpdateData from cc_common.data_model.schema.provider.record import ( PROVIDER_ACCOUNT_STATE_FIELDS, @@ -540,10 +543,19 @@ def _generate_privilege_record( logger.warning('License type abbreviation not found', exc_info=e) raise CCInvalidRequestException(f'Compact or license type not supported: {e}') from e + system_owned_values = {} if original_privilege: # Copy over the original issuance date and privilege id date_of_issuance = original_privilege.dateOfIssuance privilege_id = original_privilege.privilegeId + # The record below is built fresh from the purchase inputs, so anything the purchase cannot + # express has to be carried forward explicitly or the renewal drops it + original_privilege_data = original_privilege.to_dict() + system_owned_values = { + field: original_privilege_data[field] + for field in sorted(SYSTEM_OWNED_PRIVILEGE_FIELDS) + if original_privilege_data.get(field) is not None + } else: date_of_issuance = current_datetime # Claim a privilege number for this jurisdiction @@ -570,6 +582,7 @@ def _generate_privilege_record( 'attestations': attestations, 'privilegeId': privilege_id, 'administratorSetStatus': ActiveInactiveStatus.ACTIVE, + **system_owned_values, } ) diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py index c69d91a0f1..5dca6806c7 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py @@ -66,6 +66,24 @@ class EncumbranceDetailsSchema(Schema): licenseJurisdiction = Jurisdiction(required=False, allow_none=False) +# Fields on a privilege record that CompactConnect owns rather than the practitioner's purchase. They are +# set by board actions - encumbrances and investigations - and a purchase has no way to express them. +# +# A renewal rebuilds the whole privilege record from the purchase inputs, so these have to be carried +# forward from the record being renewed or they are silently cleared, letting a practitioner drop an +# encumbrance by repurchasing. See DataClient._generate_privilege_record. This mirrors +# SYSTEM_OWNED_LICENSE_FIELDS, which solves the same problem for license re-uploads. +# +# homeJurisdictionChangeStatus and licenseDeactivatedStatus are deliberately absent: a renewal clears those +# on purpose and records the fact in the update record's removedValues. +SYSTEM_OWNED_PRIVILEGE_FIELDS = frozenset( + { + 'encumberedStatus', + 'investigationStatus', + } +) + + @BaseRecordSchema.register_schema('privilege') class PrivilegeRecordSchema(BaseRecordSchema, ValidatesLicenseTypeMixin): """ diff --git a/backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py b/backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py index 3e4f6d5750..8b33db3f5b 100644 --- a/backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py +++ b/backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py @@ -454,6 +454,100 @@ def test_data_client_updates_privilege_records_for_specific_license_type(self): )['Item'] self.assertEqual({'ky', 'ne'}, provider['privilegeJurisdictions']) + def test_renewal_preserves_board_set_privilege_statuses(self): + """A renewal rewrites the whole privilege record from the purchase inputs. Encumbrance and + investigation status are set by board actions, not by a purchase, so they have to be carried + forward - otherwise repurchasing a privilege silently clears an encumbrance the board applied. + """ + from cc_common.data_model.provider_record_util import ProviderUserRecords + + self.test_data_generator.put_default_provider_record_in_provider_table() + self.test_data_generator.put_default_license_record_in_provider_table() + encumbered_privilege = self.test_data_generator.put_default_privilege_record_in_provider_table( + {'encumberedStatus': 'encumbered', 'investigationStatus': 'underInvestigation'} + ) + + self.config.data_client.create_provider_privileges( + compact='aslp', + provider_id=DEFAULT_PROVIDER_ID, + provider_record=self.config.data_client.get_provider_top_level_record( + compact='aslp', provider_id=DEFAULT_PROVIDER_ID + ), + jurisdiction_postal_abbreviations=[encumbered_privilege.jurisdiction], + license_expiration_date=date.fromisoformat('2026-04-04'), + compact_transaction_id='renewal_transaction_id', + existing_privileges_for_license=[encumbered_privilege], + license_type=encumbered_privilege.licenseType, + attestations=self.sample_privilege_attestations, + ) + + provider_user_records: ProviderUserRecords = self.config.data_client.get_provider_user_records( + compact='aslp', provider_id=DEFAULT_PROVIDER_ID + ) + renewed_privilege = provider_user_records.get_specific_privilege_record( + jurisdiction=encumbered_privilege.jurisdiction, + license_abbreviation=encumbered_privilege.licenseTypeAbbreviation, + ) + + self.assertEqual('encumbered', renewed_privilege.encumberedStatus) + self.assertEqual('underInvestigation', renewed_privilege.investigationStatus) + # the renewal itself still happened + self.assertEqual('renewal_transaction_id', renewed_privilege.compactTransactionId) + + def test_renewal_clears_deactivation_statuses(self): + """The other half of the renewal contract: a renewal reactivates the privilege, so the two reasons + it could have been deactivated are removed from the record and reported in the update record. + + Covered here rather than only in the purchases suite because the renewed record is now built from + the record being renewed - preservation is the default, so the removals are the part that has to be + deliberate. + """ + from cc_common.data_model.provider_record_util import ProviderUserRecords + + self.test_data_generator.put_default_provider_record_in_provider_table() + self.test_data_generator.put_default_license_record_in_provider_table() + deactivated_privilege = self.test_data_generator.put_default_privilege_record_in_provider_table( + { + 'administratorSetStatus': 'inactive', + 'homeJurisdictionChangeStatus': 'inactive', + 'licenseDeactivatedStatus': 'licenseDeactivated', + } + ) + + self.config.data_client.create_provider_privileges( + compact='aslp', + provider_id=DEFAULT_PROVIDER_ID, + provider_record=self.config.data_client.get_provider_top_level_record( + compact='aslp', provider_id=DEFAULT_PROVIDER_ID + ), + jurisdiction_postal_abbreviations=[deactivated_privilege.jurisdiction], + license_expiration_date=date.fromisoformat('2026-04-04'), + compact_transaction_id='renewal_transaction_id', + existing_privileges_for_license=[deactivated_privilege], + license_type=deactivated_privilege.licenseType, + attestations=self.sample_privilege_attestations, + ) + + provider_user_records: ProviderUserRecords = self.config.data_client.get_provider_user_records( + compact='aslp', provider_id=DEFAULT_PROVIDER_ID, include_update_tier=UpdateTierEnum.TIER_THREE + ) + renewed_privilege = provider_user_records.get_specific_privilege_record( + jurisdiction=deactivated_privilege.jurisdiction, + license_abbreviation=deactivated_privilege.licenseTypeAbbreviation, + ) + + renewed_privilege_data = renewed_privilege.to_dict() + self.assertNotIn('homeJurisdictionChangeStatus', renewed_privilege_data) + self.assertNotIn('licenseDeactivatedStatus', renewed_privilege_data) + self.assertEqual('active', renewed_privilege.administratorSetStatus) + + # the update record must report exactly what was removed from the record + update_records = provider_user_records.get_update_records_for_privilege( + deactivated_privilege.jurisdiction, deactivated_privilege.licenseType + ) + self.assertEqual(1, len(update_records)) + self.assertEqual(['homeJurisdictionChangeStatus', 'licenseDeactivatedStatus'], update_records[0].removedValues) + def test_data_client_create_privilege_record_invalid_license_type(self): from cc_common.data_model.data_client import DataClient from cc_common.exceptions import CCInvalidRequestException diff --git a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_privilege.py b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_privilege.py index 68146b2140..90dcb9f111 100644 --- a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_privilege.py +++ b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_privilege.py @@ -316,3 +316,96 @@ def test_status_corrected_to_inactive_at_expiration_utc_minus_four(self): result = PrivilegeGeneralResponseSchema().load(privilege_data) self.assertEqual('inactive', result['status']) + + +class TestPrivilegeRecordFieldOwnership(TstLambdas): + """ + Guards the classification behind privilege renewals. + + DataClient._generate_privilege_record rebuilds the whole privilege record from the purchase inputs on + a renewal, so any field a purchase cannot supply is dropped unless it is deliberately carried forward. + That is how a board encumbrance and an open investigation could be cleared by repurchasing. This test + fails if a new privilege field is added without deciding which side of that line it falls on. + + Unlike the license equivalent there is no ingest schema to derive "what a purchase can supply" from - + a purchase is not schema-driven - so SET_BY_PURCHASE mirrors the dict that method builds. The + completeness check below is still mechanical: every field on the record has to land in some bucket. + """ + + # The keys DataClient._generate_privilege_record passes to PrivilegeData.create_new. + SET_BY_PURCHASE = { + 'providerId', + 'compact', + 'jurisdiction', + 'licenseJurisdiction', + 'licenseType', + 'dateOfIssuance', + 'dateOfRenewal', + 'dateOfExpiration', + 'compactTransactionId', + 'attestations', + 'privilegeId', + 'administratorSetStatus', + } + # Written by the base record schema or regenerated by PrivilegeRecordSchema's pre_dump hooks. + GENERATED_ON_WRITE = {'pk', 'sk', 'type', 'dateOfUpdate', 'compactTransactionIdGSIPK'} + # Calculated when a record is loaded and stripped again before it is written. + CALCULATED_ON_LOAD = {'status'} + # Deliberately dropped by a renewal, which reactivates the privilege. DataClient records these in the + # renewal update record's removedValues. + CLEARED_ON_RENEWAL = {'homeJurisdictionChangeStatus', 'licenseDeactivatedStatus'} + + def test_every_privilege_field_a_purchase_cannot_supply_is_classified(self): + from cc_common.data_model.schema.privilege.record import ( + SYSTEM_OWNED_PRIVILEGE_FIELDS, + PrivilegeRecordSchema, + ) + + unclassified = ( + set(PrivilegeRecordSchema().fields) + - self.SET_BY_PURCHASE + - self.GENERATED_ON_WRITE + - self.CALCULATED_ON_LOAD + - self.CLEARED_ON_RENEWAL + - SYSTEM_OWNED_PRIVILEGE_FIELDS + ) + + self.assertEqual( + set(), + unclassified, + f'New privilege record field(s) {sorted(unclassified)} are not accounted for by a renewal, ' + 'which rebuilds the record from the purchase inputs and will silently drop them. Decide which ' + 'they are and add them to the right place: SYSTEM_OWNED_PRIVILEGE_FIELDS (in ' + 'schema/privilege/record.py) if the system owns the value and it must survive a repurchase, or ' + 'one of the SET_BY_PURCHASE / GENERATED_ON_WRITE / CALCULATED_ON_LOAD / CLEARED_ON_RENEWAL sets ' + 'in this test if the purchase supplies it, it is rebuilt on every write, or a renewal is meant ' + 'to clear it.', + ) + + def test_system_owned_fields_are_not_supplied_or_cleared_by_a_purchase(self): + """A preserved field that a purchase also sets would be frozen at its first value, and one a + renewal is meant to clear cannot also be carried forward. + """ + from cc_common.data_model.schema.privilege.record import SYSTEM_OWNED_PRIVILEGE_FIELDS + + self.assertEqual(set(), SYSTEM_OWNED_PRIVILEGE_FIELDS & self.SET_BY_PURCHASE) + self.assertEqual(set(), SYSTEM_OWNED_PRIVILEGE_FIELDS & self.CLEARED_ON_RENEWAL) + + def test_classified_fields_all_exist_on_the_record(self): + """A classification naming a field the schema does not have is dead weight, and usually a rename + that was only half applied. + """ + from cc_common.data_model.schema.privilege.record import ( + SYSTEM_OWNED_PRIVILEGE_FIELDS, + PrivilegeRecordSchema, + ) + + classified = ( + SYSTEM_OWNED_PRIVILEGE_FIELDS + | self.SET_BY_PURCHASE + | self.GENERATED_ON_WRITE + | self.CALCULATED_ON_LOAD + | self.CLEARED_ON_RENEWAL + ) + + self.assertEqual(set(), classified - set(PrivilegeRecordSchema().fields)) From 47b9685a4ba994570bef0be821e7373dffec555a Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Thu, 20 Aug 2026 10:34:01 -0500 Subject: [PATCH 17/27] feedback - define provider fields not supplied by the schema --- .../unit/test_data_model/test_schema/test_provider.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_provider.py b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_provider.py index 0acbb4d2b2..e715782c86 100644 --- a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_provider.py +++ b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_provider.py @@ -142,17 +142,25 @@ def test_every_provider_field_the_license_cannot_supply_is_classified(self): from cc_common.data_model.schema.license.record import LicenseRecordSchema from cc_common.data_model.schema.provider.record import ( PROVIDER_ACCOUNT_STATE_FIELDS, + PROVIDER_AGGREGATE_FIELDS, PROVIDER_PERSON_LEVEL_FIELDS, ProviderRecordSchema, ) - not_suppliable_by_the_license = set(ProviderRecordSchema().fields) - set(LicenseRecordSchema().fields) + # We push several fields onto the provider record from the license record + # but not all fields on the provider record come from the license record. + # we define those here to catch any new fields which are added to the schema + # and not properly classified to prevent fields from being dropped. + not_suppliable_by_the_license = ( + set(ProviderRecordSchema().fields) - set(LicenseRecordSchema().fields) + ) | PROVIDER_AGGREGATE_FIELDS unclassified = ( not_suppliable_by_the_license - self.GENERATED_ON_WRITE - self.DERIVED_FROM_RECORDS - PROVIDER_ACCOUNT_STATE_FIELDS - PROVIDER_PERSON_LEVEL_FIELDS + - PROVIDER_AGGREGATE_FIELDS ) self.assertEqual( From 9a4417c7a0aa07b82cfaf98c9b7080b5889b89ed Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Fri, 21 Aug 2026 08:49:39 -0500 Subject: [PATCH 18/27] Feedback - rename field and clarify comments --- .../cc_common/data_model/data_client.py | 10 ++++----- .../data_model/schema/license/record.py | 4 ++-- .../data_model/schema/privilege/record.py | 10 ++++----- .../test_schema/test_license.py | 2 +- .../test_schema/test_privilege.py | 21 ++++++++++--------- 5 files changed, 24 insertions(+), 23 deletions(-) diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py index ddd8d8b5e3..27a911924f 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py @@ -42,7 +42,7 @@ from cc_common.data_model.schema.military_affiliation.record import MilitaryAffiliationRecordSchema from cc_common.data_model.schema.privilege import PrivilegeData, PrivilegeUpdateData from cc_common.data_model.schema.privilege.record import ( - SYSTEM_OWNED_PRIVILEGE_FIELDS, + FIELDS_PRESERVED_ON_RENEWAL, PrivilegeUpdateRecordSchema, ) from cc_common.data_model.schema.provider import ProviderData, ProviderUpdateData @@ -543,7 +543,7 @@ def _generate_privilege_record( logger.warning('License type abbreviation not found', exc_info=e) raise CCInvalidRequestException(f'Compact or license type not supported: {e}') from e - system_owned_values = {} + preserved_values = {} if original_privilege: # Copy over the original issuance date and privilege id date_of_issuance = original_privilege.dateOfIssuance @@ -551,9 +551,9 @@ def _generate_privilege_record( # The record below is built fresh from the purchase inputs, so anything the purchase cannot # express has to be carried forward explicitly or the renewal drops it original_privilege_data = original_privilege.to_dict() - system_owned_values = { + preserved_values = { field: original_privilege_data[field] - for field in sorted(SYSTEM_OWNED_PRIVILEGE_FIELDS) + for field in sorted(FIELDS_PRESERVED_ON_RENEWAL) if original_privilege_data.get(field) is not None } else: @@ -582,7 +582,7 @@ def _generate_privilege_record( 'attestations': attestations, 'privilegeId': privilege_id, 'administratorSetStatus': ActiveInactiveStatus.ACTIVE, - **system_owned_values, + **preserved_values, } ) diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py index 8ce453e1ce..bf076a8180 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/license/record.py @@ -35,8 +35,8 @@ from cc_common.data_model.update_tier_enum import UpdateTierEnum # Fields on a license record that CompactConnect owns rather than the uploading state. They are set by -# actions taken within the system (board encumbrances, investigations) or by the ingest process itself, -# and there is no way for a state to express them in a license upload. +# actions taken within the system (board encumbrances, investigations), and there is no way for a state +# to express them in a license upload. # # A license upload writes the whole record, so the uploading state is authoritative for every field it can # send - omitting one removes it, which is intentional and recorded in the update record's removedValues. diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py index 5dca6806c7..2890bee3ad 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py @@ -66,17 +66,17 @@ class EncumbranceDetailsSchema(Schema): licenseJurisdiction = Jurisdiction(required=False, allow_none=False) -# Fields on a privilege record that CompactConnect owns rather than the practitioner's purchase. They are -# set by board actions - encumbrances and investigations - and a purchase has no way to express them. +# Fields that must survive a renewal. They are set by board actions - encumbrances and investigations - +# and a purchase has no way to express them. # # A renewal rebuilds the whole privilege record from the purchase inputs, so these have to be carried # forward from the record being renewed or they are silently cleared, letting a practitioner drop an -# encumbrance by repurchasing. See DataClient._generate_privilege_record. This mirrors -# SYSTEM_OWNED_LICENSE_FIELDS, which solves the same problem for license re-uploads. +# encumbrance by repurchasing. See DataClient._generate_privilege_record. This solves the same problem +# SYSTEM_OWNED_LICENSE_FIELDS solves for license re-uploads. # # homeJurisdictionChangeStatus and licenseDeactivatedStatus are deliberately absent: a renewal clears those # on purpose and records the fact in the update record's removedValues. -SYSTEM_OWNED_PRIVILEGE_FIELDS = frozenset( +FIELDS_PRESERVED_ON_RENEWAL = frozenset( { 'encumberedStatus', 'investigationStatus', diff --git a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.py b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.py index 0e5881dca8..81d8ad7ac1 100644 --- a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.py +++ b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_license.py @@ -503,7 +503,7 @@ class TestLicenseRecordFieldOwnership(TstLambdas): Guards the contract behind SYSTEM_OWNED_LICENSE_FIELDS. A license upload writes the whole record, so any field the upload cannot supply is dropped unless the - ingest handler carries it forward.This test fails if a new field is added to the license record without + ingest handler carries it forward. This test fails if a new field is added to the license record without deciding which side of that line it falls on. """ diff --git a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_privilege.py b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_privilege.py index 90dcb9f111..3b236dcd20 100644 --- a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_privilege.py +++ b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_schema/test_privilege.py @@ -357,7 +357,7 @@ class TestPrivilegeRecordFieldOwnership(TstLambdas): def test_every_privilege_field_a_purchase_cannot_supply_is_classified(self): from cc_common.data_model.schema.privilege.record import ( - SYSTEM_OWNED_PRIVILEGE_FIELDS, + FIELDS_PRESERVED_ON_RENEWAL, PrivilegeRecordSchema, ) @@ -367,7 +367,7 @@ def test_every_privilege_field_a_purchase_cannot_supply_is_classified(self): - self.GENERATED_ON_WRITE - self.CALCULATED_ON_LOAD - self.CLEARED_ON_RENEWAL - - SYSTEM_OWNED_PRIVILEGE_FIELDS + - FIELDS_PRESERVED_ON_RENEWAL ) self.assertEqual( @@ -375,33 +375,34 @@ def test_every_privilege_field_a_purchase_cannot_supply_is_classified(self): unclassified, f'New privilege record field(s) {sorted(unclassified)} are not accounted for by a renewal, ' 'which rebuilds the record from the purchase inputs and will silently drop them. Decide which ' - 'they are and add them to the right place: SYSTEM_OWNED_PRIVILEGE_FIELDS (in ' - 'schema/privilege/record.py) if the system owns the value and it must survive a repurchase, or ' + 'they are and add them to the right place: FIELDS_PRESERVED_ON_RENEWAL (in ' + 'schema/privilege/record.py) if a purchase cannot express the value and it must survive a ' + 'repurchase, or ' 'one of the SET_BY_PURCHASE / GENERATED_ON_WRITE / CALCULATED_ON_LOAD / CLEARED_ON_RENEWAL sets ' 'in this test if the purchase supplies it, it is rebuilt on every write, or a renewal is meant ' 'to clear it.', ) - def test_system_owned_fields_are_not_supplied_or_cleared_by_a_purchase(self): + def test_preserved_fields_are_not_supplied_or_cleared_by_a_purchase(self): """A preserved field that a purchase also sets would be frozen at its first value, and one a renewal is meant to clear cannot also be carried forward. """ - from cc_common.data_model.schema.privilege.record import SYSTEM_OWNED_PRIVILEGE_FIELDS + from cc_common.data_model.schema.privilege.record import FIELDS_PRESERVED_ON_RENEWAL - self.assertEqual(set(), SYSTEM_OWNED_PRIVILEGE_FIELDS & self.SET_BY_PURCHASE) - self.assertEqual(set(), SYSTEM_OWNED_PRIVILEGE_FIELDS & self.CLEARED_ON_RENEWAL) + self.assertEqual(set(), FIELDS_PRESERVED_ON_RENEWAL & self.SET_BY_PURCHASE) + self.assertEqual(set(), FIELDS_PRESERVED_ON_RENEWAL & self.CLEARED_ON_RENEWAL) def test_classified_fields_all_exist_on_the_record(self): """A classification naming a field the schema does not have is dead weight, and usually a rename that was only half applied. """ from cc_common.data_model.schema.privilege.record import ( - SYSTEM_OWNED_PRIVILEGE_FIELDS, + FIELDS_PRESERVED_ON_RENEWAL, PrivilegeRecordSchema, ) classified = ( - SYSTEM_OWNED_PRIVILEGE_FIELDS + FIELDS_PRESERVED_ON_RENEWAL | self.SET_BY_PURCHASE | self.GENERATED_ON_WRITE | self.CALCULATED_ON_LOAD From 354149017baa61ce7c1543afae4a16b6c6bea8fd Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Fri, 21 Aug 2026 09:11:31 -0500 Subject: [PATCH 19/27] feedback - fix comment blocks and test setup --- .../data_model/schema/provider/record.py | 25 +++++++++---------- .../common/tests/function/test_data_client.py | 16 ++++++------ 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py b/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py index 2901dd8b56..a60be5a00c 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/record.py @@ -35,19 +35,10 @@ # them when an existing provider record is refreshed - see ProviderRecordUtility.populate_provider_record. PROVIDER_AGGREGATE_FIELDS = frozenset({'encumberedStatus'}) -# Fields describing the practitioner's CompactConnect *account* rather than the practitioner themselves: -# their registration, an in-flight email change, and an in-flight account recovery. A full SSN-correction -# migration deletes the old Cognito user and requires the practitioner to register again under the -# corrected provider id, so none of this may follow them onto the new provider record. -# -# currentHomeJurisdiction belongs here rather than with the person-level fields: registration sets it -# alongside the registered email address (see DataClient registration flow), and until it is set the -# provider is calculated as compact-ineligible. Carrying it would leave a provider that cannot sign in -# looking registered and eligible to purchase. + # Fields describing the practitioner rather than their licenses or their account. They are maintained by -# flows that have nothing to do with license uploads - the military file upload and audit flows - and -# cannot be rebuilt from any record a migration moves, so a full SSN-correction migration has to carry -# them onto the corrected provider id explicitly. +# flows that have nothing to do with license uploads, such as the military file upload and audit flows, +# so a full SSN-correction migration has to carry them onto the corrected provider id explicitly. # # A partial migration does not: the records that back these stay with the old provider, so a status # carried across would have no supporting documentation behind it. @@ -58,7 +49,15 @@ } ) - +# Fields describing the practitioner's CompactConnect *account* rather than the practitioner themselves: +# their registration, an in-flight email change, and an in-flight account recovery. A full SSN-correction +# migration deletes the old Cognito user and requires the practitioner to register again under the +# corrected provider id, so none of this may follow them onto the new provider record. +# +# currentHomeJurisdiction belongs here rather than with the person-level fields: registration sets it +# alongside the registered email address (see DataClient registration flow), and until it is set the +# provider is calculated as compact-ineligible. Carrying it would leave a provider that cannot sign in +# looking registered and eligible to purchase. PROVIDER_ACCOUNT_STATE_FIELDS = frozenset( { 'compactConnectRegisteredEmailAddress', diff --git a/backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py b/backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py index 8b33db3f5b..91e5c4b0f0 100644 --- a/backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py +++ b/backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py @@ -457,14 +457,18 @@ def test_data_client_updates_privilege_records_for_specific_license_type(self): def test_renewal_preserves_board_set_privilege_statuses(self): """A renewal rewrites the whole privilege record from the purchase inputs. Encumbrance and investigation status are set by board actions, not by a purchase, so they have to be carried - forward - otherwise repurchasing a privilege silently clears an encumbrance the board applied. + forward, otherwise repurchasing a privilege silently clears those status fields. + + For encumbrances, this is not an issue since a privilege can only be renewed if the practitioner + hasn't had any encumbrance for over two years. Practitioners can however renew a privilege even when + under investigation, so we need to ensure that status flag remains upon renewal. """ from cc_common.data_model.provider_record_util import ProviderUserRecords self.test_data_generator.put_default_provider_record_in_provider_table() self.test_data_generator.put_default_license_record_in_provider_table() encumbered_privilege = self.test_data_generator.put_default_privilege_record_in_provider_table( - {'encumberedStatus': 'encumbered', 'investigationStatus': 'underInvestigation'} + {'encumberedStatus': 'unencumbered', 'investigationStatus': 'underInvestigation'} ) self.config.data_client.create_provider_privileges( @@ -489,18 +493,14 @@ def test_renewal_preserves_board_set_privilege_statuses(self): license_abbreviation=encumbered_privilege.licenseTypeAbbreviation, ) - self.assertEqual('encumbered', renewed_privilege.encumberedStatus) + self.assertEqual('unencumbered', renewed_privilege.encumberedStatus) self.assertEqual('underInvestigation', renewed_privilege.investigationStatus) # the renewal itself still happened self.assertEqual('renewal_transaction_id', renewed_privilege.compactTransactionId) def test_renewal_clears_deactivation_statuses(self): - """The other half of the renewal contract: a renewal reactivates the privilege, so the two reasons + """The other half of the renewal contract: a renewal reactivates the privilege, so the reasons it could have been deactivated are removed from the record and reported in the update record. - - Covered here rather than only in the purchases suite because the renewed record is now built from - the record being renewed - preservation is the default, so the removals are the part that has to be - deliberate. """ from cc_common.data_model.provider_record_util import ProviderUserRecords From e362e4d291b12046c3e6eaf5cf95d96e0a859db0 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Fri, 21 Aug 2026 10:33:43 -0500 Subject: [PATCH 20/27] PR feedback - clarify comments and tests --- .../tests/function/test_data_client_ssn_correction.py | 2 +- .../tests/unit/test_data_model/test_data_client.py | 11 +++++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py b/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py index e4ccc59e75..bcff31d6b0 100644 --- a/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py +++ b/backend/compact-connect/lambdas/python/common/tests/function/test_data_client_ssn_correction.py @@ -503,7 +503,7 @@ def test_existing_new_provider_military_status_is_never_overwritten(self): self.assertEqual('declined', new_provider_record.get('militaryStatus')) self.assertEqual('documentation expired', new_provider_record.get('militaryStatusNote')) - def test_existing_new_provider_record_untouched_when_there_is_no_military_status_to_carry(self): + def test_existing_new_provider_record_untouched_when_there_is_no_military_status_fields_to_preserve(self): """With nothing to merge, the pre-existing record must not be written to at all.""" pre_existing_provider = self.test_data_generator.put_default_provider_record_in_provider_table( {'providerId': NEW_PROVIDER_ID, 'licenseJurisdiction': 'ky', 'privilegeJurisdictions': set()} diff --git a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py index 0204266436..b64f6f9427 100644 --- a/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py +++ b/backend/compact-connect/lambdas/python/common/tests/unit/test_data_model/test_data_client.py @@ -89,8 +89,15 @@ def test_privilege_without_a_transaction_id_does_not_raise_exception_and_logs_an ) def test_collects_the_previous_transaction_id_from_an_update_record(self): - """Only the `previous` snapshot is read. A renewal's own transaction id is written to the privilege - record in the same db transaction, so it is collected from there rather than from `updatedValues`. + """ + When collecting transaction ids to determine all the transactions that need to be updated, we + start with the privilege record itself to get the most recent renewal transaction id, then walk back + through all privilege update records and chain together all the transaction ids from the 'previous' + snapshot object, ensuring that we walk all the way back to the transaction id of the original purchase. + + This micro test specifically checks that the transaction id of the `previous` snapshot is read on the privilege + update record, and not the updatedValues, so we never accidentally break this chain for collecting all + transaction ids for a migration. """ privilege_update = self.test_data_generator.generate_default_privilege_update( value_overrides={'updatedValues': {'compactTransactionId': 'tx-new'}}, From a5a83547b8a2f1842ff15a39226ae5fb3a4356f5 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Fri, 21 Aug 2026 10:44:36 -0500 Subject: [PATCH 21/27] Remove duplicate definition --- .../data_model/schema/license/record.py | 22 ------------------- 1 file changed, 22 deletions(-) diff --git a/backend/social-work-app/lambdas/python/common/cc_common/data_model/schema/license/record.py b/backend/social-work-app/lambdas/python/common/cc_common/data_model/schema/license/record.py index 794d0e07f0..a251e77c0e 100644 --- a/backend/social-work-app/lambdas/python/common/cc_common/data_model/schema/license/record.py +++ b/backend/social-work-app/lambdas/python/common/cc_common/data_model/schema/license/record.py @@ -58,28 +58,6 @@ ) -# Fields on a license record that CompactConnect owns rather than the uploading state. They are set by -# actions taken within the system (board encumbrances, investigations) or by the ingest process itself, -# and there is no way for a state to express them in a license upload. -# -# A license upload writes the whole record, so the uploading state is authoritative for every field it can -# send - omitting one removes it, which is intentional and recorded in the update record's removedValues. -# These fields are the exception: an upload cannot assert them, so it must not be able to retract them -# either. The ingest handler carries them forward from the existing record when re-uploading a license -# that already exists. -# -# Any future field of this kind must be added here, or a routine re-upload will silently drop it. Note -# that a field must also be declared on LicenseRecordSchema below to survive at all: this schema excludes -# undeclared attributes when a record is loaded. -SYSTEM_OWNED_LICENSE_FIELDS = frozenset( - { - 'encumberedStatus', - 'investigationStatus', - 'firstUploadDate', - } -) - - @BaseRecordSchema.register_schema('license') class LicenseRecordSchema(BaseRecordSchema, LicenseCommonSchema): """ From bbf7ff7b4dede487f6ed5a2ff87dd40ddb018577 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Fri, 21 Aug 2026 12:27:26 -0500 Subject: [PATCH 22/27] Update pip version in GH action runners --- .github/workflows/check-common-cdk.yml | 8 ++++---- .../workflows/check-compact-connect-ui-app.yml | 8 ++++---- .github/workflows/check-compact-connect.yml | 15 ++++++--------- .github/workflows/check-cosmetology-app.yml | 10 ++++------ .github/workflows/check-multi-account.yml | 8 ++++---- .github/workflows/check-social-work-app.yml | 10 ++++------ .../compact-connect-ui-app/requirements-dev.txt | 2 +- backend/compact-connect/requirements-dev.txt | 2 +- backend/cosmetology-app/requirements-dev.txt | 2 +- .../control-tower/requirements-dev.txt | 2 +- backend/social-work-app/requirements-dev.txt | 2 +- 11 files changed, 31 insertions(+), 38 deletions(-) diff --git a/.github/workflows/check-common-cdk.yml b/.github/workflows/check-common-cdk.yml index 4f6c5cbfb6..208b22254c 100644 --- a/.github/workflows/check-common-cdk.yml +++ b/.github/workflows/check-common-cdk.yml @@ -25,8 +25,8 @@ jobs: - uses: actions/checkout@v5 - name: Upgrade pip - # Runner image ships pip 25.3; Upgrade to 26.1+ to include fix for CVE-2026-3219. - run: pip install --upgrade 'pip>=26.1' + # Runner image ships pip 25.3; Upgrade to 26.2+ for the fixes to CVE-2026-3219 and PYSEC-2026-3721. + run: pip install --upgrade 'pip>=26.2' - name: Install dev dependencies run: "pip install -r backend/common-cdk/requirements-dev.in" @@ -50,8 +50,8 @@ jobs: python-version: '3.14' - name: Upgrade pip - # Runner image ships pip 25.3; Upgrade to 26.1+ to include fix for CVE-2026-3219. - run: pip install --upgrade 'pip>=26.1' + # Runner image ships pip 25.3; Upgrade to 26.2+ for the fixes to CVE-2026-3219 and PYSEC-2026-3721. + run: pip install --upgrade 'pip>=26.2' - name: Install dev dependencies run: "pip install -r backend/common-cdk/requirements-dev.in" diff --git a/.github/workflows/check-compact-connect-ui-app.yml b/.github/workflows/check-compact-connect-ui-app.yml index af3890868f..00695c5aba 100644 --- a/.github/workflows/check-compact-connect-ui-app.yml +++ b/.github/workflows/check-compact-connect-ui-app.yml @@ -25,8 +25,8 @@ jobs: - uses: actions/checkout@v5 - name: Upgrade pip - # Runner image ships pip 25.3; Upgrade to 26.1+ to include fix for CVE-2026-3219. - run: pip install --upgrade 'pip>=26.1' + # Runner image ships pip 25.3; Upgrade to 26.2+ for the fixes to CVE-2026-3219 and PYSEC-2026-3721. + run: pip install --upgrade 'pip>=26.2' - name: Install dev dependencies run: "pip install -r backend/compact-connect-ui-app/requirements-dev.txt" @@ -87,8 +87,8 @@ jobs: python-version: '3.14' - name: Upgrade pip - # Runner image ships pip 25.3; Upgrade to 26.1+ to include fix for CVE-2026-3219. - run: pip install --upgrade 'pip>=26.1' + # Runner image ships pip 25.3; Upgrade to 26.2+ for the fixes to CVE-2026-3219 and PYSEC-2026-3721. + run: pip install --upgrade 'pip>=26.2' # Setup Node - name: Setup Node diff --git a/.github/workflows/check-compact-connect.yml b/.github/workflows/check-compact-connect.yml index 2950bc0865..990621d786 100644 --- a/.github/workflows/check-compact-connect.yml +++ b/.github/workflows/check-compact-connect.yml @@ -25,9 +25,8 @@ jobs: - uses: actions/checkout@v5 - name: Upgrade pip - # Runner image ships pip 25.3; Upgrade to 26.1+ to include fix for CVE-2026-3219. - # Held below 26.2, which drops pip._internal.utils.compat.stdlib_pkgs and breaks pip-sync. - run: pip install --upgrade 'pip>=26.1,<26.2' + # Runner image ships pip 25.3; Upgrade to 26.2+ for the fixes to CVE-2026-3219 and PYSEC-2026-3721. + run: pip install --upgrade 'pip>=26.2' - name: Install dev dependencies run: "pip install -r backend/compact-connect/requirements-dev.txt" @@ -88,9 +87,8 @@ jobs: python-version: '3.14' - name: Upgrade pip - # Runner image ships pip 25.3; Upgrade to 26.1+ to include fix for CVE-2026-3219. - # Held below 26.2, which drops pip._internal.utils.compat.stdlib_pkgs and breaks pip-sync. - run: pip install --upgrade 'pip>=26.1,<26.2' + # Runner image ships pip 25.3; Upgrade to 26.2+ for the fixes to CVE-2026-3219 and PYSEC-2026-3721. + run: pip install --upgrade 'pip>=26.2' # Setup Node - name: Setup Node @@ -131,9 +129,8 @@ jobs: python-version: '3.12' - name: Upgrade pip - # Runner image ships pip 25.3; Upgrade to 26.1+ to include fix for CVE-2026-3219. - # Held below 26.2, which drops pip._internal.utils.compat.stdlib_pkgs and breaks pip-sync. - run: pip install --upgrade 'pip>=26.1,<26.2' + # Runner image ships pip 25.3; Upgrade to 26.2+ for the fixes to CVE-2026-3219 and PYSEC-2026-3721. + run: pip install --upgrade 'pip>=26.2' - name: Install dependencies run: "cd backend/compact-connect/lambdas/python/purchases; pip install -r requirements.txt" diff --git a/.github/workflows/check-cosmetology-app.yml b/.github/workflows/check-cosmetology-app.yml index 10dc9aec40..a68fb8451e 100644 --- a/.github/workflows/check-cosmetology-app.yml +++ b/.github/workflows/check-cosmetology-app.yml @@ -25,9 +25,8 @@ jobs: - uses: actions/checkout@v5 - name: Upgrade pip - # Runner image ships pip 25.3; Upgrade to 26.1+ to include fix for CVE-2026-3219. - # Held below 26.2, which drops pip._internal.utils.compat.stdlib_pkgs and breaks pip-sync. - run: pip install --upgrade 'pip>=26.1,<26.2' + # Runner image ships pip 25.3; Upgrade to 26.2+ for the fixes to CVE-2026-3219 and PYSEC-2026-3721. + run: pip install --upgrade 'pip>=26.2' - name: Install dev dependencies run: "pip install -r backend/cosmetology-app/requirements-dev.txt" @@ -87,9 +86,8 @@ jobs: python-version: '3.14' - name: Upgrade pip - # Runner image ships pip 25.3; Upgrade to 26.1+ to include fix for CVE-2026-3219. - # Held below 26.2, which drops pip._internal.utils.compat.stdlib_pkgs and breaks pip-sync. - run: pip install --upgrade 'pip>=26.1,<26.2' + # Runner image ships pip 25.3; Upgrade to 26.2+ for the fixes to CVE-2026-3219 and PYSEC-2026-3721. + run: pip install --upgrade 'pip>=26.2' # Setup Node - name: Setup Node diff --git a/.github/workflows/check-multi-account.yml b/.github/workflows/check-multi-account.yml index e45de71f53..ad14f6ef68 100644 --- a/.github/workflows/check-multi-account.yml +++ b/.github/workflows/check-multi-account.yml @@ -25,8 +25,8 @@ jobs: - uses: actions/checkout@v2 - name: Upgrade pip - # Runner image ships pip 25.3; Upgrade to 26.1+ to include fix for CVE-2026-3219. - run: pip install --upgrade 'pip>=26.1' + # Runner image ships pip 25.3; Upgrade to 26.2+ for the fixes to CVE-2026-3219 and PYSEC-2026-3721. + run: pip install --upgrade 'pip>=26.2' - name: Install dev dependencies run: "pip install -r backend/multi-account/control-tower/requirements-dev.txt" @@ -50,8 +50,8 @@ jobs: python-version: '3.12' - name: Upgrade pip - # Runner image ships pip 25.3; Upgrade to 26.1+ to include fix for CVE-2026-3219. - run: pip install --upgrade 'pip>=26.1' + # Runner image ships pip 25.3; Upgrade to 26.2+ for the fixes to CVE-2026-3219 and PYSEC-2026-3721. + run: pip install --upgrade 'pip>=26.2' - name: Install dev dependencies run: "pip install -r backend/multi-account/control-tower/requirements-dev.txt" diff --git a/.github/workflows/check-social-work-app.yml b/.github/workflows/check-social-work-app.yml index 1f785d22d7..38faacf93e 100644 --- a/.github/workflows/check-social-work-app.yml +++ b/.github/workflows/check-social-work-app.yml @@ -25,9 +25,8 @@ jobs: - uses: actions/checkout@v5 - name: Upgrade pip - # Runner image ships pip 25.3; Upgrade to 26.1+ to include fix for CVE-2026-3219. - # Held below 26.2, which drops pip._internal.utils.compat.stdlib_pkgs and breaks pip-sync. - run: pip install --upgrade 'pip>=26.1,<26.2' + # Runner image ships pip 25.3; Upgrade to 26.2+ for the fixes to CVE-2026-3219 and PYSEC-2026-3721. + run: pip install --upgrade 'pip>=26.2' - name: Install dev dependencies run: "pip install -r backend/social-work-app/requirements-dev.txt" @@ -87,9 +86,8 @@ jobs: python-version: '3.14' - name: Upgrade pip - # Runner image ships pip 25.3; Upgrade to 26.1+ to include fix for CVE-2026-3219. - # Held below 26.2, which drops pip._internal.utils.compat.stdlib_pkgs and breaks pip-sync. - run: pip install --upgrade 'pip>=26.1,<26.2' + # Runner image ships pip 25.3; Upgrade to 26.2+ for the fixes to CVE-2026-3219 and PYSEC-2026-3721. + run: pip install --upgrade 'pip>=26.2' # Setup Node - name: Setup Node diff --git a/backend/compact-connect-ui-app/requirements-dev.txt b/backend/compact-connect-ui-app/requirements-dev.txt index e11c19145a..92c5df77b3 100644 --- a/backend/compact-connect-ui-app/requirements-dev.txt +++ b/backend/compact-connect-ui-app/requirements-dev.txt @@ -55,7 +55,7 @@ pip-audit==2.10.1 # via -r requirements-dev.in pip-requirements-parser==32.0.1 # via pip-audit -pip-tools==7.6.0 +pip-tools==7.6.1 # via -r requirements-dev.in platformdirs==4.10.0 # via pip-audit diff --git a/backend/compact-connect/requirements-dev.txt b/backend/compact-connect/requirements-dev.txt index f2291e56ce..fb4c7fae57 100644 --- a/backend/compact-connect/requirements-dev.txt +++ b/backend/compact-connect/requirements-dev.txt @@ -57,7 +57,7 @@ pip-audit==2.10.1 # via -r requirements-dev.in pip-requirements-parser==32.0.1 # via pip-audit -pip-tools==7.5.3 +pip-tools==7.6.1 # via -r requirements-dev.in platformdirs==4.10.0 # via pip-audit diff --git a/backend/cosmetology-app/requirements-dev.txt b/backend/cosmetology-app/requirements-dev.txt index f2291e56ce..fb4c7fae57 100644 --- a/backend/cosmetology-app/requirements-dev.txt +++ b/backend/cosmetology-app/requirements-dev.txt @@ -57,7 +57,7 @@ pip-audit==2.10.1 # via -r requirements-dev.in pip-requirements-parser==32.0.1 # via pip-audit -pip-tools==7.5.3 +pip-tools==7.6.1 # via -r requirements-dev.in platformdirs==4.10.0 # via pip-audit diff --git a/backend/multi-account/control-tower/requirements-dev.txt b/backend/multi-account/control-tower/requirements-dev.txt index 70cf3400cc..b8e927ff5d 100644 --- a/backend/multi-account/control-tower/requirements-dev.txt +++ b/backend/multi-account/control-tower/requirements-dev.txt @@ -55,7 +55,7 @@ pip-audit==2.10.0 # via -r requirements-dev.in pip-requirements-parser==32.0.1 # via pip-audit -pip-tools==7.5.3 +pip-tools==7.6.1 # via -r requirements-dev.in platformdirs==4.9.6 # via pip-audit diff --git a/backend/social-work-app/requirements-dev.txt b/backend/social-work-app/requirements-dev.txt index f2291e56ce..fb4c7fae57 100644 --- a/backend/social-work-app/requirements-dev.txt +++ b/backend/social-work-app/requirements-dev.txt @@ -57,7 +57,7 @@ pip-audit==2.10.1 # via -r requirements-dev.in pip-requirements-parser==32.0.1 # via pip-audit -pip-tools==7.5.3 +pip-tools==7.6.1 # via -r requirements-dev.in platformdirs==4.10.0 # via pip-audit From 67070d4835d2099ca794f2c3ea180c89a9566366 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Fri, 21 Aug 2026 12:28:35 -0500 Subject: [PATCH 23/27] Add logs to denote partial and full migrations for easier cloudwatch searching --- .../lambdas/python/provider-data-v1/handlers/ingest.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py b/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py index f68b5b0ccb..204dbf2f36 100644 --- a/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py +++ b/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py @@ -429,8 +429,18 @@ def _perform_ssn_correction_migration( return if result.full_migration: + logger.info( + 'SSN correction resulted in a full migration', + license_type=license_type, + new_provider_id=new_provider_id, + ) metrics.add_metric(name=SSN_CORRECTION_FULL_MIGRATION_METRIC, unit=MetricUnit.Count, value=1) else: + logger.info( + 'SSN correction resulted in a partial migration', + license_type=license_type, + new_provider_id=new_provider_id, + ) metrics.add_metric(name=SSN_CORRECTION_PARTIAL_MIGRATION_METRIC, unit=MetricUnit.Count, value=1) if result.full_migration and result.old_provider_registered_email is not None: From eaff514f6aad762c9eb89990a5918189c8c8acf0 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Fri, 21 Aug 2026 12:36:58 -0500 Subject: [PATCH 24/27] Update multi-account deps --- backend/multi-account/control-tower/requirements-dev.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/multi-account/control-tower/requirements-dev.txt b/backend/multi-account/control-tower/requirements-dev.txt index b8e927ff5d..053efa2295 100644 --- a/backend/multi-account/control-tower/requirements-dev.txt +++ b/backend/multi-account/control-tower/requirements-dev.txt @@ -38,7 +38,7 @@ markdown-it-py==4.2.0 # via rich mdurl==0.1.2 # via markdown-it-py -msgpack==1.1.2 +msgpack==1.2.1 # via cachecontrol packageurl-python==0.17.6 # via cyclonedx-python-lib From 0e80bf66b7062f579a53569958cd3f2d7ddf6d8a Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Fri, 21 Aug 2026 13:14:49 -0500 Subject: [PATCH 25/27] preserve log markers when context marker keys in multiple callers match --- .../lambdas/python/common/cc_common/utils.py | 16 ++- .../python/common/tests/unit/test_utils.py | 103 ++++++++++++++++++ .../provider-data-v1/handlers/ingest.py | 22 ++-- 3 files changed, 127 insertions(+), 14 deletions(-) diff --git a/backend/compact-connect/lambdas/python/common/cc_common/utils.py b/backend/compact-connect/lambdas/python/common/cc_common/utils.py index 5b408beb7e..996e54f546 100644 --- a/backend/compact-connect/lambdas/python/common/cc_common/utils.py +++ b/backend/compact-connect/lambdas/python/common/cc_common/utils.py @@ -232,8 +232,20 @@ def __call__(self, fn: Callable): def wrapped(*args, **kwargs): if not self.arg_names: raise ValueError('No argument names provided to logger_inject_kwargs') - with self.logger.append_context_keys(**{k: kwargs.get(k) for k in self.arg_names}): - return fn(*args, **kwargs) + injected_keys = {k: kwargs.get(k) for k in self.arg_names} + # append_context_keys removes every key it set when it exits, including keys the caller had + # already set under the same name. Nearly every decorated method injects names a caller is + # likely to be using itself - 'compact' and 'provider_id' above all - so without putting those + # back, returning from one of these methods silently strips the caller's context and leaves the + # rest of its logs unsearchable by those keys. + current_keys = self.logger.get_current_keys() + displaced_keys = {k: current_keys[k] for k in injected_keys if k in current_keys} + try: + with self.logger.append_context_keys(**injected_keys): + return fn(*args, **kwargs) + finally: + if displaced_keys: + self.logger.append_keys(**displaced_keys) return wrapped diff --git a/backend/compact-connect/lambdas/python/common/tests/unit/test_utils.py b/backend/compact-connect/lambdas/python/common/tests/unit/test_utils.py index 86dc48e685..a6bc8e2054 100644 --- a/backend/compact-connect/lambdas/python/common/tests/unit/test_utils.py +++ b/backend/compact-connect/lambdas/python/common/tests/unit/test_utils.py @@ -39,3 +39,106 @@ def mock_send_messages(Entries): # noqa N803 AWS defines the kwargs ) self.assertEqual([f'licenseNumber-{i}' for i in range(5)], failed_license_numbers) + + +class TestLoggerInjectKwargs(TstLambdas): + """ + Guards the logging context that decorated methods share with their callers. + + Powertools' append_context_keys removes every key it set when it exits, including keys the caller had + already set under the same name. Since nearly every decorated method injects 'compact' and 'provider_id' + - the same names handlers wrap their work in - returning from one used to silently delete the caller's + context, leaving every later log line in that handler unsearchable by those keys. + """ + + @staticmethod + def _context_keys(logger, *names): + current_keys = logger.get_current_keys() + return {name: current_keys[name] for name in names if name in current_keys} + + def test_decorated_call_preserves_context_keys_the_caller_already_set(self): + from aws_lambda_powertools import Logger + from cc_common.utils import logger_inject_kwargs + + logger = Logger(service='test_logger_inject_kwargs') + + @logger_inject_kwargs(logger, 'compact', 'provider_id') + def decorated(*, compact, provider_id): # noqa: ARG001 read from kwargs by the decorator + return 'done' + + with logger.append_context_keys(compact='aslp', provider_id='caller-provider-id'): + decorated(compact='aslp', provider_id='callee-provider-id') + + self.assertEqual( + {'compact': 'aslp', 'provider_id': 'caller-provider-id'}, + self._context_keys(logger, 'compact', 'provider_id'), + "The caller's context keys must survive a call into a decorated method", + ) + + def test_decorated_call_restores_the_caller_value_not_the_injected_one(self): + """The caller's value has to come back, not whatever the decorated method was called with.""" + from aws_lambda_powertools import Logger + from cc_common.utils import logger_inject_kwargs + + logger = Logger(service='test_logger_inject_kwargs_value') + + @logger_inject_kwargs(logger, 'provider_id') + def decorated(*, provider_id): + return provider_id + + with logger.append_context_keys(provider_id='previous-provider-id'): + decorated(provider_id='new-provider-id') + + self.assertEqual( + {'provider_id': 'previous-provider-id'}, + self._context_keys(logger, 'provider_id'), + ) + + def test_decorated_call_preserves_context_keys_when_it_raises(self): + """A method that blows up must not take the caller's logging context down with it.""" + from aws_lambda_powertools import Logger + from cc_common.utils import logger_inject_kwargs + + logger = Logger(service='test_logger_inject_kwargs_raises') + + @logger_inject_kwargs(logger, 'compact') + def decorated(*, compact): # noqa: ARG001 read from kwargs by the decorator + raise ValueError('boom') + + with logger.append_context_keys(compact='aslp'): + with self.assertRaises(ValueError): + decorated(compact='aslp') + + self.assertEqual({'compact': 'aslp'}, self._context_keys(logger, 'compact')) + + def test_injected_keys_are_still_removed_when_the_caller_never_set_them(self): + """Keys the caller did not own must not leak out of the decorated call.""" + from aws_lambda_powertools import Logger + from cc_common.utils import logger_inject_kwargs + + logger = Logger(service='test_logger_inject_kwargs_leak') + + @logger_inject_kwargs(logger, 'compact') + def decorated(*, compact): + return compact + + decorated(compact='aslp') + + self.assertEqual({}, self._context_keys(logger, 'compact')) + + def test_injected_keys_are_visible_inside_the_decorated_method(self): + """The decorator's original purpose still has to work.""" + from aws_lambda_powertools import Logger + from cc_common.utils import logger_inject_kwargs + + logger = Logger(service='test_logger_inject_kwargs_inside') + observed = {} + + @logger_inject_kwargs(logger, 'compact', 'provider_id') + def decorated(*, compact, provider_id): # noqa: ARG001 read from kwargs by the decorator + observed.update(self._context_keys(logger, 'compact', 'provider_id')) + + with logger.append_context_keys(compact='aslp', provider_id='caller-provider-id'): + decorated(compact='aslp', provider_id='callee-provider-id') + + self.assertEqual({'compact': 'aslp', 'provider_id': 'callee-provider-id'}, observed) diff --git a/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py b/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py index 204dbf2f36..341ef4436b 100644 --- a/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py +++ b/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py @@ -412,7 +412,11 @@ def _perform_ssn_correction_migration( old Cognito user deletion and re-registration email follow here. A concurrency conflict inside the migration raises, letting SQS redeliver the message after the visibility timeout. """ - with logger.append_context_keys(previous_provider_id=previous_provider_id): + with logger.append_context_keys( + previous_provider_id=previous_provider_id, + new_provider_id=new_provider_id, + license_type=license_type + ): logger.info('Performing SSN correction migration') result = config.data_client.migrate_provider_for_ssn_correction( @@ -424,23 +428,17 @@ def _perform_ssn_correction_migration( new_ssn_last_four=new_ssn_last_four, ) if not result.migration_performed: - logger.info('No records to migrate for previous provider id; proceeding with normal ingest') + logger.info( + 'No records to migrate for previous provider id; proceeding with normal ingest', + ) metrics.add_metric(name=SSN_CORRECTION_NO_MIGRATION_METRIC, unit=MetricUnit.Count, value=1) return if result.full_migration: - logger.info( - 'SSN correction resulted in a full migration', - license_type=license_type, - new_provider_id=new_provider_id, - ) + logger.info('SSN correction resulted in a full migration') metrics.add_metric(name=SSN_CORRECTION_FULL_MIGRATION_METRIC, unit=MetricUnit.Count, value=1) else: - logger.info( - 'SSN correction resulted in a partial migration', - license_type=license_type, - new_provider_id=new_provider_id, - ) + logger.info('SSN correction resulted in a partial migration') metrics.add_metric(name=SSN_CORRECTION_PARTIAL_MIGRATION_METRIC, unit=MetricUnit.Count, value=1) if result.full_migration and result.old_provider_registered_email is not None: From d0aef79d897452e1e4c8646518cbc2cdbc626461 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Fri, 21 Aug 2026 13:15:00 -0500 Subject: [PATCH 26/27] Apply marker fix to other compacts --- .../lambdas/python/common/cc_common/utils.py | 16 ++- .../python/common/tests/unit/test_utils.py | 103 ++++++++++++++++++ .../lambdas/python/common/cc_common/utils.py | 16 ++- .../python/common/tests/unit/test_utils.py | 103 ++++++++++++++++++ 4 files changed, 234 insertions(+), 4 deletions(-) diff --git a/backend/cosmetology-app/lambdas/python/common/cc_common/utils.py b/backend/cosmetology-app/lambdas/python/common/cc_common/utils.py index 314ff796cf..1f3212188c 100644 --- a/backend/cosmetology-app/lambdas/python/common/cc_common/utils.py +++ b/backend/cosmetology-app/lambdas/python/common/cc_common/utils.py @@ -232,8 +232,20 @@ def __call__(self, fn: Callable): def wrapped(*args, **kwargs): if not self.arg_names: raise ValueError('No argument names provided to logger_inject_kwargs') - with self.logger.append_context_keys(**{k: kwargs.get(k) for k in self.arg_names}): - return fn(*args, **kwargs) + injected_keys = {k: kwargs.get(k) for k in self.arg_names} + # append_context_keys removes every key it set when it exits, including keys the caller had + # already set under the same name. Nearly every decorated method injects names a caller is + # likely to be using itself - 'compact' and 'provider_id' above all - so without putting those + # back, returning from one of these methods silently strips the caller's context and leaves the + # rest of its logs unsearchable by those keys. + current_keys = self.logger.get_current_keys() + displaced_keys = {k: current_keys[k] for k in injected_keys if k in current_keys} + try: + with self.logger.append_context_keys(**injected_keys): + return fn(*args, **kwargs) + finally: + if displaced_keys: + self.logger.append_keys(**displaced_keys) return wrapped diff --git a/backend/cosmetology-app/lambdas/python/common/tests/unit/test_utils.py b/backend/cosmetology-app/lambdas/python/common/tests/unit/test_utils.py index 9e3aee13e6..413f4a3855 100644 --- a/backend/cosmetology-app/lambdas/python/common/tests/unit/test_utils.py +++ b/backend/cosmetology-app/lambdas/python/common/tests/unit/test_utils.py @@ -39,3 +39,106 @@ def mock_send_messages(Entries): # noqa N803 AWS defines the kwargs ) self.assertEqual([f'licenseNumber-{i}' for i in range(5)], failed_license_numbers) + + +class TestLoggerInjectKwargs(TstLambdas): + """ + Guards the logging context that decorated methods share with their callers. + + Powertools' append_context_keys removes every key it set when it exits, including keys the caller had + already set under the same name. Since nearly every decorated method injects 'compact' and 'provider_id' + - the same names handlers wrap their work in - returning from one used to silently delete the caller's + context, leaving every later log line in that handler unsearchable by those keys. + """ + + @staticmethod + def _context_keys(logger, *names): + current_keys = logger.get_current_keys() + return {name: current_keys[name] for name in names if name in current_keys} + + def test_decorated_call_preserves_context_keys_the_caller_already_set(self): + from aws_lambda_powertools import Logger + from cc_common.utils import logger_inject_kwargs + + logger = Logger(service='test_logger_inject_kwargs') + + @logger_inject_kwargs(logger, 'compact', 'provider_id') + def decorated(*, compact, provider_id): # noqa: ARG001 read from kwargs by the decorator + return 'done' + + with logger.append_context_keys(compact='aslp', provider_id='caller-provider-id'): + decorated(compact='aslp', provider_id='callee-provider-id') + + self.assertEqual( + {'compact': 'aslp', 'provider_id': 'caller-provider-id'}, + self._context_keys(logger, 'compact', 'provider_id'), + "The caller's context keys must survive a call into a decorated method", + ) + + def test_decorated_call_restores_the_caller_value_not_the_injected_one(self): + """The caller's value has to come back, not whatever the decorated method was called with.""" + from aws_lambda_powertools import Logger + from cc_common.utils import logger_inject_kwargs + + logger = Logger(service='test_logger_inject_kwargs_value') + + @logger_inject_kwargs(logger, 'provider_id') + def decorated(*, provider_id): + return provider_id + + with logger.append_context_keys(provider_id='previous-provider-id'): + decorated(provider_id='new-provider-id') + + self.assertEqual( + {'provider_id': 'previous-provider-id'}, + self._context_keys(logger, 'provider_id'), + ) + + def test_decorated_call_preserves_context_keys_when_it_raises(self): + """A method that blows up must not take the caller's logging context down with it.""" + from aws_lambda_powertools import Logger + from cc_common.utils import logger_inject_kwargs + + logger = Logger(service='test_logger_inject_kwargs_raises') + + @logger_inject_kwargs(logger, 'compact') + def decorated(*, compact): # noqa: ARG001 read from kwargs by the decorator + raise ValueError('boom') + + with logger.append_context_keys(compact='aslp'): + with self.assertRaises(ValueError): + decorated(compact='aslp') + + self.assertEqual({'compact': 'aslp'}, self._context_keys(logger, 'compact')) + + def test_injected_keys_are_still_removed_when_the_caller_never_set_them(self): + """Keys the caller did not own must not leak out of the decorated call.""" + from aws_lambda_powertools import Logger + from cc_common.utils import logger_inject_kwargs + + logger = Logger(service='test_logger_inject_kwargs_leak') + + @logger_inject_kwargs(logger, 'compact') + def decorated(*, compact): + return compact + + decorated(compact='aslp') + + self.assertEqual({}, self._context_keys(logger, 'compact')) + + def test_injected_keys_are_visible_inside_the_decorated_method(self): + """The decorator's original purpose still has to work.""" + from aws_lambda_powertools import Logger + from cc_common.utils import logger_inject_kwargs + + logger = Logger(service='test_logger_inject_kwargs_inside') + observed = {} + + @logger_inject_kwargs(logger, 'compact', 'provider_id') + def decorated(*, compact, provider_id): # noqa: ARG001 read from kwargs by the decorator + observed.update(self._context_keys(logger, 'compact', 'provider_id')) + + with logger.append_context_keys(compact='aslp', provider_id='caller-provider-id'): + decorated(compact='aslp', provider_id='callee-provider-id') + + self.assertEqual({'compact': 'aslp', 'provider_id': 'callee-provider-id'}, observed) diff --git a/backend/social-work-app/lambdas/python/common/cc_common/utils.py b/backend/social-work-app/lambdas/python/common/cc_common/utils.py index beb8bdfa6c..c4bee53357 100644 --- a/backend/social-work-app/lambdas/python/common/cc_common/utils.py +++ b/backend/social-work-app/lambdas/python/common/cc_common/utils.py @@ -232,8 +232,20 @@ def __call__(self, fn: Callable): def wrapped(*args, **kwargs): if not self.arg_names: raise ValueError('No argument names provided to logger_inject_kwargs') - with self.logger.append_context_keys(**{k: kwargs.get(k) for k in self.arg_names}): - return fn(*args, **kwargs) + injected_keys = {k: kwargs.get(k) for k in self.arg_names} + # append_context_keys removes every key it set when it exits, including keys the caller had + # already set under the same name. Nearly every decorated method injects names a caller is + # likely to be using itself - 'compact' and 'provider_id' above all - so without putting those + # back, returning from one of these methods silently strips the caller's context and leaves the + # rest of its logs unsearchable by those keys. + current_keys = self.logger.get_current_keys() + displaced_keys = {k: current_keys[k] for k in injected_keys if k in current_keys} + try: + with self.logger.append_context_keys(**injected_keys): + return fn(*args, **kwargs) + finally: + if displaced_keys: + self.logger.append_keys(**displaced_keys) return wrapped diff --git a/backend/social-work-app/lambdas/python/common/tests/unit/test_utils.py b/backend/social-work-app/lambdas/python/common/tests/unit/test_utils.py index a414be70c0..0d2c930d09 100644 --- a/backend/social-work-app/lambdas/python/common/tests/unit/test_utils.py +++ b/backend/social-work-app/lambdas/python/common/tests/unit/test_utils.py @@ -39,3 +39,106 @@ def mock_send_messages(Entries): # noqa N803 AWS defines the kwargs ) self.assertEqual([f'licenseNumber-{i}' for i in range(5)], failed_license_numbers) + + +class TestLoggerInjectKwargs(TstLambdas): + """ + Guards the logging context that decorated methods share with their callers. + + Powertools' append_context_keys removes every key it set when it exits, including keys the caller had + already set under the same name. Since nearly every decorated method injects 'compact' and 'provider_id' + - the same names handlers wrap their work in - returning from one used to silently delete the caller's + context, leaving every later log line in that handler unsearchable by those keys. + """ + + @staticmethod + def _context_keys(logger, *names): + current_keys = logger.get_current_keys() + return {name: current_keys[name] for name in names if name in current_keys} + + def test_decorated_call_preserves_context_keys_the_caller_already_set(self): + from aws_lambda_powertools import Logger + from cc_common.utils import logger_inject_kwargs + + logger = Logger(service='test_logger_inject_kwargs') + + @logger_inject_kwargs(logger, 'compact', 'provider_id') + def decorated(*, compact, provider_id): # noqa: ARG001 read from kwargs by the decorator + return 'done' + + with logger.append_context_keys(compact='aslp', provider_id='caller-provider-id'): + decorated(compact='aslp', provider_id='callee-provider-id') + + self.assertEqual( + {'compact': 'aslp', 'provider_id': 'caller-provider-id'}, + self._context_keys(logger, 'compact', 'provider_id'), + "The caller's context keys must survive a call into a decorated method", + ) + + def test_decorated_call_restores_the_caller_value_not_the_injected_one(self): + """The caller's value has to come back, not whatever the decorated method was called with.""" + from aws_lambda_powertools import Logger + from cc_common.utils import logger_inject_kwargs + + logger = Logger(service='test_logger_inject_kwargs_value') + + @logger_inject_kwargs(logger, 'provider_id') + def decorated(*, provider_id): + return provider_id + + with logger.append_context_keys(provider_id='previous-provider-id'): + decorated(provider_id='new-provider-id') + + self.assertEqual( + {'provider_id': 'previous-provider-id'}, + self._context_keys(logger, 'provider_id'), + ) + + def test_decorated_call_preserves_context_keys_when_it_raises(self): + """A method that blows up must not take the caller's logging context down with it.""" + from aws_lambda_powertools import Logger + from cc_common.utils import logger_inject_kwargs + + logger = Logger(service='test_logger_inject_kwargs_raises') + + @logger_inject_kwargs(logger, 'compact') + def decorated(*, compact): # noqa: ARG001 read from kwargs by the decorator + raise ValueError('boom') + + with logger.append_context_keys(compact='aslp'): + with self.assertRaises(ValueError): + decorated(compact='aslp') + + self.assertEqual({'compact': 'aslp'}, self._context_keys(logger, 'compact')) + + def test_injected_keys_are_still_removed_when_the_caller_never_set_them(self): + """Keys the caller did not own must not leak out of the decorated call.""" + from aws_lambda_powertools import Logger + from cc_common.utils import logger_inject_kwargs + + logger = Logger(service='test_logger_inject_kwargs_leak') + + @logger_inject_kwargs(logger, 'compact') + def decorated(*, compact): + return compact + + decorated(compact='aslp') + + self.assertEqual({}, self._context_keys(logger, 'compact')) + + def test_injected_keys_are_visible_inside_the_decorated_method(self): + """The decorator's original purpose still has to work.""" + from aws_lambda_powertools import Logger + from cc_common.utils import logger_inject_kwargs + + logger = Logger(service='test_logger_inject_kwargs_inside') + observed = {} + + @logger_inject_kwargs(logger, 'compact', 'provider_id') + def decorated(*, compact, provider_id): # noqa: ARG001 read from kwargs by the decorator + observed.update(self._context_keys(logger, 'compact', 'provider_id')) + + with logger.append_context_keys(compact='aslp', provider_id='caller-provider-id'): + decorated(compact='aslp', provider_id='callee-provider-id') + + self.assertEqual({'compact': 'aslp', 'provider_id': 'callee-provider-id'}, observed) From 12b42adf65e76df19d2e4f51ad08a5b73ad51df4 Mon Sep 17 00:00:00 2001 From: Landon Shumway Date: Fri, 21 Aug 2026 13:37:24 -0500 Subject: [PATCH 27/27] Add explicit log markers to migration logs --- .../provider-data-v1/handlers/ingest.py | 22 ++++++++----- .../function/test_handlers/test_ingest.py | 32 +++++++++++++++++++ 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py b/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py index 341ef4436b..b7f3f8b585 100644 --- a/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py +++ b/backend/compact-connect/lambdas/python/provider-data-v1/handlers/ingest.py @@ -412,12 +412,17 @@ def _perform_ssn_correction_migration( old Cognito user deletion and re-registration email follow here. A concurrency conflict inside the migration raises, letting SQS redeliver the message after the visibility timeout. """ - with logger.append_context_keys( - previous_provider_id=previous_provider_id, - new_provider_id=new_provider_id, - license_type=license_type - ): - logger.info('Performing SSN correction migration') + # These three identifiers are passed to every log call below *in addition to* being set on the + # surrounding context, which is deliberate rather than redundant. A migration is the one operation that + # moves a practitioner's records between two provider ids, and reconstructing what happened afterwards + # in cloudwatch depends on being able to search these lines by either id. + migration_log_fields = { + 'previous_provider_id': previous_provider_id, + 'new_provider_id': new_provider_id, + 'license_type': license_type, + } + with logger.append_context_keys(**migration_log_fields): + logger.info('Performing SSN correction migration', **migration_log_fields) result = config.data_client.migrate_provider_for_ssn_correction( compact=compact, @@ -430,15 +435,16 @@ def _perform_ssn_correction_migration( if not result.migration_performed: logger.info( 'No records to migrate for previous provider id; proceeding with normal ingest', + **migration_log_fields, ) metrics.add_metric(name=SSN_CORRECTION_NO_MIGRATION_METRIC, unit=MetricUnit.Count, value=1) return if result.full_migration: - logger.info('SSN correction resulted in a full migration') + logger.info('SSN correction resulted in a full migration', **migration_log_fields) metrics.add_metric(name=SSN_CORRECTION_FULL_MIGRATION_METRIC, unit=MetricUnit.Count, value=1) else: - logger.info('SSN correction resulted in a partial migration') + logger.info('SSN correction resulted in a partial migration', **migration_log_fields) metrics.add_metric(name=SSN_CORRECTION_PARTIAL_MIGRATION_METRIC, unit=MetricUnit.Count, value=1) if result.full_migration and result.old_provider_registered_email is not None: diff --git a/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py b/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py index 021deae33f..b4aa1cc7ec 100644 --- a/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py +++ b/backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_ingest.py @@ -1046,6 +1046,8 @@ class TestIngestSsnCorrection(TstFunction): NEW_PROVIDER_ID = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' NEW_SSN_LAST_FOUR = '6789' OLD_REGISTERED_EMAIL = 'old-provider@example.com' + # the license type on the corrected upload, from the event-bridge-message.json fixture + MIGRATED_LICENSE_TYPE = 'speech-language pathologist' # firstUploadDate tracks when a license was first uploaded; migration must carry it forward unchanged LICENSE_FIRST_UPLOAD_DATE = datetime.fromisoformat('2020-01-01T00:00:00+00:00') @@ -1293,6 +1295,36 @@ def test_partial_migration_emits_partial_migration_metric(self, mock_metrics): name='ssn-correction-partial-migration', unit=MetricUnit.Count, value=1 ) + @patch('handlers.ingest.logger') + def test_partial_migration_log_identifies_both_provider_ids(self, mock_logger): + """A partial migration leaves two live provider ids behind, so its log line has to name both. + + These fields are passed to logger.info explicitly rather than left to the surrounding context, so + asserting on the call is asserting on what the line actually carries. + """ + self._put_old_provider_records(with_second_license=True) + self._run_ingest_with_previous_provider_id() + + mock_logger.info.assert_any_call( + 'SSN correction resulted in a partial migration', + previous_provider_id=self.OLD_PROVIDER_ID, + new_provider_id=self.NEW_PROVIDER_ID, + license_type=self.MIGRATED_LICENSE_TYPE, + ) + + @patch('handlers.ingest.logger') + def test_full_migration_log_identifies_both_provider_ids(self, mock_logger): + """The full migration branch carries the same identifiers.""" + self._put_old_provider_records() + self._run_ingest_with_previous_provider_id() + + mock_logger.info.assert_any_call( + 'SSN correction resulted in a full migration', + previous_provider_id=self.OLD_PROVIDER_ID, + new_provider_id=self.NEW_PROVIDER_ID, + license_type=self.MIGRATED_LICENSE_TYPE, + ) + @patch('handlers.ingest.metrics') def test_no_op_migration_still_ingests_license_normally(self, mock_metrics): # the previousSSN resolved to a provider id with no records at all