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

fix: analytics_logs.node_type should reference text component type rather than integer for new and historic records #2578

Merged
merged 4 commits into from
Dec 19, 2023

Conversation

jessicamcinchak
Copy link
Member

@jessicamcinchak jessicamcinchak commented Dec 18, 2023

Changes:

  • Alter the column type from integer to text (requires dropping & recreating dependent view)
  • Do a one-time update of historic records
  • Update analytics provider to insert text / human-readable node_type for all new analytics_logs created

Dependencies:

  • Once merged to production, I expect this will break a number of Metabase queries that currently reference the integer node_type in a condition - we'll have to update these independently !

@jessicamcinchak
Copy link
Member Author

Copy link

github-actions bot commented Dec 18, 2023

Removed vultr server and associated DNS entries

@jessicamcinchak jessicamcinchak marked this pull request as ready for review December 18, 2023 19:08
@jessicamcinchak jessicamcinchak requested a review from a team December 18, 2023 19:08
Copy link
Contributor

@DafyddLlyr DafyddLlyr left a comment

Choose a reason for hiding this comment

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

Perfect! A much better solution that my original thought here 👌

Tested and getting the expected results on Pizza 🎉

image image

WHEN node_type = '10' THEN 'DrawBoundary'
WHEN node_type = '11' THEN 'PlanningConstraints'
WHEN node_type = '12' THEN 'PropertyInformation'
WHEN node_type = '100' THEN 'Statement'
Copy link
Contributor

Choose a reason for hiding this comment

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

This has always felt like a bit of a tricky one - if we get feedback that this is oddly named we can very easily update to "Question" or something in future 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep agree - that's probably a change we should consider making at the root type definition rather than just analytics! Hopefully ambiguous text is still a huge upgrade from integer for starters though!

Copy link
Contributor

@Mike-Heneghan Mike-Heneghan 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 🥳

@jessicamcinchak jessicamcinchak merged commit 287e12c into main Dec 19, 2023
12 checks passed
@jessicamcinchak jessicamcinchak deleted the jess/change-analytics-logs-node-type branch December 19, 2023 09:40
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