Remove crashes edits layer #1572

roseeichelmann · 2024-10-07T16:10:00Z

Associated issues

Closes cityofaustin/atd-data-tech#19185
PR #1551 revealed that there is need for our data model to enable null user overrides to CRIS values. This PR is a proof of concept on the crashes tables that implements the 2 layer data model structure i proposed awhile back in the data model prototyping stage. There are only two tables, the crashes_cris table which only contains values straight from cris, and the crashes table which acts as the unified table that contains both cris values and user edits if there was a user override. This is the table that users will be updating. We are basically just nixing the whole _edits layer.

Testing

URL to test:
Local

Steps to test:

Check out this branch, get a fresh replica of prod data. Apply hasura migrations and metadata.
Lets test a user edit to the crashes table:

select rpt_street_name from crashes where id = 1;
select rpt_street_name from crashes_cris where id = 1;
update crashes set rpt_street_name = 'VZ EDIT' where id = 1;
select rpt_street_name from crashes where id = 1;

Lets test a cris update to that same record, the unified record should show the VZ edit:

update crashes_cris set rpt_street_name = 'CRIS UPDATE' where id = 1;
select rpt_street_name from crashes where id = 1;
select rpt_street_name from crashes_cris where id = 1;

Lets test a cris update to a non edited value, the unified record should show the new cris value:

select rpt_street_name from crashes where id = 2;
select rpt_street_name from crashes_cris where id = 2;
update crashes_cris set rpt_street_name = 'CRIS UPDATE' where id = 2;
select rpt_street_name from crashes where id = 2;
select rpt_street_name from crashes_cris where id = 1;

Now lets test a user null override. The unified record should show null instead of the cris value

select rpt_street_name from crashes where id = 10;
select rpt_street_name from crashes_cris where id = 10;
update crashes set rpt_street_name = null where id = 10;
select rpt_street_name from crashes where id = 10;
select rpt_street_name from crashes_cris where id = 10;

Next we can test the cris import script. You can download some extract zips from the AWS S3 bucket and put them in /vision-zero/etl/cris_import/extracts then follow the test steps in cris_import/README.md to run the cris import docker image and run the following command to process those extract zips:

./cris_import.py --csv

You can also inspect the change_log_crashes and change_log_crashes_cris and make sure you are seeing your changes there as expected
Test the down migrations

Ship list

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

roseeichelmann · 2024-10-07T21:27:50Z

database/metadata/databases/default/tables/public_crashes.yaml

@@ -17,15 +17,6 @@ object_relationships:
  - name: crashes_cri
    using:
      foreign_key_constraint_on: id
-  - name: crashes_list_view


in the next PR where changes to the VZE are addressed i will need to recreate this view without references to the edits table, same for the locations view

roseeichelmann · 2024-10-07T21:29:28Z

database/metadata/databases/default/tables/public_crashes.yaml

@@ -325,3 +316,138 @@ select_permissions:
        - position
      filter: {}
    comment: ""
+update_permissions:
+  - role: editor


crashes table now has update permissions

roseeichelmann · 2024-10-07T21:50:24Z

...e/migrations/default/1728066705364_drop_crashes_edits_update_crashes_cris_functions/down.sql

+        new.wthr_cond_id
+    );
+    -- insert new (editable) vz record (only record ID)
+    INSERT INTO public.crashes_edits (id) values (new.id);


this is the part of the function i deleted in the up migration

roseeichelmann · 2024-10-07T21:51:51Z

...ase/migrations/default/1728066705364_drop_crashes_edits_update_crashes_cris_functions/up.sql

+        -- if the new cris value doesn't match the old cris value
+        if(new_cris_jb -> column_name <> old_cris_jb -> column_name) then
+            -- see if the unified record has the same value as the old cris value
+            if (unified_record_jb -> column_name = old_cris_jb -> column_name) then


this line is updated. now we are checking in the unified record value is equal to the old cris value, if so then there hasnt been a user edit so we want update the unified record with the new cris value.

cool! this makes sense to me, and it is pretty nice to take advantage of the availability of the new and old values of the CRIS record and compare to the current crashes record to determine what is or is not an edit. 😎

It's been a while since I've looked at this code so it was helpful for me to see the change isolated to the crashes table too. 💯

roseeichelmann · 2024-10-07T21:52:25Z

...e/migrations/default/1728066705364_drop_crashes_edits_update_crashes_cris_functions/down.sql

+        -- if the new value doesn't match the old
+        if(new_cris_jb -> column_name <> old_cris_jb -> column_name) then
+            -- see if the vz record has a value for this field
+            if (edit_record_jb ->> column_name is  null) then


this is the part we changed in the up migration

johnclary

wowww—it works so well! code looks great; test steps look great. my only real feedback is related to the down migrations, which are missing some changes that were applied since we launched v2.0.

one other note wrt to testing, i added to your last test like so:

