PR feedback

Author: landonshumway-ia

backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py

@@ -1065,7 +1065,7 @@ def _get_privilege_update_records_directly(
 
         return [PrivilegeUpdateData.from_database_record(item) for item in response_items]
 
-    @logger_inject_kwargs(logger, 'compact', 'provider_id', 'detail', 'jurisdiction', 'license_type')
+    @logger_inject_kwargs(logger, 'compact', 'provider_id', 'detail', 'jurisdiction', 'license_type_abbr')
     def get_privilege_data(
         self,
         *,
@@ -1117,7 +1117,7 @@ def get_privilege_data(
 
         return result
 
-    @logger_inject_kwargs(logger, 'compact', 'provider_id', 'jurisdiction', 'license_type')
+    @logger_inject_kwargs(logger, 'compact', 'provider_id', 'jurisdiction', 'license_type_abbr')
     def deactivate_privilege(
         self, *, compact: str, provider_id: str, jurisdiction: str, license_type_abbr: str, deactivation_details: dict
     ) -> None:

backend/compact-connect/lambdas/python/disaster-recovery/handlers/rollback_license_upload.py

@@ -181,7 +181,7 @@ def from_dict(cls, data: dict) -> 'RollbackResults':
                         RevertedLicense(
                             jurisdiction=reverted_license['jurisdiction'],
                             license_type=reverted_license['licenseType'],
-                            revision_id=uuid4(),
+                            revision_id=reverted_license['revisionId'],
                             action=reverted_license['action'],
                         )
                         for reverted_license in summary.get('licensesReverted', [])
@@ -190,7 +190,7 @@ def from_dict(cls, data: dict) -> 'RollbackResults':
                         RevertedPrivilege(
                             jurisdiction=reverted_privilege['jurisdiction'],
                             license_type=reverted_privilege['licenseType'],
-                            revision_id=uuid4(),
+                            revision_id=reverted_privilege['revisionId'],
                             action=reverted_privilege['action'],
                         )
                         for reverted_privilege in summary.get('privilegesReverted', [])
@@ -723,7 +723,7 @@ def add_delete(pk: str, sk: str, update_record: bool):
                         IneligibleUpdate(
                             record_type='privilegeUpdate',
                             type_of_update=privilege_update.updateType,
-                            update_time=privilege_update.dateOfUpdate.isoformat(),
+                            update_time=privilege_update.createDate.isoformat(),
                             license_type=privilege_update.licenseType,
                             # include privilege jurisdiction in reason
                             reason=f"Privilege in jurisdiction '{privilege_update.jurisdiction}' was updated "
@@ -736,7 +736,7 @@ def add_delete(pk: str, sk: str, update_record: bool):
                         IneligibleUpdate(
                             record_type='privilegeUpdate',
                             type_of_update=privilege_update.updateType,
-                            update_time=privilege_update.dateOfUpdate.isoformat(),
+                            update_time=privilege_update.createDate.isoformat(),
                             license_type=privilege_update.licenseType,
                             # include privilege jurisdiction in reason
                             reason=f"Privilege in jurisdiction '{privilege_update.jurisdiction}' was deactivated "
@@ -839,6 +839,11 @@ def add_delete(pk: str, sk: str, update_record: bool):
             )
         # license was not first uploaded during the upload window, revert it to last previous state before the upload
         else:
+            # if the provider is ineligible for rollback, the list of license updates may be empty, and we need to
+            # defensively check for that here and continue to the next license
+            if not license_updates_in_window:
+                continue
+
             # Find the earliest update in the window to get the previous state
             license_updates_in_window.sort(key=lambda x: x.createDate)
             earliest_update_in_window = license_updates_in_window[0]
@@ -1037,7 +1042,7 @@ def _write_results_to_s3(key: str, results: RollbackResults):
         )
         logger.info('Results written to S3', bucket=config.disaster_recovery_results_bucket_name, key=key)
     # handle json serialization errors
-    except json.JSONDecodeError as e:
+    except TypeError as e:
         logger.error(f'Error writing results to S3: {str(e)}')
         raise
     # handle other errors by logging the full object and raising the exception

backend/compact-connect/lambdas/python/disaster-recovery/tests/function/test_rollback_license_upload.py

@@ -1088,6 +1088,14 @@ def test_rollback_handles_pagination_when_provider_id_present_in_event_input(sel
         self.assertEqual(0, result_first['providersFailed'])
         self.assertEqual(mock_second_provider_id, result_first['continueFromProviderId'])
 
+        # Verify: S3 results contain first provider with revision id
+        s3_key = f'licenseUploadRollbacks/{MOCK_EXECUTION_NAME}/results.json'
+        s3_obj = self.config.s3_client.get_object(Bucket=self.config.disaster_recovery_results_bucket_name, Key=s3_key)
+        first_results_data = json.loads(s3_obj['Body'].read().decode('utf-8'))
+
+        # grab the revision id from the results which we will use when asserting on the final object
+        revision_id = first_results_data['revertedProviderSummaries'][0]['licensesReverted'][0]['revisionId']
+
         # Execute: Second invocation (continue from where we left off)
         # Reset mock time for second invocation
         mock_time.time.side_effect = [0, 1]  # Won't timeout this time
@@ -1117,7 +1125,7 @@ def test_rollback_handles_pagination_when_provider_id_present_in_event_input(sel
                                 'action': 'REVERT',
                                 'jurisdiction': 'oh',
                                 'licenseType': 'speech-language pathologist',
-                                'revisionId': ANY,
+                                'revisionId': revision_id,
                             }
                         ],
                         'privilegesReverted': [],
@@ -1132,6 +1140,7 @@ def test_rollback_handles_pagination_when_provider_id_present_in_event_input(sel
                                 'action': 'REVERT',
                                 'jurisdiction': 'oh',
                                 'licenseType': 'speech-language pathologist',
+                                # unknown random UUID, we won't check for it here
                                 'revisionId': ANY,
                             }
                         ],

backend/compact-connect/lambdas/python/migration/migrate_update_sort_keys/main.py

@@ -18,7 +18,7 @@ def on_create(self, properties: dict) -> None:
 
     def on_update(self, properties: dict) -> None:
         """
-        No-op on delete.
+        No-op on update.
         """
 
     def on_delete(self, _properties: dict) -> CustomResourceResponse | None:
@@ -33,9 +33,10 @@ def on_delete(self, _properties: dict) -> CustomResourceResponse | None:
 def do_migration(_properties: dict) -> None:
     """
     This migration performs the following:
-    - Scans the provider table for all privilege update records
-    - For each update record, adds effectiveDate and createDate equal to that updates dateOfUpdate
-    - Handles batching for cases where there are more than 100 records to update
+    - Scans the provider table for all update records
+    - For each update record, load the records and serialize it again,
+        so the schema classes will generate the new sort key patterns
+    - Recreate the records by deleting the update records with the old sort key and storing the migrated records.
     """
     logger.info('Starting update record sort key migration')
 

backend/compact-connect/lambdas/python/provider-data-v1/tests/function/__init__.py

@@ -166,7 +166,10 @@ def create_provider_table(self):
                         {'AttributeName': 'licenseUploadDateGSIPK', 'KeyType': 'HASH'},
                         {'AttributeName': 'licenseUploadDateGSISK', 'KeyType': 'RANGE'},
                     ],
-                    'Projection': {'ProjectionType': 'KEYS_ONLY'},
+                    'Projection': {
+                        'ProjectionType': 'INCLUDE',
+                        'NonKeyAttributes': ['providerId'],
+                    },
                 },
             ],
         )

