Convert RB.Review to Backbone.js
Review Request #3990 — Created March 22, 2013 and submitted
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 … |
 chipx86
|
|
|
No quotes. Why is review_request not reviewRequest? Or rather, not just parentObject? |
chipx86
|
|
|
No quotes. |
chipx86
|
|
|
Remove the trailing comma. |
chipx86
|
|
|
Remove trailing comma. |
chipx86
|
|
|
Combine these? |
chipx86
|
|
|
We call almost the same thing in two places, so maybe just provide different 'options' depending on the loaded/isNew state? |
chipx86
|
|
|
=== |
chipx86
|
|
|
Can this be combined? |
chipx86
|
|
|
Maybe just call this errorText? It's only in the case of an error. (errorText doesn't exist in the XHR specs.) |
chipx86
|
-
-
reviewboard/static/rb/js/models/draftReviewModel.js (Diff revision 1) 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)?
-
reviewboard/static/rb/js/models/reviewModel.js (Diff revision 1) No quotes. Why is review_request not reviewRequest? Or rather, not just parentObject?
-
-
-
-
Change Summary:
Fix up the easy parts.
-
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
Diff: |
Revision 3 (+241 -219)
|
|---|
-
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
-
-
reviewboard/static/rb/js/models/draftReviewModel.js (Diff revisions 2 - 3) Could this be passed to RB.BaseResource.prototype.ready as the 'ready' callback, instead of repeating the the parentObject.ready below?
Change Summary:
More final implementation.
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+286 -231)
|
-
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
-
-
reviewboard/static/rb/js/models/draftReviewModel.js (Diff revisions 3 - 4) We call almost the same thing in two places, so maybe just provide different 'options' depending on the loaded/isNew state?
-
-
-
reviewboard/static/rb/js/models/baseResourceModel.js (Diff revision 4) Seems okay, but my concern is that there may be existing code expecting a different error result from resources. Is that an issue?
Diff: |
Revision 5 (+286 -234)
|
|---|
-
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
-
Awesome. One small thing, but otherwise I'm happy.
-
reviewboard/static/rb/js/models/baseResourceModel.js (Diff revisions 4 - 5) Maybe just call this errorText? It's only in the case of an error. (errorText doesn't exist in the XHR specs.)
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