Summary

Multi user collaboration on review requests

Review Request #3242 — Created July 22, 2012 and discarded April 23, 2014, 3:55 p.m.

Information

Owner

CrystalLokKoo

Repository

Review Board

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This is what I have so far for the multi user collaboration on review requests. So far, what I have done is allow all reviewers (users in the target people and groups) to be allowed to change the review request. This would all the reviewers to all the normal changing review request functionalities. 

I'm not sure if we have decided who can be able to modify the review request. For now, I put them as all the reviewers.

Also, I've included the last_updated_by field to the review request model in order to display who has last modified the review request or published new review on the dashboard.

Testing Done

Manual testing. At first, I've ran the unit tests and had a bunch of errors and one failure. The errors are all expected due to the change of the ReviewRequestDraft.create() function and I need to look into that one failure. I think it's expected failure though.

As of diff 2, all the test failures/errors have been fixed.

Manual testing include: 
- Creating new review requests and changing submitter to another account. Permissions to edit the review request behaved correctly for new user
- The original author correctly still has permission.
- Changing the submitter back to the original submitter. Previous submitter has no permission to edit.
- Check Admin. Admin is always able to edit any review request.
- Discarding and undiscarding a review request. 
- Dealing with previous review request that existed before my change. The submitter field is populated correctly. Edit permissions acting properly before changing submitter. Changed submitter and still acting correctly.

Issues

Description	From	Last Updated
Two blank lines.	chipx86	Aug. 30, 2012, 11:48 p.m.
Should be one line, like: """A column ..."""	chipx86	Aug. 30, 2012, 11:48 p.m.
Can you use super() here, instead of old-style Column initialization?	chipx86	Aug. 30, 2012, 11:48 p.m.
Can be shortened to: return obj.last_updated_by or ""	chipx86	Aug. 30, 2012, 11:48 p.m.
Two blank lines.	chipx86	Aug. 30, 2012, 11:48 p.m.
Blank line isn't needed.	chipx86	Aug. 30, 2012, 11:48 p.m.
I think we need to figure out what the rules should be for modifying a review request. I don't think …	chipx86	Oct. 7, 2012, 12:29 p.m.
This will fail for any drafts that exist at the time of deployment of this change. We should do an …	chipx86	Aug. 30, 2012, 11:48 p.m.
Same here.	chipx86	Aug. 30, 2012, 11:48 p.m.
Keep the blank line.	chipx86	Aug. 30, 2012, 11:48 p.m.
No spaces in the {{}} Should remove the space after the "." (and maybe remove the period, since we didn't …	chipx86	Aug. 30, 2012, 11:48 p.m.
I'm a bit worried about the pk= for these User queries in the tests. Can you change them instead to …	chipx86	Aug. 30, 2012, 11:48 p.m.
CODE T?M B?Y R?I CHA N?I	RO ronaldoit	Oct. 28, 2012, 5:18 p.m.
This really can just be one evolution. We tend to do one per feature/change. .. And actually, I don't see …	chipx86	Oct. 28, 2012, 5:12 p.m.
Can you add a trailing comma? One less modified line next time something is added.	chipx86	Oct. 28, 2012, 5:12 p.m.
Mind changing this to use parens instead of \ ?	chipx86	Oct. 28, 2012, 5:12 p.m.
No need for \, since it's in parens.	chipx86	Oct. 28, 2012, 5:13 p.m.
No need for \	chipx86	Oct. 28, 2012, 5:16 p.m.
While you're here, mind changing them to === ? (We shouldn't be using ==, but I was young and naïve …	chipx86	Oct. 28, 2012, 5:16 p.m.
Indentation looks wrong.	chipx86	Oct. 28, 2012, 5:17 p.m.
Two blank lines.	chipx86	Oct. 28, 2012, 5:17 p.m.
!==	chipx86	Oct. 28, 2012, 5:18 p.m.
Keep the blank line. Two between things.	chipx86	Oct. 28, 2012, 5:18 p.m.
Is it worth folding the permission into that variable?	chipx86	Oct. 28, 2012, 5:25 p.m.

Change Summary:

Fixed the unit tests errors and failures.

Diff:

Revision 2 (+108 -47)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/tests.py
	reviewboard/changedescs/models.py
	reviewboard/changedescs/evolutions/__init__.py
	reviewboard/changedescs/evolutions/change_desc_user.py
	reviewboard/reviews/datagrids.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/models.py
	12 more

I haven't run this patch yet, but this looks like the right idea to me. You're on the right track. Let me know when you're done iterating, and I'll give it a full review.

Thanks!

Change Summary:

Updated "Testing Done" details.

Summary:

-	Multi user collaboration on review requests [DRAFT]
+	Multi user collaboration on review requests

Testing Done:

~		Manual testing. I've ran the unit tests and had a bunch of errors and one failure. The errors are all expected due to the change of the ReviewRequestDraft.create() function and I need to look into that one failure. I think it's expected failure though.
	~	Manual testing. At first, I've ran the unit tests and had a bunch of errors and one failure. The errors are all expected due to the change of the ReviewRequestDraft.create() function and I need to look into that one failure. I think it's expected failure though.

~		I'll go through them all and make modifications/fixes as necessary.
	~	As of diff 2, all the test failures/errors have been fixed.

Would someone be able to kindly give me a brief code review please? :)

