Skip to content

Fixes unexpected deletion of related objects.#97

Open
gibsonbailey wants to merge 2 commits into
beda-software:masterfrom
gibsonbailey:master
Open

Fixes unexpected deletion of related objects.#97
gibsonbailey wants to merge 2 commits into
beda-software:masterfrom
gibsonbailey:master

Conversation

@gibsonbailey

@gibsonbailey gibsonbailey commented Dec 24, 2019

Copy link
Copy Markdown

When updating an instance with foreign key relations, the objects linking to the instance should not be deleted. Instead, the foreign key should simply be removed from the objects to the instance. This commit checks if the related field is a foreign key, and, if so, it unlinks the object. If the field cannot be null, the object still gets deleted, as it does in the current state of the master branch.

…king to the instance should not be deleted. Instead, the foreign key should simply be removed from the objects to the instance. This commit checks if the related field is a foreign key, and, if so, it unlinks the object. If the field cannot be null, the object gets deleted.
@codecov-io

codecov-io commented Dec 24, 2019

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.61%. Comparing base (87a1476) to head (771a4f7).
⚠️ Report is 126 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
+ Coverage   98.59%   98.61%   +0.01%     
==========================================
  Files           3        3              
  Lines         213      216       +3     
==========================================
+ Hits          210      213       +3     
  Misses          3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ruscoder

Copy link
Copy Markdown
Member

@gibsonbailey Thanks for the contribution. Could you please add a failing test which is fixed with your changes?

@gibsonbailey

gibsonbailey commented Dec 26, 2019

Copy link
Copy Markdown
Author

Yes, thanks for the consideration.

…gn key, i.e. s, are not deleted when they are removed from a profile.
@gibsonbailey

Copy link
Copy Markdown
Author

@ruscoder Test has been added. Thanks Vadim!

@ruscoder

Copy link
Copy Markdown
Member

@gibsonbailey Thanks for the tests.
I'm not sure that this logic should work in this way.
Let's consider how Django works with null=True and on_delete=CASCADE.
https://docs.djangoproject.com/en/2.2/ref/models/fields/#django.db.models.ForeignKey.on_delete

With CASCADE, Django deletes the instance if the related instance is deleted. To avoid this you should pass SET_NULL/SET_DEFAULT.

So, I think the logic should be changed to use on_delete param instead of null

@ruscoder

Copy link
Copy Markdown
Member

My comment is still actual. So I don't close it and don't accept for now.

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