Summary

Delete related objects when deleting a review request.

Review Request #13735 — Created April 16, 2024 and submitted April 23, 2024, 3:53 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change adds a new signal handler for pre_delete on
ReviewRequest. In this handler, we explicitly delete change
descriptions, file attachments (both active and inactive), screenshots
(both active and inactive), and the diffset history.

A future change will add a management command to clean up any existing
orphaned objects.

Testing Done


Ran unit tests.
Individually disabled lines in the signal handler and verified that

  without this, we were leaving these objects in the DB, and with it,

  they get deleted.

Commits

Summary	ID
Delete related objects when deleting a review request. We never really designed Review Board to delete data, and we (at least I) also didn't fully understand how foreign keys and deletion worked when we first designed many aspects of our database schema. The result of this is that when a review request was deleted, we'd leave a bunch of related things in the database, now completely orphaned. The biggest offender here was file attachments, which not only use up space in the database, but can also use up a lot of space on the filesystem. This change adds a new signal handler for `pre_delete` on `ReviewRequest`. In this handler, we explicitly delete change descriptions, file attachments (both active and inactive), screenshots (both active and inactive), and the diffset history. A future change will add a management command to clean up any existing orphaned objects. Testing Done: - Ran unit tests. - Individually disabled lines in the signal handler and verified that without this, we were leaving these objects in the DB, and with it, they get deleted.	b528d0f1f78ec9c045d9378be3dda83d7e03ef10

Summary

Delete related objects when deleting a review request.

We never really designed Review Board to delete data, and we (at least I) also didn't fully understand how foreign keys and deletion worked when we first designed many aspects of our database schema. The result of this is that when a review request was deleted, we'd leave a bunch of related things in the database, now completely orphaned. The biggest offender here was file attachments, which not only use up space in the database, but can also use up a lot of space on the filesystem. This change adds a new signal handler for `pre_delete` on `ReviewRequest`. In this handler, we explicitly delete change descriptions, file attachments (both active and inactive), screenshots (both active and inactive), and the diffset history. A future change will add a management command to clean up any existing orphaned objects. Testing Done: - Ran unit tests. - Individually disabled lines in the signal handler and verified that without this, we were leaving these objects in the DB, and with it, they get deleted.

b528d0f1f78ec9c045d9378be3dda83d7e03ef10

Issues

Description	From	Last Updated
You could pull out instance.file_attachments_count and instance.inactive_file_attachments_count into variables since we access them again later on.	maubin	April 17, 2024, 7:18 p.m.
I know it'd be a lot of additional work, but given that we're deleting content, it'd be really nice to …	chipx86	April 23, 2024, 3:14 p.m.

flake8 passed.

JSHint passed.

reviewboard/reviews/signal_handlers.py (Diff revision 1)

The issue has been dropped. Show all issues

You could pull out instance.file_attachments_count and instance.inactive_file_attachments_count into variables since we access them again later on.

david April 17, 2024, 7:18 p.m.

It's just one extra dot access, and this is not a performance bottleneck since it happens extremely rarely. I'd rather keep the code simpler.

Ship it!

```
Ship It!
```

```
Looks great!
```

reviewboard/reviews/tests/test_signal_handlers.py (Diff revision 1)

The issue has been dropped. Show all issues

I know it'd be a lot of additional work, but given that we're deleting content, it'd be really nice to use assertQueries here and in other tests, ensure we can see exactly what operations are being executed.

Status:: Completed
Change Summary:: Pushed to release-7.x (c43cb1e)