Allow reviewers to change the reviewers list

[jiayao, 2008-05-10 01:00:40]

I've simply allowed non-owner to change the reviewers list. Not sure if this is
desirable. But currently non-owners can already make comments to any issue, and
I think it is OK for anyone to register interests in an issue by adding herself
to the reviewer list.

[GvR, 2008-05-10 15:44:23]

I've thought about this and I agree that this is the way to go. There's really
no need to have separate Reviewers and CC lists, even though Mondrian does it
that way.

I have two nits and one suggestion for an enhancement.

http://codereview.appspot.com/776/diff/1/3
File rietveld/views.py (right):

http://codereview.appspot.com/776/diff/1/3#newcode949
Line 949: 'reviewers': ', '.join(issue.reviewers),
I would suggest one more addition: add the current user's email to the reviewers
if it's not already there (and this is not the issue owner).

http://codereview.appspot.com/776/diff/1/3#newcode965
Line 965: issue.reviewers = reviewers
I'd move this one line downfor symmetry with the 'if' clause (set subject first,
set issue attributes second).

http://codereview.appspot.com/776/diff/1/3#newcode1250
Line 1250: 
Don't add an extra blank line at the end.

[jiayao, 2008-05-11 11:33:26]

I've made your suggested changes. And made a further change in _get_reviewers to
remove duplicates in the reviewers list.

http://codereview.appspot.com/776/diff/1/3
File rietveld/views.py (right):

http://codereview.appspot.com/776/diff/1/3#newcode949
Line 949: 'reviewers': ', '.join(issue.reviewers),
Good suggestion, I've added that. Just another thought: should we use nickname
when displaying and allow users enter either email or nickname? That would make
the list shorter in general.

http://codereview.appspot.com/776/diff/1/3#newcode965
Line 965: issue.reviewers = reviewers
On 2008/05/10 15:44:24, GvR wrote:
> I'd move this one line downfor symmetry with the 'if' clause (set subject
first,
> set issue attributes second).

Done.

http://codereview.appspot.com/776/diff/1/3#newcode1250
Line 1250: 
On 2008/05/10 15:44:24, GvR wrote:
> Don't add an extra blank line at the end.

Done.

[GvR, 2008-05-11 19:20:48]

http://codereview.appspot.com/776/diff/101/122
File rietveld/views.py (right):

http://codereview.appspot.com/776/diff/101/122#newcode30
Line 30: import sets
This is Python 2.5, it has a built-in set type. (Even 2.4 does :-)

http://codereview.appspot.com/776/diff/101/122#newcode591
Line 591: return list(reviewers)
I'd prefer to keep reviewers in the order that they were entered. The list is
never long enough to worry about quadratic behavior.

http://codereview.appspot.com/776/diff/101/122#newcode950
Line 950: in issue.reviewers):
You can use 'not in' for the latter test.

http://codereview.appspot.com/776/diff/101/122#newcode951
Line 951: issue.reviewers.append(request.user.email())
I wouldn't assign to issue.reviewers here, since you're not actually going to
write it back.  Use a local variable that's a copy of issue.reviewers, and pass
that to the form_class() call below.

[jiayao, 2008-05-11 19:56:30]

Done. :)

[GvR, 2008-05-11 23:27:40]

Very close!

http://codereview.appspot.com/776/diff/125/43
File rietveld/views.py (right):

http://codereview.appspot.com/776/diff/125/43#newcode189
Line 189: send_mail = forms.BooleanField()
The order of the fields should be the same as in PublishForm -- i.e. send_mail
should come *after* reviewers.

http://codereview.appspot.com/776/diff/125/43#newcode1013
Line 1013: everyone = addressees + [my_email]
What would you think of *not* adding my_email in this branch? That way you could
opt out from receiving mail if you explicitly remove yourself from the
reviewers.

[jiayao, 2008-05-12 09:36:34]

http://codereview.appspot.com/776/diff/125/43
File rietveld/views.py (right):

http://codereview.appspot.com/776/diff/125/43#newcode1013
Line 1013: everyone = addressees + [my_email]
Make sense. It would be confusing if this person receives an email this time but
not the subsequent conversations.

[GvR, 2008-05-12 13:57:02]

Looks good!

You can check it in yourself, I've added you to the project.

[GvR, 2008-05-12 17:50:19]

JiaYao, this is now live!