-- original test
select rpt_street_name from crashes where id = 10;
select rpt_street_name from crashes_cris where id = 10;
update crashes set rpt_street_name = null where id = 10;
select rpt_street_name from crashes where id = 10;
select rpt_street_name from crashes_cris where id = 10;
-- extra test - reset the unified value to match the _cris schema
update crashes set rpt_street_name = 'BELL' where id = 10;
-- edit the cris value
update crashes_cris set rpt_street_name = 'CRIS EDIT' where id = 10;
-- observe that the unified value is now back in sync with the cris value
select rpt_street_name from crashes where id = 10;

amazing stuff, Rose—thank you!

i'll hold off on approving this since it's now the collector branch for this feature 🚀

johnclary · 2024-10-08T15:19:56Z

...e/migrations/default/1728066705364_drop_crashes_edits_update_crashes_cris_functions/down.sql

looks like down is missing investigator_narrative_ocr_processed_at column, and also change log insert trigger is missing. it might be good to use pg_dump with the --schema-only flag to make sure we captured everything that needs to be restored. but i might be overthinking it.

ah, yeah—also the on delete constraints on these foreign key references have changed to restrict. so i think the best move is to run a fresh schema dump and copy it into this migration.

thanks for catching this -- just updated with new and improved down migra 🚀

johnclary · 2024-10-08T15:22:09Z

...e/migrations/default/1728066705364_drop_crashes_edits_update_crashes_cris_functions/down.sql

+
+drop trigger if exists insert_new_crashes_cris on public.crashes_cris;
+
+drop function if exists public.crashes_cris_insert_rows;


nbd, but i don't think you need drop the trigger of function since you're using if exists later on (ditto for the up migration)

i was able to delete this drop trigger part by adding cascade to the end of the drop function commands :)

johnclary · 2024-10-09T18:22:43Z

...e/migrations/default/1728066705364_drop_crashes_edits_update_crashes_cris_functions/down.sql

+
+SET default_tablespace = '';
+
+SET default_table_access_method = heap;


looks good! you can remove this cruft ☝️and also the statements down below that set ownership to visionzero. i think that's more of a best practice than anything.

mddilley

Awesome! I walked through your test steps, and I saw the expected results from the queries. I was able to trace the changes through the change log entries really easily as well. And, the import ran without error.

Really cool to see the power and flexibility of the json iterating triggers! 🚀 The future of nullability is looking bright!

mddilley · 2024-10-10T22:33:04Z

...ase/migrations/default/1728066705364_drop_crashes_edits_update_crashes_cris_functions/up.sql

+        -- if the new cris value doesn't match the old cris value
+        if(new_cris_jb -> column_name <> old_cris_jb -> column_name) then
+            -- see if the unified record has the same value as the old cris value
+            if (unified_record_jb -> column_name = old_cris_jb -> column_name) then


cool! this makes sense to me, and it is pretty nice to take advantage of the availability of the new and old values of the CRIS record and compare to the current crashes record to determine what is or is not an edit. 😎

It's been a while since I've looked at this code so it was helpful for me to see the change isolated to the crashes table too. 💯

Update DB docs to remove _edits layer

Update OCR narrative ETL to work without _edits layer

…ision-zero into 19366_two_layer_model_vze

Make VZE compatible with the two-layer model

…edits Remove units and people edits layers

mddilley · 2024-10-30T22:45:13Z

@johnclary @roseeichelmann I'm reviewing this now, but I wanted to make sure y'all knew that I needed to merge main branch into this one to get the local migrations working for testing.

Going through the front end right now, and it is looking good. 😎

mddilley · 2024-10-30T23:05:05Z

@roseeichelmann still reviewing but I caught one place where engineering_area needs to be updated to engineering_area_id. The fatalities view export is not working.

mddilley

I tested all of the UI with the admin role, and I only found one tiny change in the fatalities export config that I noted above. All else looks great and good to go! Happy to see all this work come together! 🙌 🎉

johnclary · 2024-10-30T23:14:45Z

thanks, Mike! wanted to mention that i am also starting a review of this based on a full replica of prod that includes the change log tables 🧑‍🔬

johnclary

Aside from the issue Mike flagged, everything looks great to me. I think it would be good to merge main before we merge back, just for good measure.

🚢

i fixed it

johnclary · 2024-10-31T16:43:20Z

I patched this, removed Mike's review request, and i'm merging it now

roseeichelmann added 4 commits October 7, 2024 11:07

remove crashes edits layer metadata related stuff

e1bf846

drop crashes edits table and crashes edits change log

ce7b658

update the cris crash insert trigger function

753540e

update the cris crash update trigger function

1bf450f

roseeichelmann added the WIP Work in progress label Oct 7, 2024

roseeichelmann added 6 commits October 7, 2024 11:14

update this variable name to unified_record for clarity

b02db85

delete these migrations

36b11c7

delete these migrations

e5759fa

delete these migrations

413c1f7

consolidate migrations into one file

ec7195c

change order of migra file

e5b4f54

roseeichelmann commented Oct 7, 2024

View reviewed changes

didnt mean to delete this view from being tracked

74fafb7

roseeichelmann commented Oct 7, 2024

View reviewed changes

roseeichelmann requested review from johnclary, frankhereford, mateoclarke, chiaberry, tillyw and mddilley October 7, 2024 21:52

roseeichelmann removed the WIP Work in progress label Oct 7, 2024

johnclary reviewed Oct 8, 2024

View reviewed changes

update up/down migras

43b56e2

roseeichelmann requested a review from johnclary October 9, 2024 18:08

johnclary reviewed Oct 9, 2024

View reviewed changes

remove cruft

653078c

mddilley reviewed Oct 10, 2024

View reviewed changes

roseeichelmann and others added 17 commits October 23, 2024 16:24

update this wording

7f762ab

update diagram

daaf15d

update this wording

3079a08

updates to wording

d03c530

fix typos, make things more clear

0a1cc63

export new datamodel excalidraw and delete old ones

638774c

rename data_model png and delete unused/old db_overview

e1bee6c

fix path to data model diagram

9674a87

make it clear that ordering matters

81f9602

remove this

ded1fde

Merge pull request #1587 from cityofaustin/19367_update_docs_two_layer

87a693f

Update DB docs to remove _edits layer

Merge pull request #1586 from cityofaustin/19534_update_ocr_narrative

ee554f1

Update OCR narrative ETL to work without _edits layer

Restore aggregation permissions

2963ccd

Merge branch '19366_two_layer_model_vze' of github.com:cityofaustin/v…

6e7b8bd

…ision-zero into 19366_two_layer_model_vze

Restore aggregation queries for locations view

9bb5ba1

Merge pull request #1582 from cityofaustin/19366_two_layer_model_vze

3d9aa7b

Make VZE compatible with the two-layer model

Merge pull request #1578 from cityofaustin/19398_remove_units_people_…

597ea55

…edits Remove units and people edits layers

roseeichelmann requested review from mddilley and johnclary October 30, 2024 21:48

mddilley previously requested changes Oct 30, 2024

View reviewed changes

Merge branch 'main' into 19185_null_overrides_spike

16c8626

johnclary approved these changes Oct 30, 2024

View reviewed changes

fix engineering area field in fatality view export

d3e4b03

johnclary merged commit 9498ef0 into main Oct 31, 2024
4 of 5 checks passed

johnclary deleted the 19185_null_overrides_spike branch October 31, 2024 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove crashes edits layer #1572

Remove crashes edits layer #1572

roseeichelmann commented Oct 7, 2024 •

edited

Loading

roseeichelmann Oct 7, 2024 •

edited

Loading

roseeichelmann Oct 7, 2024

roseeichelmann Oct 7, 2024

roseeichelmann Oct 7, 2024

mddilley Oct 10, 2024

roseeichelmann Oct 7, 2024

johnclary left a comment •

edited

Loading

johnclary Oct 8, 2024

johnclary Oct 8, 2024 •

edited

Loading

roseeichelmann Oct 9, 2024

johnclary Oct 8, 2024

roseeichelmann Oct 9, 2024 •

edited

Loading

johnclary Oct 9, 2024 •

edited

Loading

mddilley left a comment •

edited

Loading

mddilley Oct 10, 2024

mddilley commented Oct 30, 2024

mddilley commented Oct 30, 2024 •

edited

Loading

mddilley left a comment

johnclary commented Oct 30, 2024

johnclary left a comment

johnclary commented Oct 31, 2024


		drop trigger if exists insert_new_crashes_cris on public.crashes_cris;

		drop function if exists public.crashes_cris_insert_rows;


		SET default_tablespace = '';

		SET default_table_access_method = heap;

Remove crashes edits layer #1572

Remove crashes edits layer #1572

Conversation

roseeichelmann commented Oct 7, 2024 • edited Loading

Associated issues

Testing

Ship list

roseeichelmann Oct 7, 2024 • edited Loading

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 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnclary Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roseeichelmann Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

johnclary Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

mddilley left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mddilley commented Oct 30, 2024

mddilley commented Oct 30, 2024 • edited Loading

mddilley left a comment

Choose a reason for hiding this comment

johnclary commented Oct 30, 2024

johnclary left a comment

Choose a reason for hiding this comment

johnclary commented Oct 31, 2024

roseeichelmann commented Oct 7, 2024 •

edited

Loading

roseeichelmann Oct 7, 2024 •

edited

Loading

johnclary left a comment •

edited

Loading

johnclary Oct 8, 2024 •

edited

Loading

roseeichelmann Oct 9, 2024 •

edited

Loading

johnclary Oct 9, 2024 •

edited

Loading

mddilley left a comment •

edited

Loading

mddilley commented Oct 30, 2024 •

edited

Loading