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

models: fix EventRecord default saved_at timestamp type #171

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cedricco
Copy link
Contributor

We used the fancy https://docs.djangoproject.com/fr/2.2/ref/contrib/postgres/functions/#transactionnow
it returns the START of the database transaction.
This is actually not a desirable behaviour for a saved_at field. We want the time
at which it was recorded, not when the enclosing transaction begun, which may be
an hour ago for example with a very long job.

Note: I think TransactionNow would be useful if we had a timestamp to
assess the time validity of an object. In that case, knowing when the transaction
that wrote it started would indeed be nice.

We used the fancy https://docs.djangoproject.com/fr/2.2/ref/contrib/postgres/functions/#transactionnow
it returns the START of the database transaction.
This is actually not a desirable behaviour for a `saved_at` field. We want the time
at which it was recorded, not when the enclosing transaction begun, which may be
an hour ago for example with a very long job.

Note: I think TransactionNow would be useful if we had a timestamp to
assess the time validity of an object. In that case, knowing when the transaction
that wrote it started would indeed be nice.
name='saved_at',
field=models.DateTimeField(auto_now=True, db_index=True),
),
]
Copy link
Contributor

Choose a reason for hiding this comment

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

AlterField migrations are post-deploy.

@@ -5,6 +5,7 @@ Changelog
-------------------

- Aggregator field on the Application admin was wrongly required.
- Fix default `saved_at` for event records
Copy link
Contributor

Choose a reason for hiding this comment

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

You're not in par with the latest master.

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.

2 participants