Skip to content

Documented auto_now usage with bulk_update_with_history #1055

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Documented auto_now usage with bulk_update_with_history #1055

wants to merge 4 commits into from

Conversation

@twolfson
Copy link
Contributor

@twolfson twolfson commented Oct 31, 2022

Description

When using bulk_update_with_history, a DateTimeField using auto_now doesn't automatically update like it does with Model.save()

This PR documents the issue under "Common Issues" and offers a convention-based solution

Related Issue

#1054

Motivation and Context

The motivation is to spread awareness of the issue and guide people towards a solution. It seems like a rather quiet bug that can really hurt data accuracy.

How Has This Been Tested?

The code is excerpts from our code

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run the pre-commit run command to format and lint.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have added my name and/or github handle to AUTHORS.rst
  • I have added my change to CHANGES.rst
  • All new and existing tests passed.

@codecov
Copy link

codecov bot commented Oct 31, 2022 •
edited
Loading

Codecov Report

Merging #1055 (dd38fb0) into master (6d09c3e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1055   +/-   ##
=======================================
  Coverage   97.23%   97.23%           
=======================================
  Files          23       23           
  Lines        1231     1231           
  Branches      199      199           
=======================================
  Hits         1197     1197           
  Misses         16       16           
  Partials       18       18

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rossmechanic
Copy link

rossmechanic commented Dec 2, 2022

@twolfson what's the point? Don't you already have those auto_now dates on the history objects? I've traditionally ignored updated/created columns from my base model in my historical model

@twolfson
Copy link
Contributor Author

twolfson commented Dec 2, 2022 •
edited
Loading

I think there's 2 ways to interpret your response so I'll respond to both:

  • Why do you need an updated_at column on your normal model? (this is the fix I'm proposing to document)
    • This is a great question!
    • It's not used in application code so much, but it's very useful to avoid looking at extra pages or doing complex joins when looking into issues
  • Why does the historical model need an updated_at column?
    • It doesn't but this issue isn't about that. It's about the first one -- when using bulk_update_with_history, we want to automatically update the auto_now column on the normal model
    • Arguably this isn't django-simple-history responsibility, but that's why it's only being proposed as documentation, not as a code-based solution

@ddabble ddabble added the docs label May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

docs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@twolfson @rossmechanic @ddabble