Skip CRIS import record updates if a key column is missing a value #1437

mddilley · 2024-04-15T20:31:49Z

Associated issues

Fixes [Bug] CRIS import fails when prsn_occpnt_pos_id is null atd-data-tech#16823
See https://austininnovation.slack.com/archives/CM9MK950S/p1713190489691989

TLDR: Older records in the person and primary person tables that are missing the prsn_occpnt_pos_id are breaking the SQL that we use to query and update records that receive updates from CRIS. This is one of the columns that we use to check if a person row is unique.

Testing

URL to test:
Local

Steps to test:

Ask me for a zip file that I created that contains csvs with only one crash and its related person, primary person, and unit records that can replicate the original bug 🪄🪱 You can see our current values for the people records in the test csv with the following query (which some of these appear to be duplicates):

SELECT
	atc.crash_id,
	atc.crash_date,
	atp.prsn_nbr,
	atp.unit_nbr,
	atp.prsn_type_id,
	atp.prsn_occpnt_pos_id
FROM
	atd_txdot_person atp
	LEFT JOIN atd_txdot_crashes atc ON atp.crash_id = atc.crash_id
WHERE
	atc.crash_id = 14003860;

See readme for local testing instructions
Run the import on the zip and you should see the person records that are missing the prsn_occpnt_pos_id column value logged as skipped

Releasing this:
We should see the production image of the CR3 import update after merging this to master. GH action code

Ship list

Check migrations for any conflicts with latest migrations in master branch
Confirm Hasura role permissions for necessary access
Code reviewed
Product manager approved

mddilley · 2024-04-15T20:43:32Z

atd-etl/cris_import/cris_import.py

+                for key_column in key_columns:
+                    key_column_value = source[key_column]
+                    if key_column_value == None:
+                        print("\nSkipping because unique key column is missing")    


Super open to whatever amount of logging we want here. 🙏 I used this because it works good for testing.

this looks good to me. maybe add another print here that prints the entire source, so that we have some hope of tracking this record down?

Mike pointed out that we don't want to print people records to our logs. i retract this suggestion 🙏

chatted with John about this and decided to keep as is so we don't print out sensitive people data. 🔒

mddilley · 2024-04-15T20:44:48Z

atd-etl/cris_import/cris_import.py

            for source in imported_records:
+                # Check unique key columns to make sure they all have a value


We discussed specifically looking at people tables, but this should protect us from updating records in other tables when we don't have all the info we need to make sure we are updating the correct record.

johnclary · 2024-04-15T20:53:38Z

atd-etl/cris_import/README.md

+Drop a CRIS extract zip file into your development folder as described above, and run the import script:
+```bash
+docker compose run cris-import
+```


johnclary · 2024-04-15T20:58:05Z

atd-etl/cris_import/cris_import.py

+                # If we are missing a column that uniquely identifies the record, we should skip the update
+                if should_skip_update:
+                    for key_column in key_columns:
+                        print(f"{key_column}: {source[key_column]}")


i think you can remove this print since you have the more verbose one above 👍

since we aren't printing the source record, i'll keep this here since it will gives us the details to track down which record was skipped no matter what table it is in.

johnclary · 2024-04-15T20:58:16Z

atd-etl/cris_import/env-template

thank you!!!

johnclary

i have not tested, but this looks fantastic too me. thank you @mddilley!! 🚢 🚢 🚢 🚢 🚢

johnclary · 2024-04-15T21:00:21Z

atd-etl/cris_import/cris_import.py

+                # Check unique key columns to make sure they all have a value
+                for key_column in key_columns:
+                    key_column_value = source[key_column]
+                    if key_column_value == None:


super minor, but i always see this test written as is None instead of == None. there is some arcane pythonic reason for it that i am too lazy to look up 😅

sounds great 🐍 I'll push this up ~~along with the other minor ones above~~. Thanks! 🙏

frankhereford

Before the fix:

Traceback (most recent call last):
  File "/app/./cris_import.py", line 831, in <module>
    main()
  File "/app/./cris_import.py", line 108, in main
    align_records_token = align_records(typed_token)
  File "/app/./cris_import.py", line 622, in align_records
    if util.fetch_target_record(pg, output_map, table, record_key_sql):
  File "/app/lib/sql.py", line 169, in fetch_target_record
    cursor.execute(sql)
  File "/usr/local/lib/python3.10/dist-packages/psycopg2/extras.py", line 236, in execute
    return super().execute(query, vars)
psycopg2.errors.UndefinedColumn: column "none" does not exist
LINE 5: ...id = 2 and public.atd_txdot_person.prsn_occpnt_pos_id = None

After the fix

Finding updated records

Skipping because unique key column is missing
Table: person
Missing key column: prsn_occpnt_pos_id
crash_id: 1*******0
unit_nbr: 2
prsn_nbr: 3
prsn_type_id: 2
prsn_occpnt_pos_id: None

Skipping because unique key column is missing
Table: person
Missing key column: prsn_occpnt_pos_id
crash_id: 1********0
unit_nbr: 2
prsn_nbr: 4
prsn_type_id: 2
prsn_occpnt_pos_id: None
root@85b604a1dcaa:/app#

Perfect! 🚢🚢🚢🚢

mddilley added 3 commits April 15, 2024 14:27

Add env-template and add to README

f0f709c

Skip update if a unique key column for a record is missing

ca3ae69

Remove space and typo

fae3c9d

mddilley commented Apr 15, 2024

View reviewed changes

mddilley requested review from frankhereford, chiaberry, johnclary, tillyw and roseeichelmann April 15, 2024 20:45

johnclary reviewed Apr 15, 2024

View reviewed changes

atd-etl/cris_import/env-template Outdated

Copy link

Member

johnclary Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!!!

johnclary approved these changes Apr 15, 2024

View reviewed changes

frankhereford approved these changes Apr 15, 2024

View reviewed changes

Update check for None

fa587d9

mddilley merged commit 0a66f60 into master Apr 15, 2024
9 checks passed

mddilley deleted the md-patch-cris-import-prsn branch April 15, 2024 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip CRIS import record updates if a key column is missing a value #1437

Skip CRIS import record updates if a key column is missing a value #1437

mddilley commented Apr 15, 2024 •

edited by johnclary

Loading

mddilley Apr 15, 2024

johnclary Apr 15, 2024

johnclary Apr 15, 2024

mddilley Apr 15, 2024

mddilley Apr 15, 2024

johnclary Apr 15, 2024

johnclary Apr 15, 2024

mddilley Apr 15, 2024

johnclary Apr 15, 2024

johnclary left a comment

johnclary Apr 15, 2024

mddilley Apr 15, 2024 •

edited

Loading

frankhereford left a comment

		for source in imported_records:
		# Check unique key columns to make sure they all have a value

Skip CRIS import record updates if a key column is missing a value #1437

Skip CRIS import record updates if a key column is missing a value #1437

Conversation

mddilley commented Apr 15, 2024 • edited by johnclary Loading

Associated issues

Testing

Ship list

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnclary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mddilley Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

frankhereford left a comment

Choose a reason for hiding this comment

Before the fix:

After the fix

mddilley commented Apr 15, 2024 •

edited by johnclary

Loading

mddilley Apr 15, 2024 •

edited

Loading