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
	Add on_delete to ForeignKey definitions.

Screenshots

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)
Show all issues
```
E501 line too long (83 > 79 characters)
```

Commits:

	Summary
-		Add on_delete to ForeignKey definitions.
+		Add on_delete to ForeignKey definitions.

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.

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
-		Add on_delete to ForeignKey definitions.
+		Add on_delete to ForeignKey definitions.

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)