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

Support for the DateTime field where we have set auto_now as True #194

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

pradeep-sukhwani
Copy link

@pradeep-sukhwani pradeep-sukhwani commented Feb 9, 2022

Description

Currently the django-dirtyfields detects the fields which have been updated. This changes is for the fields which gets updated once we save the object. Fields such as last_updated = models.DateTime(auto_now=True) doesn't get detected. So to make sure that such fields get detected, we have added a check and updated the field's value. We have also made sure that it consider the timezone if the support for timezone is enabled.

Test case

  • New test cases

Pradeep Sukhwani added 3 commits February 9, 2022 14:01
calling .save_dirty_fields() will also check if any DateTime field has auto_now=True
@LincolnPuzey
Copy link
Collaborator

LincolnPuzey commented Feb 28, 2022

Hi thanks for the PR,

Is there a particular reason or example code for why you need this change?

@sidmitra
Copy link

sidmitra commented Feb 28, 2022

@LincolnPuzey Appreciate your response

The main reason is we've been evaluating using django-dirtyfields instead of using .save(update_fields=[...]).
The main reasons for this would be

  • We only "update" the fields that have been touched, hence there's a performance benefit.
  • It would also avoid accidentally skipping some fields in the update_fields parameter after making a change; and this is hard to catch in pull requests.

It seems that while django-dirtyfields does update all the touched fields. We do have timestamp fields like last_updated (auto_now=True, which get updated by django itself). They also need to updated alongside because otherwise the data in the timestamp field is now stale, as opposed to if we'd just called .save()

Perhaps this behaviour of updating timestamp fields also can be made configurable?

@pssukhwani Feel free to add if i left out anything or stated something incorrectly.

@pradeep-sukhwani
Copy link
Author

Hi @LincolnPuzey, Thanks for your response.

@sidmitra thanks for the detailed overview. Adding to your point, the last commit also make sure that we consider the timezone if it is enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants