Status API part 3: Refactor entries on review detail page.
Review Request #8339 — Created Aug. 19, 2016 and submitted
The review detail page is huge and ugly and multi-tentacled. This change
attempts to clean things up just a little bit, by creating some classes for the
various types of "entries" (the review and change boxes).This is primarily code motion, but I did discover an obscure bug while I was
doing this related to default expansion for draft reply comments (theentry
variable was never assigned in that case, leaving it to be whatever it was
before).There's obviously a ton more that could be done here, but I want to take this
one step at a time.
Verified the appearance of the review detail page with multiple reviews,
replies, and change descriptions. Everything works as it did before.
| Description | From | Last Updated |
|---|---|---|
|
Does .. seealso: :py:data:`...` work here? |
 brennie
|
|
|
Can we call this something like BaseReviewRequestPageEntry? It'll be more clear what it's for when reading other code. |
 chipx86
|
|
|
Can you elaborate on this, going into what an "entry" means? There's a lot going on on this page, and … |
chipx86
|
|
|
"JavaScript" |
chipx86
|
|
|
bool |
chipx86
|
|
|
bool |
chipx86
|
|
|
:py:attr: Referenced attributes (or any we want to make public, really) also need to be listed in the class's docstring … |
chipx86
|
|
|
This uses keyword arguments, but the others don't. We should be consistent here in our usage. (I'd say probably go … |
chipx86
|
|
|
Missing period. |
chipx86
|
|
|
Can we use keyword arguments, so this is self-documenting? |
chipx86
|
|
|
Might be nicer to just pull this out into a fieldsets variable first, and then iterate through that. |
chipx86
|
- Commit:
-
e8a0ba7f7b3854c7d6241eae00865afa0a19b81fd2ac1cf034cf8d20cb65c2321baabef072c86af2
- Diff:
-
Revision 2 (+311 -233)
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/change.js reviewboard/templates/reviews/boxes/review.js reviewboard/templates/reviews/boxes/change.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/change.js reviewboard/templates/reviews/boxes/review.js reviewboard/templates/reviews/boxes/change.html
-
Great cleanup :) I like where this could go.
Got some comments about naming and docs, but the logic seems fine. Tried going through the collapsing logic and I think it looks good.
Could the replies end up being moved into the entries, and the "should this be collapsed" logic? I don't care about that for this change, just wondering about planned next steps.
-
Can we call this something like
BaseReviewRequestPageEntry? It'll be more clear what it's for when reading other code. -
Can you elaborate on this, going into what an "entry" means? There's a lot going on on this page, and I want new contributors to be clear on this.
-
-
:py:attr:Referenced attributes (or any we want to make public, really) also need to be listed in the class's docstring under "Attributes:"
-
This uses keyword arguments, but the others don't. We should be consistent here in our usage. (I'd say probably go with keyword arguments everywhere.)
-
-
- Commit:
-
d2ac1cf034cf8d20cb65c2321baabef072c86af2aa42de30fa5bc7e2255e0c656eceadd48a6d18d6
- Diff:
-
Revision 3 (+344 -233)
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/change.js reviewboard/templates/reviews/boxes/review.js reviewboard/templates/reviews/boxes/change.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/change.js reviewboard/templates/reviews/boxes/review.js reviewboard/templates/reviews/boxes/change.html
- Commit:
-
aa42de30fa5bc7e2255e0c656eceadd48a6d18d67d15a8fb752a3f7ff34e5c28dfae404bfae11f52
- Diff:
-
Revision 4 (+346 -233)
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/change.js reviewboard/templates/reviews/boxes/review.js reviewboard/templates/reviews/boxes/change.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/change.js reviewboard/templates/reviews/boxes/review.js reviewboard/templates/reviews/boxes/change.html
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/change.js reviewboard/templates/reviews/boxes/review.js reviewboard/templates/reviews/boxes/change.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/detail.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/boxes/review.html reviewboard/templates/reviews/boxes/change.js reviewboard/templates/reviews/boxes/review.js reviewboard/templates/reviews/boxes/change.html