Summary

Compare IDs instead of model instances when possible.

Review Request #10380 — Created Jan. 13, 2019 and submitted Jan. 22, 2019, 5:33 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

To reduce overhead, we now compare the IDs, which is faster even if the
instances were already loaded. This cuts down on a number of queries in
common usage.

Test coverage for these checks have been improved, with many
accessibility checks that were previously covered implicitly through
other test suites now having explicit test coverage.

There's also a fix for a bad permission name, which impacted the tests.
The can_edit_status permission for StatusUpdate needed to be
change_statusupdate.

Testing Done

Unit tests pass.

Commits

Summary	ID
Compare IDs instead of model instances when possible. We had several regularly-invoked model instance comparisons in the product that could result in unwanted database queries. These were usually found in accessibility checks, comparing if a requesting user matches the user owning an object. To reduce overhead, we now compare the IDs, which is faster even if the instances were already loaded. This cuts down on a number of queries in common usage. Test coverage for these checks have been improved, with many accessibility checks that were previously covered implicitly through other test suites now having explicit test coverage. There's also a fix for a bad permission name, which impacted the tests. The `can_edit_status` permission for `StatusUpdate` needed to be `change_statusupdate`.	228c527eb4dda2c60bd0359fccf0ada7ef3f3beb

Summary

Compare IDs instead of model instances when possible.

We had several regularly-invoked model instance comparisons in the product that could result in unwanted database queries. These were usually found in accessibility checks, comparing if a requesting user matches the user owning an object. To reduce overhead, we now compare the IDs, which is faster even if the instances were already loaded. This cuts down on a number of queries in common usage. Test coverage for these checks have been improved, with many accessibility checks that were previously covered implicitly through other test suites now having explicit test coverage. There's also a fix for a bad permission name, which impacted the tests. The `can_edit_status` permission for `StatusUpdate` needed to be `change_statusupdate`.

228c527eb4dda2c60bd0359fccf0ada7ef3f3beb

Issues

Description	From	Last Updated
F821 undefined name 'AnonymousUser'	reviewbot	Jan. 13, 2019, 4:26 a.m.
F821 undefined name 'AnonymousUser'	reviewbot	Jan. 13, 2019, 4:26 a.m.
E128 continuation line under-indented for visual indent	reviewbot	Jan. 13, 2019, 4:27 a.m.
F401 'reviewboard.reviews.models.StatusUpdate' imported but unused	reviewbot	Jan. 13, 2019, 4:27 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/attachments/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F821 undefined name 'AnonymousUser'
```
reviewboard/attachments/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F821 undefined name 'AnonymousUser'
```
reviewboard/oauth/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/reviews/tests/test_status_update.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'reviewboard.reviews.models.StatusUpdate' imported but unused
```

Change Summary:


Added a missing commit for an import.
Fixed style and unused import issues.

Commits:

	Summary	ID
	Compare IDs instead of model instances when possible. We had several regularly-invoked model instance comparisons in the product that could result in unwanted database queries. These were usually found in accessibility checks, comparing if a requesting user matches the user owning an object. To reduce overhead, we now compare the IDs, which is faster even if the instances were already loaded. This cuts down on a number of queries in common usage. Test coverage for these checks have been improved, with many accessibility checks that were previously covered implicitly through other test suites now having explicit test coverage. There's also a fix for a bad permission name, which impacted the tests. The `can_edit_status` permission for `StatusUpdate` needed to be `change_statusupdate`.	3796c463b65406bbb04bb0d52c0701270318f2c4
	Compare IDs instead of model instances when possible. We had several regularly-invoked model instance comparisons in the product that could result in unwanted database queries. These were usually found in accessibility checks, comparing if a requesting user matches the user owning an object. To reduce overhead, we now compare the IDs, which is faster even if the instances were already loaded. This cuts down on a number of queries in common usage. Test coverage for these checks have been improved, with many accessibility checks that were previously covered implicitly through other test suites now having explicit test coverage. There's also a fix for a bad permission name, which impacted the tests. The `can_edit_status` permission for `StatusUpdate` needed to be `change_statusupdate`.	228c527eb4dda2c60bd0359fccf0ada7ef3f3beb

Diff:

Revision 2 (+786 -42)

Show changes

	reviewboard/accounts/models.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/oauth/models.py
	reviewboard/oauth/tests.py
	reviewboard/reviews/context.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models/base_comment.py
	8 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-3.0.x (cd33f6d)

reviewboard/oauth/tests.py (Diff revision 1)