django-model-utils icon indicating copy to clipboard operation
django-model-utils copied to clipboard

#435 Fix instance creation state check

Open MaximZemskov opened this issue 4 years ago • 6 comments

Problem

FieldTracker has incorrect behaviour if PK field isn't a AutoFIeld. Link to issue https://github.com/jazzband/django-model-utils/issues/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

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

MaximZemskov avatar Jul 27 '20 20:07 MaximZemskov

@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.

MaximZemskov avatar Jul 28 '20 08:07 MaximZemskov

@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?

martey avatar Jul 28 '20 17:07 martey

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

auvipy avatar Jul 28 '20 23:07 auvipy

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.

martey avatar Jul 29 '20 05:07 martey

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 avatar Jul 29 '20 07:07 MaximZemskov

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 avatar Aug 03 '20 07:08 MaximZemskov