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.

Sign up for GitHub

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

Jump to bottom

removes micromobility device flag #1284

Merged
merged 5 commits into from
Sep 7, 2023
Merged

removes micromobility device flag #1284

merged 5 commits into from
Sep 7, 2023

Conversation

tillyw
Copy link
Contributor

@tillyw tillyw commented Aug 31, 2023

Associated issues

fixes cityofaustin/atd-data-tech#12012

this is the last step in dropping the micromobility_device_flag column from vzd and representing it as a mode like all others. this was so straightforward thanks to @mddilley's amazing work setting up hasura migrations on vzd! this should also be really simple to test and integrate into the next release. the automated migrations are already paying off!

Testing

URL to test:
local

Steps to test:

  • start up a local instance of vze
  • go to any crash page - the 'Micromobility device flag' column should no longer appear in the flags table
  • connect to your local database using a sql client or ./vision-zero hasura-console
  • open the atd_txdot_crashes table and note that the micromobility_device_flag column no longer appears

Ship list

  • Code reviewed
  • Product manager approved

@mddilley
Copy link
Contributor

mddilley commented Aug 31, 2023 •
edited
Loading

@tillyw yeah!!! a migration! 🚀 I wrote up some steps for what happens on merge here that I plan to put somewhere. I'm thinking that it could go in the /atd-vzd/README.md (which i just realized is now outdated 🙃). Let me know if you have any thoughts on where it would be helpful to have that info. 🙏

mddilley
mddilley reviewed Sep 5, 2023
View reviewed changes
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.

One small change needed, and this is good to go! Thanks for diving right into a migration. It was really helpful to have this to test out the steps in #1285. 🙏

...16393057_alter_table_public_atd_txdot_crashes_drop_column_micromobility_device_flag/down.sql Outdated
@@ -0,0 +1 @@
alter table "public"."atd_txdot_crashes" add "column micromobility_device_flag" character varying(1) DEFAULT 'N'::character varying NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested running this down migration and ran into an error with some syntax in here. column from add column made its way into the column name, and there is a comma at the end of the statement instead of a semicolon. I tested it with those changes, and it works perfectly!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for catching those typos! i fixed them and confirmed the migration worked

atd-vzd/README.md Outdated
@@ -35,7 +35,7 @@ We use a standard metabase container for business intelligence analytics as well

2. Once you changes have been made, copy the SQL statement that modified the table to our [migrations folder](/migrations) for version control. Example:
```sql
ALTER TABLE "public"."atd_txdot_crashes" ADD COLUMN "micromobility_device_flag" varchar(1) NOT NULL DEFAULT 'N';
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow - what are the odds that this exact column was used in this doc!

Copy link
Contributor Author

@tillyw tillyw Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

i know!! thanks for updating the doc!

@tillyw
Copy link
Contributor Author

tillyw commented Sep 7, 2023 •
edited
Loading

@tillyw yeah!!! a migration! 🚀 I wrote up some steps for what happens on merge here that I plan to put somewhere. I'm thinking that it could go in the /atd-vzd/README.md (which i just realized is now outdated 🙃). Let me know if you have any thoughts on where it would be helpful to have that info. 🙏

the readme is the perfect place! thank you for writing up those steps 🙏

@tillyw tillyw requested a review from mddilley September 7, 2023 22:27
mddilley
mddilley approved these changes Sep 7, 2023
View reviewed changes
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.

Works great! Tested the up and down migrations locally, and I see that the micromobility device flag is gone from the crash page. Mode normalization fully complete. ✅ 🎉🚢

@tillyw
Copy link
Contributor Author

tillyw commented Sep 7, 2023

@md

Works great! Tested the up and down migrations locally, and I see that the micromobility device flag is gone from the crash page. Mode normalization fully complete. ✅ 🎉🚢

yay thanks @mddilley !!

@tillyw tillyw merged commit f1d8feb into master Sep 7, 2023
11 checks passed
@tillyw tillyw deleted the 12012_remove_micromobility_device_flag branch September 22, 2023 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mddilley mddilley mddilley approved these changes

@roseeichelmann roseeichelmann Awaiting requested review from roseeichelmann

@frankhereford frankhereford Awaiting requested review from frankhereford

@patrickm02L patrickm02L Awaiting requested review from patrickm02L

@chiaberry chiaberry Awaiting requested review from chiaberry

@johnclary johnclary Awaiting requested review from johnclary

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove micromobility device flag from Flags table
2 participants
@tillyw @mddilley