Fully represent review request entry data using new models.

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

Christian Hammond
Review Board
release-3.0.x
32dc14d...
reviewboard

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.

  • 0
  • 0
  • 9
  • 78
  • 87
Description From Last Updated
Checks run (2 failed)
flake8 failed.
JSHint failed.

flake8

JSHint

David Trowbridge
  1. 
      
  2. 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

  3. reviewboard/static/rb/js/views/reviewView.es6.js (Diff revision 1)
     
     
     
     
     
     

    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/

  4. 
      
Christian Hammond
Review request changed

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

David Trowbridge
  1. Ship It!
  2. 
      
Christian Hammond
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (c2dcb30)
Loading...