Skip to content

Conversation

@samredai
Copy link
Contributor

@samredai samredai commented May 14, 2025

Summary

This changes the enum type for activity type and entity type to a string, only in the database model. This will let us modify/extend the history events in the source code more easily without having to do database migrations.

Test Plan

Deployment Plan

@netlify
Copy link

netlify bot commented May 14, 2025

Deploy Preview for thriving-cassata-78ae72 canceled.

Name Link
🔨 Latest commit 8083a48
🔍 Latest deploy log https://app.netlify.com/sites/thriving-cassata-78ae72/deploys/6824bec8b73eae0008abdffc

with op.batch_alter_table("history", schema=None) as batch_op:
batch_op.alter_column(
"entity_type",
existing_type=postgresql.ENUM(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also drop the enum types from Postgres? Maybe with something like:

batch_op.execute("DROP TYPE IF EXISTS entitytype")
batch_op.execute("DROP TYPE IF EXISTS activitytype")

Copy link
Member

@agorajek agorajek left a comment

Choose a reason for hiding this comment

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

@samredai this looks good, but I think you should try running this locally and post the outcome on the PR. Just left one comment inline.

entity_type: Mapped[Optional[EntityType]] = mapped_column(
Enum(EntityType),
entity_type: Mapped[Optional[str]] = mapped_column(
String(20),
Copy link
Member

Choose a reason for hiding this comment

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

Why only 20?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason, I just thought that the postgres default of infinite length seemed unnecessary for what will just be a string representation of an enum value. Should I make this larger? Or maybe don't specify the length at all?

@samredai
Copy link
Contributor Author

@samredai this looks good, but I think you should try running this locally and post the outcome on the PR. Just left one comment inline.

Good idea, let me populate it with enum values locally then run a migration and see what happens to the data.

@agorajek
Copy link
Member

@samredai is this still on your radar? That seems like a good idea.

@samredai
Copy link
Contributor Author

@agorajek thanks for the reminder comment. Time was tight this past week but I'll try to do this testing soon and get this in.

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