backend/compact-connect/tests/smoke/rollback_license_upload_smoke_tests.py

@@ -178,7 +178,7 @@ def wait_for_all_providers_created(staff_headers: dict, expected_count: int, max
 
     last_key = None
     page_num = 1
-    all_provider_ids = []
+    all_provider_ids: set[str] = set()
     while time.time() - start_time < max_wait_time:
         # Collect all providers across all pages
         while True:
@@ -203,9 +203,9 @@ def wait_for_all_providers_created(staff_headers: dict, expected_count: int, max
             providers = response_data.get('providers', [])
             pagination = response_data.get('pagination', {})
 
-            # Collect provider IDs from this page
+            # Collect provider IDs from this page and add to set
             page_provider_ids = [p['providerId'] for p in providers]
-            all_provider_ids.extend(page_provider_ids)
+            all_provider_ids.update(page_provider_ids)
 
             logger.info(
                 f'Page {page_num}: Found {len(page_provider_ids)} providers '
@@ -227,7 +227,7 @@ def wait_for_all_providers_created(staff_headers: dict, expected_count: int, max
 
         if num_found >= expected_count:
             logger.info(f'All {expected_count} providers found!')
-            return all_provider_ids  # Return only the expected count
+            return list(all_provider_ids)  # Return only the expected count
 
         elapsed = time.time() - start_time
         if elapsed < max_wait_time:
@@ -325,7 +325,6 @@ def get_rollback_results_from_s3(results_s3_key: str):
     Retrieve rollback results from S3.
 
     :param results_s3_key: S3 URI or key to the results file
-    :param bucket_name: S3 bucket name
     :return: Parsed results data
     """
     s3_client = boto3.client('s3')
@@ -632,7 +631,6 @@ def rollback_license_upload_smoke_test():
             street_address='123 Test Street',
         )
         first_upload_end_time = datetime.now(tz=UTC)
-
         logger.info(
             f'First upload time window: {first_upload_start_time.isoformat()} to {first_upload_end_time.isoformat()}'
         )
@@ -641,6 +639,7 @@ def rollback_license_upload_smoke_test():
         logger.info('=' * 80)
         logger.info('Waiting for first upload providers and license records to be created...')
         logger.info('=' * 80)
+        time.sleep(10)
         wait_for_all_providers_created(staff_headers, len(uploaded_licenses))
         logger.info('✅ All first upload license records have been created')