Summary

Convert RB.Review to Backbone.js

Review Request #3990 — Created March 22, 2013 and submitted March 28, 2013, 7:34 p.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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().

Testing Done

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.

Issues

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	March 23, 2013, 2:28 a.m.
No quotes. Why is review_request not reviewRequest? Or rather, not just parentObject?	chipx86	March 22, 2013, 8:48 a.m.
No quotes.	chipx86	March 22, 2013, 8:48 a.m.
Remove the trailing comma.	chipx86	March 22, 2013, 8:48 a.m.
Remove trailing comma.	chipx86	March 22, 2013, 8:48 a.m.
Combine these?	chipx86	March 22, 2013, 8:49 a.m.
We call almost the same thing in two places, so maybe just provide different 'options' depending on the loaded/isNew state?	chipx86	March 27, 2013, 6:29 p.m.
===	chipx86	March 27, 2013, 6:29 p.m.
Can this be combined?	chipx86	March 27, 2013, 6:29 p.m.
Maybe just call this errorText? It's only in the case of an error. (errorText doesn't exist in the XHR specs.)	chipx86	March 28, 2013, 8:42 p.m.

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 revision 1)

The issue has been resolved. 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)?

chipx86 March 22, 2013, 8:47 a.m.

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?

david March 22, 2013, 8:54 a.m.

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.

david March 23, 2013, 2:28 a.m.

Okay, I think I've improved this a fair amount.

reviewboard/static/rb/js/models/reviewModel.js (Diff revision 1)

The issue has been resolved. Show all issues

No quotes.

Why is review_request not reviewRequest? Or rather, not just parentObject?

david March 22, 2013, 8:51 a.m.

review_request was just a copy-o from the datastore.js version, and is unused.

reviewboard/static/rb/js/models/reviewModel.js (Diff revision 1)
The issue has been resolved. Show all issues
```
No quotes.
```
reviewboard/static/rb/js/models/reviewModel.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Remove the trailing comma.
```
reviewboard/static/rb/js/models/reviewModel.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Remove trailing comma.
```
reviewboard/static/rb/js/models/tests/reviewModelTests.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Combine these?
```

Change Summary:

Fix up the easy parts.

Diff:

Revision 2 (+245 -219)

Show changes

	reviewboard/settings.py
	reviewboard/static/rb/js/datastore.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/reviewReplyModel.js
	reviewboard/static/rb/js/models/tests/reviewModelTests.js

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

Ship it!

```
Ship It!
```

Diff:

Revision 3 (+241 -219)

Show changes

	reviewboard/settings.py
	reviewboard/static/rb/js/datastore.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/reviewReplyModel.js
	reviewboard/static/rb/js/models/tests/reviewModelTests.js

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:

[WIP] Convert RB.Review to Backbone.js

Convert RB.Review to Backbone.js

Description:

~		[WIP] Convert RB.Review to Backbone.js
	~	Convert RB.Review to Backbone.js

~		This is a preliminary change to convert RB.Review to Backbone.js. I think the
~		code is mostly complete but I need to reread through all the code and make sure
~		I understand why everything is there, and do a lot more testing, especially with
~		non-diff comments.
	~	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().

Testing Done:

		Played around a fair bit creating draft reviews, adding comments, saving them,
~		and publishing the resulting reviews.
	~	and publishing or discarding the resulting reviews. Also verified the quick
	+	"ship it" review functionality.

Diff:

Revision 4 (+286 -231)

Show changes

	reviewboard/settings.py
	reviewboard/static/rb/js/datastore.js
	reviewboard/static/rb/js/reviews.js
	reviewboard/static/rb/js/models/baseResourceModel.js
	reviewboard/static/rb/js/models/draftReviewModel.js
	reviewboard/static/rb/js/models/reviewModel.js
	reviewboard/static/rb/js/models/tests/reviewModelTests.js

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)

The issue has been resolved. Show all issues

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/draftReviewModel.js (Diff revisions 3 - 4)
The issue has been resolved. Show all issues
```
===
```
reviewboard/static/rb/js/models/draftReviewModel.js (Diff revisions 3 - 4)
The issue has been resolved. Show all issues
```
Can this be combined?
```

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?

david March 27, 2013, 6:36 p.m.
```
Just one. I'll fix it.
```

Diff:

Revision 5 (+286 -234)

Show changes

	reviewboard/settings.py
	reviewboard/static/rb/js/datastore.js
	reviewboard/static/rb/js/reviews.js
	reviewboard/static/rb/js/models/baseResourceModel.js
	reviewboard/static/rb/js/models/draftReviewModel.js
	reviewboard/static/rb/js/models/reviewModel.js
	reviewboard/static/rb/js/models/tests/reviewModelTests.js
	reviewboard/static/rb/js/views/commentDialogView.js
	1 more

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)

The issue has been resolved. 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.)

Status:: Completed
Change Summary:: Pushed to master (fe6433e).