Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Create versioning for segments #4138
feat: Create versioning for segments #4138
Changes from 13 commits
2ad4381
f6746e1
8df7f91
a582253
730d064
42f93a9
c7eb529
f06cb3d
641c02d
d40d5ed
7341357
41f086d
ed56775
922001d
4842576
b6bbf72
8ed85c4
a37def6
934d2cd
17b2c9d
558bc40
c76211d
1eb9469
7bb7384
ec6f40b
4481e4b
669a079
34e7e46
8f38dcd
e5c7b47
70e974a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have clone functions in other models in the code base where we take a slightly different approach, e.g.:
The benefit of this approach is that it maintains itself when new fields are added to those model classes. With the approach that you're using here, if a new field is added to the segment, we'll need to remember to add it here I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I'll implement that strategy. Just to double check, the timestamps will still be set to
now
for the new model, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok this is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question - I don't know. We should verify in a test. Although I guess the question is whether we want that to be the case. Since we're backfilling the history, wouldn't we want e.g. the
created_at
not to change?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it turns out that none of the models involved in this PR have
created_at
orupdated_at
timestamps, although they probably should, at least on the segments. Should I add them to this PR? If so, what should the default be for existing objectsNone
ornow()
? Should I include conditions and rules in having timestamps as well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add these fields to all models in the segments app, yes, but let's do it in a separate PR. Regarding the default, we could be smart and look in the objects history but that might end up being a fairly mammoth migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sounds good I've made a new ticket for this.
#4211
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is up for the timestamps:
#4236