Multi user collaboration on review requests

Review Request #3242 — Created July 22, 2012 and discarded

Information

Review Board

Reviewers

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.
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.
Description From Last Updated

Two blank lines.

chipx86chipx86

Should be one line, like: """A column ..."""

chipx86chipx86

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

chipx86chipx86

Can be shortened to: return obj.last_updated_by or ""

chipx86chipx86

Two blank lines.

chipx86chipx86

Blank line isn't needed.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Same here.

chipx86chipx86

Keep the blank line.

chipx86chipx86

No spaces in the {{}} Should remove the space after the "." (and maybe remove the period, since we didn't …

chipx86chipx86

I'm a bit worried about the pk= for these User queries in the tests. Can you change them instead to …

chipx86chipx86

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

RO ronaldoit

This really can just be one evolution. We tend to do one per feature/change. .. And actually, I don't see …

chipx86chipx86

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

chipx86chipx86

Mind changing this to use parens instead of \ ?

chipx86chipx86

No need for \, since it's in parens.

chipx86chipx86

No need for \

chipx86chipx86

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

chipx86chipx86

Indentation looks wrong.

chipx86chipx86

Two blank lines.

chipx86chipx86

!==

chipx86chipx86

Keep the blank line. Two between things.

chipx86chipx86

Is it worth folding the permission into that variable?

chipx86chipx86
CR
mike_conley
  1. 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!
  2. 
      
CR
CR
  1. Would someone be able to kindly give me a brief code review please? :)
  2. 
      
chipx86
  1. 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.
  2. reviewboard/reviews/datagrids.py (Diff revision 2)
     
     
     
     
    Show all issues
    Two blank lines.
  3. reviewboard/reviews/datagrids.py (Diff revision 2)
     
     
     
     
    Show all issues
    Should be one line, like:
    
    """A column ..."""
  4. reviewboard/reviews/datagrids.py (Diff revision 2)
     
     
     
    Show all issues
    Can you use super() here, instead of old-style Column initialization?
  5. reviewboard/reviews/datagrids.py (Diff revision 2)
     
     
     
     
     
    Show all issues
    Can be shortened to:
    
    return obj.last_updated_by or ""
  6. reviewboard/reviews/datagrids.py (Diff revision 2)
     
     
     
     
    Show all issues
    Two blank lines.
  7. reviewboard/reviews/forms.py (Diff revision 2)
     
     
     
     
    Show all issues
    Blank line isn't needed.
  8. 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.
  9. 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.
  10. reviewboard/reviews/models.py (Diff revision 2)
     
     
    Show all issues
    Same here.
  11. reviewboard/reviews/views.py (Diff revision 2)
     
     
     
     
    Show all issues
    Keep the blank line.
  12. 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)
  13. 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.)
  14. 
      
CR
CR
RO
  1. Ship It!
  2. 
      
RO
  1. 
      
  2. reviewboard/reviews/datagrids.py (Diff revision 4)
     
     
    Show all issues
    CODE T?M B?Y R?I CHA N?I
    1. 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!
  3. 
      
chipx86
  1. 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?
    1. 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.
  2. 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?
    1. I forgot to add the new file "add_reviewrequest_owner". I merged those two files into one and included it.
  3. Show all issues
    Can you add a trailing comma? One less modified line next time something is added.
  4. reviewboard/reviews/models.py (Diff revision 4)
     
     
     
     
    Show all issues
    Mind changing this to use parens instead of \ ?
  5. reviewboard/reviews/models.py (Diff revision 4)
     
     
    Show all issues
    No need for \, since it's in parens.
  6. reviewboard/reviews/models.py (Diff revision 4)
     
     
     
     
    We do this in a couple places. Might be time for a Manager function.
  7. reviewboard/reviews/models.py (Diff revision 4)
     
     
    Show all issues
    No need for \
  8. 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.)
  9. reviewboard/static/rb/js/reviews.js (Diff revision 4)
     
     
     
     
    Show all issues
    Indentation looks wrong.
  10. reviewboard/static/rb/js/reviews.js (Diff revision 4)
     
     
     
     
    Show all issues
    Two blank lines.
  11. reviewboard/static/rb/js/reviews.js (Diff revision 4)
     
     
    Show all issues
    !==
  12. reviewboard/static/rb/js/reviews.js (Diff revision 4)
     
     
    Show all issues
    Keep the blank line. Two between things.
  13. Show all issues
    Is it worth folding the permission into that variable?
    1. 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".
  14. 
      
CR
CR
CR
Review request changed
Status:
Discarded