-
Notifications
You must be signed in to change notification settings - Fork 4
Cleanup crash address formatting #1908
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Deploy Preview for atd-vze-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
johnclary
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, these addresses look soo good! Thank you for taking care of all of this—there are so many parts of our codebase to update and refactor as a result of this mega improvement 🙌
I think my feedback is pretty minor. I did not run any of our ETLs, but i'll probably do a test run of the CRIS import and Socrata exporter just to be safe.
database/metadata/databases/default/tables/public_crashes_list_view.yaml
Show resolved
Hide resolved
| id | ||
| address_primary | ||
| address_secondary | ||
| address_display |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
database/metadata/databases/default/tables/public_locations_list_view.yaml
Outdated
Show resolved
Hide resolved
database/metadata/databases/default/tables/public_socrata_export_crashes_view.yaml
Show resolved
Hide resolved
database/migrations/default/1767430748998_crash_address_display_trigger/up.sql
Show resolved
Hide resolved
| CREATE INDEX idx_crashes_address_display ON crashes(address_display); | ||
|
|
||
| -- Function to format highway addresses | ||
| CREATE OR REPLACE FUNCTION format_highway_address( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love having these two separate functions for highways vs streets. Really easy to follow what's happening 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed!
| -- Only add if not a duplicate of previous (case-insensitive) | ||
| IF UPPER(current_part) != UPPER(prev_part) THEN | ||
| deduped_parts := array_append(deduped_parts, current_part); | ||
| END IF; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works so well—nice!
|
|
||
|
|
||
| -- Main function to generate complete crash address | ||
| CREATE OR REPLACE FUNCTION generate_crash_address( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a strongly held opinion, but I think it'd be fine to simplify this function signature to accept a crash record, rather than itemizing every single column input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im gonna leave it as is 😅
| return `${crash.address_primary ? crash.address_primary : ""} ${ | ||
| crash.address_secondary ? "& " + crash.address_secondary : "" | ||
| }`; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
chiaberry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is such a giant quality improvement and so many cases to handle! I am still reviewing and wrapping my head around the SQL, but I came across this crash 21182993 and the address says "TODD LN & E BEN WHITE BLVD EB BLVD" and I dont know if it slipped through the formatting or the input is to blame.
✅ Deploy Preview for atd-vze-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…ove unnecessary if statement
|
@chiaberry unfortunately we are only really able to handle repeats that come right after each other like RD RD. so in the case you shared the input is just so messed up and its gonna have to be manually fixed |
|
Alright i updated my migration to replace |
chiaberry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rose this is such a massive undertaking and huge quality improvement. 🥇
| - created_at | ||
| - created_at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roseeichelmann I'm still reviewing but it looks some column names were duplicated probably through the merging. I know how tedious the metadata can get!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh so frustrating i have no idea how that happened! thank you for catching this and your thorough review of the code 🙏
mddilley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll approve this after the metadata column duplicates that I noted are removed, but this is amazing! Love the steps to dedupe and also the highway vs. streets treatment. Easy to follow and looks ready for any future adjustments if CRIS changes anything up on us. 🚀
Also a big upgrade to have less address processing logic in the front end code. 🍄 🙌
| -- Drop old columns | ||
|
|
||
| ALTER TABLE crashes DROP COLUMN address_primary CASCADE; | ||
|
|
||
| ALTER TABLE crashes DROP COLUMN address_secondary CASCADE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i had forgotten that these weren't CRIS columns at all - goodbye old address columns! 👋
mateoclarke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great Rose. Monster up migration but such a huge improvement. I bet this was tedious but it look so much better! 🚀
mddilley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢 ✨✨✨
johnclary
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test steps check out and I tested the CRIS import and socrata export ETLs. Everything looks great. Thanks, Rose! 🚢 🚢 🚢 🚢
Rose has addressed the issues! Thank you!

Associated issues
Closes cityofaustin/atd-data-tech#6972
Testing
URL to test:
Local
Steps to test:
address_displayand then refresh the locations materialized view that also will need to update w the newaddress_displayaddress_displayis used, so on the fatalities map view, all the list pages, also on the EMS details page under "Possible CR3 people matches"Ship list
mainbranch