Summary

Add "Publish and Archive" to the review dialog.

Review Request #9220 — Created Sept. 24, 2017 and submitted Jan. 15, 2018, 8:39 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

Commit

9275765...

Reviewers

Groups

reviewboard, students

People

Description

This change adds a shortcut allowing people who are publishing a review
to publish the review and archive the review request all in one shot.

Testing Done

Manually tested adding a comment and clicking "Publish and Archive" from
the banner. Once published, the review request is marked as archived.
Manually tested adding a comment, clicking "Edit Review" from the
banner, and clicking "Publish and Archive" from the review dialog view.
Once published, the review request is marked as archived.

Added 3 unit tests, which all pass (along with all other tests). One
scenario creates a new visit and archives the review request. The other
two scenarios update existing visits from visible to visible, and
visible to archived.

Added a javascript test to ensure that onPublishClicked is called with
the correct values.

Issues

Description	From	Last Updated
I think you can remove "WIP" from your summary now.	david	Oct. 21, 2017, 12:47 a.m.
No space between . and "	brennie	Sept. 29, 2017, 8:18 p.m.
Docstrings should be in the imperitive mood (i.e., they should read as instructions). e.g. "update" instead of "updates". No space …	brennie	Oct. 8, 2017, 3:44 p.m.
"creates" For our testing docstrings, we wrap them at 79 but we don't really follow docstring convention, e.g. it is …	brennie	Sept. 29, 2017, 8:21 p.m.
E501 line too long (84 > 79 characters)	reviewbot	Sept. 29, 2017, 9:28 p.m.
Not necessary to pull this out.	brennie	Sept. 29, 2017, 8:29 p.m.
E501 line too long (86 > 79 characters)	reviewbot	Sept. 29, 2017, 9:28 p.m.
We could also make this method return the visit object since we're using that in the tests. It would make …	brennie	Oct. 15, 2017, 11:18 p.m.
You can do user=user here and it will look it up by primary key. Same below.	brennie	Oct. 8, 2017, 3:51 p.m.
"updates"	brennie	Sept. 29, 2017, 8:21 p.m.
E501 line too long (100 > 79 characters)	reviewbot	Sept. 29, 2017, 9:28 p.m.
Not necessary to pull this out.	brennie	Sept. 29, 2017, 8:30 p.m.
Blank line between these.	brennie	Sept. 29, 2017, 8:22 p.m.
E501 line too long (106 > 79 characters)	reviewbot	Sept. 29, 2017, 9:28 p.m.
E501 line too long (90 > 79 characters)	reviewbot	Sept. 29, 2017, 9:29 p.m.
"updates"	brennie	Sept. 29, 2017, 8:22 p.m.
E501 line too long (100 > 79 characters)	reviewbot	Sept. 29, 2017, 9:29 p.m.
Not necessary to pull this out.	brennie	Sept. 29, 2017, 8:30 p.m.
E501 line too long (106 > 79 characters)	reviewbot	Sept. 29, 2017, 9:29 p.m.
E501 line too long (103 > 79 characters)	reviewbot	Sept. 29, 2017, 9:29 p.m.
Trailing comma	brennie	Sept. 29, 2017, 8:24 p.m.
Trailing comma	brennie	Sept. 29, 2017, 8:24 p.m.
Trailing comma	brennie	Sept. 29, 2017, 8:25 p.m.
The inner () should be {}. Space after :.	brennie	Sept. 29, 2017, 8:25 p.m.
Col: 34 Expected ')' to match '(' from line 114 and instead saw ':'.	reviewbot	Sept. 29, 2017, 9:21 p.m.
Col: 35 Expected ')' and instead saw 'true'.	reviewbot	Sept. 29, 2017, 9:21 p.m.
Col: 39 Missing semicolon.	reviewbot	Sept. 29, 2017, 9:21 p.m.
Col: 13 Expected an identifier and instead saw ')'.	reviewbot	Sept. 29, 2017, 9:21 p.m.
Col: 13 Expected an assignment or function call and instead saw an expression.	reviewbot	Sept. 29, 2017, 9:21 p.m.
Col: 14 Missing semicolon.	reviewbot	Sept. 29, 2017, 9:22 p.m.
Col: 14 Expected an identifier and instead saw ')'.	reviewbot	Sept. 29, 2017, 9:22 p.m.
Col: 14 Expected an assignment or function call and instead saw an expression.	reviewbot	Sept. 29, 2017, 9:22 p.m.
E501 line too long (91 > 79 characters)	reviewbot	Sept. 29, 2017, 9:29 p.m.
Can you alphabetically sort this with the other imports?	brennie	Sept. 29, 2017, 8:26 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Sept. 29, 2017, 9:30 p.m.
Blank line between these.	brennie	Sept. 29, 2017, 8:26 p.m.
E501 line too long (97 > 79 characters)	reviewbot	Sept. 29, 2017, 9:31 p.m.
E501 line too long (89 > 79 characters)	reviewbot	Sept. 29, 2017, 9:31 p.m.
E501 line too long (84 > 79 characters)	reviewbot	Sept. 29, 2017, 9:34 p.m.
E501 line too long (86 > 79 characters)	reviewbot	Sept. 29, 2017, 9:34 p.m.
E501 line too long (100 > 79 characters)	reviewbot	Sept. 29, 2017, 9:34 p.m.
E501 line too long (106 > 79 characters)	reviewbot	Sept. 29, 2017, 9:34 p.m.
E501 line too long (90 > 79 characters)	reviewbot	Sept. 29, 2017, 9:34 p.m.
E501 line too long (100 > 79 characters)	reviewbot	Sept. 29, 2017, 9:34 p.m.
E501 line too long (106 > 79 characters)	reviewbot	Sept. 29, 2017, 9:34 p.m.
E501 line too long (103 > 79 characters)	reviewbot	Sept. 29, 2017, 9:34 p.m.
E501 line too long (91 > 79 characters)	reviewbot	Sept. 29, 2017, 9:34 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Sept. 29, 2017, 9:34 p.m.
E501 line too long (97 > 79 characters)	reviewbot	Sept. 29, 2017, 9:34 p.m.
E501 line too long (89 > 79 characters)	reviewbot	Sept. 29, 2017, 9:34 p.m.
E501 line too long (103 > 79 characters)	reviewbot	Sept. 29, 2017, 9:31 p.m.
E501 line too long (122 > 79 characters)	reviewbot	Sept. 29, 2017, 9:31 p.m.
E501 line too long (106 > 79 characters)	reviewbot	Sept. 29, 2017, 9:31 p.m.
E501 line too long (122 > 79 characters)	reviewbot	Sept. 29, 2017, 9:31 p.m.
E501 line too long (103 > 79 characters)	reviewbot	Sept. 29, 2017, 9:31 p.m.
E501 line too long (91 > 79 characters)	reviewbot	Sept. 29, 2017, 9:32 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Sept. 29, 2017, 9:32 p.m.
E501 line too long (97 > 79 characters)	reviewbot	Sept. 29, 2017, 9:32 p.m.
E501 line too long (89 > 79 characters)	reviewbot	Sept. 29, 2017, 9:32 p.m.
It was like this before, but can you fix up this docstring? There should be no space between the """ …	david	Oct. 9, 2017, 10:21 a.m.
Same comments on this docstring--space, imperative mood, and "Args" documentation.	david	Oct. 9, 2017, 10:22 a.m.
We should include a trailing comma on this line too.	david	Oct. 8, 2017, 3:21 p.m.
Blank line between these two.	david	Oct. 8, 2017, 3:19 p.m.
Add a trailing comma here.	david	Oct. 8, 2017, 3:22 p.m.
This should be reviewboard.reviews.models.review_request.ReviewRequest. The reviews.models.ReviewRequest is an alias but won't necessarily link to the right place. You can wrap …	david	Oct. 9, 2017, 10:23 a.m.
Same here.	david	Oct. 9, 2017, 10:23 a.m.
See my reply to previous reviews.	david	Oct. 9, 2017, 10:23 a.m.
This line in too long. Can you do it as: expect(view._onPublishClicked).toHaveBeenCalledWith({ publishAndArchive: true, });	david	Oct. 9, 2017, 10:25 a.m.
This is getting big enough that we should probably put each parameter on its own line.	david	Oct. 9, 2017, 11:23 a.m.
W291 trailing whitespace	reviewbot	Oct. 9, 2017, 11:26 a.m.
ReviewRequest should be aligned with reviewboard.	brennie	Oct. 19, 2017, 8:08 p.m.
This should be aligned like: review_request (reviewboard.reviews.models.review_request. ReviewRequest): The review request to update the visibility of.	david	Oct. 19, 2017, 8:10 p.m.
How about "The new visibility".	brennie	Oct. 19, 2017, 8:09 p.m.
This isn't a .es6.js file, so there shouldn't be a comma after the last item in the object.	david	Oct. 19, 2017, 8:13 p.m.
Can you put the last item on the next line to keep in wrapping in 80 columns?	david	Oct. 21, 2017, 12:46 a.m.
Can you change to "Unarchive a review request"	chipx86	Jan. 13, 2018, 12:03 a.m.
The "Returns the visit" needs to be removed from the summary. Description also needs to be fleshed out to describe …	chipx86	Jan. 13, 2018, 12:05 a.m.
Should ideally list the visibility constants.	chipx86	Jan. 13, 2018, 12:08 a.m.
Can you swap these so they're in alphabetical order?	chipx86	Jan. 13, 2018, 12:08 a.m.
Here, too.	chipx86	Jan. 13, 2018, 12:08 a.m.
And here.	chipx86	Jan. 13, 2018, 12:08 a.m.
Indentation is kinda wonky. Can you change this to: attrs: [ ... ],	chipx86	Jan. 13, 2018, 12:09 a.m.
4.0?	chipx86	Jan. 13, 2018, 12:10 a.m.
Looks like this is the wrong way now.	chipx86	Jan. 15, 2018, 8:38 p.m.

flake8 failed.

JSHint failed.

flake8

reviewboard/accounts/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (84 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (86 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (100 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (106 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (90 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (100 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (106 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (103 > 79 characters)
```
reviewboard/webapi/resources/base_archived_object.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (91 > 79 characters)
```
reviewboard/webapi/resources/base_review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/webapi/resources/base_review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (97 > 79 characters)
```
reviewboard/webapi/resources/base_review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (89 > 79 characters)
```

JSHint

reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 34
 Expected ')' to match '(' from line 114 and instead saw ':'.
```
reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 35
 Expected ')' and instead saw 'true'.
```
reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 39
 Missing semicolon.
```
reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 13
 Expected an identifier and instead saw ')'.
```
reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 13
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 14
 Missing semicolon.
```
reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 14
 Expected an identifier and instead saw ')'.
```
reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 14
 Expected an assignment or function call and instead saw an expression.
```

This looks great. You can probably remove the WIP tag :)

reviewboard/accounts/managers.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No space between . and "
```

reviewboard/accounts/managers.py (Diff revision 1)

The issue has been resolved. Show all issues

Docstrings should be in the imperitive mood (i.e., they should read as instructions). e.g. "update" instead of "updates".

No space around the docstring.

Should be formatted as:

"""Single line summary.

Multi line description (if applicable).

Args:
    review_request (type of review_request):
        ...
"""

If this returned something it would also need:

"""
Returns:
    return_type:
    Description of returned value.
"""

giuliacm Sept. 29, 2017, 9:42 p.m.

Would I need to account for Args: user and new_visibility in this case?

brennie Oct. 1, 2017, 2:15 p.m.

Yes, you',ll have to document all of them.

reviewboard/accounts/tests.py (Diff revision 1)

The issue has been resolved. Show all issues

"creates"

For our testing docstrings, we wrap them at 79 but we
don't really follow docstring convention, e.g. it is ok to do

"""Testing some_some_very_long_method
does some thing
"""

reviewboard/accounts/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Not necessary to pull this out.
```

reviewboard/accounts/tests.py (Diff revision 1)

The issue has been resolved. Show all issues

We could also make this method return the visit object since we're using that in the tests. It would make them shorter and involve fewer queries.

giuliacm Sept. 29, 2017, 9:42 p.m.

Just to clarify, do you mean the method test_update_visibility_create(), or ReviewRequestVisitManager.update_visibility() ?

brennie Oct. 1, 2017, 2:15 p.m.

I mean update_visibility can return the visit to use it here. Commented in the wrong place :)

giuliacm Oct. 8, 2017, 4:03 p.m.

Okay, so adding "return visit" to update_visibility() wouldn't affect any other parts of my code, but would shorten the testing?

brennie Oct. 12, 2017, 3:05 p.m.
```
Yes.
```

reviewboard/accounts/tests.py (Diff revision 1)

The issue has been resolved. Show all issues

You can do user=user here and it will look it up by primary key.

Same below.

giuliacm Sept. 29, 2017, 8:36 p.m.

Would it look something like this?

visit = ReviewRequestVisit.objects.get(user=user)

brennie Oct. 1, 2017, 2:15 p.m.

Exactly that, except you'll want to do review_request=review_request here as well.

giuliacm Oct. 8, 2017, 4:03 p.m.

Ok, and not review_request=review_request.id (how it was before)?

brennie Oct. 12, 2017, 3:08 p.m.

Yeah. Technically that will work but its not super idiomatic. Looking up an object by its id would be review_request_id=review_request.id. Doing review_request=review_request does that basically behind the scenes (looks it up by ID).

reviewboard/accounts/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"updates"
```
reviewboard/accounts/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Not necessary to pull this out.
```
reviewboard/accounts/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/accounts/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"updates"
```
reviewboard/accounts/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Not necessary to pull this out.
```
reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing comma
```
reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing comma
```
reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing comma
```
reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js (Diff revision 1)
The issue has been resolved. Show all issues
```
The inner () should be {}.
Space after :.
```
reviewboard/webapi/resources/base_review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you alphabetically sort this with the other imports?
```
reviewboard/webapi/resources/base_review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line between these.
```

Commit:

-	0d3d440a6186d1c24a207a3bc894654f3c8bb51b
+	be07e3c00c4e71080e8738353cf4abd7968fe2df

Diff:

Revision 2 (+120 -21)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/tests.py
	reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
	reviewboard/webapi/resources/base_archived_object.py
	reviewboard/webapi/resources/base_review.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/accounts/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (84 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (86 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (100 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (106 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (90 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (100 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (106 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (103 > 79 characters)
```
reviewboard/webapi/resources/base_archived_object.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (91 > 79 characters)
```
reviewboard/webapi/resources/base_review.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/webapi/resources/base_review.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (97 > 79 characters)
```
reviewboard/webapi/resources/base_review.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (89 > 79 characters)
```

Commit:

-	be07e3c00c4e71080e8738353cf4abd7968fe2df
+	f4adfb8c6ab4cc1e071ca4368576e3132931af77

Diff:

Revision 3 (+125 -21)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/tests.py
	reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
	reviewboard/webapi/resources/base_archived_object.py
	reviewboard/webapi/resources/base_review.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/accounts/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E501 line too long (103 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E501 line too long (122 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E501 line too long (106 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E501 line too long (122 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E501 line too long (103 > 79 characters)
```
reviewboard/webapi/resources/base_archived_object.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E501 line too long (91 > 79 characters)
```
reviewboard/webapi/resources/base_review.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/webapi/resources/base_review.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E501 line too long (97 > 79 characters)
```
reviewboard/webapi/resources/base_review.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E501 line too long (89 > 79 characters)
```

Commit:

-	f4adfb8c6ab4cc1e071ca4368576e3132931af77
+	4cb23a759f57f84f8f51e96ebc2d08949a400241

Diff:

Revision 4 (+135 -21)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/tests.py
	reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
	reviewboard/webapi/resources/base_archived_object.py
	reviewboard/webapi/resources/base_review.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The substance of the change looks good. Just a few formatting/documentation comments.

reviewboard/accounts/managers.py (Diff revision 4)

The issue has been resolved. Show all issues

It was like this before, but can you fix up this docstring? There should be no space between the """ and the first line, the summary line should be in the imperative mood instead of passive ("Unarchive" vs. "Unarchives"), and we should document the review_request parameter.

