diff --git a/reviewboard/settings.py b/reviewboard/settings.py index 359bb36df33f5043dc650f1255e2fdfe8272d7c1..3009d41126c01a035d9393701a30e859ccbcef04 100644 --- a/reviewboard/settings.py +++ b/reviewboard/settings.py @@ -317,6 +317,7 @@ PIPELINE_JS = { 'rb/js/common.js', 'rb/js/datastore.js', 'rb/js/models/baseResourceModel.js', + 'rb/js/models/draftResourceModelMixin.js', 'rb/js/models/reviewModel.js', 'rb/js/models/draftReviewModel.js', 'rb/js/models/baseCommentModel.js', diff --git a/reviewboard/static/rb/js/models/baseResourceModel.js b/reviewboard/static/rb/js/models/baseResourceModel.js index 67b946dd88ff6e8cf26cd225fded32478cfe2177..f46fb18ee596b3c4a1379e359d1973c00e2cd9c0 100644 --- a/reviewboard/static/rb/js/models/baseResourceModel.js +++ b/reviewboard/static/rb/js/models/baseResourceModel.js @@ -398,7 +398,18 @@ RB.BaseResource = Backbone.Model.extend({ this, options, context); if (parentObject) { - parentObject.ready(destroyObject); + /* + * XXX This is temporary to support older-style resource + * objects. We should just use ready() once we're moved + * entirely onto BaseResource. + */ + if (parentObject.cid) { + parentObject.ready(_.defaults({ + ready: destroyObject + }, _.bindCallbacks(options, context))); + } else { + parentObject.ready(destroyObject); + } } else { destroyObject(); } @@ -411,7 +422,8 @@ RB.BaseResource = Backbone.Model.extend({ * readiness and creation checks of this object and its parent. */ _destroyObject: function(options, context) { - var url = _.result(this, 'url'); + var self = this, + url = _.result(this, 'url'); options = options || {}; @@ -427,8 +439,21 @@ RB.BaseResource = Backbone.Model.extend({ this.ready({ ready: function() { - Backbone.Model.prototype.destroy.call( - this, _.bindCallbacks(options, context)); + Backbone.Model.prototype.destroy.call(this, _.defaults({ + wait: true, + success: function() { + /* Reset the object so it's new again. */ + self.set({ + id: null, + loaded: false, + links: null + }); + + if (_.isFunction(options.success)) { + options.success.apply(self, arguments); + } + } + }, _.bindCallbacks(options, context))); }, error: _.isFunction(options.error) ? _.bind(options.error, context) diff --git a/reviewboard/static/rb/js/models/draftResourceModelMixin.js b/reviewboard/static/rb/js/models/draftResourceModelMixin.js new file mode 100644 index 0000000000000000000000000000000000000000..9376c3e7a0106b91699264437f97c2bd58510088 --- /dev/null +++ b/reviewboard/static/rb/js/models/draftResourceModelMixin.js @@ -0,0 +1,152 @@ +/* + * Mixin for resources that have special "draft" URLs. + * + * Some resources contain a "draft/" singleton URL that will either redirect to + * the URL for an existing draft, or indicate there's no draft (and requiring + * that one be created). + * + * These resources need a little more logic to look up the draft state and + * craft the proper URL. They can use this mixin to do that work for them. + */ +RB.DraftResourceModelMixin = { + /* + * Calls a function when the object is ready to use. + * + * If the object is unloaded, we'll likely need to grab the draft + * resource, particularly if we haven't already retrieved a draft. + * + * Otherwise, we delegate to the parent's ready(). + */ + ready: function(options, context) { + var self = this, + sup = _.super(this); + + if (!this.get('loaded') && this.isNew() && + this._needDraft === undefined) { + this._needDraft = true; + } + + if (this._needDraft) { + /* + * Start by delegating to the parent ready() function. Because the + * object is "new", this will make sure that the parentObject is + * ready. + */ + sup.ready.call( + this, + _.defaults({ + ready: function() { + self._retrieveDraft(options, context); + }, + }, options), + context); + } else { + sup.ready.call(this, options, context); + } + }, + + /* + * Destroys the object. + * + * If destruction is successful, we'll reset the needDraft state so we'll + * look up the draft the next time an operation is performed. + */ + destroy: function(options, context) { + options = _.bindCallbacks(options || {}); + + _.super(this).destroy.call( + this, + _.defaults({ + success: function() { + /* We need to fetch the draft resource again. */ + this._needDraft = true; + + if (_.isFunction(options.success)) { + options.success.apply(context, arguments); + } + } + }, options), + this); + }, + + /* + * Custom URL implementation which will return the special draft resource + * if we have yet to redirect and otherwise delegate to the prototype + * implementation. + */ + url: function() { + var parentObject, + links, + linkName; + + if (this._needDraft) { + parentObject = this.get('parentObject'); + linkName = _.result(this, 'listKey'); + + /* XXX Work with old-style and new-style resources' links. */ + if (parentObject.cid) { + links = parentObject.get('links'); + } else { + links = parentObject.links; + } + + /* + * Chrome hyper-aggressively caches things it shouldn't, and + * appears to do so in a subtlely broken way. + * + * If we do a DELETE on a reply's URL, then later a GET (resulting + * from a redirect from a GET to draft/), Chrome will somehow get + * confused and associate the GET's caching information with a 404. + * + * In order to prevent this, we need to make requests to draft/ + * appear unique. We can do this by appending the timestamp here. + * Chrome will no longer end up with broken state for our later + * GETs. + * + * Much of this is only required in the case of sqlite, which, + * with Django, may reuse row IDs, causing problems when making + * a reply, deleting, and making a new one. In production, this + * shouldn't be a problem, but it's very confusing during + * development. + */ + return links[linkName].href + 'draft/?' + $.now(); + } else { + return _.super(this).url.call(this); + } + }, + + /* + * Try to retrieve an existing draft from the server. This uses the + * special draft/ resource within the reosurce list, which will redirect to + * an existing draft if one exists. + */ + _retrieveDraft: function(options, context) { + var self = this; + + Backbone.Model.prototype.fetch.call(this, { + success: function() { + /* + * There was an existing draft, and we were redirected to it + * and pulled data from it. We're done. + */ + self._needDraft = false; + + if (options && _.isFunction(options.ready)) { + options.ready.call(self); + } + }, + error: function(model, xhr) { + if (xhr.status === 404) { + /* + * We now know we don't have an existing draft to work with, + * and will eventually need to POST to create a new one. + */ + self._needDraft = false; + options.ready.call(context); + } else if (options && _.isFunction(options.error)) { + options.error.call(xhr, xhr.status); + } + } + }); + } +}; diff --git a/reviewboard/static/rb/js/models/draftReviewModel.js b/reviewboard/static/rb/js/models/draftReviewModel.js index 242dae84436b74c6fc1452bd04743351868a7e7f..b9e6aa11bf21a590057576dffc3322562d001b34 100644 --- a/reviewboard/static/rb/js/models/draftReviewModel.js +++ b/reviewboard/static/rb/js/models/draftReviewModel.js @@ -6,71 +6,4 @@ * special resource exists at /reviews/draft/ which will redirect to the * existing draft if one exists, and return 404 if not. */ -RB.DraftReview = RB.Review.extend({ - /* - * Calls a function when the object is ready to use. - */ - ready: function(options, context) { - var self = this; - - if (!this.get('loaded') && this.isNew()) { - /* - * Start by delegating to RB.Review.prototype.ready. Because the - * object is "new", this will make sure that the parentObject is - * ready. - */ - RB.Review.prototype.ready.call( - this, - _.defaults({ - ready: function() { - self._retrieveDraft.call(self, options, context); - }, - }, options), - context); - } else { - RB.Review.prototype.ready.call(this, options, context); - } - }, - - /* - * Custom URL implementation which will return the special draft resource if - * we have yet to redirect and otherwise delegate to the prototype - * implementation. - */ - url: function() { - if (!this.get('loaded') && this.isNew()) { - return this.get('parentObject').links.reviews.href + 'draft/'; - } else { - return RB.Review.prototype.url.call(this); - } - }, - - /* - * Try to retrieve an existing draft review from the server. This uses the - * special draft/ resource within the reviews list, which will redirect to - * an existing draft review if one exists. - */ - _retrieveDraft: function(options, context) { - var self = this; - - console.assert(!this.get('loaded')); - console.assert(this.isNew()); - - Backbone.Model.prototype.fetch.call(this, { - success: function() { - if (options && _.isFunction(options.ready)) { - options.ready.call(self); - } - }, - error: function(model, xhr) { - if (xhr.status === 404) { - self.ready = RB.Review.prototype.ready; - self.url = RB.Review.prototype.url; - RB.Review.prototype.ready.call(self, options, context); - } else if (options && _.isFunction(options.error)) { - options.error.call(xhr, status, err); - } - } - }); - } -}); +RB.DraftReview = RB.Review.extend(RB.DraftResourceModelMixin); diff --git a/reviewboard/static/rb/js/models/reviewModel.js b/reviewboard/static/rb/js/models/reviewModel.js index 9d92e86c17cc63ee48b7a234ab3829f80574eb87..d13ac7eb374ac5bc83784f3bfcca5bc7aacb934d 100644 --- a/reviewboard/static/rb/js/models/reviewModel.js +++ b/reviewboard/static/rb/js/models/reviewModel.js @@ -9,13 +9,12 @@ RB.Review = RB.BaseResource.extend({ shipIt: false, public: false, bodyTop: null, - bodyBottom: null + bodyBottom: null, + draftReply: null }, RB.BaseResource.prototype.defaults), rspNamespace: 'review', - draftReply: null, - toJSON: function() { var data = { ship_it: (this.get('shipIt') ? 1 : 0), @@ -75,12 +74,15 @@ RB.Review = RB.BaseResource.extend({ }, createReply: function() { - if (this.draftReply == null) { - this.draftReply = new RB.ReviewReply({ + var draftReply = this.get('draftReply'); + + if (draftReply === null) { + draftReply = new RB.ReviewReply({ parentObject: this }); + this.set('draftReply', draftReply); } - return this.draftReply; + return draftReply; } }); diff --git a/reviewboard/static/rb/js/models/reviewReplyModel.js b/reviewboard/static/rb/js/models/reviewReplyModel.js index 39b09c8a2d43171967693f46e80a902c26105176..33b7dc6fdf5b863c84f8b4c44f4104451f978abb 100644 --- a/reviewboard/static/rb/js/models/reviewReplyModel.js +++ b/reviewboard/static/rb/js/models/reviewReplyModel.js @@ -33,3 +33,4 @@ RB.ReviewReply = RB.BaseResource.extend({ return result; } }); +_.extend(RB.ReviewReply.prototype, RB.DraftResourceModelMixin); diff --git a/reviewboard/static/rb/js/models/tests/reviewReplyModelTests.js b/reviewboard/static/rb/js/models/tests/reviewReplyModelTests.js index ce485d30bc77e61b52a2da9d0dae60352d552cd7..90ed2e3aba66d9cd166968e99d0423c7161da9bd 100644 --- a/reviewboard/static/rb/js/models/tests/reviewReplyModelTests.js +++ b/reviewboard/static/rb/js/models/tests/reviewReplyModelTests.js @@ -5,6 +5,11 @@ describe('models/ReviewReply', function() { beforeEach(function() { parentObject = new RB.BaseResource({ public: true, + links: { + replies: { + href: '/api/foos/replies/' + } + } }); model = new RB.ReviewReply({ @@ -12,6 +17,217 @@ describe('models/ReviewReply', function() { }); }); + describe('destroy', function() { + var callbacks; + + beforeEach(function() { + callbacks = { + ready: function() {}, + error: function() {} + }; + + spyOn(Backbone.Model.prototype, 'destroy') + .andCallFake(function(options) { + if (options && _.isFunction(options.success)) { + options.success(); + } + }); + spyOn(model, '_retrieveDraft').andCallThrough(); + spyOn(parentObject, 'ready') + .andCallFake(function(options, context) { + if (options && _.isFunction(options.ready)) { + options.ready.call(context); + } + }); + spyOn(callbacks, 'ready'); + spyOn(callbacks, 'error'); + }); + + describe('With isNew=true', function() { + beforeEach(function() { + expect(model.isNew()).toBe(true); + expect(model.get('loaded')).toBe(false); + + spyOn(Backbone.Model.prototype, 'fetch') + .andCallFake(function(options) { + if (options && _.isFunction(options.success)) { + options.error(model, { + status: 404 + }); + } + }); + }); + + it('With callbacks', function() { + model.destroy(callbacks); + + expect(model.isNew()).toBe(true); + expect(model.get('loaded')).toBe(false); + + expect(parentObject.ready).toHaveBeenCalled(); + expect(model._retrieveDraft).toHaveBeenCalled(); + expect(Backbone.Model.prototype.fetch).toHaveBeenCalled(); + expect(Backbone.Model.prototype.destroy).toHaveBeenCalled(); + });; + }); + + describe('With isNew=false', function() { + beforeEach(function() { + model.set({ + id: 123, + loaded: true + }); + + spyOn(Backbone.Model.prototype, 'fetch'); + }); + + it('With callbacks', function() { + model.destroy(callbacks); + + expect(parentObject.ready).toHaveBeenCalled(); + expect(model._retrieveDraft).not.toHaveBeenCalled(); + expect(Backbone.Model.prototype.fetch).not.toHaveBeenCalled(); + expect(Backbone.Model.prototype.destroy).toHaveBeenCalled(); + }); + }); + }); + + describe('ready', function() { + var callbacks; + + beforeEach(function() { + callbacks = { + ready: function() {}, + error: function() {} + }; + + spyOn(parentObject, 'ready') + .andCallFake(function(options, context) { + if (options && _.isFunction(options.ready)) { + options.ready.call(context); + } + }); + spyOn(callbacks, 'ready'); + spyOn(callbacks, 'error'); + }); + + describe('With isNew=true', function() { + beforeEach(function() { + expect(model.isNew()).toBe(true); + expect(model.get('loaded')).toBe(false); + + spyOn(Backbone.Model.prototype, 'fetch') + .andCallFake(function(options) { + if (options && _.isFunction(options.success)) { + options.success(); + } + }); + spyOn(model, '_retrieveDraft') + .andCallFake(function(options, context) { + if (options && _.isFunction(options.ready)) { + options.ready.call(context); + } + }); + }); + + it('With callbacks', function() { + model.ready(callbacks); + + expect(parentObject.ready).toHaveBeenCalled(); + expect(model._retrieveDraft).toHaveBeenCalled(); + expect(callbacks.ready).toHaveBeenCalled(); + });; + }); + + describe('With isNew=false', function() { + beforeEach(function() { + model.set({ + id: 123 + }); + + spyOn(Backbone.Model.prototype, 'fetch') + .andCallFake(function(options) { + if (options && _.isFunction(options.success)) { + options.success(); + } + }); + spyOn(model, '_retrieveDraft') + .andCallFake(function(options, context) { + if (options && _.isFunction(options.ready)) { + options.ready.call(context); + } + }); + }); + + it('With callbacks', function() { + model.ready(callbacks); + + expect(parentObject.ready).toHaveBeenCalled(); + expect(model._retrieveDraft).not.toHaveBeenCalled(); + expect(callbacks.ready).toHaveBeenCalled(); + }); + }); + + it('After destruction', function() { + spyOn(model, '_retrieveDraft').andCallThrough(); + + spyOn(Backbone.Model.prototype, 'fetch').andCallFake( + function(options) { + model.set({ + id: 123, + links: { + self: { + href: '/api/foos/replies/123/' + } + }, + loaded: true + }); + + options.success(); + }); + + spyOn(Backbone.Model.prototype, 'destroy').andCallFake( + function(options) { + options.success(); + }); + + expect(model.isNew()).toBe(true); + expect(model.get('loaded')).toBe(false); + expect(model._needDraft).toBe(undefined); + + /* Make our initial ready call. */ + model.ready(callbacks); + + expect(parentObject.ready).toHaveBeenCalled(); + expect(model._retrieveDraft).toHaveBeenCalled(); + expect(Backbone.Model.prototype.fetch).toHaveBeenCalled(); + expect(callbacks.ready).toHaveBeenCalled(); + expect(model.isNew()).toBe(false); + expect(model.get('loaded')).toBe(true); + expect(model._needDraft).toBe(false); + + /* We have a loaded object. Reset it. */ + model.destroy(callbacks); + + expect(model.isNew()).toBe(true); + expect(model.get('loaded')).toBe(false); + expect(model._needDraft).toBe(true); + + parentObject.ready.reset(); + model._retrieveDraft.reset(); + callbacks.ready.reset(); + + /* Now that it's destroyed, try to fetch it again. */ + model.ready(callbacks); + + expect(parentObject.ready).toHaveBeenCalled(); + expect(model._retrieveDraft).toHaveBeenCalled(); + expect(Backbone.Model.prototype.fetch).toHaveBeenCalled(); + expect(callbacks.ready).toHaveBeenCalled(); + expect(model._needDraft).toBe(false); + }); + }); + describe('parse', function() { beforeEach(function() { model.rspNamespace = 'my_reply'; diff --git a/reviewboard/static/rb/js/utils/underscoreUtils.js b/reviewboard/static/rb/js/utils/underscoreUtils.js index ede63713e24eab774b46e44cf55a882063435950..219c599f906bda9c42216de54c1b29571cb2279a 100644 --- a/reviewboard/static/rb/js/utils/underscoreUtils.js +++ b/reviewboard/static/rb/js/utils/underscoreUtils.js @@ -29,3 +29,11 @@ _.bindCallbacks = function(callbacks, context, methodNames) { return _.defaults(wrappedCallbacks, callbacks); }; + + +/* + * Returns the parent prototype for an object. + */ +_.super = function(obj) { + return Object.getPrototypeOf(Object.getPrototypeOf(obj)); +}; diff --git a/reviewboard/static/rb/js/views/tests/screenshotThumbnailViewTests.js b/reviewboard/static/rb/js/views/tests/screenshotThumbnailViewTests.js index 4cc703da52e6276db4ecbb8662fd9b4334dca688..3377301decc71c6b9dfff2ccf8a36d0dcb8d2b38 100644 --- a/reviewboard/static/rb/js/views/tests/screenshotThumbnailViewTests.js +++ b/reviewboard/static/rb/js/views/tests/screenshotThumbnailViewTests.js @@ -71,7 +71,7 @@ describe('views/ScreenshotThumbnail', function() { expect($.ajax).toHaveBeenCalled(); expect(model.destroy).toHaveBeenCalled(); - expect(model.trigger.calls[2].args[0]).toBe('destroy'); + expect(model.trigger.calls[0].args[0]).toBe('destroy'); expect(view.$el.fadeOut).toHaveBeenCalled(); expect(view.remove).toHaveBeenCalled(); }); diff --git a/reviewboard/webapi/resources.py b/reviewboard/webapi/resources.py index 16123f7f5878d123231105605138da2de5b96abb..8911e22a7deb6c07e7cc870c74177c2bba1222b4 100644 --- a/reviewboard/webapi/resources.py +++ b/reviewboard/webapi/resources.py @@ -5434,6 +5434,10 @@ class ReviewReplyFileAttachmentCommentResource(BaseFileAttachmentCommentResource q = q.filter(review=reply_id, review__base_reply_to=review_id) return q + def has_delete_permissions(self, request, comment, *args, **kwargs): + review = comment.review.get() + return not review.public and review.user == request.user + @webapi_check_local_site @webapi_login_required @webapi_response_errors(DOES_NOT_EXIST, INVALID_FORM_DATA, @@ -5828,7 +5832,7 @@ class ReviewReplyDraftResource(WebAPIResource): def get(self, request, *args, **kwargs): """Returns the location of the current draft reply. - If the draft reply exists, this will return :http:`301` with + If the draft reply exists, this will return :http:`302` with a ``Location`` header pointing to the URL of the draft. Any operations on the draft can be done at that URL. @@ -5845,7 +5849,7 @@ class ReviewReplyDraftResource(WebAPIResource): if not reply: return DOES_NOT_EXIST - return 301, {}, { + return 302, {}, { 'Location': review_reply_resource.get_href(reply, request, *args, **kwargs), } @@ -6073,6 +6077,8 @@ class ReviewReplyResource(BaseReviewResource): return 200, { self.item_result_key: reply, + }, { + 'Last-Modified': self.get_last_modified(request, reply), } review_reply_resource = ReviewReplyResource() @@ -6097,7 +6103,7 @@ class ReviewDraftResource(WebAPIResource): if not review: return DOES_NOT_EXIST - return 301, {}, { + return 302, {}, { 'Location': review_resource.get_href(review, request, *args, **kwargs), }