Summary

Fix Default reviewers not added to review request for committed change

Review Request #7691 — Created Oct. 10, 2015 and submitted Oct. 13, 2015, 2:17 p.m.

Information

Owner

xutong

Repository

Review Board

Branch

release-2.0.x

Bugs

3900

Depends On

Reviewers

Groups

reviewboard

People

Description

Create a review request for a committed change via the "New Review Request" page. Default reviewer is not shown. The season of this bug is because the code of adding default reviewer is only called when posting change via RBtools.
To solve this, add default reviewers when creating a review request from a committed change

Testing Done


Create a review request from "New Review Request" web UI, the default reviewer was shown. 
Run all the unit tests, and pass all the tests.

Issues

Description	From	Last Updated
Col: 1 W293 blank line contains whitespace	reviewbot	Oct. 10, 2015, 1:46 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Oct. 10, 2015, 1:46 p.m.
Col: 77 W291 trailing whitespace	reviewbot	Oct. 10, 2015, 1:46 p.m.
Undo this change.	brennie	Oct. 11, 2015, 9:23 a.m.
Undo this change.	brennie	Oct. 11, 2015, 9:23 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Oct. 11, 2015, 9:27 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Oct. 11, 2015, 9:29 a.m.

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



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

reviewboard/reviews/managers.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/reviews/managers.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/reviews/managers.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 77
 W291 trailing whitespace
```

Diff:

Revision 2 (+4 -1)

Show changes

reviewboard/reviews/managers.py

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



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

Hey Xutong! Thanks for the patch. Before this can land, we'll probably want a more comprehensive Summary, Description and Testing Done.

Please see: https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/

Summary:

-	add function review_request.add_default_reviewers()
+	Fix Default reviewers not added to review request for committed change

Your summary is much improved but you still need more extensive description and testing done fields. Please also add the bug number into the bugs field.

Description:

~		add function review_request.add_default_reviewers(), fix bug 3900
	~	Fix the problem in reviewboard/reviews/managers.py
	+	When a review request has been created and save. Default reviewer did not been added to the review request. Therefore, default groups and people did not show. Problem solved after adding add_default_reviewers() after review_request.save(). Then, after review request been saved, defaul_reviewer can be appended.

Bugs:

+	3900

Testing Done:

	+	First, locating where the add_default_reviewers function is more important. Then, figuring out that the add_default_reviewers function has not been alled. So, finding where the add_default_reviewers is key to solve the bug.
	+	After located where the update_from_commit_id() function been called, I added the add_default_reviewers() blow, but it does not fix the bug. Becasue the request review does not been saved and default_reviewer can not been depended in database somewhere. Therefore, adding the add_default_reviewers() under review_request.save(), then the problem solved.

I have only two minor nitpicks for the code and a few general comments about your description and testing done.

Your summary doesn't need to mention what files were edited, that's what the diff is for :)
The description should be a high-level overview of what the issue was and how it was fixed.

Also, your testing done should describe how you tested the code works, e.g.

- Ran unit tests
- Created a post-commit review request and saw that the appropriate default reviewers were added.

You also have some grammar issues in your description and should probably give this a read: https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/

reviewboard/reviews/managers.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Undo this change.
```
reviewboard/reviews/managers.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Undo this change.
```

Diff:

Revision 3 (+4 -1)

Show changes

reviewboard/reviews/managers.py

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



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

reviewboard/reviews/managers.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

Diff:

Revision 4 (+4 -1)

Show changes

reviewboard/reviews/managers.py

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



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

reviewboard/reviews/managers.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

Diff:

Revision 5 (+3)

Show changes

reviewboard/reviews/managers.py

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



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

Description:

~		Fix the problem in reviewboard/reviews/managers.py
	~	A new review request is created without any default reviewer attached. Therefore, default groups and people did not show. Problem solved after adding add_default_reviewers() after review_request.save(). Then, after review request been saved, defaul_reviewer can be appended.
-		When a review request has been created and save. Default reviewer did not been added to the review request. Therefore, default groups and people did not show. Problem solved after adding add_default_reviewers() after review_request.save(). Then, after review request been saved, defaul_reviewer can be appended.

Description:

~		A new review request is created without any default reviewer attached. Therefore, default groups and people did not show. Problem solved after adding add_default_reviewers() after review_request.save(). Then, after review request been saved, defaul_reviewer can be appended.
	~	A new review request is created without any default reviewer attached. Therefore, default groups and people did not show. The problem is fixed by add default reviewer after a new review request created and saved.

Testing Done:

~		First, locating where the add_default_reviewers function is more important. Then, figuring out that the add_default_reviewers function has not been alled. So, finding where the add_default_reviewers is key to solve the bug.
	~	First, test by trying to reproduce the bug, click a New Review Request and then select already commited change, the default reviewers and default groups have been shown. Then, run all unit tests proved for review board to make sure all other functions works as well.
-		After located where the update_from_commit_id() function been called, I added the add_default_reviewers() blow, but it does not fix the bug. Becasue the request review does not been saved and default_reviewer can not been depended in database somewhere. Therefore, adding the add_default_reviewers() under review_request.save(), then the problem solved.

Description:

~		A new review request is created without any default reviewer attached. Therefore, default groups and people did not show. The problem is fixed by add default reviewer after a new review request created and saved.
	~	A new review request is created without any default reviewer attached. Therefore, default groups and people did not show. The problem is fixed by add default reviewer after a new review request created and saved. Then the default reviewers can be appended after a new review request has been saved.

Your description and testing done are somewhat difficult to follow. I'd suggest being a little less verbose in the description, and making testing done a set of bullet points. For example:

When creating a review request for a committed change via the "New Review
Request" page, default reviewers were not added to it. This was happening
because the code to add those reviewers was only called when posting changes
from RBTools.

To fix this, in the review request manager, we now explicitly add default
reviewers when creating a review request from a committed change.

Testing done:
- Created a new review request from the web UI and saw that default reviewers
  were correctly added.
- Ran unit tests.

Description:

~		A new review request is created without any default reviewer attached. Therefore, default groups and people did not show. The problem is fixed by add default reviewer after a new review request created and saved. Then the default reviewers can be appended after a new review request has been saved.
	~	Create a review request for a commited change via the "New Review Request" page. Default reviewer is not shown. The season of this bug is because the code of adding defaul reviewer is only called when posting change via RBtools.
	+	To solve this, add default reviewers when creating a review request from a committed change

Testing Done:

~		First, test by trying to reproduce the bug, click a New Review Request and then select already commited change, the default reviewers and default groups have been shown. Then, run all unit tests proved for review board to make sure all other functions works as well.
	~	Create a review request from "New Review Request" web UI, the default reviewer was shown.
	+	Run all the unit tests, and pass all the tests.

Description:

~		Create a review request for a commited change via the "New Review Request" page. Default reviewer is not shown. The season of this bug is because the code of adding defaul reviewer is only called when posting change via RBtools.
	~	Create a review request for a committed change via the "New Review Request" page. Default reviewer is not shown. The season of this bug is because the code of adding default reviewer is only called when posting change via RBtools.
		To solve this, add default reviewers when creating a review request from a committed change

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (34dbd74)

Ship it!

```
Ship It!
```