giuliacm Oct. 8, 2017, 4:06 p.m.

I'm unsure what type would go in the () for the review_request parameter.

david Oct. 8, 2017, 8:56 p.m.

reviewboard.reviews.models.review_request.ReviewRequest

This can be wrapped after any period to stay within the 80-column limit.

reviewboard/accounts/managers.py (Diff revision 4)

The issue has been resolved. Show all issues

Same comments on this docstring--space, imperative mood, and "Args" documentation.

giuliacm Oct. 8, 2017, 4:06 p.m.

Also unsure what type would go in the () for review_request, user and new_visibility.

david Oct. 8, 2017, 8:56 p.m.

user is django.contrib.auth.models.User
new_visibility is unicode (since it's one of the constants defined in the ReviewRequestVisit mode).

reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js (Diff revision 4)
The issue has been resolved. Show all issues
```
We should include a trailing comma on this line too.
```
reviewboard/static/rb/js/views/draftReviewBannerView.es6.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Blank line between these two.
```

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

The issue has been resolved. Show all issues

Add a trailing comma here.

giuliacm Oct. 8, 2017, 4:06 p.m.

I made the change but there is also no trailing comma in the alternative for publishToSubmitterOnly, which was already existing code. Is there a rule for when to use trailing commas?

david Oct. 8, 2017, 8:56 p.m.

We should always have them in files that end in .es6.js. For ones that are just .js, it shouldn't have it on the last item in a list because some versions of IE choke. Probably what happened was we missed the existing one when converting this file to ES6.

giuliacm Oct. 9, 2017, 10:30 a.m.
```
Thanks, that makes sense!
```

Commit:

-	4cb23a759f57f84f8f51e96ebc2d08949a400241
+	4926b6b109621177e80b9e2c976c44308dd692ee

Diff:

Revision 5 (+152 -22)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/tests.py
	reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
	reviewboard/webapi/resources/base_archived_object.py
	reviewboard/webapi/resources/base_review.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/accounts/managers.py (Diff revision 5)

The issue has been resolved. Show all issues

This should be reviewboard.reviews.models.review_request.ReviewRequest. The reviews.models.ReviewRequest is an alias but won't necessarily link to the right place. You can wrap this after any period to stay within the 80-column limit.

reviewboard/accounts/managers.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/accounts/managers.py (Diff revision 5)
The issue has been resolved. Show all issues
```
See my reply to previous reviews.
```

reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js (Diff revision 5)

The issue has been resolved. Show all issues

This line in too long. Can you do it as:

expect(view._onPublishClicked).toHaveBeenCalledWith({
    publishAndArchive: true,
});

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

The issue has been resolved. Show all issues

This is getting big enough that we should probably put each parameter on its own line.

giuliacm Oct. 9, 2017, 10:29 a.m.

Do you want me to separate the first line of parameters to be on their own lines (self, requst, review, public=None) ?

david Oct. 9, 2017, 10:45 a.m.

Yeah, like so:

def update_review(self,
                  request,
                  review,
                  public=None,
                  ...

giuliacm Oct. 9, 2017, 11:23 a.m.

Ok thanks, is that just convention when parameter list is getting too long vs. what fits in each line?

david Oct. 14, 2017, 1:56 p.m.

Yeah. Up to about 3 lines, it's still pretty readable wrapping with multiple parameters per line. Once it goes above that, it's usually best to split each one out.

Commit:

-	4926b6b109621177e80b9e2c976c44308dd692ee
+	3bfc5bbc04393724ea0ca0f8d024d30a9c5c6da4

Diff:

Revision 6 (+156 -22)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/tests.py
	reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
	reviewboard/webapi/resources/base_archived_object.py
	reviewboard/webapi/resources/base_review.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

-	3bfc5bbc04393724ea0ca0f8d024d30a9c5c6da4
+	56a5c5fe778c71c9a1cb05ade7485db68c388c99

Diff:

Revision 7 (+163 -24)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/tests.py
	reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
	reviewboard/webapi/resources/base_archived_object.py
	reviewboard/webapi/resources/base_review.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/base_review.py (Diff revision 7)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```

Commit:

-	56a5c5fe778c71c9a1cb05ade7485db68c388c99
+	61f8a0da8805fc27522501586dfc89771c334757

Diff:

Revision 8 (+163 -24)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/tests.py
	reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
	reviewboard/webapi/resources/base_archived_object.py
	reviewboard/webapi/resources/base_review.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

-	61f8a0da8805fc27522501586dfc89771c334757
+	9c74b098bfd0e9ce41a6e39c40512119a5fe0eb5

Diff:

Revision 9 (+160 -24)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/tests.py
	reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
	reviewboard/webapi/resources/base_archived_object.py
	reviewboard/webapi/resources/base_review.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/accounts/managers.py (Diff revision 9)

The issue has been resolved. Show all issues

This should be aligned like:

review_request (reviewboard.reviews.models.review_request.
                ReviewRequest):
    The review request to update the visibility of.

reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js (Diff revision 9)
The issue has been resolved. Show all issues
```
This isn't a .es6.js file, so there shouldn't be a comma after the last item in the object.
```

Pending these nits & David's, I'm happy with this change!

reviewboard/accounts/managers.py (Diff revision 9)
The issue has been resolved. Show all issues
```
ReviewRequest should be aligned with reviewboard.
```
reviewboard/accounts/managers.py (Diff revision 9)
The issue has been resolved. Show all issues
```
How about "The new visibility".
```

Commit:

-	9c74b098bfd0e9ce41a6e39c40512119a5fe0eb5
+	291bd64635b6ba8e1db3c87b63ef6af0b629b46b

Diff:

Revision 10 (+160 -24)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/tests.py
	reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
	reviewboard/webapi/resources/base_archived_object.py
	reviewboard/webapi/resources/base_review.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
Looking great!
```

The issue has been resolved. Show all issues

I think you can remove "WIP" from your summary now.

reviewboard/static/rb/js/views/reviewDialogView.es6.js (Diff revision 10)
The issue has been resolved. Show all issues
```
Can you put the last item on the next line to keep in wrapping in 80 columns?
```

Summary:

-	[WIP] Add "Publish and Archive" to the review dialog.
+	Add "Publish and Archive" to the review dialog.

Commit:

-	291bd64635b6ba8e1db3c87b63ef6af0b629b46b
+	653546b0c5b9db0e3575964e3c017510fc7621a9

Diff:

Revision 11 (+161 -24)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/tests.py
	reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
	reviewboard/webapi/resources/base_archived_object.py
	reviewboard/webapi/resources/base_review.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Change Summary:

Reassign ownership to me.

Owner:

-	giuliacm
+	david

Change Summary:

Rebase to merge with latest changes, fix up text in UI to match other options in the drop-down.

Branch:

-	release-3.0.x
+	release-4.0.x

Commit:

-	653546b0c5b9db0e3575964e3c017510fc7621a9
+	4d8eb64060db2506cfc2209103a1433e99a24e85

Diff:

Revision 12 (+165 -24)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/tests.py
	reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
	reviewboard/webapi/resources/base_archived_object.py
	reviewboard/webapi/resources/base_review.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/accounts/managers.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Can you change to "Unarchive a review request"
```

reviewboard/accounts/managers.py (Diff revision 12)

The issue has been resolved. Show all issues

The "Returns the visit" needs to be removed from the summary.

Description also needs to be fleshed out to describe what updating the visibility means.

reviewboard/accounts/managers.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Should ideally list the visibility constants.
```
reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js (Diff revision 12)
The issue has been resolved. Show all issues
```
Can you swap these so they're in alphabetical order?
```
reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js (Diff revision 12)
The issue has been resolved. Show all issues
```
Here, too.
```
reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js (Diff revision 12)
The issue has been resolved. Show all issues
```
And here.
```

reviewboard/static/rb/js/views/reviewDialogView.es6.js (Diff revision 12)

The issue has been resolved. Show all issues

Indentation is kinda wonky. Can you change this to:

attrs: [
    ...
],

reviewboard/webapi/resources/base_review.py (Diff revision 12)
The issue has been resolved. Show all issues
```
4.0?
```

Commit:

-	4d8eb64060db2506cfc2209103a1433e99a24e85
+	92757653fd45004dce9245be1c56ab1bcd37ee68

Diff:

Revision 13 (+179 -25)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/tests.py
	reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
	reviewboard/webapi/resources/base_archived_object.py
	reviewboard/webapi/resources/base_review.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js (Diff revision 13)
The issue has been resolved. Show all issues
```
Looks like this is the wrong way now.
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (29aa306)