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 created to group membership model #9184

Merged
merged 1 commit into from
Dec 6, 2024
Merged

Conversation

seanh
Copy link
Contributor

@seanh seanh commented Dec 4, 2024

Add a created column to models.GroupMembership.

Fixes #9103. This should automatically mean that new group memberships get their created and updated times tracked, without any further changes.

Before we can merge this PR we'll need a migration to add the new columns.

Then after merging this PR we'll need another PR to add the created date to the responses from various APIs.

While we're doing this, might as well add updated as well.

There's a desire for this column to be nullable so that pre-existing
memberships can have NULL in this column, see:
https://hypothes-is.slack.com/archives/C4K6M7P5E/p1733330444569829

For that reason we can't use h.db.mixins.Timestamps because that mixin
adds non-nullable created and updated columns.

I don't think it really matters but if we did change the
GroupMembership model to inherit from the Timestamps mixin that
would add the created and updated columns to the start of the
table, whereas in the staging and production DBs a migration is going to
add the new columns to the end of the table. Meaning that DBs created
from the models (e.g. in dev and on CI) would actually have the columns
in a different order than staging and production. Not sure if there
would be a way to fix the column order while using mixins.Timestamps.
Moot point anyway since we need them to be nullable.

@seanh seanh requested a review from marcospri December 4, 2024 17:59
Copy link
Member

@marcospri marcospri left a comment

Choose a reason for hiding this comment

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

Looks good 👍

We do need the migration before merging this as you say.

@seanh seanh force-pushed the membership-created-model branch from 559b469 to 032a781 Compare December 5, 2024 17:47
@seanh seanh requested a review from marcospri December 5, 2024 18:47
Add a `created` column to `models.GroupMembership`.

While we're doing this, might as well add `updated` as well.

There's a desire for this column to be nullable so that pre-existing
memberships can have `NULL` in this column, see:
https://hypothes-is.slack.com/archives/C4K6M7P5E/p1733330444569829

For that reason we can't use `h.db.mixins.Timestamps` because that mixin
adds *non-nullable* `created` and `updated` columns.

For the same reason we can't use `server_default` as it would set the
pre-existing rows to the current time.

I don't think it really matters but if we did change the
`GroupMembership` model to inherit from the `Timestamps` mixin that
would add the `created` and `updated` columns to the _start_ of the
table, whereas in the staging and production DBs a migration is going to
add the new columns to the _end_ of the table. Meaning that DBs created
from the models (e.g. in dev and on CI) would actually have the columns
in a different order than staging and production. Not sure if there
would be a way to fix the column order while using `mixins.Timestamps`.
Moot point anyway since we need them to be nullable.
@seanh seanh force-pushed the membership-created-model branch from 032a781 to ef890d3 Compare December 6, 2024 09:24
@seanh seanh removed the Do not merge label Dec 6, 2024
@seanh seanh merged commit fdf3d6b into main Dec 6, 2024
9 checks passed
@seanh seanh deleted the membership-created-model branch December 6, 2024 09:54
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.

Record creation times of group memberships
2 participants