Summary

Issue 3532: Linking to a collapsed review does not expand the review

Review Request #6332 — Created Sept. 17, 2014 and submitted Sept. 20, 2014, 7:34 a.m.

Information

Owner

nicolexin

Repository

Review Board

Branch

master

Bugs

3532

Depends On

Commit

1671274...

Reviewers

Groups

reviewboard, students

People

Description

Linking to a collapsed review does not expand the review. Fixed by parsing the url and force the linked box to open.

Testing Done

Tested on local server, add /#review{review_id} to url will trigger the corresponding box to open.

All unit tests passed.

Issues

Description	From	Last Updated
Empty line contains whitespace.	brennie	Sept. 19, 2014, 7:56 a.m.
Use spaces instead of tabs.	brennie	Sept. 19, 2014, 7:56 a.m.
var statements must always be at the top of the function. There should also only be one single var, like …	chipx86	Sept. 19, 2014, 12:22 p.m.
Variable names are always namedLikeThis in our JavaScript code. No underscores.	chipx86	Sept. 19, 2014, 12:22 p.m.
Space after if, and always have a single blank line separating other code and an if statement.	chipx86	Sept. 19, 2014, 12:22 p.m.
This has tabs and not spaces. Likewise for the next few lines.	brennie	Sept. 19, 2014, 10:49 a.m.
The .removeClass and .addClass should be indented 4 spaces relative to the element they're called on.	chipx86	Sept. 19, 2014, 12:22 p.m.
list comprehension redefines 'file_attachment' from line 589	reviewbot	Sept. 19, 2014, 1:07 p.m.
These could be combined into a single statement since you only use url once.	david	Sept. 20, 2014, 7:11 a.m.
Add a space between ) and {. Also, javascript comparisons should almost always use === instead of == Can you …	david	Sept. 20, 2014, 7:11 a.m.
These are indented too much.	david	Sept. 20, 2014, 7:11 a.m.
Since '#review' may not be in the URL, we should first check for the presence. I know JavaScript will (at …	chipx86	Sept. 20, 2014, 7:30 a.m.
In JavaScript, you want to use === for comparison (though it'll require that the types be compatible, meaning you'll need …	chipx86	Sept. 20, 2014, 7:30 a.m.

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewBoxView.js
    AUTHORS



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewBoxView.js
    AUTHORS

Bugs:

+	3532

reviewboard/static/rb/js/views/reviewBoxView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Empty line contains whitespace.
```
reviewboard/static/rb/js/views/reviewBoxView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Use spaces instead of tabs.
```

You should also probably use a more descriptive review request title; using "fixed issue 3532" requires someone going to the bug tracker to find out about the change.

Description:

~		fixed issue 3532
	~	fixed issue 3532: https://code.google.com/p/reviewboard/issues/detail?id=3532

Testing Done:

Tested on local server, add /#review{review_id} to url will trigger the corresponding box to open.

Summary:

-	fixed issue 3532
+	Issue 3532: Linking to a collapsed review does not expand the review

Change Summary:

Added the students group

Groups:

+	students

Commit:

-	3421a6a3d7bf48ca4c42968dd9bee2282ea733f5
+	34fac40515e3c1f342e2b4d1e53cfc7ddeb6d8b3

Diff:

Revision 2 (+10)

Show changes

	AUTHORS
	reviewboard/static/rb/js/views/reviewBoxView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewBoxView.js
    AUTHORS



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewBoxView.js
    AUTHORS

Commit:

-	34fac40515e3c1f342e2b4d1e53cfc7ddeb6d8b3
+	a395e44b136bbe8e0e3fb9d31e9207e9c402e251

Diff:

Revision 3 (+10)

Show changes

	AUTHORS
	reviewboard/static/rb/js/views/reviewBoxView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewBoxView.js
    AUTHORS



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewBoxView.js
    AUTHORS

Description:

~		fixed issue 3532: https://code.google.com/p/reviewboard/issues/detail?id=3532
	~	Linking to a collapsed review does not expand the review. Fixed by parsing the url and force the linked box to open.

Testing Done:

		Tested on local server, add /#review{review_id} to url will trigger the corresponding box to open.
	+
	+	All unit tests passed.

reviewboard/static/rb/js/views/reviewBoxView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
This has tabs and not spaces. Likewise for the next few lines.
```

Commit:

-	a395e44b136bbe8e0e3fb9d31e9207e9c402e251
+	64fdbf1a151f28958867f595891aa317cb643afc

Diff:

Revision 4 (+10)

Show changes

	AUTHORS
	reviewboard/static/rb/js/views/reviewBoxView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewBoxView.js
    AUTHORS



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewBoxView.js
    AUTHORS

Some general comments involving coding style, but for this particular change, I thought we were going to fix this up in the Python code?

The Python code (review_detail in reviewboard/reviews/views.py) controls the initial state of the boxes. It should be checking the #review and setting the matching collapsed state to False. This way, the page is rendered in the correct state.

The Python code is already doing this for other things (like comparing timestamps and such) to set the default states, which is why we want it there.

reviewboard/static/rb/js/views/reviewBoxView.js (Diff revision 3)

The issue has been dropped. Show all issues

var statements must always be at the top of the function. There should also only be one single var, like this:
var foo = 1,
    bar = 2,
    s = '';

reviewboard/static/rb/js/views/reviewBoxView.js (Diff revision 3)
The issue has been dropped. Show all issues
```
Variable names are always namedLikeThis in our JavaScript code. No underscores.
```
reviewboard/static/rb/js/views/reviewBoxView.js (Diff revision 3)
The issue has been dropped. Show all issues
```
Space after if, and always have a single blank line separating other code and an if statement.
```

reviewboard/static/rb/js/views/reviewBoxView.js (Diff revision 3)

The issue has been dropped. Show all issues

The .removeClass and .addClass should be indented 4 spaces relative to the element they're called on.

Summary:

-	Issue 3532: Linking to a collapsed review does not expand the review
+	[WIP] Issue 3532: Linking to a collapsed review does not expand the review

Testing Done:

-		Tested on local server, add /#review{review_id} to url will trigger the corresponding box to open.
-
-		All unit tests passed.

Commit:

-	64fdbf1a151f28958867f595891aa317cb643afc
+	11a384c7b3dcfdd7a17a27d8352add4b2a217f75

Diff:

Revision 5 (+7)

Show changes

	AUTHORS
	reviewboard/reviews/views.py

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

Ignored Files:
    AUTHORS



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

Ignored Files:
    AUTHORS

reviewboard/reviews/views.py (Diff revision 5)
The issue has been dropped. Show all issues
```
 list comprehension redefines 'file_attachment' from line 589
```

Description:

~		Linking to a collapsed review does not expand the review. Fixed by parsing the url and force the linked box to open.
	~	Linking to a collapsed review does not expand the review. Fixed by parsing the url and force the linked box to open. But somehow it doesn't work.

Description:

		Linking to a collapsed review does not expand the review. Fixed by parsing the url and force the linked box to open. But somehow it doesn't work.
	+
	+	I think it's because request.get_full_path() won’t include the anchor at the end, here is the reference:
	+	https://code.djangoproject.com/ticket/18456
	+
	+	So, have to do this at front-end?

Have you looked into Fragment Identifiers? As best I understand the part of the URL after the # isn't sent to the server. A plaintext dump of the url at the point you're working on should verify that.

Summary:

-	[WIP] Issue 3532: Linking to a collapsed review does not expand the review
+	Issue 3532: Linking to a collapsed review does not expand the review

Description:

~		Linking to a collapsed review does not expand the review. Fixed by parsing the url and force the linked box to open. But somehow it doesn't work.
	~	Linking to a collapsed review does not expand the review. Fixed by parsing the url and force the linked box to open.
-
-		I think it's because request.get_full_path() won’t include the anchor at the end, here is the reference:
-		https://code.djangoproject.com/ticket/18456
-
-		So, have to do this at front-end?

Testing Done:

	+	Tested on local server, add /#review{review_id} to url will trigger the corresponding box to open.
	+
	+	All unit tests passed.

Commit:

-	11a384c7b3dcfdd7a17a27d8352add4b2a217f75
+	7867de13cfc44770c040195a96d62dd3612004af

Diff:

Revision 6 (+11 -1)

Show changes

	AUTHORS
	reviewboard/static/rb/js/views/reviewBoxView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewBoxView.js
    AUTHORS



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewBoxView.js
    AUTHORS

reviewboard/static/rb/js/views/reviewBoxView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
These could be combined into a single statement since you only use url once.
```

reviewboard/static/rb/js/views/reviewBoxView.js (Diff revision 6)

The issue has been resolved. Show all issues

Add a space between ) and {. Also, javascript comparisons should almost always use === instead of ==
Can you also add a comment above this conditional explaining why we're expanding here?

nicolexin Sept. 20, 2014, 7:11 a.m.

loadReviewID is a string while this.model.id is a number, so if I change "==" to "===" it won't work.

david Sept. 20, 2014, 7:13 a.m.

Can we explicitly parseInt the loadReviewID?

reviewboard/static/rb/js/views/reviewBoxView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
These are indented too much.
```

Commit:

-	7867de13cfc44770c040195a96d62dd3612004af
+	988566e97483d0f94cab677e117f18f08f3c1ed8

Diff:

Revision 7 (+11 -1)

Show changes

	AUTHORS
	reviewboard/static/rb/js/views/reviewBoxView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewBoxView.js
    AUTHORS



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewBoxView.js
    AUTHORS

reviewboard/static/rb/js/views/reviewBoxView.js (Diff revision 7)

The issue has been resolved. Show all issues

Since '#review' may not be in the URL, we should first check for the presence. I know JavaScript will (at least on Chrome) just return undefined, but this code makes it look like '#review' is always expected to be there.

reviewboard/static/rb/js/views/reviewBoxView.js (Diff revision 7)

The issue has been resolved. Show all issues

In JavaScript, you want to use === for comparison (though it'll require that the types be compatible, meaning you'll need to parseInt(loadReviewID, 10).

Commit:

-	988566e97483d0f94cab677e117f18f08f3c1ed8
+	167127475e4c5139550caf34dd66e1a15ccb4905

Diff:

Revision 8 (+14 -1)

Show changes

	AUTHORS
	reviewboard/static/rb/js/views/reviewBoxView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewBoxView.js
    AUTHORS



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewBoxView.js
    AUTHORS

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (c5d577c)