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)