-
Notifications
You must be signed in to change notification settings - Fork 55
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
2119 created updated #2149
Merged
Merged
2119 created updated #2149
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
axelstudios
force-pushed
the
2119_created_updated
branch
from
March 25, 2020 21:40
0525051
to
746c83d
Compare
adrian-lara
commented
Mar 25, 2020
def forwards(apps, schema_editor): | ||
property_sql = ( | ||
'UPDATE seed_propertystate ' | ||
'SET updated = (SELECT created ' |
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.
This and the tax lot version of this change looks good - thanks for making it Alex!
axelstudios
approved these changes
Mar 25, 2020
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.
Tested, looks good to me
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
---------This PR contains a migration---------
Any background context you want to provide?
See #2118 for the 2.6.1 release version of this fix - that was more of a hot-fix.
This is presumably a more thorough fix.
From that PR:
What's this PR do?
This PR shifts users away from created and updated values coming from the canonical objects and uses the values coming from the -State objects, with the main focus being on what's provided in the inventory list and detail pages.
Also, on the inventory detail page, all the historical created and updated values are provided as well.
How should this be manually tested?
Import, merge, update, etc. records and sanity-check created and updated times on inventory list and detail pages.
On a production database, cross-check unedited, unmerged records' created and updated times with the corresponding file import time. I've spot checked this myself, but more checks from others couldn't hurt. (see Further considerations)
As far as code review, there aren't many code changes, but commits can be used to help distinguish the "reason" behind changes.
Further considerations
I was expecting to have to backfill the created and updated -State values with -related audit logs, but that didn't seem to be the case. The -State records seem to have created values that would indicate they are the actual created times when looking at corresponding import file dates - the hour might be off but I think that might be caused by timezone differences. Anyone know why the -State records looks to have valid values already?The migrations filling in these values can be found in 0111_rehash.py.Initial record edits trigger the creation of a new -State. As far as I know, users aren't relying on the created time for bulk downstream processes. For non-bulk created times analysis, since historical records now have these values, created times can be read off the "oldest" version of the record. (See screenshot below). I don't think needs to be changed but is worth noting.
What are the relevant tickets?
#2119
Screenshots (if appropriate)