Summary

Refactor queries for the review request page.

Review Request #8347 — Created Aug. 25, 2016 and submitted Sept. 13, 2016, 10:17 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

1179d6c...

Reviewers

Groups

reviewboard

People

Description

This change moves all the data querying logic out of the review_detail view
into a new ReviewRequestPageData object. The view itself is now primarily
concerned with computation of the etag, creation of the entries, and rendering
of the template--as it should be. This new object documents its attributes, and
is passed into the builtin fields which wish to use it to optimize their
queries, rather than the old locals_vars hack, which I believe was possibly
literally one of the gateways to hell.

As I was doing this I cleaned up some code which was no longer used and moved
some of the work from pre-etag to post-etag. This also changes some of the
timestamp comparisons to work off of integers (ctimes) instead of datetimes,
since before we had a few instances where we were comparing datetimes to 0.

Testing Done


Did lots of testing with a few different review requests with lots of change

  description and review boxes.
Ran unit tests.

Issues

Description	From	Last Updated
Col: 80 E501 line too long (117 > 79 characters)	reviewbot	Aug. 25, 2016, 4:39 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Aug. 25, 2016, 4:39 p.m.
Col: 80 E501 line too long (113 > 79 characters)	reviewbot	Aug. 25, 2016, 4:39 p.m.
'GeneralComment' imported but unused	reviewbot	Aug. 25, 2016, 4:40 p.m.
'ScreenshotComment' imported but unused	reviewbot	Aug. 25, 2016, 4:40 p.m.
'FileAttachmentComment' imported but unused	reviewbot	Aug. 25, 2016, 4:40 p.m.
Col: 80 E501 line too long (117 > 79 characters)	reviewbot	Aug. 25, 2016, 4:43 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Aug. 25, 2016, 4:43 p.m.
Col: 80 E501 line too long (113 > 79 characters)	reviewbot	Aug. 25, 2016, 4:43 p.m.
Col: 80 E501 line too long (117 > 79 characters)	reviewbot	Sept. 8, 2016, 5:07 p.m.
I know this was here before, but while updating this code, maybe move to a try/except KeyError, to reduce this …	chipx86	Sept. 12, 2016, 11:12 p.m.
"great number more queries" sounds kinda odd. Maybe just "many more queries than necessary?"	chipx86	Sept. 12, 2016, 11:10 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Sept. 8, 2016, 5:07 p.m.
Col: 80 E501 line too long (113 > 79 characters)	reviewbot	Sept. 8, 2016, 5:07 p.m.
Let's say "The HTTP request object," just to help differentiate it from "review request" for those newer to the concepts.	chipx86	Sept. 12, 2016, 11:10 p.m.
Missing period.	chipx86	Sept. 12, 2016, 11:10 p.m.
Can you go into the details of the ctime additions in the description? I'm assuming they fix some bug.	chipx86	Sept. 12, 2016, 11:11 p.m.
Col: 80 E501 line too long (117 > 79 characters)	reviewbot	Sept. 12, 2016, 10:49 p.m.
Col: 80 E501 line too long (117 > 79 characters)	reviewbot	Sept. 12, 2016, 11:18 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/reviews/detail.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/reviews/detail.py

reviewboard/reviews/builtin_fields.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```
reviewboard/reviews/detail.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/reviews/detail.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (113 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'GeneralComment' imported but unused
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'ScreenshotComment' imported but unused
```
reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'FileAttachmentComment' imported but unused
```

Commit:

84c66e3491a6e079a3f405bd057f6128572f2a71

5327eed055df4f6cfafce51d91d28261a4d9b95c

Diff:

Revision 2 (+442 -345)

Show changes

	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/reviews/detail.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/reviews/detail.py

reviewboard/reviews/builtin_fields.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```
reviewboard/reviews/detail.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/reviews/detail.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (113 > 79 characters)
```

Change Summary:

Fix ordering of replies to body-top and body-bottom (since reviews are now fetched in reverse order to make timestamp calculation faster for the ETag).

Commit:

5327eed055df4f6cfafce51d91d28261a4d9b95c

22cd0ef90be84b8d515eadf14262f6bf61d4d96a

Diff:

Revision 3 (+443 -345)

Show changes

	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/reviews/detail.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/reviews/detail.py

reviewboard/reviews/builtin_fields.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```
reviewboard/reviews/detail.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/reviews/detail.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (113 > 79 characters)
```

Change Summary:

Fix some issues that somehow snuck through my previous unit test runs.

Commit:

22cd0ef90be84b8d515eadf14262f6bf61d4d96a

d32282d1e72c72b9f81fdadfb3c8ebb828686be0

Diff:

Revision 4 (+455 -345)

Show changes

	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/reviews/detail.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/reviews/detail.py

reviewboard/reviews/builtin_fields.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```

```
Very nice.
```

reviewboard/reviews/builtin_fields.py (Diff revision 3)

The issue has been dropped. Show all issues

I know this was here before, but while updating this code, maybe move to a try/except KeyError, to reduce this from two lookups to one?

david Sept. 12, 2016, 11:12 p.m.

I thought about this when I was looking at this code, but replacing two O(1) lookups with one doesn't seem worth the uglier code (nested try/excepts to catch KeyError and then DoesNotExist)

chipx86 Sept. 12, 2016, 11:15 p.m.
```
Yeah, that's fair.
```

reviewboard/reviews/detail.py (Diff revision 3)

The issue has been resolved. Show all issues

"great number more queries" sounds kinda odd. Maybe just "many more queries than necessary?"

reviewboard/reviews/detail.py (Diff revision 3)

The issue has been resolved. Show all issues

Let's say "The HTTP request object," just to help differentiate it from "review request" for those newer to the concepts.

reviewboard/reviews/detail.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing period.
```

reviewboard/reviews/views.py (Diff revision 3)

The issue has been resolved. Show all issues

Can you go into the details of the ctime additions in the description? I'm assuming they fix some bug.

Change Summary:

Requested changes.

Description:

		This change moves all the data querying logic out of the review_detail view
		into a new `ReviewRequestPageData` object. The view itself is now primarily
		concerned with computation of the etag, creation of the entries, and rendering
		of the template--as it should be. This new object documents its attributes, and
		is passed into the builtin fields which wish to use it to optimize their
		queries, rather than the old `locals_vars` hack, which I believe was possibly
		literally one of the gateways to hell.

		As I was doing this I cleaned up some code which was no longer used and moved
~		some of the work from pre-etag to post-etag.
	~	some of the work from pre-etag to post-etag. This also changes some of the
	+	timestamp comparisons to work off of integers (ctimes) instead of datetimes,
	+	since before we had a few instances where we were comparing datetimes to 0.

Commit:

d32282d1e72c72b9f81fdadfb3c8ebb828686be0

1179d6c69c86c023d69eba1054738d8408d4b377

Diff:

Revision 5 (+455 -345)

Show changes

	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/reviews/detail.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/views.py
    reviewboard/reviews/detail.py

reviewboard/reviews/builtin_fields.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```

Ship it!

```
Ship It!
```

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