Hey. This is looking great. As I mention below, we need to figure out what the rules should be for who can modify another user's review requests, because I don't think target reviewers is quite correct. Let's discuss it.

reviewboard/reviews/datagrids.py (Diff revision 2)
Show all issues
```
Two blank lines.
```
reviewboard/reviews/datagrids.py (Diff revision 2)
Show all issues
```
Should be one line, like:

"""A column ..."""
```

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

Show all issues

Can you use super() here, instead of old-style Column initialization?

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

Show all issues

Can be shortened to:

return obj.last_updated_by or ""

reviewboard/reviews/datagrids.py (Diff revision 2)
Show all issues
```
Two blank lines.
```
reviewboard/reviews/forms.py (Diff revision 2)
Show all issues
```
Blank line isn't needed.
```

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

Show all issues

I think we need to figure out what the rules should be for modifying a review request.

I don't think necessarily that every user listed should have the ability (and this query is expensive anyway).

My feeling is that the ability to modify someone else's review request should be pretty limited, and may even be something we want to be opt-in in an organization (or part of an organization).

Perhaps we should have a discussion in IRC about what's most correct here.

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

Show all issues

This will fail for any drafts that exist at the time of deployment of this change. We should do an || on these.

reviewboard/reviews/models.py (Diff revision 2)
Show all issues
```
Same here.
```
reviewboard/reviews/views.py (Diff revision 2)
Show all issues
```
Keep the blank line.
```

reviewboard/templates/reviews/review_detail.html (Diff revision 2)

Show all issues

No spaces in the {{}}

Should remove the space after the "." (and maybe remove the period, since we didn't have it before)

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

Show all issues

I'm a bit worried about the pk= for these User queries in the tests. Can you change them instead to look up by username? (This and all others.)

Change Summary:

Modified is_mutable_by method so that it doesn't query for reviewers' groups or people anymore. Basically reverted back to where it was before. When we decide on what type of users can modify the review request, we can modify that method again to accommodate the multi user collaboration. 

I still really like the idea of having the idea where the submitter can add a list of other users as owners of the ReviewRequest. I think that is simple and intuitive but may take a bit of work to redo the UI on the ReviewRequest. I love to hear suggestions or feedback! :)

Diff:

Revision 3 (+126 -45)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/tests.py
	reviewboard/changedescs/models.py
	reviewboard/changedescs/evolutions/__init__.py
	reviewboard/changedescs/evolutions/change_desc_user.py
	reviewboard/reviews/datagrids.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/models.py
	12 more

Change Summary:

Allowed to change the owner of a ReviewRequest. The field 'Submitter' into the UI is changed to 'Owner'. Refactored some of the JS to use the jQuery autocompleter for the owner field as well. Added the ability of the owner to be able to modify a ReviewRequest. 

The submitter is now shown right underneath the summary and beside the created time.

Diff:

Revision 4 (+236 -64)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/tests.py
	reviewboard/changedescs/models.py
	reviewboard/changedescs/evolutions/__init__.py
	reviewboard/changedescs/evolutions/change_desc_user.py
	reviewboard/reviews/datagrids.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/models.py
	14 more

