Update Provider top-level 'providerDateOfUpdate' timestamps when privileges are modified#1331
Merged
isabeleliassen merged 15 commits intocsg-org:mainfrom Apr 7, 2026
Merged
Conversation
In order for states to programmatically poll for providers that have privileges within their state through the State 'two-way' API, the system relies on a 'providerDateOfUpdate' field to be updated on the provider record whenever a change occurs on their record. Currently, when a privilege is purchased, we are updating several fields on the provider record, but not the 'providerDateOfUpdate' field, resulting in providers not being returned in the API when states go to query for providers, unless they have another update take place from somewhere else in the system. A state reported an issue where a provider purchased privileges in their state on 3/15, but because that individual has not had any other updates to their records, their 'providerDateOfUpdate' field is set to 02/26 before the privilege was purchased. Which means that the expected provider is not returned from the 'two-way' API within the expected time range. This updates all the locations where we are performing updates that impact the privilege status so that we also update the 'providerDateOfUpdate' field on the top-level provider record so that the provider will be returned in the 'two-way' API when states query for provider updates.
Contributor
📝 WalkthroughWalkthroughTop-level provider-record update paths were changed to set Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_privileges.py (1)
307-339: Consider reusing_request_deactivation_with_scopesin this new test.The request assembly here largely duplicates existing helper logic; reusing the helper would keep setup consistent and reduce drift risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_privileges.py` around lines 307 - 339, The test test_deactivate_privilege_updates_provider_date_of_update duplicates request assembly logic; replace the manual event construction with a call to the existing helper _request_deactivation_with_scopes to keep setup consistent: locate the test method and remove the manual manipulation of event['requestContext']['authorizer']['claims']['scope'], ['sub'], event['pathParameters'] and event['body'] and instead call _request_deactivation_with_scopes(...) (the helper that builds the event with scopes, sub, pathParameters and body) passing the same scope string 'openid email aslp/admin', TEST_STAFF_USER_ID, DEFAULT_PROVIDER_ID and TEST_NOTE so the rest of the test (calling deactivate_privilege and assertions) remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py`:
- Around line 910-917: The current fix updates providerDateOfUpdate only in
end_military_affiliation(), but other update paths still set only dateOfUpdate;
update the DynamoDB UpdateExpression and ExpressionAttributeValues in
complete_military_affiliation_initialization() and process_military_audit() so
they also set providerDateOfUpdate to
self.config.current_standard_datetime.isoformat() (add providerDateOfUpdate to
the SET clause and add ':providerDateOfUpdate' to ExpressionAttributeValues),
ensuring all write paths keep providerDateOfUpdate in sync with dateOfUpdate.
- Around line 910-917: The UpdateExpression blocks currently call
self.config.current_standard_datetime.isoformat() twice, risking different
timestamps; capture the timestamp once into a local variable (e.g., ts =
self.config.current_standard_datetime.isoformat()) in the method where the
DynamoDB update is built and use that single ts for both ':dateOfUpdate' and
':providerDateOfUpdate' ExpressionAttributeValues (and anywhere ':dateOfUpdate'
/ ':providerDateOfUpdate' pairs are set). Apply the same single-capture pattern
to the other affected update sites mentioned (the similar blocks that set
':dateOfUpdate' and ':providerDateOfUpdate' at the other occurrences) so both
attributes always receive the identical timestamp.
- Around line 1588-1600: The provider item update currently only sets
providerDateOfUpdate when provider_encumbered_status changes, so child-record
encumbrance changes can leave the top-level provider timestamp stale and break
get_providers_sorted_by_updated(); modify the update logic around
provider_data.serialize_to_database_record() to always write a provider-only
timestamp update (set providerDateOfUpdate = current_standard_datetime) for
every encumbrance/lift transaction, and when provider_encumbered_status actually
changes fold that status change into the same UpdateExpression (e.g., set
providerDateOfUpdate = :providerDateOfUpdate, dateOfUpdate = :dateOfUpdate and
conditionally include encumberedStatus = :status); ensure
ExpressionAttributeValues include the timestamp value and status only when used
so the provider item is always touched even if its summary status is unchanged.
In
`@backend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py`:
- Around line 1249-1272: The test
test_post_purchase_privileges_updates_provider_date_of_update is currently
relying on setup (process_registration_values) having already set dateOfUpdate
to the expected value; before invoking post_purchase_privileges you should seed
the provider item with an older timestamp so the update is meaningful: use the
test helper _provider_table to update the provider record (pk
f'{TEST_COMPACT}#PROVIDER#{TEST_PROVIDER_ID}', sk f'{TEST_COMPACT}#PROVIDER')
and set dateOfUpdate and providerDateOfUpdate to an earlier value (e.g.,
2000-01-01T00:00:00+00:00) in setUp or just before calling
post_purchase_privileges, then assert that post_purchase_privileges changed them
to the expected_datetime.
---
Nitpick comments:
In
`@backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_privileges.py`:
- Around line 307-339: The test
test_deactivate_privilege_updates_provider_date_of_update duplicates request
assembly logic; replace the manual event construction with a call to the
existing helper _request_deactivation_with_scopes to keep setup consistent:
locate the test method and remove the manual manipulation of
event['requestContext']['authorizer']['claims']['scope'], ['sub'],
event['pathParameters'] and event['body'] and instead call
_request_deactivation_with_scopes(...) (the helper that builds the event with
scopes, sub, pathParameters and body) passing the same scope string 'openid
email aslp/admin', TEST_STAFF_USER_ID, DEFAULT_PROVIDER_ID and TEST_NOTE so the
rest of the test (calling deactivate_privilege and assertions) remains
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 535a8690-e228-4658-8f99-25fde01abbff
📒 Files selected for processing (6)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/lambdas/python/data-events/tests/function/test_home_jurisdiction_events.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_privileges.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users.pybackend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Show resolved
Hide resolved
...ct-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py`:
- Around line 530-536: The timestamp variable now is defined inside the
jurisdiction loop but later reused for the provider-level update (the block
constructing 'UpdateExpression' and 'ExpressionAttributeValues' containing
':dateOfUpdate' and ':providerDateOfUpdate'), which causes UnboundLocalError if
the loop never ran; fix by assigning now = datetime.datetime.utcnow() (or
equivalent) before entering the jurisdiction loop, and add a guard that fails
fast (raise ValueError or return with an explicit error) if
jurisdiction_postal_abbreviations is empty prior to building the transaction so
the provider-level update never references an undefined now.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6af0ec25-2b2a-4c7f-a17d-97c19644d4a8
📒 Files selected for processing (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.pybackend/compact-connect/lambdas/python/purchases/tests/function/test_handlers/test_purchase_privileges.py
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Show resolved
Hide resolved
Collaborator
|
This test is failing for me locally but doesnt seem to be here so not sure whats going on there: test_compact_admin_can_deactivate_privilege |
ChiefStief
approved these changes
Apr 6, 2026
Collaborator
ChiefStief
left a comment
ChiefStief
left a comment
There was a problem hiding this comment.
Looks good to me, I think the outstanding code rabbit comment is correct and I had the one test failing but other than that, ready to go.
Collaborator
Author
|
@ChiefStief I am not able to reproduce the test failure for 'test_compact_admin_can_deactivate_privilege'. Everything looks good there so it is likely a local environment problem that we can take a look at later. I have also run our smoke test suite against the endpoints that were modified and verified the tests continue to pass. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/compact-connect/tests/smoke/home_jurisdiction_change_smoke_tests.py (1)
253-265:⚠️ Potential issue | 🟡 MinorIncorrect state referenced in comments: 'ar' is Arkansas, not Alabama.
The postal abbreviation
'ar'corresponds to Arkansas, not Alabama (which is'al'). The docstring and inline comment both incorrectly reference Alabama.📝 Proposed fix to correct the comments
- NOTE: this test assumes that the compact your test provider is registered in has Alabama as a live state in - its configuration, if that state is not live in your environment for that compact, you will need to add it or + NOTE: this test assumes that the compact your test provider is registered in has Arkansas as a live state in + its configuration, if that state is not live in your environment for that compact, you will need to add it or change the jurisdiction """ logger.info('Running home jurisdiction change test - changing to jurisdiction with valid license') # Get the provider's information before making any changes provider_info_before = call_provider_users_me_endpoint() original_jurisdiction = provider_info_before.get('currentHomeJurisdiction') original_expiration_date = provider_info_before['licenses'][0]['dateOfExpiration'] - new_jurisdiction = 'ar' # Alabama - assuming the provider doesn't have a license here + new_jurisdiction = 'ar' # Arkansas - assuming the provider doesn't have a license here🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/compact-connect/tests/smoke/home_jurisdiction_change_smoke_tests.py` around lines 253 - 265, The comment and variable in the home jurisdiction change smoke test incorrectly reference Alabama but use the Arkansas postal code; update the test so the docstring and inline comment match the actual postal code by changing the new_jurisdiction value from 'ar' to 'al' and update any surrounding text that says "Alabama" or "assuming the provider doesn't have a license here" to accurately reflect the chosen state; look for the new_jurisdiction variable and the test docstring/comment in home_jurisdiction_change_smoke_tests.py to make these edits.
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
443-467:⚠️ Potential issue | 🟡 MinorPass the shared batch timestamp into
_generate_privilege_record().
nowis only reused for the privilege update rows and the top-level provider update._generate_privilege_record()still callsconfig.current_standard_datetimeinternally fordateOfRenewaland newdateOfIssuance, so one purchase transaction can still persist slightly different timestamps across the privilege rows versus the provider/update rows.Based on learnings: in `create_provider_privileges`, a single captured timestamp should be reused so the privilege records, privilege update records, and top-level provider update share the same timestamp within one transaction.💡 Suggested change
- privilege_record: PrivilegeData = self._generate_privilege_record( + privilege_record: PrivilegeData = self._generate_privilege_record( compact=compact, provider_id=provider_id, jurisdiction_postal_abbreviation=postal_abbreviation, license_expiration_date=license_expiration_date, compact_transaction_id=compact_transaction_id, attestations=attestations, license_type=license_type, license_jurisdiction=license_jurisdiction, original_privilege=original_privilege, + current_datetime=now, )Update
_generate_privilege_record()to accept and usecurrent_datetimeinstead of reading the clock again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py` around lines 443 - 467, The code captures now in create_provider_privileges but still lets _generate_privilege_record call config.current_standard_datetime, causing inconsistent timestamps; update _generate_privilege_record signature to accept a current_datetime (or current_standard_datetime) parameter, change its internal uses of config.current_standard_datetime for dateOfRenewal and new dateOfIssuance to use that parameter, and in create_provider_privileges pass the captured now into the _generate_privilege_record(...) call so privilege records, privilege update rows, and the provider update all share the same timestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py`:
- Around line 1502-1515: The Update operations that target top-level provider
records (sk == f'{compact}#PROVIDER') must include a condition to prevent
accidental upserts; add "ConditionExpression": "attribute_exists(pk)" to the
Update dicts in the four places named: create_provider_privileges,
deactivate_privilege, _generate_set_provider_encumbered_status_item, and
_get_provider_record_transaction_items_for_jurisdiction_with_no_known_license so
the DynamoDB Update fails if the provider pk does not already exist (apply the
same pattern used elsewhere in the file).
---
Outside diff comments:
In `@backend/compact-connect/tests/smoke/home_jurisdiction_change_smoke_tests.py`:
- Around line 253-265: The comment and variable in the home jurisdiction change
smoke test incorrectly reference Alabama but use the Arkansas postal code;
update the test so the docstring and inline comment match the actual postal code
by changing the new_jurisdiction value from 'ar' to 'al' and update any
surrounding text that says "Alabama" or "assuming the provider doesn't have a
license here" to accurately reflect the chosen state; look for the
new_jurisdiction variable and the test docstring/comment in
home_jurisdiction_change_smoke_tests.py to make these edits.
---
Duplicate comments:
In
`@backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py`:
- Around line 443-467: The code captures now in create_provider_privileges but
still lets _generate_privilege_record call config.current_standard_datetime,
causing inconsistent timestamps; update _generate_privilege_record signature to
accept a current_datetime (or current_standard_datetime) parameter, change its
internal uses of config.current_standard_datetime for dateOfRenewal and new
dateOfIssuance to use that parameter, and in create_provider_privileges pass the
captured now into the _generate_privilege_record(...) call so privilege records,
privilege update rows, and the provider update all share the same timestamp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de1ed5f9-9838-4968-a9e3-a8c49b1659b4
📒 Files selected for processing (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.pybackend/compact-connect/tests/smoke/home_jurisdiction_change_smoke_tests.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.py
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Show resolved
Hide resolved
jlkravitz
reviewed
Apr 7, 2026
Collaborator
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
few small comments + plus one or two unresolved coderabbit comments!
...act-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
Show resolved
Hide resolved
...ct/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.py
Show resolved
Hide resolved
backend/compact-connect/tests/smoke/home_jurisdiction_change_smoke_tests.py
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
443-467:⚠️ Potential issue | 🟡 MinorUse the batch
nowfor the privilege rows too.Line 443 hoists a single purchase timestamp, but
_generate_privilege_record()still re-readsconfig.current_standard_datetimefordateOfRenewaland newdateOfIssuance. If the loop crosses a second boundary, the privilege rows drift from the provider/update rows this refactor is trying to keep aligned.Based on learnings, `create_provider_privileges` should reuse one captured timestamp across privilege records, privilege update records, and the top-level provider update within the same transaction.♻️ Suggested change
- def _generate_privilege_record( + def _generate_privilege_record( self, compact: str, provider_id: str, jurisdiction_postal_abbreviation: str, license_expiration_date: date, compact_transaction_id: str, attestations: list[dict], license_type: str, license_jurisdiction: str, + current_datetime: datetime, original_privilege: PrivilegeData | None = None, ) -> PrivilegeData: - current_datetime = config.current_standard_datetime try: license_type_abbreviation = self.config.license_type_abbreviations[compact][license_type] ... privilege_record: PrivilegeData = self._generate_privilege_record( compact=compact, provider_id=provider_id, jurisdiction_postal_abbreviation=postal_abbreviation, license_expiration_date=license_expiration_date, compact_transaction_id=compact_transaction_id, attestations=attestations, license_type=license_type, license_jurisdiction=license_jurisdiction, + current_datetime=now, original_privilege=original_privilege, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py` around lines 443 - 467, The loop in create_provider_privileges captures a single timestamp (now) but _generate_privilege_record still reads config.current_standard_datetime internally, causing timestamps to drift; modify _generate_privilege_record to accept a timestamp argument (e.g., captured_now or batch_now) and use that for dateOfRenewal and dateOfIssuance (and any other uses of config.current_standard_datetime), then pass the already-captured now from create_provider_privileges into each call so all privilege rows and the provider/update rows share the exact same timestamp.
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
533-546:⚠️ Potential issue | 🟠 MajorGuard top-level provider
Updates withattribute_exists(pk).This provider write is still unconditional. If the top-level provider row is missing or concurrently deleted, DynamoDB will upsert a skeletal
#PROVIDERitem and the transaction will succeed with corrupted state instead of failing fast. The same pattern still exists at Line 1504, Line 1604, and Line 3009.Based on learnings, expected provider-record writes in this codebase should fail fast on missing provider rows because that represents a data consistency bug, not something to silently tolerate.🛡️ Suggested change
'Update': { 'TableName': self.config.provider_table_name, 'Key': { 'pk': {'S': f'{compact}#PROVIDER#{provider_id}'}, 'sk': {'S': f'{compact}#PROVIDER'}, }, 'UpdateExpression': 'ADD `#privilegeJurisdictions` :newJurisdictions ' 'SET dateOfUpdate = :dateOfUpdate, providerDateOfUpdate = :providerDateOfUpdate', + 'ConditionExpression': 'attribute_exists(pk)', 'ExpressionAttributeNames': {'#privilegeJurisdictions': 'privilegeJurisdictions'}, 'ExpressionAttributeValues': { ':newJurisdictions': {'SS': jurisdiction_postal_abbreviations}, ':dateOfUpdate': {'S': now.isoformat()}, ':providerDateOfUpdate': {'S': now.isoformat()},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py` around lines 533 - 546, The Update call that writes the top-level provider item (the dict using self.config.provider_table_name with Key pk=f'{compact}#PROVIDER#{provider_id}' and sk=f'{compact}#PROVIDER') must be made conditional so it fails if the provider row is missing; add a ConditionExpression like "attribute_exists(pk)" (and/or include attribute_exists(sk) for extra safety) to that Update request so the transaction aborts instead of upserting a skeletal item, and apply the same change to the other provider Update occurrences referenced (the similar Update blocks at the other locations cited).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py`:
- Around line 443-467: The loop in create_provider_privileges captures a single
timestamp (now) but _generate_privilege_record still reads
config.current_standard_datetime internally, causing timestamps to drift; modify
_generate_privilege_record to accept a timestamp argument (e.g., captured_now or
batch_now) and use that for dateOfRenewal and dateOfIssuance (and any other uses
of config.current_standard_datetime), then pass the already-captured now from
create_provider_privileges into each call so all privilege rows and the
provider/update rows share the exact same timestamp.
---
Duplicate comments:
In
`@backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py`:
- Around line 533-546: The Update call that writes the top-level provider item
(the dict using self.config.provider_table_name with Key
pk=f'{compact}#PROVIDER#{provider_id}' and sk=f'{compact}#PROVIDER') must be
made conditional so it fails if the provider row is missing; add a
ConditionExpression like "attribute_exists(pk)" (and/or include
attribute_exists(sk) for extra safety) to that Update request so the transaction
aborts instead of upserting a skeletal item, and apply the same change to the
other provider Update occurrences referenced (the similar Update blocks at the
other locations cited).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1220dc8f-8b83-4966-9fa7-0be20d7f1738
📒 Files selected for processing (3)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.pybackend/compact-connect/tests/smoke/home_jurisdiction_change_smoke_tests.py
✅ Files skipped from review due to trivial changes (1)
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_provider_users_email.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/tests/smoke/home_jurisdiction_change_smoke_tests.py
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
531-548:⚠️ Potential issue | 🟠 MajorRollback currently undoes the new existence check.
If this top-level provider
Updatefails onattribute_exists(pk), theexcept ClientErrorpath still calls_rollback_privilege_transactions(), and that helper alwaysPutsprovider_recordback. That recreates the missing provider item from cached state instead of surfacing the integrity failure this condition was added to catch.🛡️ Suggested fix
def _rollback_privilege_transactions( self, processed_transactions: list[dict], provider_record: ProviderData, license_type: str, existing_privileges_for_license_type: list[PrivilegeData], ): """Roll back successful privilege transactions after a failure.""" rollback_transactions = [] + provider_record_db = provider_record.serialize_to_database_record() + provider_update_key = { + 'pk': {'S': provider_record_db['pk']}, + 'sk': {'S': provider_record_db['sk']}, + } @@ - rollback_transactions.append( - { - 'Put': { - 'TableName': self.config.provider_table_name, - 'Item': TypeSerializer().serialize(provider_record.serialize_to_database_record())['M'], - } - } - ) + if any(item.get('Update', {}).get('Key') == provider_update_key for item in processed_transactions): + rollback_transactions.append( + { + 'Put': { + 'TableName': self.config.provider_table_name, + 'Item': TypeSerializer().serialize(provider_record_db)['M'], + } + } + )Based on learnings, when a provider record is expected to exist, a missing provider record should fail fast as a data-consistency issue rather than being recreated from cached state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py` around lines 531 - 548, The provider Update uses a conditional 'attribute_exists(pk)' but the except ClientError path always calls _rollback_privilege_transactions() which unconditionally Put's provider_record; change the error handling so that if the ClientError is a ConditionalCheckFailedException originating from the provider Update (detect by checking error.response['Error']['Code']=='ConditionalCheckFailedException' and/or by inspecting the failing transaction matching the Update with 'attribute_exists(pk)'), do not call _rollback_privilege_transactions() to re-create provider_record and instead re-raise or return an integrity error; alternatively modify _rollback_privilege_transactions() to accept a flag (e.g., skip_put_provider) and pass skip_put_provider=True when the failure is the attribute_exists(pk) check so the cached provider is not restored.
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
443-467: 🛠️ Refactor suggestion | 🟠 MajorThread the shared
nowinto_generate_privilege_record().This loop now reuses one timestamp for the privilege update records and the top-level provider update, but each privilege record still reads
config.current_standard_datetimeinside_generate_privilege_record(). A single purchase can still persist slightly different timestamps across records that are meant to represent the same transaction.♻️ Suggested fix
def _generate_privilege_record( self, compact: str, provider_id: str, jurisdiction_postal_abbreviation: str, license_expiration_date: date, compact_transaction_id: str, attestations: list[dict], license_type: str, license_jurisdiction: str, + current_datetime: datetime, original_privilege: PrivilegeData | None = None, ) -> PrivilegeData: - current_datetime = config.current_standard_datetime try: license_type_abbreviation = self.config.license_type_abbreviations[compact][license_type] @@ privilege_record: PrivilegeData = self._generate_privilege_record( compact=compact, provider_id=provider_id, jurisdiction_postal_abbreviation=postal_abbreviation, license_expiration_date=license_expiration_date, compact_transaction_id=compact_transaction_id, attestations=attestations, license_type=license_type, license_jurisdiction=license_jurisdiction, + current_datetime=now, original_privilege=original_privilege, )Based on learnings, in
create_provider_privileges,now = config.current_standard_datetimeshould be assigned once before the jurisdiction loop so all privilege records, privilege update records, and the top-level provider record update share the same timestamp within a single transaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py` around lines 443 - 467, create_provider_privileges reads config.current_standard_datetime into now but _generate_privilege_record still calls config.current_standard_datetime internally, causing inconsistent timestamps; pass the already-read now into _generate_privilege_record (add a now parameter) and use that value inside _generate_privilege_record instead of calling config.current_standard_datetime, so all records created by create_provider_privileges (privilege records, privilege update records, and provider update) share the same timestamp; update call sites of _generate_privilege_record within create_provider_privileges to supply the now variable and adjust the _generate_privilege_record signature and internal usage accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py`:
- Around line 531-548: The provider Update uses a conditional
'attribute_exists(pk)' but the except ClientError path always calls
_rollback_privilege_transactions() which unconditionally Put's provider_record;
change the error handling so that if the ClientError is a
ConditionalCheckFailedException originating from the provider Update (detect by
checking error.response['Error']['Code']=='ConditionalCheckFailedException'
and/or by inspecting the failing transaction matching the Update with
'attribute_exists(pk)'), do not call _rollback_privilege_transactions() to
re-create provider_record and instead re-raise or return an integrity error;
alternatively modify _rollback_privilege_transactions() to accept a flag (e.g.,
skip_put_provider) and pass skip_put_provider=True when the failure is the
attribute_exists(pk) check so the cached provider is not restored.
---
Duplicate comments:
In
`@backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py`:
- Around line 443-467: create_provider_privileges reads
config.current_standard_datetime into now but _generate_privilege_record still
calls config.current_standard_datetime internally, causing inconsistent
timestamps; pass the already-read now into _generate_privilege_record (add a now
parameter) and use that value inside _generate_privilege_record instead of
calling config.current_standard_datetime, so all records created by
create_provider_privileges (privilege records, privilege update records, and
provider update) share the same timestamp; update call sites of
_generate_privilege_record within create_provider_privileges to supply the now
variable and adjust the _generate_privilege_record signature and internal usage
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50cea1a2-4b1a-484e-bd0a-f97596e15537
📒 Files selected for processing (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py (1)
488-494: Minor formatting inconsistency.The closing brace placement at line 493 (
})) differs from the pattern used in the other test setups (e.g., lines 155-158 where})is on a separate line). This is a nitpick and doesn't affect functionality.🔧 Optional: Consistent formatting
# add provider record self.test_data_generator.put_default_provider_record_in_provider_table( - value_overrides={ - 'providerId': provider_uuid, - 'privilegeJurisdictions': {} - }) + value_overrides={ + 'providerId': provider_uuid, + 'privilegeJurisdictions': {} + } + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py` around lines 488 - 494, Move the closing brace for the value_overrides dict onto its own line to match the project’s test formatting pattern: update the call to put_default_provider_record_in_provider_table so that the dictionary passed to value_overrides (and the closing parenthesis) places the final "}" and ")" on separate lines; locate the invocation of put_default_provider_record_in_provider_table that uses value_overrides with provider_uuid and adjust the brace/paren placement to mirror the other test setups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py`:
- Around line 488-494: Move the closing brace for the value_overrides dict onto
its own line to match the project’s test formatting pattern: update the call to
put_default_provider_record_in_provider_table so that the dictionary passed to
value_overrides (and the closing parenthesis) places the final "}" and ")" on
separate lines; locate the invocation of
put_default_provider_record_in_provider_table that uses value_overrides with
provider_uuid and adjust the brace/paren placement to mirror the other test
setups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4040a43a-87db-4c02-9b66-dbb149230171
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/common/tests/function/test_data_client.pybackend/compact-connect/tests/smoke/smoke_common.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/tests/smoke/smoke_common.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py (1)
155-158: Useset()for emptyprivilegeJurisdictionsoverrides.
{}is an empty dict in Python, not an empty set. TheprivilegeJurisdictionsfield in the provider schema is defined withSet(String, load_default=set()), and all test assertions expect set objects. For consistency with the schema semantics and with other usage in this test file (e.g., line 526), useset()instead.♻️ Suggested patch
- provider_record = self.test_data_generator.put_default_provider_record_in_provider_table( - value_overrides={'privilegeJurisdictions': {}} - ) + provider_record = self.test_data_generator.put_default_provider_record_in_provider_table( + value_overrides={'privilegeJurisdictions': set()} + ) - self.test_data_generator.put_default_provider_record_in_provider_table( - value_overrides={'providerId': provider_uuid, 'privilegeJurisdictions': {}} - ) + self.test_data_generator.put_default_provider_record_in_provider_table( + value_overrides={'providerId': provider_uuid, 'privilegeJurisdictions': set()} + ) - self.test_data_generator.put_default_provider_record_in_provider_table( - value_overrides={'providerId': provider_uuid, 'privilegeJurisdictions': {}} - ) + self.test_data_generator.put_default_provider_record_in_provider_table( + value_overrides={'providerId': provider_uuid, 'privilegeJurisdictions': set()} + )Also applies to: 252-255, 485-488
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py` around lines 155 - 158, The test is passing an empty dict for privilegeJurisdictions but the provider schema and assertions expect a set; update calls to put_default_provider_record_in_provider_table (and any similar test_data_generator invocations that set 'privilegeJurisdictions') to use value_overrides={'privilegeJurisdictions': set()} instead of {} (also fix the two other occurrences in the same test file where {} is used for privilegeJurisdictions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py`:
- Around line 155-158: The test is passing an empty dict for
privilegeJurisdictions but the provider schema and assertions expect a set;
update calls to put_default_provider_record_in_provider_table (and any similar
test_data_generator invocations that set 'privilegeJurisdictions') to use
value_overrides={'privilegeJurisdictions': set()} instead of {} (also fix the
two other occurrences in the same test file where {} is used for
privilegeJurisdictions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 577eda6d-7901-4cd8-9fb6-ff569ed56d81
📒 Files selected for processing (1)
backend/compact-connect/lambdas/python/common/tests/function/test_data_client.py
jlkravitz
approved these changes
Apr 7, 2026
Collaborator
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
@isabeleliassen This is good to merge. Once merged, I'll do a prod deploy.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In order for states to programmatically poll for providers that have privileges within their state through the State 'two-way' API, the system relies on a 'providerDateOfUpdate' field to be updated on the provider record whenever a change occurs on their record. Currently, when a privilege is purchased, we are updating several fields on the provider record, but not the 'providerDateOfUpdate' field, resulting in providers not being returned in the API when states go to query for providers, unless they have another update take place from somewhere else in the system.
A state reported an issue where a provider purchased privileges in their state on 3/15, but because that individual has not had any other updates to their records, their 'providerDateOfUpdate' field is set to 02/26 before the privilege was purchased. Which means that the expected provider is not returned from the 'two-way' API within the expected time range.
This updates all the locations where we are performing updates that impact the privilege status so that we also update the 'providerDateOfUpdate' field on the top-level provider record so that the provider will be returned in the 'two-way' API when states query for provider updates.
Requirements List
Description List
Closes #1330
Summary by CodeRabbit
Bug Fixes
Tests
Chores