• 
      

    Fully represent review request entry data using new models.

    Review Request #9037 — Created June 26, 2017 and submitted

    Information

    Review Board
    release-3.0.x
    32dc14d...

    Reviewers

    This change introduces a handful of new JavaScript models for
    representing data going into the review request entry boxes (change
    descriptions, initial status updates, and reviews). These store the
    ReviewRequestEditor used, data on review(s) (for the review box and
    status updates), and data on the diff comments (used to load diff
    fragments).

    The templates for these entries no longer need to call JavaScript
    methods, beyond adding the boxes and models. The data is all
    self-contained. This will later allow for more easily reloading data for
    entries without reloading the entire page.

    The new models live in a js/reviewRequestPage/ directory. We'll be able
    to move other models/views specific to the review request page here
    later on, helping keep things organized.

    Unit tests passed.

    Tested loading diff comments for reviews (in all boxes) and replying to
    them.

    Description From Last Updated

    Missing the return type.

    daviddavid

    Trailing comma.

    daviddavid

    Missing the return type.

    daviddavid

    Trailing comma.

    daviddavid

    Missing the return type.

    daviddavid

    This would be quite a bit nicer as: reviews = reviewsData.map( reviewData => reviewRequest.createReview( reviewData.id, _.pick(reviewData, 'bodyBottom', 'bodyTop', 'public', 'shipIt')));

    daviddavid

    Trailing comma.

    daviddavid

    Trailing comma.

    daviddavid

    Trailing comma.

    daviddavid

    Trailing comma.

    daviddavid

    diffCommentsData.forEach(diffCommentData => page.queueLoadDiff(diffCommentData[0], diffCommentData[1]));

    daviddavid

    E501 line too long (97 > 79 characters)

    reviewbotreviewbot

    E501 line too long (91 > 79 characters)

    reviewbotreviewbot

    E501 line too long (97 > 79 characters)

    reviewbotreviewbot

    E501 line too long (104 > 79 characters)

    reviewbotreviewbot

    E501 line too long (80 > 79 characters)

    reviewbotreviewbot

    E501 line too long (86 > 79 characters)

    reviewbotreviewbot

    E501 line too long (93 > 79 characters)

    reviewbotreviewbot

    E501 line too long (86 > 79 characters)

    reviewbotreviewbot

    Col: 2 Expected '}' to match '{' from line 5 and instead saw '%'.

    reviewbotreviewbot

    Col: 5 Expected ']' to match '[' from line 4 and instead saw 'for'.

    reviewbotreviewbot

    Col: 9 Expected '}' to match '{' from line 3 and instead saw 'comment'.

    reviewbotreviewbot

    Col: 50 Expected an identifier and instead saw '}'.

    reviewbotreviewbot

    Col: 13 Expected an operator and instead saw '['.

    reviewbotreviewbot

    Col: 14 Expected ')' and instead saw '{{comment.id}}'.

    reviewbotreviewbot

    Col: 109 Expected ':' and instead saw ']'.

    reviewbotreviewbot

    Col: 111 Expected '}' to match '{' from line 7 and instead saw '%'.

    reviewbotreviewbot

    Col: 113 Expected '}' to match '{' from line 1 and instead saw 'if'.

    reviewbotreviewbot

    Col: 116 Expected ')' and instead saw 'not'.

    reviewbotreviewbot

    Col: 120 Expected ')' and instead saw 'forloop'.

    reviewbotreviewbot

    Col: 134 Expected an identifier and instead saw '}'.

    reviewbotreviewbot

    Col: 137 Expected '}' to match '{' from line 7 and instead saw '%'.

    reviewbotreviewbot

    Col: 137 Expected an assignment or function call and instead saw an expression.

    reviewbotreviewbot

    Col: 138 Missing semicolon.

    reviewbotreviewbot

    Col: 146 Expected an identifier and instead saw '}'.

    reviewbotreviewbot

    Col: 146 Expected an assignment or function call and instead saw an expression.

    reviewbotreviewbot

    Col: 147 Missing semicolon.

    reviewbotreviewbot

    Col: 2 Expected an identifier and instead saw '%'.

    reviewbotreviewbot

    Col: 2 Expected an assignment or function call and instead saw an expression.

    reviewbotreviewbot

    Col: 3 Missing semicolon.

    reviewbotreviewbot

    Col: 13 Expected an assignment or function call and instead saw an expression.

    reviewbotreviewbot

    Col: 13 Expected an identifier and instead saw '}'.

    reviewbotreviewbot

    Col: 14 Missing semicolon.

    reviewbotreviewbot

    Col: 9 Expected an identifier and instead saw ']'.

    reviewbotreviewbot

    Col: 10 Expected an operator and instead saw ','.

    reviewbotreviewbot

    Col: 10 Expected an assignment or function call and instead saw an expression.

    reviewbotreviewbot

    Col: 11 Missing semicolon.

    reviewbotreviewbot

    Col: 32 Expected an assignment or function call and instead saw an expression.

    reviewbotreviewbot

    Col: 34 Missing semicolon.

    reviewbotreviewbot

    E501 line too long (97 > 79 characters)

    reviewbotreviewbot

    E501 line too long (91 > 79 characters)

    reviewbotreviewbot

    E501 line too long (97 > 79 characters)

    reviewbotreviewbot

    E501 line too long (104 > 79 characters)

    reviewbotreviewbot

    E501 line too long (80 > 79 characters)

    reviewbotreviewbot

    E501 line too long (86 > 79 characters)

    reviewbotreviewbot

    E501 line too long (93 > 79 characters)

    reviewbotreviewbot

    E501 line too long (86 > 79 characters)

    reviewbotreviewbot

    Col: 2 Expected '}' to match '{' from line 5 and instead saw '%'.

    reviewbotreviewbot

    Col: 5 Expected ']' to match '[' from line 4 and instead saw 'for'.

    reviewbotreviewbot

    Col: 9 Expected '}' to match '{' from line 3 and instead saw 'comment'.

    reviewbotreviewbot

    Col: 50 Expected an identifier and instead saw '}'.

    reviewbotreviewbot

    Col: 13 Expected an operator and instead saw '['.

    reviewbotreviewbot

    Col: 14 Expected ')' and instead saw '{{comment.id}}'.

    reviewbotreviewbot

    Col: 109 Expected ':' and instead saw ']'.

    reviewbotreviewbot

    Col: 111 Expected '}' to match '{' from line 7 and instead saw '%'.

    reviewbotreviewbot

    Col: 113 Expected '}' to match '{' from line 1 and instead saw 'if'.

    reviewbotreviewbot

    Col: 116 Expected ')' and instead saw 'not'.

    reviewbotreviewbot

    Col: 120 Expected ')' and instead saw 'forloop'.

    reviewbotreviewbot

    Col: 134 Expected an identifier and instead saw '}'.

    reviewbotreviewbot

    Col: 137 Expected '}' to match '{' from line 7 and instead saw '%'.

    reviewbotreviewbot

    Col: 137 Expected an assignment or function call and instead saw an expression.

    reviewbotreviewbot

    Col: 138 Missing semicolon.

    reviewbotreviewbot

    Col: 146 Expected an identifier and instead saw '}'.

    reviewbotreviewbot

    Col: 146 Expected an assignment or function call and instead saw an expression.

    reviewbotreviewbot

    Col: 147 Missing semicolon.

    reviewbotreviewbot

    Col: 2 Expected an identifier and instead saw '%'.

    reviewbotreviewbot

    Col: 2 Expected an assignment or function call and instead saw an expression.

    reviewbotreviewbot

    Col: 3 Missing semicolon.

    reviewbotreviewbot

    Col: 13 Expected an identifier and instead saw '}'.

    reviewbotreviewbot

    Col: 13 Expected an assignment or function call and instead saw an expression.

    reviewbotreviewbot

    Col: 14 Missing semicolon.

    reviewbotreviewbot

    Col: 9 Expected an identifier and instead saw ']'.

    reviewbotreviewbot

    Col: 10 Expected an operator and instead saw ','.

    reviewbotreviewbot

    Col: 10 Expected an assignment or function call and instead saw an expression.

    reviewbotreviewbot

    Col: 11 Missing semicolon.

    reviewbotreviewbot

    Col: 32 Expected an assignment or function call and instead saw an expression.

    reviewbotreviewbot

    Col: 34 Missing semicolon.

    reviewbotreviewbot
    Checks run (2 failed)
    flake8 failed.
    JSHint failed.

    flake8

    JSHint

    david
    1. 
        
    2. Show all issues

      Missing the return type.

    3. Show all issues

      Trailing comma.

    4. Show all issues

      Missing the return type.

    5. Show all issues

      Missing the return type.

    6. Show all issues

      This would be quite a bit nicer as:

      reviews = reviewsData.map(
          reviewData => reviewRequest.createReview(
              reviewData.id,
              _.pick(reviewData, 'bodyBottom', 'bodyTop',
                     'public', 'shipIt')));
      
      1. _.pick is a pretty slow method. Useful at times when dealing with dynamic lists of keys or when rarely called, but not worth it here, particularly when we know all the data up-front that we care about. It has to loop through each item and then do what this code is doing, but manually.

        I'll switch to map, though. That's faster.

        https://jsperf.com/map-vs-for-loop-with-pick-vs-manual-building

    7. reviewboard/static/rb/js/views/reviewView.es6.js (Diff revision 1)
       
       
       
       
       
       
      Show all issues

      diffCommentsData.forEach(diffCommentData =>
          page.queueLoadDiff(diffCommentData[0], diffCommentData[1]));
      
      1. Standard for loops are a lot faster than .forEach. Going for performance here, too.

        https://jsperf.com/for-loop-vs-foreach-with-callback/

    8. 
        
    chipx86
    Review request changed
    Change Summary:
    • Added missing trailing commas.
    • Converted a for loop to a map.
    • Added missing return types in docs.
    Commit:
    a63c421032737cac753a1d84977a62ccc6d0cfe4
    32dc14ddad0bc5f0bbe509bc0430945262986bdd

    Checks run (2 failed)

    flake8 failed.
    JSHint failed.

    flake8

    JSHint

    david
    1. Ship It!
    2. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-3.0.x (c2dcb30)