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

Use model timestamps when available #18

Merged
merged 6 commits into from
Jan 29, 2025
Merged

Conversation

andrepiske
Copy link
Collaborator

@andrepiske andrepiske commented Jan 27, 2025

It changes how the value for created_at column in irontrail_changes rows is determined. Originally, it was just set to whatever postgres's NOW() function would return. This changes it so that it considers the models' timestamp attributes (created_at and updated_at). Note: it doesn't account for *_on or other name variations for timestamps.

The logic is the following:

  1. When inserting a record, it tries to use the value of the created_at column. Otherwise*, it uses the value of NOW().
  2. When updating a record, it tries to use the value of the updated_at column. Otherwise*, it uses the value of NOW().
  3. When deleting a record, it just uses the value of NOW().

Related tasks:

This is an alternative to #17 -- there could be a hybrid of both, but I feel for now let's take the simpler route of this PR only. The only "odd" case is that it would be harder to determine the creation date of irontrail_changes records in a test environment, since there's no way to mock it. But I understand that the way it is covers most uses cases with less overall complexity, while still preserving a predictable and good behavior in production environments.

@andrepiske andrepiske changed the title WIP: Use model timestamps when available Use model timestamps when available Jan 28, 2025
@andrepiske andrepiske requested review from izn, cuchi and cuzik January 28, 2025 12:57
@andrepiske andrepiske marked this pull request as ready for review January 28, 2025 12:57
@andrepiske andrepiske self-assigned this Jan 28, 2025
Copy link
Contributor

@izn izn left a comment

Choose a reason for hiding this comment

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

LGTM! 💯

@andrepiske andrepiske merged commit ed20667 into main Jan 29, 2025
7 checks passed
@andrepiske andrepiske deleted the use-model-timestamps branch January 29, 2025 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants