Summary

Changing Review Request Ownership

Review Request #6990 — Created Feb. 27, 2015 and submitted Nov. 23, 2015, 5:37 p.m.

Information

Owner

Chenxi

Repository

Review Board

Branch

master

Bugs

Depends On

Blocks

7765

Commit

05c1791...

Reviewers

Groups

reviewboard, students

People

Description

make the ownership changeable on the UI level

Main code describtions:

In builtin_fields.py we change the 'submitter' field to inherit BaseEditableField so that the 'submitter' field will be editable; The override function--record_change_entry disposes what will be showed in change description; Then override the function to render the only one value for a change description string to HTML instead of a list.

In changedescs/models.py we describe the 'new' and 'old' changes in 'submitter' field and render the value to HTML.

Changes in models/review_request_draft.py are to add 'owner' field to override 'submitter' field. Since 'submitter' field is a property attribute of draftreviewrequest, if we change the'submitter' field from property to attribute directly, 'submitter' of older data in the dababase will be null. This will cause troubles.

Changes in reviewboard/reviews/evolutions are about Writing Database Evolutions of adding 'owner' field to the draft table.

In webapi/resources/review_request_draft.py we get the change information of 'submitter' field and set draft.owner if the change is available.

In reviewRequestEditorView.js we make the field can be autocompleted then show in proper format when updated.

In models/reviewRequestEditorModel.js we detect the changes of 'submitter' field in the front end before sending message to the back end. And show the confirm notification only when 'submitter' field was modified.

In models/draftReviewRequestModel.js: parseResourceData we set the 'submitter' info to the data from response message so that we can use it to update the field.

Testing Done

Passed the http://localhost:8000/js-tests/ for the front end.
Passed tests for the back end.

Added tests:

./reviewboard/manage.py test -- reviewboard.webapi.tests.test_review_request_draft:ResourceTests.test_put_publish_with_new_submitter
This test is about wether the feature works to set the submitter field.

reviewRequestEditorModelTests:
Check to set empty value.
Check if set value successfully.
Check if error thrown when set invalid value.

reviewRequestEditorViewTests:
Check confirm notification works before publish.
Check field can be autocomplete.
Check field can edit.
Check save works.
Check value format after update.
Check edit count successfully.

draftReviewRequestModelTests:
Check parse works for 'submitter' field.

Issues

Description	From	Last Updated
Col: 80 E501 line too long (100 > 79 characters)	reviewbot	March 3, 2015, 6:56 a.m.
'model_cache' imported but unused	reviewbot	March 3, 2015, 6:56 a.m.
Col: 80 E501 line too long (98 > 79 characters)	reviewbot	March 3, 2015, 6:56 a.m.
Col: 21 E265 block comment should start with '# '	reviewbot	March 6, 2015, 10:21 p.m.
Col: 47 E127 continuation line over-indented for visual indent	reviewbot	March 3, 2015, 6:56 a.m.
Col: 47 E127 continuation line over-indented for visual indent	reviewbot	March 3, 2015, 6:57 a.m.
Col: 13 E265 block comment should start with '# '	reviewbot	March 3, 2015, 6:57 a.m.
Col: 55 E261 at least two spaces before inline comment	reviewbot	March 3, 2015, 6:57 a.m.
Col: 56 E262 inline comment should start with '# '	reviewbot	March 3, 2015, 6:57 a.m.
Col: 9 E128 continuation line under-indented for visual indent	reviewbot	March 26, 2015, 6:51 a.m.
Col: 21 E265 block comment should start with '# '	reviewbot	March 6, 2015, 10:28 p.m.
'UserResource' imported but unused	reviewbot	March 6, 2015, 10:33 p.m.
Col: 13 E265 block comment should start with '# '	reviewbot	March 6, 2015, 10:34 p.m.
Col: 22 E225 missing whitespace around operator	reviewbot	March 26, 2015, 6:51 a.m.
Col: 9 E128 continuation line under-indented for visual indent	reviewbot	March 26, 2015, 6:51 a.m.
I guess assocated is a typo	Chester	April 6, 2015, 7:30 a.m.
Remove trailing commas.	VT VTL-Developer	March 29, 2015, 10:33 p.m.
Use single quotes	brennie	March 29, 2015, 10:52 p.m.
Use single quotes.	brennie	March 29, 2015, 10:52 p.m.
We don't support ternary operators, you can do self.owner or self.review_request.submitter	VT VTL-Developer	March 29, 2015, 10:43 p.m.
how about a white space before else if?	Chester	March 26, 2015, 5:15 p.m.
Use single quotes.	brennie	March 29, 2015, 11:10 p.m.
I think the indentation of each argument should be consistent. Please see line 195-197	Chester	March 26, 2015, 5:17 p.m.
Remove blank lines.	VT VTL-Developer	March 29, 2015, 10:44 p.m.
Use single quotes for this. The quote marks should line up. You may want to assign confirm(gettext(...)) to a value …	brennie	March 29, 2015, 11:49 p.m.
Should this be 'submitter' instead?	XU xuanyi	March 29, 2015, 11:02 p.m.
"Convert"	brennie	March 29, 2015, 10:55 p.m.
How about makeLink or convertToLink ?	brennie	March 29, 2015, 10:58 p.m.
I guess we could do this also if we do want to explictly return '' if item is null. if …	XU xuanyi	March 27, 2015, 8:32 a.m.
I try to stay away from variables named data because technically all variables are data :)	brennie	March 29, 2015, 10:59 p.m.
We do processing for other fields in this way because we can have multiple values in them. I don't think …	brennie	March 29, 2015, 10:59 p.m.
Blank line between these.	brennie	March 29, 2015, 10:59 p.m.
Col: 36 E128 continuation line under-indented for visual indent	reviewbot	March 26, 2015, 5:14 p.m.
Col: 36 E128 continuation line under-indented for visual indent	reviewbot	March 26, 2015, 5:14 p.m.
Might be worth adding a comment about why we need to put old_value and new_value into tuples.	mike_conley	March 30, 2015, 8:34 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	March 30, 2015, 8:31 p.m.
We might want to add some documentation saying that the user will have the chance to cancel the publish in …	mike_conley	March 30, 2015, 8:38 p.m.
I think this might be clearer: 'Are you sure you want to change the ownership of this review request? Doing …	mike_conley	March 30, 2015, 8:40 p.m.
Should start with a capital letter and end in a period.	david	June 9, 2015, 1:34 a.m.
Undo this change (lists in python should have trailing commas)	david	June 9, 2015, 1:34 a.m.
This should have from __future__ import unicode_literals as the first line, and then a blank line, then the rest.	david	June 9, 2015, 1:04 a.m.
There will only ever be one submitter, so we can just use gettext, not ngettext.	david	June 9, 2015, 1:01 a.m.
These two lines can be combined (define and assign in one go)	david	June 10, 2015, 10:19 p.m.
You could even just move the gettext call inline here, since that text is only used once.	david	June 10, 2015, 10:19 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	June 14, 2015, 4:19 a.m.
Col: 39 E127 continuation line over-indented for visual indent	reviewbot	June 14, 2015, 4:18 a.m.
Blank line between these.	brennie	June 19, 2015, 8:47 p.m.
You can put $link on the previous line. Make sure to dedent the following two lines.	brennie	June 19, 2015, 8:56 p.m.
You can just build it all at once using a template, e.g. _linkTemplate: _.template('<a href="<%- url %>"><%- label %></a>'), /* …	brennie	June 24, 2015, 8:29 p.m.
Isn't $field.children() already a jQuery wrapper around the DOM elements?	brennie	June 24, 2015, 8:52 p.m.
submitter before summary	brennie	June 24, 2015, 9:09 p.m.
submitter before summary	brennie	June 24, 2015, 9:11 p.m.
Blank line between these.	brennie	June 24, 2015, 9:11 p.m.
Can you either move the template definition into the initialize method (storing it as a variable on the object), or …	david	July 30, 2015, 7:23 a.m.
These should be indented 4 more spaces.	david	July 30, 2015, 7:22 a.m.
This is indented too far.	david	July 30, 2015, 7:23 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

reviewboard/reviews/evolutions/add_owner_to_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (100 > 79 characters)
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'model_cache' imported but unused
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (98 > 79 characters)
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 21
 E265 block comment should start with '# '
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 47
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 47
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 13
 E265 block comment should start with '# '
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 55
 E261 at least two spaces before inline comment
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 56
 E262 inline comment should start with '# '
```

Could you please make the description more detailed? Because it is kind of hard to understand what you are doing in here.

Change Summary:

fix the bug that nothing shows when modify the submitter

Description:

		make the ownership changeable on the UI level
	+
	+	Some fields are unnecessary since WIP
	+
	+	Main code describtions:
	+
	+	Changes in builtin_fields.py are to make the UI shows the 'submitter' field editable.
	+
	+	Changes in models/review_request_draft.py are to add 'owner' field to override 'submitter' field. Since 'submitter' field is a property attribute of draftreviewrequest, if we change the'submitter' field from property to attribute directly, 'submitter' of older data in the dababase will be null. This will cause troubles.
	+
	+	Changes in reviewboard/reviews/evolutions are about Writing Database Evolutions
	+
	+	Changes in models/draftReviewRequestModel.js: parseResourceData set the 'submitter' info to the data from response message.

Commit:

b6161f593972924c3d31511c7b39e23672e08b6b

abd6e9d61a339a0e595620df7897c3ce14920d1d

Diff:

Revision 2 (+119 -2)

Show changes

	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_owner_to_draft.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
	reviewboard/static/rb/js/resources/models/reviewRequestModel.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/webapi/resources/review_request_draft.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/resources/models/reviewRequestModel.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

reviewboard/reviews/models/review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 9
 E128 continuation line under-indented for visual indent
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 21
 E265 block comment should start with '# '
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 'UserResource' imported but unused
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 13
 E265 block comment should start with '# '
```

Change Summary:

Store submitter field when publish and Make changed field shows correctly after publish.

Description:

		make the ownership changeable on the UI level

		Some fields are unnecessary since WIP

		Main code describtions:

~		Changes in builtin_fields.py are to make the UI shows the 'submitter' field editable.
	~	Changes in builtin_fields.py are to make the UI shows the 'submitter' field editable. The override funcs make publish successfully and changed field shows correctly after publish.

		Changes in models/review_request_draft.py are to add 'owner' field to override 'submitter' field. Since 'submitter' field is a property attribute of draftreviewrequest, if we change the'submitter' field from property to attribute directly, 'submitter' of older data in the dababase will be null. This will cause troubles.

		Changes in reviewboard/reviews/evolutions are about Writing Database Evolutions

		Changes in models/draftReviewRequestModel.js: parseResourceData set the 'submitter' info to the data from response message.

Commit:

abd6e9d61a339a0e595620df7897c3ce14920d1d

45f78d19a7aebca8c75e6c7afd918bad1b5ff227

Diff:

Revision 3 (+129 -2)

Show changes

	reviewboard/changedescs/models.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_owner_to_draft.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/webapi/resources/review_request_draft.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py
    reviewboard/changedescs/models.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py
    reviewboard/changedescs/models.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

reviewboard/changedescs/models.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 22
 E225 missing whitespace around operator
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 9
 E128 continuation line under-indented for visual indent
```

Change Summary:

Add tests and fix some bugs

Description:

		make the ownership changeable on the UI level

-		Some fields are unnecessary since WIP
-
		Main code describtions:

~		Changes in builtin_fields.py are to make the UI shows the 'submitter' field editable. The override funcs make publish successfully and changed field shows correctly after publish.
	~	In builtin_fields.py we change the 'submitter' field to inherit BaseEditableField so that the 'submitter' field will be editable; The override function--record_change_entry disposes what will be showed in change description; Then override the function to render the only one value for a change description string to HTML instead of a list.
	+
	+	In changedescs/models.py we describe the 'new' and 'old' changes in 'submitter' field and render the value to HTML.

		Changes in models/review_request_draft.py are to add 'owner' field to override 'submitter' field. Since 'submitter' field is a property attribute of draftreviewrequest, if we change the'submitter' field from property to attribute directly, 'submitter' of older data in the dababase will be null. This will cause troubles.

~		Changes in reviewboard/reviews/evolutions are about Writing Database Evolutions
	~	Changes in reviewboard/reviews/evolutions are about Writing Database Evolutions of adding 'owner' field to the draft table.
	+
	+	In webapi/resources/review_request_draft.py we get the change information of 'submitter' field and set draft.owner if the change is available.
	+
	+	In reviewRequestEditorView.js we make the field can be autocompleted then show in proper format when updated.
	+
	+	In models/reviewRequestEditorModel.js we detect the changes of 'submitter' field in the front end before sending message to the back end. And show the confirm notification only when 'submitter' field was modified.

~		Changes in models/draftReviewRequestModel.js: parseResourceData set the 'submitter' info to the data from response message.
	~	In models/draftReviewRequestModel.js: parseResourceData we set the 'submitter' info to the data from response message so that we can use it to update the field.

Testing Done:

	+	Passed the http://localhost:8000/js-tests/ for the front end.
	+	Passed tests for the back end.
	+
	+	Added tests:
	+
	+	./reviewboard/manage.py test -- reviewboard.webapi.tests.test_review_request_draft:ResourceTests.test_put_publish_with_new_submitter
	+	This test is about wether the feature works to set the submitter field.
	+
	+	reviewRequestEditorModelTests:
	+	Check to set empty value.
	+	Check if set value successfully.
	+	Check if error thrown when set invalid value.
	+
	+	reviewRequestEditorViewTests:
	+	Check confirm notification works before publish.
	+	Check field can be autocomplete.
	+	Check field can edit.
	+	Check save works.
	+	Check value format after update.
	+	Check edit count successfully.
	+
	+	draftReviewRequestModelTests:
	+	Check parse works for 'submitter' field.

Commit:

45f78d19a7aebca8c75e6c7afd918bad1b5ff227

5d6184e7603d75acab385f697bcaa2aee2acf205

Groups:

students

Diff:

Revision 4 (+275 -5)

Show changes

	reviewboard/changedescs/models.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_owner_to_draft.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/static/rb/js/models/reviewRequestEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
	5 more

Summary:: [WIP]Changing Review Request Ownership
Changing Review Request Ownership

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py
    reviewboard/changedescs/models.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py
    reviewboard/changedescs/models.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

reviewboard/webapi/resources/review_request_draft.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 36
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 36
 E128 continuation line under-indented for visual indent
```

reviewboard/reviews/models/review_request_draft.py (Diff revisions 3 - 4)
The issue has been resolved. Show all issues
```
I guess assocated is a typo
```
reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 4)
The issue has been resolved. Show all issues
```
how about a white space before else if?
```

reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 4)

The issue has been resolved. Show all issues

I think the indentation of each argument should be consistent. Please see line 195-197

reviewboard/changedescs/models.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Remove trailing commas.
```
1. VT
  
  VTL-Developer March 26, 2015, 9:55 p.m.
  Sorry, meant remove that trailing comma.
reviewboard/reviews/models/review_request_draft.py (Diff revision 4)
The issue has been resolved. Show all issues
```
We don't support ternary operators, you can do self.owner or self.review_request.submitter
```
reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Remove blank lines.
```

reviewboard/reviews/models/review_request_draft.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Use single quotes
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Use single quotes.
```
reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Use single quotes.
```

reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 4)

The issue has been resolved. Show all issues

Use single quotes for this. The quote marks should line up.

You may want to assign confirm(gettext(...)) to a value and check against that for readability.

Also change "the managing privileges" to "all managing privileges."

chipx86 March 29, 2015, 11:23 p.m.

I just answered this on Slack.
One important note is that for calls to gettext, we actually can't concatenate to wrap. In fact, we can't wrap. Otherwise, the logic for localizing will fail to see the second part of the string. So for text wrapped in gettext, we need to just let the line be as long as it needs.

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
"Convert"
```
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
How about makeLink or convertToLink ?
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 4)
The issue has been resolved. Show all issues
```
I try to stay away from variables named data because technically all variables are data :)
```

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

The issue has been resolved. Show all issues

We do processing for other fields in this way because we can have multiple values in them. I don't think we should do this for the submitter field because we only expect one value.

I think we should just take what we are given and try to look up exactly that.

reviewboard/webapi/resources/review_request_draft.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Blank line between these.
```

reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Should this be 'submitter' instead?
```

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 4)

The issue has been dropped. Show all issues

I guess we could do this also if we do want to explictly return '' if item is null.
if (item) {
    str += '<a href="';
    str += (urlFunc ? urlFunc(item) : item);
    str += '">';
    str += (textFunc ? textFunc(item) : item);
    str += '</a>';
}

return str

XU

xuanyi March 26, 2015, 9:53 p.m.
```
do not*
```

Change Summary:

amend some typo and line up bug

Commit:

5d6184e7603d75acab385f697bcaa2aee2acf205

6425e3ed00f8d023d3442de7693a9178bab16fa1

Diff:

Revision 5 (+274 -6)

Show changes

	reviewboard/changedescs/models.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_owner_to_draft.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/static/rb/js/models/reviewRequestEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
	5 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py
    reviewboard/changedescs/models.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py
    reviewboard/changedescs/models.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

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

Ship it!

Hey chenxi - this is looking really really good. Not too many more comments frome me. Great job!

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

The issue has been resolved. Show all issues

Might be worth adding a comment about why we need to put old_value and new_value into tuples.

reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 5)

The issue has been resolved. Show all issues

We might want to add some documentation saying that the user will have the chance to cancel the publish in the event that the submitter has been changed.

reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 5)

The issue has been resolved. Show all issues

I think this might be clearer:

'Are you sure you want to change the ownership of this review request? Doing so may prevent you from editing the review request afterwards.'

Change Summary:

add some comments

Commit:

6425e3ed00f8d023d3442de7693a9178bab16fa1

26503c705e9b673f56272d24aeaa3d60807a0b5f

Diff:

Revision 6 (+280 -7)

Show changes

	reviewboard/changedescs/models.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_owner_to_draft.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/static/rb/js/models/reviewRequestEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
	5 more

reviewboard/changedescs/models.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Should start with a capital letter and end in a period.
```
reviewboard/changedescs/models.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Undo this change (lists in python should have trailing commas)
```

reviewboard/reviews/evolutions/add_owner_to_draft.py (Diff revision 6)

The issue has been resolved. Show all issues

This should have from __future__ import unicode_literals as the first line, and then a blank line, then the rest.

reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 6)

The issue has been resolved. Show all issues

There will only ever be one submitter, so we can just use gettext, not ngettext.

reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 6)
The issue has been resolved. Show all issues
```
These two lines can be combined (define and assign in one go)
```

reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 6)

The issue has been resolved. Show all issues

You could even just move the gettext call inline here, since that text is only used once.

Chenxi June 9, 2015, 1:35 a.m.

I did this before, but since the text line is too long(Christian said in gettext we shouldn't break the text into two lines) so I write in this way to avoid that. Still should I fix this?

david June 10, 2015, 8:56 p.m.

I think since that line is going to be super long no matter what, we can just have it inline and have this one be the super long line instead of moving it above.

Change Summary:

combine lines where the text used only once

Commit:

26503c705e9b673f56272d24aeaa3d60807a0b5f

ae9712f72716d2768baaac29bf223422fd934304

Diff:

Revision 7 (+277 -6)

Show changes

	reviewboard/changedescs/models.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_owner_to_draft.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/static/rb/js/models/reviewRequestEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
	5 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py
    reviewboard/changedescs/models.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py
    reviewboard/changedescs/models.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

reviewboard/changedescs/models.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 39
 E127 continuation line over-indented for visual indent
```

Change Summary:

fix continuation line over-indented

Commit:

ae9712f72716d2768baaac29bf223422fd934304

dd93b181d54c3345450b6927783f86a6e037720b

Diff:

Revision 8 (+277 -6)

Show changes

	reviewboard/changedescs/models.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_owner_to_draft.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/static/rb/js/models/reviewRequestEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
	5 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py
    reviewboard/changedescs/models.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py
    reviewboard/changedescs/models.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

This looks good. I've just got a few nitpicks.

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Blank line between these.
```
1. CH
  
  Chenxi June 24, 2015, 10:44 p.m.
  Can you tell we where we should leave blank line? I will remember next time.
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 8)
The issue has been resolved. Show all issues
```
You can put $link on the previous line. Make sure to dedent the following two lines.
```

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 8)

The issue has been resolved. Show all issues

You can just build it all at once using a template, e.g.

_linkTemplate: _.template('<a href="<%- url %>"><%- label %></a>'),

/*
 * ...
 */
convertToLink: function(item, urlFunc, textFunc) {
    if (!item) {
        return '';
    }

    return _linkTemplate({
        url: urlFunc ? urlFunc(item) : item,
        label: textFunc ? textFunc(item) : item
    });

reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Isn't $field.children() already a jQuery wrapper around the DOM elements?
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 8)
The issue has been resolved. Show all issues
```
submitter before summary
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 8)
The issue has been resolved. Show all issues
```
submitter before summary
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Blank line between these.
```

Change Summary:

rebase and merge

Commit:

dd93b181d54c3345450b6927783f86a6e037720b

05c1791ea6a5a20011fb58b9aa458b59844d4af9

Diff:

Revision 9 (+293 -6)

Show changes

	reviewboard/changedescs/models.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_owner_to_draft.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/static/rb/js/models/reviewRequestEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
	5 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py
    reviewboard/changedescs/models.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py
    reviewboard/changedescs/models.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 9)

The issue has been resolved. Show all issues

Can you either move the template definition into the initialize method (storing it as a variable on the object), or after the if (!item) check below?

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 9)
The issue has been resolved. Show all issues
```
These should be indented 4 more spaces.
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 9)
The issue has been resolved. Show all issues
```
This is indented too far.
```

Change Summary:

fix over-indented lines

Diff:

Revision 10 (+292 -6)

Show changes

	reviewboard/changedescs/models.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/add_owner_to_draft.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/static/rb/js/models/reviewRequestEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
	5 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py
    reviewboard/changedescs/models.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/evolutions/__init__.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py
    reviewboard/reviews/builtin_fields.py
    reviewboard/reviews/evolutions/add_owner_to_draft.py
    reviewboard/changedescs/models.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

Status:: Completed
Change Summary:: Pushed to feature/review-request-ownership (bcaa2ff)