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

Fixed #28147 -- Fixed loss of parent pk in child model when saving child after parent. #8434

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ErwinJunge

ErwinJunge commented Apr 28, 2017

@auvipy

release notes needed

@ErwinJunge

This comment has been minimized.

Show comment
Hide comment
@ErwinJunge

ErwinJunge May 19, 2017

@auvipy Do you get notified when I push new commits to this PR? If so, sorry for spamming you. If not, could you have a look at the current state of this PR?

ErwinJunge commented May 19, 2017

@auvipy Do you get notified when I push new commits to this PR? If so, sorry for spamming you. If not, could you have a look at the current state of this PR?

@auvipy

This comment has been minimized.

Show comment
Hide comment
@auvipy

auvipy May 19, 2017

Contributor

yes. no problemo.

Contributor

auvipy commented May 19, 2017

yes. no problemo.

@timgraham timgraham changed the title from Fix issue with saving parent after seting on child (#28147) to Fixed #28147 -- Fixed loss of parent pk in child model when saving child after parent. Jun 7, 2017

@timgraham

When updating, please squash commits.

@@ -986,6 +986,9 @@ def formfield(self, **kwargs):
def save_form_data(self, instance, data):
if isinstance(data, self.remote_field.model):
setattr(instance, self.name, data)
elif data is None:

This comment has been minimized.

@timgraham

timgraham Jun 7, 2017

Member

Can you add a comment about this? Maybe add an if data is None branch under the existing else clause rather than duplicating setattr(instance, self.attname, data)?

@timgraham

timgraham Jun 7, 2017

Member

Can you add a comment about this? Maybe add an if data is None branch under the existing else clause rather than duplicating setattr(instance, self.attname, data)?

@@ -62,6 +62,8 @@ Bugfixes
* Fixed crash when overriding the template of
``django.views.static.directory_index()`` (:ticket:`28122`).
* Fixed unexpected data loss when saving parent object after setting on child (:ticket:`28147`).

This comment has been minimized.

@timgraham

timgraham Jun 7, 2017

Member

I'm a bit uncertain if this change might have unexpected consequences so I'd rather not backport it. This issue exists as far back as I checked (Django 1.8) and isn't really a critical data loss problem, I think.

@timgraham

timgraham Jun 7, 2017

Member

I'm a bit uncertain if this change might have unexpected consequences so I'd rather not backport it. This issue exists as far back as I checked (Django 1.8) and isn't really a critical data loss problem, I think.

@@ -510,6 +510,14 @@ def test_fk_assignment_and_related_object_cache(self):
with self.assertRaisesMessage(ValueError, msg):
ToFieldChild.objects.create(parent=p)
# Create parent and child, save parent, save child, parent_id should be set

This comment has been minimized.

@timgraham

timgraham Jun 7, 2017

Member

A separate test method is fine. The existing super long test is probably a docstring test that was never cleaned up.

@timgraham

timgraham Jun 7, 2017

Member

A separate test method is fine. The existing super long test is probably a docstring test that was never cleaned up.

@amymok

This comment has been minimized.

Show comment
Hide comment
@amymok

amymok Aug 17, 2017

I attempted to resolve the conflict as well as fix the broken test. It is on my own branch here since I cannot edit @ErwinJunge branch.

@timgraham I wasn't sure if you want me to start another PR on this or how to proceed. Also I am not sure I am able to answer the other questions you posted here as well.

amymok commented Aug 17, 2017

I attempted to resolve the conflict as well as fix the broken test. It is on my own branch here since I cannot edit @ErwinJunge branch.

@timgraham I wasn't sure if you want me to start another PR on this or how to proceed. Also I am not sure I am able to answer the other questions you posted here as well.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Aug 20, 2017

Member

Yes, you'd have to create a new pull request if you want to update this.

Member

timgraham commented Aug 20, 2017

Yes, you'd have to create a new pull request if you want to update this.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Jul 21, 2018

Member

Closing due to inactivity.

Member

timgraham commented Jul 21, 2018

Closing due to inactivity.

@timgraham timgraham closed this Jul 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment