Changing Review Request Ownership

Review Request #6990 - Created Feb. 28, 2015 and submitted

Chenxi Ni
Review Board
master
7765
05c1791...
reviewboard, students

make the ownership changeable on the UI level

Main code describtions:

In builtin_fields.py we change the 'submitter' field to inherit BaseEditableField so that the 'submitter' field will be editable; The override function--record_change_entry disposes what will be showed in change description; Then override the function to render the only one value for a change description string to HTML instead of a list.

In changedescs/models.py we describe the 'new' and 'old' changes in 'submitter' field and render the value to HTML.

Changes in models/review_request_draft.py are to add 'owner' field to override 'submitter' field. Since 'submitter' field is a property attribute of draftreviewrequest, if we change the'submitter' field from property to attribute directly, 'submitter' of older data in the dababase will be null. This will cause troubles.

Changes in reviewboard/reviews/evolutions are about Writing Database Evolutions of adding 'owner' field to the draft table.

In webapi/resources/review_request_draft.py we get the change information of 'submitter' field and set draft.owner if the change is available.

In reviewRequestEditorView.js we make the field can be autocompleted then show in proper format when updated.

In models/reviewRequestEditorModel.js we detect the changes of 'submitter' field in the front end before sending message to the back end. And show the confirm notification only when 'submitter' field was modified.

In models/draftReviewRequestModel.js: parseResourceData we set the 'submitter' info to the data from response message so that we can use it to update the field.

Passed the http://localhost:8000/js-tests/ for the front end.
Passed tests for the back end.

Added tests:

./reviewboard/manage.py test -- reviewboard.webapi.tests.test_review_request_draft:ResourceTests.test_put_publish_with_new_submitter
This test is about wether the feature works to set the submitter field.

reviewRequestEditorModelTests:
Check to set empty value.
Check if set value successfully.
Check if error thrown when set invalid value.

reviewRequestEditorViewTests:
Check confirm notification works before publish.
Check field can be autocomplete.
Check field can edit.
Check save works.
Check value format after update.
Check edit count successfully.

draftReviewRequestModelTests:
Check parse works for 'submitter' field.

  • 0
  • 0
  • 55
  • 1
  • 56
Description From Last Updated
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. Col: 80
     E501 line too long (100 > 79 characters)
    
  3.  'model_cache' imported but unused
    
  4. Col: 80
     E501 line too long (98 > 79 characters)
    
  5. Col: 21
     E265 block comment should start with '# '
    
  6. Col: 47
     E127 continuation line over-indented for visual indent
    
  7. Col: 47
     E127 continuation line over-indented for visual indent
    
  8. Col: 13
     E265 block comment should start with '# '
    
  9. Col: 55
     E261 at least two spaces before inline comment
    
  10. Col: 56
     E262 inline comment should start with '# '
    
  11. 
      
Chester Li
  1. Could you please make the description more detailed? Because it is kind of hard to understand what you are doing in here.

  2. 
      
Chenxi Ni
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. Col: 9
     E128 continuation line under-indented for visual indent
    
  3. Col: 21
     E265 block comment should start with '# '
    
  4.  'UserResource' imported but unused
    
  5. Col: 13
     E265 block comment should start with '# '
    
  6. 
      
Chenxi Ni
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. reviewboard/changedescs/models.py (Diff revision 3)
     
     
    Col: 22
     E225 missing whitespace around operator
    
  3. Col: 9
     E128 continuation line under-indented for visual indent
    
  4. 
      
Chenxi Ni
Chenxi Ni
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
        reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
        reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
  2. Col: 36
     E128 continuation line under-indented for visual indent
    
  3. Col: 36
     E128 continuation line under-indented for visual indent
    
  4. 
      
Chester Li
  1. 
      
  2. I guess assocated is a typo

  3. how about a white space before else if?

  4. I think the indentation of each argument should be consistent. Please see line 195-197

  5. 
      
Vincent Le
  1. 
      
  2. reviewboard/changedescs/models.py (Diff revision 4)
     
     

    Remove trailing commas.

    1. Sorry, meant remove that trailing comma.

  3. We don't support ternary operators, you can do self.owner or self.review_request.submitter

  4. Remove blank lines.

  5. 
      
Barret Rennie
  1. 
      
  2. Use single quotes

  3. Use single quotes.

  4. Use single quotes for this. The quote marks should line up.

    You may want to assign confirm(gettext(...)) to a value and check against that for readability.

    Also change "the managing privileges" to "all managing privileges."

    1. I just answered this on Slack.

      One important note is that for calls to gettext, we actually can't concatenate to wrap. In fact, we can't wrap. Otherwise, the logic for localizing will fail to see the second part of the string. So for text wrapped in gettext, we need to just let the line be as long as it needs.

  5. How about makeLink or convertToLink ?

  6. I try to stay away from variables named data because technically all variables are data :)

  7. We do processing for other fields in this way because we can have multiple values in them. I don't think we should do this for the submitter field because we only expect one value.

    I think we should just take what we are given and try to look up exactly that.

  8. Blank line between these.

  9. 
      
Xuanyi Lin
  1. 
      
  2. Should this be 'submitter' instead?

  3. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 4)
     
     
     
     
     
     
     
     
     
     

    I guess we could do this also if we do want to explictly return '' if item is null.

    if (item) {
        str += '<a href="';
        str += (urlFunc ? urlFunc(item) : item);
        str += '">';
        str += (textFunc ? textFunc(item) : item);
        str += '</a>';
    }
    
    return str
    
  4. 
      
Chenxi Ni
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
        reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
        reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
  2. Col: 80
     E501 line too long (82 > 79 characters)
    
  3. 
      
Mike Conley
  1. Hey chenxi - this is looking really really good. Not too many more comments frome me. Great job!

  2. reviewboard/changedescs/models.py (Diff revision 5)
     
     
     

    Might be worth adding a comment about why we need to put old_value and new_value into tuples.

  3. We might want to add some documentation saying that the user will have the chance to cancel the publish in the event that the submitter has been changed.

  4. I think this might be clearer:

    'Are you sure you want to change the ownership of this review request? Doing so may prevent you from editing the review request afterwards.'

  5. 
      
Chenxi Ni
David Trowbridge
  1. 
      
  2. reviewboard/changedescs/models.py (Diff revision 6)
     
     
     

    Should start with a capital letter and end in a period.

  3. reviewboard/changedescs/models.py (Diff revision 6)
     
     

    Undo this change (lists in python should have trailing commas)

  4. This should have from __future__ import unicode_literals as the first line, and then a blank line, then the rest.

  5. There will only ever be one submitter, so we can just use gettext, not ngettext.

  6. These two lines can be combined (define and assign in one go)

  7. You could even just move the gettext call inline here, since that text is only used once.

    1. I did this before, but since the text line is too long(Christian said in gettext we shouldn't break the text into two lines) so I write in this way to avoid that. Still should I fix this?

    2. I think since that line is going to be super long no matter what, we can just have it inline and have this one be the super long line instead of moving it above.

  8. 
      
Chenxi Ni
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
        reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
        reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
  2. reviewboard/changedescs/models.py (Diff revision 7)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. Col: 39
     E127 continuation line over-indented for visual indent
    
  4. 
      
Chenxi Ni
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
        reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
        reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
  2. 
      
Barret Rennie
  1. This looks good. I've just got a few nitpicks.

  2. Blank line between these.

    1. Can you tell we where we should leave blank line? I will remember next time.

  3. You can put $link on the previous line. Make sure to dedent the following two lines.

  4. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    You can just build it all at once using a template, e.g.

    _linkTemplate: _.template('<a href="<%- url %>"><%- label %></a>'),
    
    /*
     * ...
     */
    convertToLink: function(item, urlFunc, textFunc) {
        if (!item) {
            return '';
        }
    
        return _linkTemplate({
            url: urlFunc ? urlFunc(item) : item,
            label: textFunc ? textFunc(item) : item
        });
    
  5. Isn't $field.children() already a jQuery wrapper around the DOM elements?

  6. submitter before summary

  7. submitter before summary

  8. Blank line between these.

  9. 
      
Chenxi Ni
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
        reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
        reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
  2. 
      
David Trowbridge
  1. 
      
  2. Can you either move the template definition into the initialize method (storing it as a variable on the object), or after the if (!item) check below?

  3. These should be indented 4 more spaces.

  4. This is indented too far.

  5. 
      
Chenxi Ni
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
        reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/evolutions/add_owner_to_draft.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/tests/draftReviewRequestModelTests.js
        reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    
    
  2. 
      
Chenxi Ni
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to feature/review-request-ownership (bcaa2ff)
Loading...