Skip to content
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

Add total person comp cost to the crash details page #1513

Merged
merged 4 commits into from
Aug 20, 2024

Conversation

roseeichelmann
Copy link
Contributor

@roseeichelmann roseeichelmann commented Aug 19, 2024

Associated issues

Closes cityofaustin/atd-data-tech#18683

Testing

URL to test:
Local data model

Steps to test:

  1. See the new " Total Est. Comprehensive Cost" in the crash details page and the renamed "Maximum Est. Comprehensive Cost"

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

- cris_fatality_count
- est_comp_cost_crash_based
- est_total_person_comp_cost
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just added this and other stuff got rearranged

@@ -41,6 +41,7 @@
- "!include lookups_veh_mod.yaml"
- "!include lookups_wthr_cond.yaml"
- "!include public__column_metadata.yaml"
- "!include public__cris_import_log.yaml"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk why this moved 🤷

Copy link
Contributor

@mddilley mddilley left a comment

Choose a reason for hiding this comment

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

Tested locally, and the new field looks great in the UI. The values that I see in the crash page match up with what I'm seeing in crash_injury_metrics_view too. 🚢 😎

Copy link
Contributor

Choose a reason for hiding this comment

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

Took me a second to understand the difference between these two fields but the DB showed me the way ✨

Copy link
Member

@johnclary johnclary left a comment

Choose a reason for hiding this comment

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

Looks great! just the smallest possible change needed

format: "dollars",
},
est_total_person_comp_cost: {
label: " Total Est. Comprehensive Cost",
Copy link
Member

@johnclary johnclary Aug 20, 2024

Choose a reason for hiding this comment

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

Looks like this leading whitespace space can be removed

@johnclary johnclary self-requested a review August 20, 2024 15:09
Copy link
Member

@johnclary johnclary left a comment

Choose a reason for hiding this comment

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

i fixed the whitespace and am gonna merge! 🙏

@johnclary johnclary merged commit 6cf4545 into data-model-v2 Aug 20, 2024
@johnclary johnclary deleted the 18683_person_comp_cost branch August 21, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants