Summary

Depends On & Blocks Relationships to Request Box.

Review Request #3881 — Created Feb. 18, 2013 and submitted May 12, 2013, 10:18 a.m.

Information

Owner

Demelo

Repository

Review Board

Branch

master

Bugs

281

Depends On

Reviewers

Groups

reviewboard

People

Description

Feature
- This feature adds the ability to link requests together by
  means of two fields; depends on, and blocks. These relationships
  are interconnected in the sense that, if Request A depends on 
  Request B, then inversely Request B's blocks field will display
  a link back to Request A.
- This request adds the ability to add, edit, and remove dependencies
  from both the backend admin panel as well as in the front-end.
- Blocking relationships are built as a reverse of dependency, and
  thus are automatically generated upon a dependency change.

Testing Done

Backend
- Select from Review Request Dependency box a new request to depend on - Relationship has been added.
- Select from Review Request Draft Dependency box a new request to depend on - Relationship has been added.
- Viewed these changes on the front end - Blocks relationship has been adjusted in response for the selected request.

Frontend
- Clicked to change dependencies, added a proper request after a comma and space - properly checked and added to draft.
- Clicked to change dependencies, added an improper request after a comma and space - properly checked discarded with an accompanying warning.
- Clicked to change dependencies, added a proper request after a space - properly checked and added to draft.
- Clicked to change dependencies, added an improper request after a space - properly checked discarded with an accompanying warning.
- Removed a dependency - Changes remain removed.
- Published Changes - Changes remain added.
- Enter in one incorrect request vs. multiple incorrect requests - Error adjusts to pluralized form.
- Provide a change to an already published request - change log reports adjustment.

Files

Issues

Description	From	Last Updated
Since djblets is part of Django and reviewboard is part of Reviewboard, I think there should exists a blank line …	GR gregwym	Feb. 20, 2013, 8:45 p.m.
I prefer "Do not change the things that are working, unless there is a good reason." This doesn't change anything …	GR gregwym	Feb. 21, 2013, 10:38 p.m.
Same here, these blank lines are probably been inserted intentionally.	GR gregwym	Feb. 20, 2013, 8:45 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Feb. 20, 2013, 8:58 p.m.
Col: 47 E127 continuation line over-indented for visual indent	reviewbot	Feb. 20, 2013, 8:58 p.m.
Col: 47 E127 continuation line over-indented for visual indent	reviewbot	Feb. 20, 2013, 8:58 p.m.
Col: 47 E127 continuation line over-indented for visual indent	reviewbot	Feb. 20, 2013, 8:58 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Feb. 20, 2013, 8:58 p.m.
Col: 49 E128 continuation line under-indented for visual indent	reviewbot	Feb. 20, 2013, 8:58 p.m.
Col: 67 W291 trailing whitespace	reviewbot	Feb. 20, 2013, 9 p.m.
Col: 47 E127 continuation line over-indented for visual indent	reviewbot	Feb. 20, 2013, 9:01 p.m.
Col: 47 E127 continuation line over-indented for visual indent	reviewbot	Feb. 20, 2013, 9:01 p.m.
Col: 47 E127 continuation line over-indented for visual indent	reviewbot	Feb. 20, 2013, 9:01 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Feb. 20, 2013, 9:01 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Feb. 20, 2013, 9:02 p.m.
Col: 58 E127 continuation line over-indented for visual indent	reviewbot	Feb. 20, 2013, 9:02 p.m.
Col: 67 W291 trailing whitespace	reviewbot	Feb. 20, 2013, 9:05 p.m.
Col: 56 E128 continuation line under-indented for visual indent	reviewbot	Feb. 20, 2013, 9:10 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Feb. 20, 2013, 9:10 p.m.
Col: 56 E128 continuation line under-indented for visual indent	reviewbot	Feb. 20, 2013, 9:10 p.m.
Looks like this is missing a space.	chipx86	Feb. 25, 2013, 1:50 a.m.
Okay, so this is a good lesson in query optimization. Say we load 30 rows in the datagrid. What this …	chipx86	March 20, 2013, 6:51 p.m.
This looks... interesting :) What are you trying to do here?	chipx86	Feb. 25, 2013, 1:51 a.m.
The other wrapping was more correct. If ReviewBot is yelling at you here, ignore it. Don't worry about fixing this …	chipx86	Feb. 25, 2013, 1:51 a.m.
_() is for localizing human-readable strings. This is an identifier, which won't work so well if, say, a Russian translation …	chipx86	Feb. 25, 2013, 2:02 a.m.
The indentation of all this is wrong. THey're off by one character.	chipx86	Feb. 25, 2013, 2:02 a.m.
Col: 40 E128 continuation line under-indented for visual indent	reviewbot	Feb. 20, 2013, 9:12 p.m.
Col: 40 E128 continuation line under-indented for visual indent	reviewbot	Feb. 20, 2013, 9:12 p.m.
Col: 40 E128 continuation line under-indented for visual indent	reviewbot	Feb. 20, 2013, 9:12 p.m.
Same comment about not using _().	chipx86	Feb. 25, 2013, 2:02 a.m.
Can you change these to use === instead of == ? (In JavaScript, == casts, and === doesn't.. the latter …	chipx86	Feb. 25, 2013, 2:03 a.m.
Trailing commas will break many browsers.	chipx86	Feb. 25, 2013, 2:03 a.m.
What's this all about?	chipx86	Feb. 25, 2013, 1:52 a.m.
Note the other template tags. The {% should always be unindented, and the contents inside should be indented relative to …	chipx86	Feb. 25, 2013, 2:04 a.m.
This looks wrong.	chipx86	Feb. 25, 2013, 1:52 a.m.
These should explicitly say "review request". Same with the other field strings.	chipx86	Feb. 25, 2013, 2:07 a.m.
This doesn't look right to me, and I feel very uncomfortable with changing old code like this that doesn't have …	chipx86	Feb. 25, 2013, 2:05 a.m.
Some indentation weirdness. I would change this to start parameters on the second line, indented 4 spaces from the "obj". …	chipx86	March 20, 2013, 6:52 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 20, 2013, 9:13 p.m.
This shouldn't be here.	chipx86	Feb. 25, 2013, 1:52 a.m.
"review request's" Also, trailing space should be removed.	chipx86	Feb. 25, 2013, 2:09 a.m.
Col: 40 E128 continuation line under-indented for visual indent	reviewbot	Feb. 25, 2013, 1:59 a.m.
Col: 40 E128 continuation line under-indented for visual indent	reviewbot	Feb. 25, 2013, 1:59 a.m.
Col: 40 E128 continuation line under-indented for visual indent	reviewbot	Feb. 25, 2013, 1:59 a.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 25, 2013, 1:57 a.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Feb. 25, 2013, 2:21 a.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Feb. 25, 2013, 2:21 a.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Feb. 25, 2013, 2:21 a.m.
Col: 41 E128 continuation line under-indented for visual indent	reviewbot	Feb. 25, 2013, 2:18 a.m.
Col: 41 E126 continuation line over-indented for hanging indent	reviewbot	Feb. 25, 2013, 2:18 a.m.
Col: 41 E128 continuation line under-indented for visual indent	reviewbot	Feb. 25, 2013, 2:24 a.m.
Col: 41 E126 continuation line over-indented for hanging indent	reviewbot	Feb. 25, 2013, 2:24 a.m.
Col: 41 E128 continuation line under-indented for visual indent	reviewbot	Feb. 25, 2013, 11:26 p.m.
Col: 41 E126 continuation line over-indented for hanging indent	reviewbot	Feb. 25, 2013, 11:26 p.m.
Col: 39 E711 comparison to None should be 'if cond is None	reviewbot	Feb. 25, 2013, 11:31 p.m.
Col: 41 E128 continuation line under-indented for visual indent	reviewbot	Feb. 25, 2013, 11:28 p.m.
I think this is worth some documentation.	mike_conley	March 20, 2013, 6:51 p.m.
Col: 41 E128 continuation line under-indented for visual indent	reviewbot	Feb. 25, 2013, 11:32 p.m.
This isn't something the others are doing yet, but you should use newer-style parent construction: super(DependsOnColumn, self).__init__(self, args, *kwargs)	chipx86	April 2, 2013, 7:40 a.m.
No blank line here.	chipx86	April 2, 2013, 7:40 a.m.
I'm seeing some stuff like this that I've commented on on previous reviews, which were marked as fixed. Can you …	chipx86	April 2, 2013, 7:41 a.m.
If you're going to break this up like this, I think I'd prefer to be consistent about putting the space …	mike_conley	April 2, 2013, 7:50 a.m.
Don't change things that aren't directly related to your change.	chipx86	April 2, 2013, 7:41 a.m.
Same.	chipx86	April 2, 2013, 7:41 a.m.
Even if pep8 complains, modifying this in your change only makes it harder for us to track down issues later. …	chipx86	April 2, 2013, 7:41 a.m.
Nope, put it back :)	chipx86	April 2, 2013, 7:42 a.m.
"by" should be wrapped with trans tags (although the ordering when translated might differ...by might come after...)... I assume you …	mike_conley	April 2, 2013, 7:50 a.m.
ReviewBot might have a point. Maybe: obj = ReviewRequest.objects.get( Q(local_id=value) & Q(local_site=local_site))	mike_conley	April 2, 2013, 7:50 a.m.
Col: 41 E128 continuation line under-indented for visual indent	reviewbot	March 20, 2013, 7:44 p.m.
Undo this change.	mike_conley	April 2, 2013, 7:42 a.m.
Undo this change.	mike_conley	April 2, 2013, 7:43 a.m.
Undo this change.	mike_conley	April 2, 2013, 7:43 a.m.
Undo this change.	mike_conley	April 2, 2013, 7:43 a.m.
Undo this change.	mike_conley	April 2, 2013, 7:43 a.m.
Undo this change.	mike_conley	April 2, 2013, 7:43 a.m.
Undo these changes.	mike_conley	April 2, 2013, 7:43 a.m.
Undo this change.	mike_conley	April 2, 2013, 7:44 a.m.
Undo this change.	mike_conley	April 2, 2013, 7:44 a.m.
Undo this change.	mike_conley	April 2, 2013, 7:44 a.m.
Undo this change.	mike_conley	April 2, 2013, 7:44 a.m.
Undo this change.	mike_conley	April 2, 2013, 7:45 a.m.
Undo this change.	mike_conley	April 2, 2013, 7:46 a.m.
Undo these changes.	mike_conley	April 2, 2013, 7:46 a.m.
Undo thsi change.	mike_conley	April 2, 2013, 7:46 a.m.
Undo this change.	mike_conley	April 2, 2013, 7:46 a.m.
Undo these changes.	mike_conley	April 2, 2013, 7:47 a.m.
Undo this change.	mike_conley	April 2, 2013, 7:47 a.m.
Undo this change.	mike_conley	April 2, 2013, 7:47 a.m.
Col: 41 E128 continuation line under-indented for visual indent	reviewbot	March 26, 2013, 5:02 a.m.
Undo this change.	mike_conley	April 2, 2013, 7:47 a.m.
related_name should never be localized. No _(..)	chipx86	April 2, 2013, 8:26 a.m.
Alignment issues. The parameters should be aligned to 'ReviewRequest'. The 'ReviewRequest' here should be able to be ReviewRequest (no quotes).	chipx86	April 2, 2013, 8:42 a.m.
Col: 40 E128 continuation line under-indented for visual indent	reviewbot	April 2, 2013, 7:53 a.m.
Col: 40 E128 continuation line under-indented for visual indent	reviewbot	April 2, 2013, 7:55 a.m.
Col: 40 E128 continuation line under-indented for visual indent	reviewbot	April 2, 2013, 7:53 a.m.
Same here.	chipx86	April 2, 2013, 8:42 a.m.
These should really be ===. Can you change all three?	chipx86	April 2, 2013, 8:42 a.m.
This looks broken. I don't see how it can compile.	chipx86	April 2, 2013, 8:42 a.m.
Where did this go?	chipx86	April 2, 2013, 8:42 a.m.
===	chipx86	April 2, 2013, 8:42 a.m.
"Review request"	chipx86	April 2, 2013, 8:42 a.m.
===	chipx86	April 2, 2013, 8:42 a.m.
"Review requests"	chipx86	April 2, 2013, 8:42 a.m.
These should be unindented, but with the contents indented relative to the nearest template tags (like: {% foo %}). Same …	chipx86	April 2, 2013, 8:42 a.m.
class="" should be removed.	chipx86	April 2, 2013, 8:42 a.m.
"for this review request."	chipx86	April 2, 2013, 8:42 a.m.
"for this review request."	chipx86	April 2, 2013, 8:42 a.m.
"for this review request."	chipx86	April 2, 2013, 8:42 a.m.
We have this split in two places, and both are slightly different. One is "[, ]+" and one is "[,\s*]". …	chipx86	April 5, 2013, 3:27 p.m.
Rather than this, can you add a new function to ReviewRequestManager (reviews/managers.py) called, let's say, for_id(), that would contain this …	chipx86	April 5, 2013, 4:03 p.m.
pk=value	chipx86	April 2, 2013, 8:42 a.m.
Col: 41 E128 continuation line under-indented for visual indent	reviewbot	April 2, 2013, 7:55 a.m.
No blank line here.	chipx86	April 2, 2013, 8:42 a.m.
"review request's"	chipx86	April 2, 2013, 8:42 a.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	April 2, 2013, 8:25 a.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	April 2, 2013, 9:52 a.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	April 2, 2013, 9:52 a.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	April 2, 2013, 9:52 a.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	April 2, 2013, 9:42 a.m.
I believe verbose_name needs to be wrapped in _()	mike_conley	April 5, 2013, 3:30 p.m.
Same here	mike_conley	April 5, 2013, 3:30 p.m.
Template code should be indented within the `{% ... %}` label? So this might become, {% spaceless %} {% for …	GR gregwym	April 6, 2013, 9:02 p.m.
I'm not sure what this code is intending to do, but `,\s` is quite different than `[,\s]` in regex. `,\s*` …	GR gregwym	April 5, 2013, 3:30 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	April 2, 2013, 12:10 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	April 2, 2013, 12:11 p.m.
Col: 45 E128 continuation line under-indented for visual indent	reviewbot	April 5, 2013, 4:30 p.m.
Col: 32 E128 continuation line under-indented for visual indent	reviewbot	April 5, 2013, 4:30 p.m.
What happened to Christian's request? "Yeah, we should use the one we have in reviews/models.py." See the comments in my …	mike_conley	April 6, 2013, 2:46 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	April 5, 2013, 4:36 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	April 6, 2013, 9:04 p.m.

Summary:

-	{WIP] - Redone Dependencies: Request <-> Request AND Draft Request -> Request FROM Admin Panel
+	[WIP] - Redone Dependencies: Request <-> Request AND Draft Request -> Request FROM Admin Panel

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/datagrids.py
    reviewboard/reviews/admin.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/models.py
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/templates/reviews/review_request_box.html

Description:

		Removed original architecture for dependencies, and replaces it with a slimmed down m2m for now. So far allows ReviewRequests to depend on other ReviewRequests and the opposite for blocking, as well as review requests drafts to depend on review requests.
		Admin panel editing only atm.
		Removing remenants of old code from previous design.
~		Also, did a run through of PEP8 standards on a portion of touched files for a mid-task coding standards cleanup.
	~	Also, did a run through of PEP8 standards on a portion of touched files (reviews/models.py, ./admin.py)for a mid-task coding standards cleanup.

People:

-	jon

Did you mean to post this as a new review request? You already have one open.

Make sure your description is always explaining the feature in detail, and not the changes you've made within your branch.

Demelo Feb. 18, 2013, 7:36 p.m.

I've edited it per your recommendations.

Change Summary:

Edit for better description and deleted the other request as it was merely for testing purposes.

Description:

~		Removed original architecture for dependencies, and replaces it with a slimmed down m2m for now. So far allows ReviewRequests to depend on other ReviewRequests and the opposite for blocking, as well as review requests drafts to depend on review requests.
~		Admin panel editing only atm.
~		Removing remenants of old code from previous design.
~		Also, did a run through of PEP8 standards on a portion of touched files (reviews/models.py, ./admin.py)for a mid-task coding standards cleanup.
	~	Feature
	~	- This feature adds the ability to link requests together by
	~	means of two fields; depends on, and blocks. These relationships
	~	are interconnected in the sense that, if Request A depends on
	+	Request B, then inversely Request B's blocks field will display
	+	a link back to Request A.
	+	- This request simply adds this feature's back-end and admin panel
	+	access. Therefore, you are able to view these relationships by
	+	adding new relationships via the review request/draft review request
	+	"depends on" field area in the admin database section.
	+	- The next code iteration has the goal of adding front-end editing
	+	support.
	+
	+	Change Log
	+	- Removed original architecture for dependencies, and replaces
	+	it with a slimmed down m2m for now. So far allows ReviewRequests
	+	to depend on other ReviewRequests and the opposite for blocking,
	+	as well as review requests drafts to depend on review requests.
	+	- Admin panel editing only atm.
	+	- Removing remenants of old code from previous design.
	+	- Also, did a run through of PEP8 standards on a portion of
	+	touched files (reviews/models.py, ./admin.py)for a mid-task
	+	coding standards cleanup.

reviewboard/reviews/models.py (Diff revision 1)

Show all issues

Since djblets is part of Django and reviewboard is part of Reviewboard, I think there should exists a blank line between their imports.

reviewboard/reviews/models.py (Diff revision 1)

Show all issues

I prefer "Do not change the things that are working, unless there is a good reason." 

This doesn't change anything except the format, which has nothing to do with your patch's intension, but may introduce unnecessary merge conflict if someone has changed this part. 

And there are many of others in this file.

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

Show all issues

Same here, these blank lines are probably been inserted intentionally.

Demelo Feb. 20, 2013, 4:55 a.m.

I agree with what your saying personally, especially about the formatting, however all these changes were from the PEP8 requirements, and proper formatting should be a part of every submission.

david Feb. 20, 2013, 4:59 a.m.

Greg is correct about (most of) the blank lines between different imports. PEP-8 says to group imports into 3 sections--standard library, "third party", and "this package".

In any case, it's better to split this up into two separate changes--one that fixes formatting of existing code, and one for your feature.

gregwym Feb. 20, 2013, 5 a.m.

I think, only the chunks of code we have changed are part of our submission, not the entire file. Basically when submitting a patch, a diff is everything.

Change Summary:

First non-WIP Submission.

Summary:

-	[WIP] - Redone Dependencies: Request <-> Request AND Draft Request -> Request FROM Admin Panel
+	Depends On & Blocks Relationships to Request Box.

Description:

		Feature
		- This feature adds the ability to link requests together by
		means of two fields; depends on, and blocks. These relationships
		are interconnected in the sense that, if Request A depends on
		Request B, then inversely Request B's blocks field will display
		a link back to Request A.
~		- This request simply adds this feature's back-end and admin panel
~		access. Therefore, you are able to view these relationships by
~		adding new relationships via the review request/draft review request
~		"depends on" field area in the admin database section.
	~	- This request adds the ability to add, edit, and remove dependencies
	~	from both the backend admin panel as well as in the front-end.
	~	- Blocking relationships are build as a reverse of dependency, and
	~	thus are automatically generated upon dependency change.
-		- The next code iteration has the goal of adding front-end editing
-		support.
-
-		Change Log
-		- Removed original architecture for dependencies, and replaces
-		it with a slimmed down m2m for now. So far allows ReviewRequests
-		to depend on other ReviewRequests and the opposite for blocking,
-		as well as review requests drafts to depend on review requests.
-		- Admin panel editing only atm.
-		- Removing remenants of old code from previous design.
-		- Also, did a run through of PEP8 standards on a portion of
-		touched files (reviews/models.py, ./admin.py)for a mid-task
-		coding standards cleanup.

Testing Done:

	+	Backend
	+	- Select from Review Request Dependency box a new request to depend on - Relationship has been added.
	+	- Select from Review Request Draft Dependency box a new request to depend on - Relationship has been added.
	+	- Viewed these changes on the front end - Blocks relationship has been adjusted in response for the selected request.
	+
	+	Frontend
	+	- Clicked to change dependencies, added a proper request after a comma and space - properly checked and added to draft.
	+	- Clicked to change dependencies, added an improper request after a comma and space - properly checked discarded with an accompanying warning.
	+	- Clicked to change dependencies, added a proper request after a space - properly checked and added to draft.
	+	- Clicked to change dependencies, added an improper request after a space - properly checked discarded with an accompanying warning.
	+	- Removed a dependency - Changes remain removed.
	+	- Published Changes - Changes remain added.
	+	- Enter in one incorrect request vs. multiple incorrect requests - Error adjusts to pluralized form.
	+	- Provide a change to an already published request - change log reports adjustment.

Diff:

Revision 2 (+197 -95)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/datagrids.py
	reviewboard/reviews/models.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/datastore.js
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/webapi/resources.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/models.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/datagrids.py
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/static/rb/js/datastore.js
    reviewboard/templates/reviews/review_request_box.html

reviewboard/reviews/models.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```

reviewboard/reviews/models.py (Diff revision 2)

Show all issues

Col: 47
 E127 continuation line over-indented for visual indent

reviewboard/reviews/models.py (Diff revision 2)

Show all issues

Col: 47
 E127 continuation line over-indented for visual indent

reviewboard/reviews/models.py (Diff revision 2)

Show all issues

Col: 47
 E127 continuation line over-indented for visual indent

reviewboard/reviews/models.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```

reviewboard/webapi/resources.py (Diff revision 2)

Show all issues

Col: 49
 E128 continuation line under-indented for visual indent

Change Summary:

fixed PEP8 checker results

Diff:

Revision 3 (+198 -95)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/datagrids.py
	reviewboard/reviews/models.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/datastore.js
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/webapi/resources.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/models.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/datagrids.py
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/static/rb/js/datastore.js
    reviewboard/templates/reviews/review_request_box.html

reviewboard/reviews/models.py (Diff revision 3)
Show all issues
```
Col: 67
 W291 trailing whitespace
```

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

Show all issues

Col: 47
 E127 continuation line over-indented for visual indent

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

Show all issues

Col: 47
 E127 continuation line over-indented for visual indent

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

Show all issues

Col: 47
 E127 continuation line over-indented for visual indent

reviewboard/reviews/models.py (Diff revision 3)
Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/resources.py (Diff revision 3)
Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```

reviewboard/webapi/resources.py (Diff revision 3)

Show all issues

Col: 58
 E127 continuation line over-indented for visual indent

Description:

		Feature
		- This feature adds the ability to link requests together by
		means of two fields; depends on, and blocks. These relationships
		are interconnected in the sense that, if Request A depends on
		Request B, then inversely Request B's blocks field will display
		a link back to Request A.
		- This request adds the ability to add, edit, and remove dependencies
		from both the backend admin panel as well as in the front-end.
~		- Blocking relationships are build as a reverse of dependency, and
~		thus are automatically generated upon dependency change.
	~	- Blocking relationships are built as a reverse of dependency, and
	~	thus are automatically generated upon a dependency change.

Diff:

Revision 4 (+201 -97)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/datagrids.py
	reviewboard/reviews/models.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/datastore.js
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/webapi/resources.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/models.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/datagrids.py
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/static/rb/js/datastore.js
    reviewboard/templates/reviews/review_request_box.html

reviewboard/reviews/models.py (Diff revision 4)
Show all issues
```
Col: 67
 W291 trailing whitespace
```

reviewboard/webapi/resources.py (Diff revision 4)

Show all issues

Col: 56
 E128 continuation line under-indented for visual indent

Demelo Feb. 20, 2013, 9:10 p.m.

There's no way to format this without having a PEP8 error.

reviewboard/webapi/resources.py (Diff revision 4)

Show all issues

Col: 80
 E501 line too long (80 > 79 characters)

Demelo Feb. 20, 2013, 9:10 p.m.

There's no way to format this without having a PEP8 error.

reviewboard/webapi/resources.py (Diff revision 4)

Show all issues

Col: 56
 E128 continuation line under-indented for visual indent

Demelo Feb. 20, 2013, 9:10 p.m.

There's no way to format this without having a PEP8 error.

Diff:

Revision 5 (+185 -96)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/datagrids.py
	reviewboard/reviews/models.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/datastore.js
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/webapi/resources.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/models.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/datagrids.py
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/static/rb/js/datastore.js
    reviewboard/templates/reviews/review_request_box.html

reviewboard/reviews/models.py (Diff revision 5)

Show all issues

Col: 40
 E128 continuation line under-indented for visual indent

reviewboard/reviews/models.py (Diff revision 5)

Show all issues

Col: 40
 E128 continuation line under-indented for visual indent

reviewboard/reviews/models.py (Diff revision 5)

Show all issues

Col: 40
 E128 continuation line under-indented for visual indent

reviewboard/webapi/resources.py (Diff revision 5)
Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```

In general, this is shaping up nicely, but I have some comments. These fall primarily into two categories:

1) Fixes you need to make to your change.
2) Changes you need to revert.

It's very important not to mix feature/bug fix changes and cleanup changes. Half this change are cleanup changes that have nothing to do with your work, and the cleanups aren't really things we want in some cases (ReviewBot isn't always 100% correct). I mention this below, but I'd ask that you undo every change in here that isn't related to your project. (Unfortunately, unless you did all that in their own commits, this might be a pain to do.)

reviewboard/reviews/datagrids.py (Diff revision 5)
Show all issues
```
Looks like this is missing a space.
```
1. DE
  
  Demelo Feb. 25, 2013, 2:01 a.m.
  Dropped due to being in datagrid.

reviewboard/reviews/datagrids.py (Diff revision 5)

Show all issues

Okay, so this is a good lesson in query optimization. Say we load 30 rows in the datagrid. What this will do is perform this query 30 times, and bring in a lot of data with it. That's a huge increase in load.

Instead, you want to augment the queryset. Look at the augment_queryset functions in that class for a bunch of different examples. Some of them are going to be more complex than others.

What does the data look like that you're trying to show? Are these all summaries? If so, that's going to be very hard to read in a UI like the datagrid.

Demelo Feb. 25, 2013, 1:53 a.m.

Should I drop this one since it's related to the datagrid?

reviewboard/reviews/datagrids.py (Diff revision 5)

Show all issues

This looks... interesting :) What are you trying to do here?

reviewboard/reviews/models.py (Diff revision 5)

Show all issues

The other wrapping was more correct. If ReviewBot is yelling at you here, ignore it. Don't worry about fixing this and other fields unless you're working with them directly. It'll just make it harder for us to find the source of problems later when looking at who last touched the lines.

Cleanup changes are fine, but don't merge them in with feature changes. I'd say, undo all that in this change. It'll also be easier for us, as we can focus on what actually changed in your code.

reviewboard/reviews/models.py (Diff revision 5)

Show all issues

_() is for localizing human-readable strings. This is an identifier, which won't work so well if, say, a Russian translation changes it.

reviewboard/reviews/models.py (Diff revision 5)

Show all issues

The indentation of all this is wrong. THey're off by one character.

reviewboard/reviews/models.py (Diff revision 5)
Show all issues
```
Same comment about not using _().
```

reviewboard/static/rb/js/datastore.js (Diff revision 5)

Show all issues

Can you change these to use === instead of == ? (In JavaScript, == casts, and === doesn't.. the latter is faster).

reviewboard/static/rb/js/reviews.js (Diff revision 5)
Show all issues
```
Trailing commas will break many browsers.
```
reviewboard/templates/reviews/review_request_box.html (Diff revision 5)
Show all issues
```
What's this all about?
```

reviewboard/templates/reviews/review_request_box.html (Diff revision 5)

Show all issues

Note the other template tags. The {% should always be unindented, and the contents inside should be indented relative to the other template tags.

reviewboard/templates/reviews/review_request_box.html (Diff revision 5)
Show all issues
```
This looks wrong.
```

reviewboard/webapi/resources.py (Diff revision 5)

Show all issues

These should explicitly say "review request". Same with the other field strings.

reviewboard/webapi/resources.py (Diff revision 5)

Show all issues

This doesn't look right to me, and I feel very uncomfortable with changing old code like this that doesn't have any bugs associated with it. What are you trying to do?

reviewboard/webapi/resources.py (Diff revision 5)

Show all issues

Some indentation weirdness. I would change this to start parameters on the second line, indented 4 spaces from the "obj".

There's also some danger here with how you're doing this lookup. An ID is *very* different depending on whether it's connected to a Local Site or not. You'll need to query differently depending on whether local_site is None.

Demelo Feb. 25, 2013, 11:32 p.m.

I believe I've addressed this. I'll wait on confirmation before hitting Fixed.

reviewboard/webapi/resources.py (Diff revision 5)
Show all issues
```
This shouldn't be here.
```

reviewboard/webapi/resources.py (Diff revision 5)

Show all issues

"review request's"

Also, trailing space should be removed.

Change Summary:

Addressed auto-PEP8 incident by removing unneeded changes.

Diff:

Revision 6 (+387 -108)

Show changes

	setup.py
	docs/manual/docs.db
	docs/manual/webapi/2.0/resources/default-reviewer.txt
	docs/manual/webapi/2.0/resources/default-reviewer-list.txt
	docs/manual/webapi/2.0/resources/index.txt
	docs/releasenotes/reviewboard/1.6.16.txt
	docs/releasenotes/reviewboard/1.7.6.txt
	docs/releasenotes/reviewboard/index.txt
	10 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/models.py
    reviewboard/__init__.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/datagrids.py
    setup.py
  Ignored Files:
    docs/manual/webapi/2.0/resources/default-reviewer.txt
    docs/manual/webapi/2.0/resources/default-reviewer-list.txt
    docs/manual/webapi/2.0/resources/index.txt
    reviewboard/static/rb/js/datastore.js
    docs/releasenotes/reviewboard/index.txt
    reviewboard/cmdline/conf/reviewboard.wsgi.in
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/js/reviews.js
    docs/manual/docs.db
    docs/releasenotes/reviewboard/1.6.16.txt
    docs/releasenotes/reviewboard/1.7.6.txt

reviewboard/reviews/models.py (Diff revision 6)

Show all issues

Col: 40
 E128 continuation line under-indented for visual indent

reviewboard/reviews/models.py (Diff revision 6)

Show all issues

Col: 40
 E128 continuation line under-indented for visual indent

reviewboard/reviews/models.py (Diff revision 6)

Show all issues

Col: 40
 E128 continuation line under-indented for visual indent

reviewboard/webapi/resources.py (Diff revision 6)
Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```

Change Summary:

Addressed Bot and issue tracker.

Diff:

Revision 7 (+386 -107)

Show changes

	setup.py
	docs/manual/docs.db
	docs/manual/webapi/2.0/resources/default-reviewer.txt
	docs/manual/webapi/2.0/resources/default-reviewer-list.txt
	docs/manual/webapi/2.0/resources/index.txt
	docs/releasenotes/reviewboard/1.6.16.txt
	docs/releasenotes/reviewboard/1.7.6.txt
	docs/releasenotes/reviewboard/index.txt
	10 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/models.py
    reviewboard/__init__.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/datagrids.py
    setup.py
  Ignored Files:
    docs/manual/webapi/2.0/resources/default-reviewer.txt
    docs/manual/webapi/2.0/resources/default-reviewer-list.txt
    docs/manual/webapi/2.0/resources/index.txt
    reviewboard/static/rb/js/datastore.js
    docs/releasenotes/reviewboard/index.txt
    reviewboard/cmdline/conf/reviewboard.wsgi.in
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/js/reviews.js
    docs/manual/docs.db
    docs/releasenotes/reviewboard/1.6.16.txt
    docs/releasenotes/reviewboard/1.7.6.txt

reviewboard/webapi/resources.py (Diff revision 7)
Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/resources.py (Diff revision 7)
Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/webapi/resources.py (Diff revision 7)
Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```

reviewboard/webapi/resources.py (Diff revision 7)

Show all issues

Col: 41
 E128 continuation line under-indented for visual indent

reviewboard/webapi/resources.py (Diff revision 7)

Show all issues

Col: 41
 E126 continuation line over-indented for hanging indent

Diff:

Revision 8 (+389 -107)

Show changes

	setup.py
	docs/manual/docs.db
	docs/manual/webapi/2.0/resources/default-reviewer.txt
	docs/manual/webapi/2.0/resources/default-reviewer-list.txt
	docs/manual/webapi/2.0/resources/index.txt
	docs/releasenotes/reviewboard/1.6.16.txt
	docs/releasenotes/reviewboard/1.7.6.txt
	docs/releasenotes/reviewboard/index.txt
	10 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/models.py
    reviewboard/__init__.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/datagrids.py
    setup.py
  Ignored Files:
    docs/manual/webapi/2.0/resources/default-reviewer.txt
    docs/manual/webapi/2.0/resources/default-reviewer-list.txt
    docs/manual/webapi/2.0/resources/index.txt
    reviewboard/static/rb/js/datastore.js
    docs/releasenotes/reviewboard/index.txt
    reviewboard/cmdline/conf/reviewboard.wsgi.in
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/js/reviews.js
    docs/manual/docs.db
    docs/releasenotes/reviewboard/1.6.16.txt
    docs/releasenotes/reviewboard/1.7.6.txt

reviewboard/webapi/resources.py (Diff revision 8)

Show all issues

Col: 41
 E128 continuation line under-indented for visual indent

reviewboard/webapi/resources.py (Diff revision 8)

Show all issues

Col: 41
 E126 continuation line over-indented for hanging indent

Diff:

Revision 9 (+389 -107)

Show changes

	setup.py
	docs/manual/docs.db
	docs/manual/webapi/2.0/resources/default-reviewer.txt
	docs/manual/webapi/2.0/resources/default-reviewer-list.txt
	docs/manual/webapi/2.0/resources/index.txt
	docs/releasenotes/reviewboard/1.6.16.txt
	docs/releasenotes/reviewboard/1.7.6.txt
	docs/releasenotes/reviewboard/index.txt
	10 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/models.py
    reviewboard/__init__.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/datagrids.py
    setup.py
  Ignored Files:
    docs/manual/webapi/2.0/resources/default-reviewer.txt
    docs/manual/webapi/2.0/resources/default-reviewer-list.txt
    docs/manual/webapi/2.0/resources/index.txt
    reviewboard/static/rb/js/datastore.js
    docs/releasenotes/reviewboard/index.txt
    reviewboard/cmdline/conf/reviewboard.wsgi.in
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/js/reviews.js
    docs/manual/docs.db
    docs/releasenotes/reviewboard/1.6.16.txt
    docs/releasenotes/reviewboard/1.7.6.txt

reviewboard/webapi/resources.py (Diff revision 9)

Show all issues

Col: 41
 E128 continuation line under-indented for visual indent

reviewboard/webapi/resources.py (Diff revision 9)

Show all issues

Col: 41
 E126 continuation line over-indented for hanging indent

Diff:

Revision 10 (+391 -107)

Show changes

	setup.py
	docs/manual/docs.db
	docs/manual/webapi/2.0/resources/default-reviewer.txt
	docs/manual/webapi/2.0/resources/default-reviewer-list.txt
	docs/manual/webapi/2.0/resources/index.txt
	docs/releasenotes/reviewboard/1.6.16.txt
	docs/releasenotes/reviewboard/1.7.6.txt
	docs/releasenotes/reviewboard/index.txt
	10 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/models.py
    reviewboard/__init__.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/datagrids.py
    setup.py
  Ignored Files:
    docs/manual/webapi/2.0/resources/default-reviewer.txt
    docs/manual/webapi/2.0/resources/default-reviewer-list.txt
    docs/manual/webapi/2.0/resources/index.txt
    reviewboard/static/rb/js/datastore.js
    docs/releasenotes/reviewboard/index.txt
    reviewboard/cmdline/conf/reviewboard.wsgi.in
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/js/reviews.js
    docs/manual/docs.db
    docs/releasenotes/reviewboard/1.6.16.txt
    docs/releasenotes/reviewboard/1.7.6.txt

reviewboard/webapi/resources.py (Diff revision 10)

Show all issues

Col: 39
 E711 comparison to None should be 'if cond is None

reviewboard/webapi/resources.py (Diff revision 10)

Show all issues

Col: 41
 E128 continuation line under-indented for visual indent

Diff:

Revision 11 (+391 -107)

Show changes

	setup.py
	docs/manual/docs.db
	docs/manual/webapi/2.0/resources/default-reviewer.txt
	docs/manual/webapi/2.0/resources/default-reviewer-list.txt
	docs/manual/webapi/2.0/resources/index.txt
	docs/releasenotes/reviewboard/1.6.16.txt
	docs/releasenotes/reviewboard/1.7.6.txt
	docs/releasenotes/reviewboard/index.txt
	10 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/models.py
    reviewboard/__init__.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/datagrids.py
    setup.py
  Ignored Files:
    docs/manual/webapi/2.0/resources/default-reviewer.txt
    docs/manual/webapi/2.0/resources/default-reviewer-list.txt
    docs/manual/webapi/2.0/resources/index.txt
    reviewboard/static/rb/js/datastore.js
    docs/releasenotes/reviewboard/index.txt
    reviewboard/cmdline/conf/reviewboard.wsgi.in
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/js/reviews.js
    docs/manual/docs.db
    docs/releasenotes/reviewboard/1.6.16.txt
    docs/releasenotes/reviewboard/1.7.6.txt

reviewboard/webapi/resources.py (Diff revision 11)

Show all issues

Col: 41
 E128 continuation line under-indented for visual indent

Change Summary:

Added two screenshots for a visual of the change.

Added Files:

Hey Jon, it looks like there are fragments from some other changes in this review request. You didn't, I assume, make these changes to the Django version requirement and docs, for example.

Can you please update this patch with just your changes?

reviewboard/reviews/datagrids.py (Diff revision 11)
Show all issues
```
I think this is worth some documentation.
```

Diff:

Revision 12 (+185 -91)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/datagrids.py
	reviewboard/reviews/models.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/webapi/resources.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/datagrids.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models.py
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/templates/reviews/review_request_box.html

reviewboard/webapi/resources.py (Diff revision 12)

Show all issues

Col: 41
 E128 continuation line under-indented for visual indent

Hey Jon,

This looks fantastic. Just some style nits from me, but I think this is looking really solid. One more review pass after these changes, and you've got my ship-it!

-Mike

reviewboard/reviews/datagrids.py (Diff revision 12)

Is there supposed to be a space in between there? Or are we supposed to be trying to use as little space as possible here?

reviewboard/reviews/models.py (Diff revision 12)

Show all issues

If you're going to break this up like this, I think I'd prefer to be consistent about putting the space at the end of the line (so move the space from the beginning of 123 to the end of 122 if possible).

reviewboard/templates/reviews/review_request_box.html (Diff revision 12)

Show all issues

"by" should be wrapped with trans tags (although the ordering when translated might differ...by might come after...)...

I assume you talked this move over with ChipX86 or David?

reviewboard/webapi/resources.py (Diff revision 12)

Show all issues

ReviewBot might have a point. Maybe:

obj = ReviewRequest.objects.get(
    Q(local_id=value) & Q(local_site=local_site))

So a huge chunk of this change is still stuff that should not have been changed. I think we went over this once, but please go through this change and remove every single modification that is not directly related to your change.

Yes, pep8 may complain about some things, but in some cases we want to ignore pep8's recommendations, and either way, such changes are best left to cleanup changes. As it is, too much time is being spent figuring out what in this change is general cleanup, and what's part of your feature.

Please make that a priority and then let me know immediately when it's done so I can give another re-review.

Demelo March 24, 2013, 2:33 a.m.

What's happened is I fixed these things, but we recently addressed an accidental master pull, and apparently the fix reverted all my changes.

chipx86 March 24, 2013, 2:35 a.m.
```
Doh! That sucks.
```

reviewboard/reviews/datagrids.py (Diff revision 12)

Show all issues

This isn't something the others are doing yet, but you should use newer-style parent construction:


    super(DependsOnColumn, self).__init__(self, *args, **kwargs)

reviewboard/reviews/models.py (Diff revision 12)
Show all issues
```
No blank line here.
```

reviewboard/reviews/models.py (Diff revision 12)

Show all issues

I'm seeing some stuff like this that I've commented on on previous reviews, which were marked as fixed. Can you go through my reviews and make sure they actually are?

reviewboard/reviews/models.py (Diff revision 12)

Show all issues

Don't change things that aren't directly related to your change.

reviewboard/reviews/models.py (Diff revision 12)
Show all issues
```
Same.
```

reviewboard/reviews/models.py (Diff revision 12)

Show all issues

Even if pep8 complains, modifying this in your change only makes it harder for us to track down issues later. It's unrelated.

reviewboard/reviews/models.py (Diff revision 12)
Show all issues
```
Nope, put it back :)
```

Change Summary:

Hopefully this new diff will have addressed the accidental PeP8 un-fixing.

Diff:

Revision 13 (+187 -92)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/datagrids.py
	reviewboard/reviews/models.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/datastore.js
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/webapi/resources.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/datagrids.py
    reviewboard/reviews/admin.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/models.py
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/static/rb/js/datastore.js
    reviewboard/templates/reviews/review_request_box.html

reviewboard/webapi/resources.py (Diff revision 13)

Show all issues

Col: 41
 E128 continuation line under-indented for visual indent

Jon,

There's still a number of things that do not belong in this patch. I've opened issues for the ones I found.

Otherwise, this is a really solid looking patch. I think I'll be pretty ready to give it a ship-it once these things are removed.

-Mike

reviewboard/reviews/admin.py (Diff revision 13)
Show all issues
```
Undo this change.
```
reviewboard/reviews/admin.py (Diff revision 13)
Show all issues
```
Undo this change.
```
reviewboard/reviews/models.py (Diff revision 13)
Show all issues
```
Undo this change.
```
reviewboard/reviews/models.py (Diff revision 13)
Show all issues
```
Undo this change.
```
reviewboard/reviews/models.py (Diff revision 13)
Show all issues
```
Undo this change.
```
reviewboard/reviews/models.py (Diff revision 13)
Show all issues
```
Undo this change.
```
reviewboard/reviews/models.py (Diff revision 13)
Show all issues
```
Undo these changes.
```
reviewboard/reviews/models.py (Diff revision 13)
Show all issues
```
Undo this change.
```
reviewboard/reviews/models.py (Diff revision 13)
Show all issues
```
Undo this change.
```
reviewboard/reviews/models.py (Diff revision 13)
Show all issues
```
Undo this change.
```
reviewboard/reviews/models.py (Diff revision 13)
Show all issues
```
Undo this change.
```
reviewboard/reviews/models.py (Diff revision 13)
Show all issues
```
Undo this change.
```
reviewboard/reviews/models.py (Diff revision 13)
Show all issues
```
Undo this change.
```
reviewboard/reviews/models.py (Diff revision 13)
Show all issues
```
Undo these changes.
```
reviewboard/reviews/models.py (Diff revision 13)
Show all issues
```
Undo thsi change.
```
reviewboard/reviews/models.py (Diff revision 13)
Show all issues
```
Undo this change.
```
reviewboard/reviews/models.py (Diff revision 13)
Show all issues
```
Undo these changes.
```
reviewboard/reviews/models.py (Diff revision 13)
Show all issues
```
Undo this change.
```
reviewboard/static/rb/js/datastore.js (Diff revision 13)
Show all issues
```
Undo this change.
```

reviewboard/webapi/resources.py (Diff revision 13)

Show all issues

Undo this change.

Demelo April 2, 2013, 7:49 a.m.

I've updated with the most recent diff. It both does not have the PEP8 mess up from before (what these are issues revolve around), as well as does not have the accidental merge that reverted my changes (thus bringing back the accidental PEP8 changes). I've also manually confirmed all these issues, so these are for sure no longer in the diff.

Change Summary:

I could type a story about this whole ordeal, but in short, I found an externally saved copy of a version of the development that existed at a safe point before the whole branch collapse occurred, so I'm overhauled it to where it needed to be, and it should be outside of these accidentally included changes from the bad merge incident.

Diff:

Revision 14 (+79 -11)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/models.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/datastore.js
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/webapi/resources.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models.py
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/static/rb/js/datastore.js
    reviewboard/templates/reviews/review_request_box.html

reviewboard/reviews/models.py (Diff revision 14)

Show all issues

Col: 40
 E128 continuation line under-indented for visual indent

reviewboard/reviews/models.py (Diff revision 14)

Show all issues

Col: 40
 E128 continuation line under-indented for visual indent

reviewboard/reviews/models.py (Diff revision 14)

Show all issues

Col: 40
 E128 continuation line under-indented for visual indent

reviewboard/webapi/resources.py (Diff revision 14)

Show all issues

Col: 41
 E128 continuation line under-indented for visual indent

Change Summary:

Addressed the Bot's PEP8 request for three lines of my own code.

Diff:

Revision 15 (+79 -11)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/models.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/datastore.js
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/webapi/resources.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models.py
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/static/rb/js/datastore.js
    reviewboard/templates/reviews/review_request_box.html

reviewboard/webapi/resources.py (Diff revision 15)
Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```

reviewboard/reviews/models.py (Diff revision 14)
Show all issues
```
related_name should never be localized. No _(..)
```

reviewboard/reviews/models.py (Diff revision 14)

Show all issues

Alignment issues. The parameters should be aligned to 'ReviewRequest'.

The 'ReviewRequest' here should be able to be ReviewRequest (no quotes).

reviewboard/reviews/models.py (Diff revision 14)
Show all issues
```
Same here.
```
reviewboard/static/rb/js/datastore.js (Diff revision 14)
Show all issues
```
These should really be ===. Can you change all three?
```

reviewboard/static/rb/js/reviews.js (Diff revision 14)

Show all issues

This looks broken. I don't see how it can compile.

Demelo April 2, 2013, 8:29 a.m.

I'm going through these changes, and I'm noticing some inconsistancies between the actual diff (the one i'm looking at on my local copy/personal github) and what is being reported as changed here. This removal isn't actually in the code, same with a few of the previous comments. Confusing. :S

Demelo April 3, 2013, 3:41 a.m.

Forgot to mention, these were fixed in one of the recent versions.

reviewboard/static/rb/js/reviews.js (Diff revision 14)
Show all issues
```
Where did this go?
```
reviewboard/static/rb/js/reviews.js (Diff revision 14)
Show all issues
```
===
```
reviewboard/static/rb/js/reviews.js (Diff revision 14)
Show all issues
```
"Review request"
```
reviewboard/static/rb/js/reviews.js (Diff revision 14)
Show all issues
```
===
```
reviewboard/static/rb/js/reviews.js (Diff revision 14)
Show all issues
```
"Review requests"
```

reviewboard/templates/reviews/review_request_box.html (Diff revision 14)

Show all issues

These should be unindented, but with the contents indented relative to the nearest template tags (like: {%    foo %}).

Same applies to all other added template tags in your change.

reviewboard/templates/reviews/review_request_box.html (Diff revision 14)
Show all issues
```
class="" should be removed.
```
reviewboard/webapi/resources.py (Diff revision 14)
Show all issues
```
"for this review request."
```
reviewboard/webapi/resources.py (Diff revision 14)
Show all issues
```
"for this review request."
```
reviewboard/webapi/resources.py (Diff revision 14)
Show all issues
```
"for this review request."
```

reviewboard/webapi/resources.py (Diff revision 14)

Show all issues

We have this split in two places, and both are slightly different. One is "[, ]+" and one is "[,\s*]". I think we should consolidate these.

I'd recommend adding a COMMA_SEPARATED_RE regex to BaseReviewRequestDetails (reviews/models.py) that is a compiled regex (re.compile(...)) and then reference that both here and in BaseReviewRequestDetails.

Demelo April 2, 2013, 12:10 p.m.

I believe what happened here was that I saw the old way it was done, i.e. the original, and tried it out on a regex tester online to see how it worked (I'm new to REGEX), and by accident I pasted over the old way with the one I made in the regex tester just to see if it worked, and forgot to change it back. Should I just change it back?

mike_conley April 5, 2013, 3:24 a.m.

Yes, you should change this back if the functionality is the same.

chipx86 April 5, 2013, 5:48 a.m.

Yeah, we should use the one we have in reviews/models.py.

reviewboard/webapi/resources.py (Diff revision 14)

Show all issues

Rather than this, can you add a new function to ReviewRequestManager (reviews/managers.py) called, let's say, for_id(), that would contain this logic. That way it's in one place. Ideally, complex queries like this only exist in a single place in the codebase (we still have work to do to actually get there).

Demelo April 2, 2013, 12:04 p.m.

So I've started to address this, but there seems to be something not clicking. 

I've created a pastie to simply illustrate what I've done: http://pastie.org/7278394

Basically, using the old ways, it works fine. i.e. RR #1 can add RR #2 as dependant, etc etc.

However, upon the new changes, no matter what, the addition is always invalid, saying that the <insert entered number> doesn't exist. I'm pretty confused. Any thoughts?

mike_conley April 5, 2013, 3:24 a.m.

Hrm. I don't. I wonder if perhaps we should try fixing this in a follow-up...

chipx86 April 5, 2013, 5:48 a.m.

Any errors in the console? Sounds like something more is going on than there seems.

Demelo April 5, 2013, 4:03 p.m.

It's the weirdest thing, it all now works as intended. Maybe my browser was wacky that day? Who knows, but it works perfect now with the new Manager change.

reviewboard/webapi/resources.py (Diff revision 14)
Show all issues
```
pk=value
```
reviewboard/webapi/resources.py (Diff revision 14)
Show all issues
```
No blank line here.
```
reviewboard/webapi/resources.py (Diff revision 14)
Show all issues
```
"review request's"
```

Change Summary:

Address a first round of Christian Hammond-suggested fixes.

Diff:

Revision 16 (+83 -12)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/models.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/datastore.js
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/webapi/resources.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models.py
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/static/rb/js/datastore.js
    reviewboard/templates/reviews/review_request_box.html

reviewboard/webapi/resources.py (Diff revision 16)
Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/resources.py (Diff revision 16)
Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/webapi/resources.py (Diff revision 16)
Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/webapi/resources.py (Diff revision 16)
Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```

Change Summary:

Addressed PEP8.

Diff:

Revision 17 (+85 -12)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/models.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/datastore.js
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/webapi/resources.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models.py
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/static/rb/js/datastore.js
    reviewboard/templates/reviews/review_request_box.html

reviewboard/webapi/resources.py (Diff revision 17)
Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/resources.py (Diff revision 17)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

So there's Christian's one outstanding comment about the modified regex - that needs to be addressed, and then these below.

Once those are fixed, I'm happy to give a ship it.

reviewboard/reviews/models.py (Diff revision 17)
Show all issues
```
I believe verbose_name needs to be wrapped in _()
```
reviewboard/reviews/models.py (Diff revision 17)
Show all issues
```
Same here
```

Nice work! I can't wait for this patch be deployed on <http://reviews.reviewboard.org>! 

The "text editor" patch is most likely going to depend on my "markdown" patch, so I might list your patch as my "markdown" patch's dependent! lmao...

reviewboard/templates/reviews/review_request_box.html (Diff revision 17)

Show all issues

Template code should be indented within the `{% ... %}` label? 

So this might become, 

    {% spaceless %}
    {%  for dependency in review_request_details.depends_on.all %}
    ...
    {%   if not forloop.last %}, {% endif %}
    {%  endfor %}
    {% endspaceless %}

mike_conley April 6, 2013, 2:36 a.m.

Instead of just dropping this issue, can you explain why it was dropped Jon?

Demelo April 6, 2013, 8:06 p.m.

Woops, I might've mixed this up, I don't know why this was closed. I'll change the formatting next.

DE

Demelo April 6, 2013, 9:02 p.m.
```
Fixed in the latest diff.
```

reviewboard/webapi/resources.py (Diff revision 17)

Show all issues

I'm not sure what this code is intending to do, but `,\s*` is quite different than `[,\s*]` in regex. 

`,\s*` will match any string that start with a comma and follow by arbitrary number of space. 

`[,\s*]` will match a character that is a comma, a space or a star(*). 


When using these two regex to perform split, the result will be totally different. 

For example, let `data = "abc,    def"`. 
- `,\s*` will split the data into "abc" and "def"
- `[,\s*]` will split the data into "abc", " ", " " and "def"

If there is a star(*) within the string, it will become more messy. You can play around with regex on <http://regexpal.com/>

Change Summary:

Addressed final issues in tracker, ready to ship!

Diff:

Revision 18 (+91 -12)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/datastore.js
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/webapi/resources.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/models.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/managers.py
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/static/rb/js/datastore.js
    reviewboard/templates/reviews/review_request_box.html

reviewboard/reviews/managers.py (Diff revision 18)

Show all issues

Col: 45
 E128 continuation line under-indented for visual indent

reviewboard/webapi/resources.py (Diff revision 18)

Show all issues

Col: 32
 E128 continuation line under-indented for visual indent

Change Summary:

PEP8 fixes.

Diff:

Revision 19 (+91 -12)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/datastore.js
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/webapi/resources.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/managers.py
    reviewboard/reviews/models.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/admin.py
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/static/rb/js/datastore.js
    reviewboard/templates/reviews/review_request_box.html

reviewboard/webapi/resources.py (Diff revision 19)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Fix it!

reviewboard/webapi/resources.py (Diff revision 19)

Show all issues

What happened to Christian's request?

"Yeah, we should use the one we have in reviews/models.py."

See the comments in my last review.

Demelo April 6, 2013, 2:39 p.m.

I thought had to remove my accidental changes, and that it was policy not to affect things that I'm not introducing? Because the only reason this is even in this review request is because I messed with it to see how it works and I forgot to revert my changes. It has nothing to do with what I'm bringing with this submission (indirectly it does, as its in the same function as some of my changes, but its not a part of my changes).

Demelo April 6, 2013, 3:15 p.m.

That being said, I might've misunderstood his comment. Is he saying to change it to: r"[, ]+", as found in BaseReviewRequestDetails for the get_bug_list function? Because that's the only split I can find in reviews/models.py

gregwym April 6, 2013, 9:27 p.m.

Split with `[, ]+` should be a better practice.

Change Summary:

Addressed Greg's issue with spacing in the template.

Diff:

Revision 20 (+91 -12)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/js/datastore.js
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/webapi/resources.py

Fix it!

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources.py
    reviewboard/reviews/models.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/managers.py
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/static/rb/js/datastore.js
    reviewboard/templates/reviews/review_request_box.html

reviewboard/webapi/resources.py (Diff revision 20)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Ship it!

Awesome job! Committing this. Only made a couple small fixes and added some unit tests. This will be part of 1.7.8.

Status: Closed (submitted)

Change Summary:

Made some small changes and pushed to release-1.7.x (a23c9e5)

Depends On & Blocks Relationships to Request Box.

Information

Reviewers

Summary:

Description:

People:

Change Summary:

Description:

Change Summary:

Summary:

Description:

Testing Done:

Diff:

Change Summary:

Diff:

Description:

Diff:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Diff:

Diff:

Diff:

Diff:

Change Summary:

Added Files:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)

Change Summary: