Convert RB.Review to Backbone.js

Review Request #3990 — Created March 22, 2013 and submitted

Information

Review Board
master

Reviewers

Convert RB.Review to Backbone.js

This change converts RB.Review to Backbone.js. This is a little bit more
complicated than one might expect because of the way draft reviews are handled.
The reviews.js code which handles pending reviews always creates review objects
without an ID, which means they think that they're "new". We then fetch a
special /draft/ resource which will redirect to an existing draft. This logic is
encapsulated in a "DraftReview" object. If we try to fetch the existing draft
and it fails, DraftReview will then fall back on the normal BaseResource
implementations and create a new review object on the server when it goes to
save().
Played around a fair bit creating draft reviews, adding comments, saving them,
and publishing or discarding the resulting reviews. Also verified the quick
"ship it" review functionality.
Description From Last Updated

To simplify things, could you just have this call the parent ready(), but passing either the provided callbacks or, if …

chipx86chipx86

No quotes. Why is review_request not reviewRequest? Or rather, not just parentObject?

chipx86chipx86

No quotes.

chipx86chipx86

Remove the trailing comma.

chipx86chipx86

Remove trailing comma.

chipx86chipx86

Combine these?

chipx86chipx86

We call almost the same thing in two places, so maybe just provide different 'options' depending on the loaded/isNew state?

chipx86chipx86

===

chipx86chipx86

Can this be combined?

chipx86chipx86

Maybe just call this errorText? It's only in the case of an error. (errorText doesn't exist in the XHR specs.)

chipx86chipx86
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/settings.py
      Ignored Files:
        reviewboard/static/rb/js/models/reviewReplyModel.js
        reviewboard/static/rb/js/datastore.js
        reviewboard/static/rb/js/models/tests/reviewModelTests.js
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/models/draftReviewModel.js
        reviewboard/static/rb/js/models/reviewModel.js
    
    
  2. 
      
chipx86
  1. 
      
  2. Show all issues
    To simplify things, could you just have this call the parent ready(), but passing either the provided callbacks or, if !loaded && isNew, then pass a wrapper that gets the ID? (Basically the retrieveDraft)?
    1. Btw, the comment I made earlier about the id was due to me thinking that we were doing something smarter in RB.Review already. I didn't realize we were just coding it in there. I think it'd be fine to just do it in url() then if isNew(). Would that help get rid of this ready() function?
    2. Hmm. I'm not sure I can do it in url() because we'll never even try to call fetch() if we think the object isNew(), but we don't know if it's actually new until after we do this check. If we GET /draft/ and it's 404, then it's new-new, if not, then we populate the id and do a real fetch.
    3. Okay, I think I've improved this a fair amount.
  3. reviewboard/static/rb/js/models/reviewModel.js (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    No quotes.
    
    Why is review_request not reviewRequest? Or rather, not just parentObject?
    1. review_request was just a copy-o from the datastore.js version, and is unused.
  4. reviewboard/static/rb/js/models/reviewModel.js (Diff revision 1)
     
     
     
     
    Show all issues
    No quotes.
  5. Show all issues
    Remove the trailing comma.
  6. Show all issues
    Remove trailing comma.
  7. Show all issues
    Combine these?
  8. 
      
david
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/settings.py
      Ignored Files:
        reviewboard/static/rb/js/models/reviewReplyModel.js
        reviewboard/static/rb/js/datastore.js
        reviewboard/static/rb/js/models/tests/reviewModelTests.js
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/models/draftReviewModel.js
        reviewboard/static/rb/js/models/reviewModel.js
    
    
  2. 
      
SW
  1. Ship It!
  2. 
      
david
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/settings.py
      Ignored Files:
        reviewboard/static/rb/js/models/reviewReplyModel.js
        reviewboard/static/rb/js/datastore.js
        reviewboard/static/rb/js/models/tests/reviewModelTests.js
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/models/draftReviewModel.js
        reviewboard/static/rb/js/models/reviewModel.js
    
    
  2. 
      
chipx86
  1. 
      
  2. Could this be passed to RB.BaseResource.prototype.ready as the 'ready' callback, instead of repeating the the parentObject.ready below?
  3. 
      
david
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/settings.py
      Ignored Files:
        reviewboard/static/rb/js/datastore.js
        reviewboard/static/rb/js/models/baseResourceModel.js
        reviewboard/static/rb/js/models/tests/reviewModelTests.js
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/models/draftReviewModel.js
        reviewboard/static/rb/js/models/reviewModel.js
    
    
  2. 
      
chipx86
  1. 
      
  2. Show all issues
    We call almost the same thing in two places, so maybe just provide different 'options' depending on the loaded/isNew state?
  3. Show all issues
    ===
  4. reviewboard/static/rb/js/models/draftReviewModel.js (Diff revisions 3 - 4)
     
     
     
    Show all issues
    Can this be combined?
  5. Seems okay, but my concern is that there may be existing code expecting a different error result from resources. Is that an issue?
    1. Just one. I'll fix it.
  6. 
      
david
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/settings.py
      Ignored Files:
        reviewboard/static/rb/js/datastore.js
        reviewboard/static/rb/js/models/baseResourceModel.js
        reviewboard/static/rb/js/models/tests/reviewModelTests.js
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/models/draftReviewModel.js
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/models/reviewModel.js
    
    
  2. 
      
chipx86
  1. Awesome. One small thing, but otherwise I'm happy.
  2. Show all issues
    Maybe just call this errorText? It's only in the case of an error. (errorText doesn't exist in the XHR specs.)
  3. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to master (fe6433e).