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)
Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```
reviewboard/reviews/detail.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/reviews/detail.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (113 > 79 characters)
```
reviewboard/reviews/views.py (Diff revision 1)
Show all issues
```
 'GeneralComment' imported but unused
```
reviewboard/reviews/views.py (Diff revision 1)
Show all issues
```
 'ScreenshotComment' imported but unused
```
reviewboard/reviews/views.py (Diff revision 1)
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)
Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```
reviewboard/reviews/detail.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/reviews/detail.py (Diff revision 2)
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)
Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```
reviewboard/reviews/detail.py (Diff revision 3)
Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/reviews/detail.py (Diff revision 3)
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)
Show all issues
```
Col: 80
 E501 line too long (117 > 79 characters)
```

```
Very nice.
```

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

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)

Show all issues

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

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

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)
Show all issues
```
Missing period.
```

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

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

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (e9b6eda)