Ship it!

```
Ship It!
```

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

Show all issues

CODE T?M B?Y R?I CHA N?I

mike_conley Oct. 28, 2012, 11:36 a.m.

Hey - this is a production instance of Review Board.  Please don't use http://reviews.reviewboard.org for testing purposes. Use http://demo.reviewboard.org instead. Thanks!

Spotted a couple things, but it's very close, and I'm hopeful we can get this into 1.7 if you feel good about it.

Remind me, is it just admins that can create drafts, or can users with the can_edit permission also do it?

CrystalLokKoo Oct. 28, 2012, 5:44 p.m.

Admins, owners and submitters can edit the review request. It's based on the is_mutable_by function in the ReviewRequest class.

Please let me know if this is not the desired behaviour.

reviewboard/reviews/evolutions/__init__.py (Diff revision 4)

Show all issues

This really can just be one evolution. We tend to do one per feature/change.

.. And actually, I don't see the other file referenced?

CrystalLokKoo Oct. 28, 2012, 5:44 p.m.

I forgot to add the new file "add_reviewrequest_owner". I merged those two files into one and included it.

reviewboard/reviews/evolutions/__init__.py (Diff revision 4)

Show all issues

Can you add a trailing comma? One less modified line next time something is added.

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

Show all issues

Mind changing this to use parens instead of \ ?

reviewboard/reviews/models.py (Diff revision 4)
Show all issues
```
No need for \, since it's in parens.
```

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

We do this in a couple places. Might be time for a Manager function.

reviewboard/reviews/models.py (Diff revision 4)
Show all issues
```
No need for \
```

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

Show all issues

While you're here, mind changing them to === ? (We shouldn't be using ==, but I was young and naïve once.)

reviewboard/static/rb/js/reviews.js (Diff revision 4)
Show all issues
```
Indentation looks wrong.
```
reviewboard/static/rb/js/reviews.js (Diff revision 4)
Show all issues
```
Two blank lines.
```
reviewboard/static/rb/js/reviews.js (Diff revision 4)
Show all issues
```
!==
```
reviewboard/static/rb/js/reviews.js (Diff revision 4)
Show all issues
```
Keep the blank line. Two between things.
```

reviewboard/templates/reviews/review_flags.js (Diff revision 4)

Show all issues

Is it worth folding the permission into that variable?

CrystalLokKoo Oct. 28, 2012, 5:44 p.m.

Oops - Apparently perms.reviews.can_edit_reviewrequest is already included in can_edit_review_request. Therefore, I removed "perms.reviews.can_edit_reviewrequest" in if statements that have "can_edit_review_request".

Change Summary:

Fixed all the minor issues that were bought up during the last code review. Added a missing evolution file. More manual testing done.

Diff:

Revision 5 (+260 -68)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/tests.py
	reviewboard/changedescs/models.py
	reviewboard/changedescs/evolutions/__init__.py
	reviewboard/changedescs/evolutions/change_desc_user.py
	reviewboard/reviews/datagrids.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/managers.py
	15 more

Change Summary:

Updated testing section.

Testing Done:

		Manual testing. At first, I've ran the unit tests and had a bunch of errors and one failure. The errors are all expected due to the change of the ReviewRequestDraft.create() function and I need to look into that one failure. I think it's expected failure though.

		As of diff 2, all the test failures/errors have been fixed.
	+
	+	Manual testing include:
	+	- Creating new review requests and changing submitter to another account. Permissions to edit the review request behaved correctly for new user
	+	- The original author correctly still has permission.
	+	- Changing the submitter back to the original submitter. Previous submitter has no permission to edit.
	+	- Check Admin. Admin is always able to edit any review request.
	+	- Discarding and undiscarding a review request.
	+	- Dealing with previous review request that existed before my change. The submitter field is populated correctly. Edit permissions acting properly before changing submitter. Changed submitter and still acting correctly.

Review Board 6.0.3 alpha 0 (dev)

Multi user collaboration on review requests

Information

Reviewers

Change Summary:

Diff:

Change Summary:

Summary:

Testing Done:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Testing Done:

Status: Discarded