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

#435 Fix instance creation state check #436

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

MaximZemskov
Copy link

Problem

FieldTracker has incorrect behaviour if PK field isn't a AutoFIeld. Link to issue #435.

Solution

Django has a ModelState object to determine if objects is in creation state or already exists in db. Replacing if not self.instance.pk is None with if self.instance._state.adding solve the problem.

Commandments

  • Write PEP8 compliant code.
  • Cover it with tests.
  • Update CHANGES.rst file to describe the changes, and quote according issue with GH-<issue_number>.
  • Pay attention to backward compatibility, or if it breaks it, explain why.
  • Update documentation (if relevant).

@MaximZemskov MaximZemskov changed the title [435] Fix instance creation state check [#435] Fix instance creation state check Jul 27, 2020
@MaximZemskov MaximZemskov changed the title [#435] Fix instance creation state check #435 Fix instance creation state check Jul 27, 2020
Copy link
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you update the tests t verify this change won't regress?

Copy link
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you make the failing tests pass?

@MaximZemskov
Copy link
Author

MaximZemskov commented Jul 28, 2020

@auvipy I fixed one of two failed tests. Trying to fix last test but can't figure out what is the problem. Stucked with fixing this test https://github.com/jazzband/django-model-utils/blob/master/tests/test_fields/test_field_tracker.py#L590.

Can't understand why there is {'id': None} in changed() call on deffered_instance.

def test_post_save_changed(self):
        self.update_instance(some_file=self.some_file)
        self.assertChanged()
        previous_file = self.instance.some_file
        self.instance.some_file = self.another_file
        self.assertChanged(some_file=previous_file)
        # test deferred file field
        deferred_instance = self.tracked_class.objects.defer('some_file')[0]
        deferred_instance.some_file  # access field to fetch from database
>       self.assertChanged(tracker=deferred_instance.tracker)

tests/test_fields/test_field_tracker.py:601:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
tests/test_fields/test_field_tracker.py:36: in assertChanged
    self.assertEqual(tracker.changed(), kwargs)
E   AssertionError: {'id': None} != {}
E   - {'id': None}
E   + {}

Will be glad if you can help me.

@auvipy auvipy requested a review from martey July 28, 2020 09:11
@martey
Copy link
Contributor

martey commented Jul 28, 2020

@auvipy I'm not sure why a review by me was requested. Was this an error, or do you want me to help with something?

@auvipy
Copy link
Contributor

auvipy commented Jul 28, 2020

GitHub shows you recently modified the files that's why I requested a review

@martey martey removed their request for review July 29, 2020 05:12
@martey
Copy link
Contributor

martey commented Jul 29, 2020

Looking at my previous contributions to this project, the only file I have edited that is in this pull request is AUTHORS.rst. Since this pull request does not contain substantial changes to that file, I will refrain from providing a review.

@MaximZemskov
Copy link
Author

Looks like at the moment of execution set_saved_fields method self.instance._state.adding is always True. It becomes False inside django's Model.from_db method

@MaximZemskov
Copy link
Author

Found the problem.
initialize_tracker called on model's post_init signal and _state at this moment contains default values
https://docs.djangoproject.com/en/3.0/ref/signals/#post-init

Note
instance._state isn’t set before sending the post_init signal, so _state attributes always have their default values. For example, _state.db is None.

@MaximZemskov MaximZemskov reopened this May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants