Summary

Delete associated files when file attachments get completely deleted.

Review Request #13142 — Created July 13, 2023 and submitted Sept. 28, 2023, 11:32 a.m.

Information

Owner

maubin

Repository

Review Board

Branch

release-6.x

Bugs

Depends On

~~13141~~, ~~13138~~

Reviewers

Groups

reviewboard

People

Description

Currently, when file attachments get completely deleted from the database we
keep their associated files lying around. There's no good reason to do this,
when the file attachment gets deleted we have no way of accessing these files
anymore.

This change deletes associated files from the filesystem when file
attachments get deleted. Any files created by the MimetypeHandler will be
deleted, and then the main file of the file attachment will be deleted.

Testing Done


Tested deleting draft .png file attachments that have thumbnail files,

  saw that they were deleted.
Tested deleting draft .txt files.
Tested deleting published files, saw that their files weren't deleted.
Tested deleting a draft revision of a file attachment that has

  been published before (has a history), saw that only the files for

  the draft revision were deleted.

Commits

Summary	ID
Delete associated files when file attachments get completely deleted.	9807aee29beba5ed20f5297ddf9336733c665959

Issues

Description	From	Last Updated
no newline at end of file Column: 46 Error code: W292	reviewbot	July 13, 2023, 4:15 p.m.
Can you add -> None so this will opt into type hints? You'll also need an annotations future import here …	chipx86	Aug. 25, 2023, 9:51 a.m.
Since we don't use this outside of typing, you should import typing.TYPE_CHECKING and then do this import in an if …	chipx86	Aug. 25, 2023, 9:53 a.m.
I think we should override FileAttachment.delete to call delete_associated_files before performing the main deletion functionality. We wouldn't need to worry …	chipx86	Aug. 25, 2023, 9:50 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/attachments/signal_handlers.py (Diff revision 1)
The issue has been dropped. Show all issues
```
no newline at end of file

Column: 46
Error code: W292
```

Description:

~		Delete associated files when draft file attachments get deleted.
	~	Currently, when draft file attachments get deleted we keep their associated
	+	files lying around. There's no good reason to do this, and when the file
	+	attachment gets deleted we have no way of accessing these files anymore. And
	+	since the file attachments are drafts that are being deleted before ever
	+	being published, we don't need to keep their files for any reason.
	+
	+	This change deletes associated files from the filesystem when draft file
	+	attachments get deleted. Any files created by the `MimetypeHandler` will be
	+	deleted, and then the main file of the file attachment will be deleted.

Commits:

	Summary	ID
	Delete associated files when draft file attachments get deleted.	55196f32e2f80261f7814f137bbc8d172d32be63
	Delete associated files when draft file attachments get deleted.	bb8fb6af50b092cbf354c021dd292c84a13f644a

Diff:

Revision 2 (+170)

Show changes

	reviewboard/attachments/apps.py
	reviewboard/attachments/signal_handlers.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Change Summary:


Moved to RB6.

Testing Done:

~		Tested deleting `.png` file attachments that have thumbnail files.
~		Tested deleting `.txt` files.
~		Tested deleting published files.
~		Tested deleting a draft revision of a file attachment that has been published before (has a history).
	~	Tested deleting draft `.png` file attachments that have thumbnail files, saw that they were deleted.
	~	Tested deleting draft `.txt` files.
	~	Tested deleting published files, saw that their files weren't deleted.
	~	Tested deleting a draft revision of a file attachment that has been published before (has a history), saw that only the files for the draft revision were deleted.

Commits:

	Summary	ID
	Delete associated files when draft file attachments get deleted.	bb8fb6af50b092cbf354c021dd292c84a13f644a
	Delete associated files when draft file attachments get deleted.	fd1283387df474f5af9bd90bf3c7197bd0983242

Branch:

-	release-5.0.x
+	release-6.x

Diff:

Revision 3 (+170)

Show changes

	reviewboard/attachments/apps.py
	reviewboard/attachments/signal_handlers.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

reviewboard/attachments/apps.py (Diff revision 3)

The issue has been resolved. Show all issues

Can you add -> None so this will opt into type hints?
You'll also need an annotations future import here and in other new files.

reviewboard/attachments/signal_handlers.py (Diff revision 3)

The issue has been resolved. Show all issues

Since we don't use this outside of typing, you should import typing.TYPE_CHECKING and then do this import in an if TYPE_CHECKING: block (which is basically treated as a 4th import group).

maubin Aug. 25, 2023, 10:04 a.m.

I need it when connecting the signal handler to the signal: pre_delete.connect(_on_file_attachment_deleted, sender=FileAttachment) so I'll keep the import as is.

reviewboard/attachments/signal_handlers.py (Diff revision 3)

The issue has been resolved. Show all issues

I think we should override FileAttachment.delete to call delete_associated_files before performing the main deletion functionality.

We wouldn't need to worry about the order of signal handling or anything in that case, and can keep all deletion logic in one place.

maubin Aug. 10, 2023, 8:27 a.m.

I was originally going to go that route, but saw in the Django docs that overridden model methods are not called on bulk operations. So this logic wouldn't run when we do a cascade delete, for example. Does this matter for us? If not then I'll go ahead and override the method, but if so then this is the only workaround that Django suggests.

maubin Aug. 10, 2023, 10:26 a.m.

We do cascade delete file attachments when users and local sites are deleted, so there would be a problem there.

chipx86 Aug. 10, 2023, 11:01 a.m.

That's a fair point. I hadn't considered that aspect.

Okay, let's keep the signal approach.

maubin Aug. 10, 2023, 11:04 a.m.
```
Sounds good, thanks.
```

Summary:

-	Delete associated files when draft file attachments get deleted.
+	Delete associated files when file attachments get completely deleted.

Description:

~		Currently, when draft file attachments get deleted we keep their associated
~		files lying around. There's no good reason to do this, and when the file
~		attachment gets deleted we have no way of accessing these files anymore. And
~		since the file attachments are drafts that are being deleted before ever
	~	Currently, when file attachments get completely deleted from the database we
	~	keep their associated files lying around. There's no good reason to do this,
	~	when the file attachment gets deleted we have no way of accessing these files
	~	anymore.
-		being published, we don't need to keep their files for any reason.

~		This change deletes associated files from the filesystem when draft file
	~	This change deletes associated files from the filesystem when file
		attachments get deleted. Any files created by the `MimetypeHandler` will be
		deleted, and then the main file of the file attachment will be deleted.

Commits:

	Summary	ID
	Delete associated files when draft file attachments get deleted.	fd1283387df474f5af9bd90bf3c7197bd0983242
	Delete associated files when file attachments get completely deleted.	9807aee29beba5ed20f5297ddf9336733c665959

Diff:

Revision 4 (+166)

Show changes

	reviewboard/attachments/apps.py
	reviewboard/attachments/signal_handlers.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-6.x (f6f2070)