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

Standarize timestamp fields #4864

Open
humitos opened this issue Nov 5, 2018 · 13 comments
Open

Standarize timestamp fields #4864

humitos opened this issue Nov 5, 2018 · 13 comments
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Milestone

Comments

@humitos
Copy link
Member

humitos commented Nov 5, 2018

Currently, we have pub_date, date, modified_date, add_date, created_dt and updated_dt.

I suggest to use created_at and updated_at for all the models.

This is useful for #4286

NOTE: for those that do not have these fields I suggest to add them. Example, Version.

@humitos humitos added the Improvement Minor improvement to code label Nov 5, 2018
@humitos humitos added this to the Refactoring milestone Nov 5, 2018
@humitos humitos added the Needed: design decision A core team decision is required label Nov 5, 2018
@ericholscher
Copy link
Member

Yea, we should just have a subclass that standardizes these fields, and everything inherits from.

@ericholscher
Copy link
Member

I can probably implement it in #4850

@ericholscher
Copy link
Member

Actually, I don't want to hold up that PR on this design decision, and we'll require code migrations anyways, so I'll hold off until we decide on names, etc.

@humitos humitos added Accepted Accepted issue on our roadmap and removed Needed: design decision A core team decision is required labels Nov 8, 2018
@humitos
Copy link
Member Author

humitos commented Nov 8, 2018

Considering that we are already using django-extensions I suggest to go with

TimeStampedModel - An Abstract Base Class model that provides self-managed created and modified fields.

https://github.com/django-extensions/django-extensions/blob/master/django_extensions/db/models.py#L14

When implementing this, we will need a migration that migrate the old datetimes values from old fields name to the new names.

@dojutsu-user
Copy link
Member

dojutsu-user commented Nov 9, 2018

Can i work on this?

and does all models needs to inherits from TimeStampedModel ??

When implementing this, we will need a migration that migrate the old datetimes values from old fields name to the new names.

and i didn't get the meaning of this line, does it mean to provide the update migrations file??

@dojutsu-user
Copy link
Member

@ericholscher
Copy link
Member

You should be able to just do the ./manage.py makemigrations and it will handle the rename of the field, I think.

@dojutsu-user
Copy link
Member

@ericholscher
aren't we going to make every model inherit from TimeStampedModel ??

@ericholscher
Copy link
Member

I think we shouldn't do every model, but just those that have either an updated or modified time. Maybe it makes sense to do an audit of them.

@ericholscher
Copy link
Member

I believe some parts of the data are also exposed in public API's (eg. the Build object) so I'm not sure the best migration path for some of this stuff.

@davidfischer
Copy link
Contributor

I believe some parts of the data are also exposed in public API's (eg. the Build object) so I'm not sure the best migration path for some of this stuff.

Since it's a read-only field on just a couple models in the v2 API, worst case we can just use a SerializerMethodField.

@dojutsu-user
Copy link
Member

Can this be made more clear as what needs to be done.
I will start working on this.

@humitos
Copy link
Member Author

humitos commented Jan 13, 2019

My proposal to solve this issue is (in this order):

  1. write some tests for APIv2 endpoints that returns datetime fields
  2. inherit all the models that have a created/modified field (named in some way) from TimeStampedModel
  3. add a migration file for this
  4. manipulate the migration file to use "RenameField" instead of "AddField" and "RemoveField" if it's not detected manually
  5. check all the endpoints from APIv2 that were returning any of these date/datetime fields and make it return the new value with the old field name (please, left a comment in the code about this)

After that, all the tests should pass and everything should continue working properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Projects
None yet
Development

No branches or pull requests

4 participants