Summary

Add on_delete to ForeignKey definitions.

Review Request #11988 — Created Jan. 24, 2022 and submitted Feb. 7, 2022, 11:07 p.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Django has made the on_delete argument to ForeignKey mandatory.
Until now, this has defaulted to CASCADE, which makes it easy to
unintentionally delete things that shouldn't be deleted. Making it a
required argument forces developers to think about each model and how it
should behave.

This change explicitly sets on_delete for every relation we have. Most
of these are CASCADE, but the diff data ones are SET_NULL because
those are effectively a one-to-many relation, and we don't want to
delete diff data that may be shared by other diffs.

Testing Done

Ran unit tests.

Commits

Summary	ID
Add on_delete to ForeignKey definitions. Django has made the `on_delete` argument to `ForeignKey` mandatory. Until now, this has defaulted to `CASCADE`, which makes it easy to unintentionally delete things that shouldn't be deleted. Making it a required argument forces developers to think about each model and how it should behave. This change explicitly sets `on_delete` for every relation we have. Most of these are `CASCADE`, but the diff data ones are `SET_NULL` because those are effectively a one-to-many relation, and we don't want to delete diff data that may be shared by other diffs. Testing Done: Ran unit tests.	a6eb4c8323c57a65ff0f425ab8ac8ec907436ea4

Summary

Add on_delete to ForeignKey definitions.

Django has made the `on_delete` argument to `ForeignKey` mandatory. Until now, this has defaulted to `CASCADE`, which makes it easy to unintentionally delete things that shouldn't be deleted. Making it a required argument forces developers to think about each model and how it should behave. This change explicitly sets `on_delete` for every relation we have. Most of these are `CASCADE`, but the diff data ones are `SET_NULL` because those are effectively a one-to-many relation, and we don't want to delete diff data that may be shared by other diffs. Testing Done: Ran unit tests.

a6eb4c8323c57a65ff0f425ab8ac8ec907436ea4

Issues

Description	From	Last Updated
Easy to miss in this change, but the diff data are all SET_NULL. Can you update the description to call …	chipx86	Jan. 26, 2022, 1:58 p.m.
E501 line too long (83 > 79 characters)	reviewbot	Jan. 24, 2022, 2:44 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/changedescs/models.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E501 line too long (83 > 79 characters)
```

Commits:

	Summary	ID
	Add on_delete to ForeignKey definitions. Django has made the `on_delete` argument to `ForeignKey` mandatory. Until now, this has defaulted to `CASCADE`, which makes it easy to unintentionally delete things that shouldn't be deleted. Making it a required argument forces developers to think about each model and how it should behave. Testing Done: Ran unit tests.	55595154ca3889f0a8493da16d79e2ff3eccad0d
	Add on_delete to ForeignKey definitions. Django has made the `on_delete` argument to `ForeignKey` mandatory. Until now, this has defaulted to `CASCADE`, which makes it easy to unintentionally delete things that shouldn't be deleted. Making it a required argument forces developers to think about each model and how it should behave. Testing Done: Ran unit tests.	1997c4e771a9be4ff6624852af91dc5b06731d33

Diff:

Revision 2 (+386 -224)

Show changes

	reviewboard/accounts/models.py
	reviewboard/attachments/models.py
	reviewboard/changedescs/models.py
	reviewboard/diffviewer/models/diffcommit.py
	reviewboard/diffviewer/models/diffset.py
	reviewboard/diffviewer/models/filediff.py
	reviewboard/hostingsvcs/models.py
	reviewboard/integrations/models.py
	14 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Easy to miss in this change, but the diff data are all SET_NULL. Can you update the description to call this out and talk about it?

Description:

		Django has made the `on_delete` argument to `ForeignKey` mandatory.
		Until now, this has defaulted to `CASCADE`, which makes it easy to
		unintentionally delete things that shouldn't be deleted. Making it a
		required argument forces developers to think about each model and how it
		should behave.
	+
	+	This change explicitly sets `on_delete` for every relation we have. Most
	+	of these are `CASCADE`, but the diff data ones are `SET_NULL` because
	+	those are effectively a one-to-many relation, and we don't want to
	+	delete diff data that may be shared by other diffs.

Commits:

	Summary	ID
	Add on_delete to ForeignKey definitions. Django has made the `on_delete` argument to `ForeignKey` mandatory. Until now, this has defaulted to `CASCADE`, which makes it easy to unintentionally delete things that shouldn't be deleted. Making it a required argument forces developers to think about each model and how it should behave. Testing Done: Ran unit tests.	1997c4e771a9be4ff6624852af91dc5b06731d33
	Add on_delete to ForeignKey definitions. Django has made the `on_delete` argument to `ForeignKey` mandatory. Until now, this has defaulted to `CASCADE`, which makes it easy to unintentionally delete things that shouldn't be deleted. Making it a required argument forces developers to think about each model and how it should behave. This change explicitly sets `on_delete` for every relation we have. Most of these are `CASCADE`, but the diff data ones are `SET_NULL` because those are effectively a one-to-many relation, and we don't want to delete diff data that may be shared by other diffs. Testing Done: Ran unit tests.	a6eb4c8323c57a65ff0f425ab8ac8ec907436ea4

Diff:

Revision 3 (+390 -224)

Show changes

	reviewboard/accounts/models.py
	reviewboard/attachments/models.py
	reviewboard/changedescs/models.py
	reviewboard/diffviewer/models/diffcommit.py
	reviewboard/diffviewer/models/diffset.py
	reviewboard/diffviewer/models/filediff.py
	reviewboard/hostingsvcs/models.py
	reviewboard/integrations/models.py
	14 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to django-3.2 (c3505d7)