diff --git a/reviewboard/static/rb/js/datastore.js b/reviewboard/static/rb/js/datastore.js index a835436f8e43654c777c43c171d803357191120f..4392c46c4aae31095f34c34919c849c64b6571a2 100644 --- a/reviewboard/static/rb/js/datastore.js +++ b/reviewboard/static/rb/js/datastore.js @@ -2017,7 +2017,7 @@ RB.apiCall = function(options) { var activityIndicator = $("#activity-indicator"); - if (!options.noActivityIndicator) { + if (RB.ajaxOptions.enableIndicator && !options.noActivityIndicator) { activityIndicator .removeClass("error") .text((options.type || options.type == "GET") @@ -2078,7 +2078,8 @@ RB.apiCall = function(options) { options.buttons.attr("disabled", false); } - if (!options.noActivityIndicator && + if (RB.ajaxOptions.enableIndicator && + !options.noActivityIndicator && !activityIndicator.hasClass("error")) { activityIndicator .delay(1000) @@ -2153,13 +2154,29 @@ RB.apiCall = function(options) { options.type = options.type || "POST"; - if (options.type != "GET") { + /* We allow disabling the function queue for the sake of unit tests. */ + if (RB.ajaxOptions.enableQueuing && options.type !== "GET") { $.funcQueue("rbapicall").add(doCall); $.funcQueue("rbapicall").start(); } else { doCall(); } -} +}; + +RB.ajaxOptions = { + enableQueuing: true, + enableIndicator: true +}; + +/* + * Call RB.apiCall instead of $.ajax. + * + * We wrap instead of assign for now so that we can hook in/override + * RB.apiCall with unit tests. + */ +Backbone.ajax = function(options) { + return RB.apiCall(options); +}; function sendFileBlob(file, save_func, obj, options) { diff --git a/reviewboard/static/rb/js/models/baseResourceModel.js b/reviewboard/static/rb/js/models/baseResourceModel.js index f17d5dd05d26d899ec5fbbcd3cacb348155a3a39..ed97c4f1987d9d2039909e97e55b26e1c8d8ad5a 100644 --- a/reviewboard/static/rb/js/models/baseResourceModel.js +++ b/reviewboard/static/rb/js/models/baseResourceModel.js @@ -153,17 +153,18 @@ RB.BaseResource = Backbone.Model.extend({ * * If we fail to fetch the resource, options.error() will be called. */ - fetch: function(options) { + fetch: function(options, context) { var parentObject, fetchObject = _.bind(function() { - Backbone.Model.prototype.fetch.call(this, options); + Backbone.Model.prototype.fetch.call( + this, this._wrapCallbacks(options, context)); }, this); options = options || {}; if (this.isNew()) { if (_.isFunction(options.error)) { - options.error( + options.error.call(context, 'fetch cannot be used on a resource without an ID'); } @@ -203,33 +204,22 @@ RB.BaseResource = Backbone.Model.extend({ * data is saved to the server. * * If we successfully save the resource, options.success() will be - * called. + * called, and the "saved" event will be triggered. * * If we fail to save the resource, options.error() will be called. */ - save: function(options) { - var url = _.result(this, 'url'), - saveObject = _.bind(function() { - Backbone.Model.prototype.save.call(this, {}, options); - }, this); - + save: function(options, context) { options = options || {}; - if (!url) { - if (_.isFunction(options.error)) { - options.error('The object must either be loaded from the ' + - 'server or have a parent object before it can ' + - 'be saved'); - } - - return; - } - this.ready({ ready: function() { - var parentObject = this.get('parentObject'); + var parentObject = this.get('parentObject'), + saveObject; if (parentObject) { + saveObject = _.bind(this._saveObject, this, options, + context); + if (parentObject.cid) { parentObject.ensureCreated({ success: saveObject, @@ -239,7 +229,7 @@ RB.BaseResource = Backbone.Model.extend({ parentObject.ensureCreated(saveObject); } } else { - saveObject(); + this._saveObject(options, context); } }, error: options.error @@ -247,6 +237,40 @@ RB.BaseResource = Backbone.Model.extend({ }, /* + * Handles the actual saving of the object's state. + * + * This is called internally by save() once we've handled all the + * readiness and creation checks of this object and its parent. + */ + _saveObject: function(options, context) { + var url = _.result(this, 'url'); + + if (!url) { + if (_.isFunction(options.error)) { + options.error.call(context, + 'The object must either be loaded from the server or ' + + 'have a parent object before it can be saved'); + } + + return; + } + + Backbone.Model.prototype.save.call(this, {}, _.defaults({ + success: _.bind(function() { + if (_.isFunction(options.success)) { + options.success.apply(context, arguments); + } + + this.trigger('saved'); + }, this), + + error: _.isFunction(options.error) + ? _.bind(options.error, context) + : undefined + }, options)); + }, + + /* * Deletes the object's resource on the server. * * An object must either be loaded or have a parent resource linking to @@ -257,28 +281,29 @@ RB.BaseResource = Backbone.Model.extend({ * * If we fail to delete the resource, options.error() will be called. */ - destroy: function(options) { + destroy: function(options, context) { var url = _.result(this, 'url'); options = options || {}; if (!url) { if (_.isFunction(options.error)) { - options.error('The object must either be loaded from the ' + - 'server or have a parent object before it can ' + - 'be deleted'); + options.error.call(context, + 'The object must either be loaded from the server or ' + + 'have a parent object before it can be deleted'); } return; } - options = options || {}; - this.ready({ ready: function() { - Backbone.Model.prototype.destroy.call(this, options); + Backbone.Model.prototype.destroy.call( + this, this._wrapCallbacks(options, context)); }, - error: options.error + error: _.isFunction(options.error) + ? _.bind(options.error, context) + : undefined }, this); }, @@ -291,7 +316,12 @@ RB.BaseResource = Backbone.Model.extend({ * BaseResource.protoype.parse as well. */ parse: function(rsp) { - var rspData = rsp[this.rspNamespace]; + var rspData; + + console.assert(this.rspNamespace, + 'rspNamespace must be defined on the resource model'); + + rspData = rsp[this.rspNamespace]; return { id: rspData.id, @@ -309,5 +339,61 @@ RB.BaseResource = Backbone.Model.extend({ */ toJSON: function() { return {}; + }, + + /* + * Handles all AJAX communication for the model and its subclasses. + * + * Backbone.js will internally call the model's sync function to + * communicate with the server, which usually uses Backbone.sync. + * + * We wrap this to convert the data to encoded form data (instead + * of Backbone's default JSON payload). + * + * We also parse the error response from Review Board so we can provide + * a more meaningful error callback. + */ + sync: function(method, model, options) { + options = options || {}; + + return Backbone.sync.call(this, method, model, _.defaults({ + /* Use form data instead of a JSON payload. */ + contentType: 'application/x-www-form-urlencoded', + data: options.attrs || model.toJSON(options), + processData: true, + + error: function(xhr, textStatus, errorThrown) { + var rsp = null, + text; + + try { + rsp = $.parseJSON(xhr.responseText); + text = rsp.err.msg; + } catch (e) { + text = 'HTTP ' + xhr.status + ' ' + xhr.statusText; + } + + options.error(model, text, xhr.statusText); + } + }, options)); + }, + + /* + * Wraps success and error callbacks with a bound context. + * + * Backbone.js's various ajax-related functions don't take a context + * with their callbacks, so BaseResource needs to do that itself when + * calling those functions. This function simplifies those call sites + * by handling the wrapping. + */ + _wrapCallbacks: function(options, context) { + return _.defaults({ + success: _.isFunction(options.success) + ? _.bind(options.success, context) + : undefined, + error: _.isFunction(options.error) + ? _.bind(options.error, context) + : undefined + }, options); } }); diff --git a/reviewboard/static/rb/js/models/tests/baseResourceModelTests.js b/reviewboard/static/rb/js/models/tests/baseResourceModelTests.js index 40662359c3c7340e1849ce4b18ee0e52cae9d40f..3cba5ce8ca3fb047141cfba997c1e9892f98f616 100644 --- a/reviewboard/static/rb/js/models/tests/baseResourceModelTests.js +++ b/reviewboard/static/rb/js/models/tests/baseResourceModelTests.js @@ -371,6 +371,7 @@ describe('models/BaseResource', function() { spyOn(callbacks, 'success'); spyOn(callbacks, 'error'); + spyOn(model, 'trigger'); }); describe('With isNew=true and parentObject', function() { @@ -390,6 +391,7 @@ describe('models/BaseResource', function() { model.set('parentObject', parentObject); + spyOn(RB, 'apiCall').andCallThrough(); spyOn($, 'ajax').andCallFake(function(request) { expect(request.type).toBe('POST'); @@ -405,9 +407,11 @@ describe('models/BaseResource', function() { expect(Backbone.Model.prototype.save).toHaveBeenCalled(); expect(parentObject.ensureCreated).toHaveBeenCalled(); + expect(RB.apiCall).toHaveBeenCalled(); expect($.ajax).toHaveBeenCalled(); expect(callbacks.success).toHaveBeenCalled(); expect(callbacks.error).not.toHaveBeenCalled(); + expect(model.trigger).toHaveBeenCalledWith('saved'); }); it('Without callbacks', function() { @@ -415,13 +419,16 @@ describe('models/BaseResource', function() { expect(Backbone.Model.prototype.save).toHaveBeenCalled(); expect(parentObject.ensureCreated).toHaveBeenCalled(); + expect(RB.apiCall).toHaveBeenCalled(); expect($.ajax).toHaveBeenCalled(); + expect(model.trigger).toHaveBeenCalledWith('saved'); }); }); describe('With isNew=true and no parentObject', function() { beforeEach(function() { spyOn(Backbone.Model.prototype, 'save').andCallThrough(); + spyOn(RB, 'apiCall').andCallThrough(); spyOn($, 'ajax').andCallFake(function(request) {}); }); @@ -430,9 +437,11 @@ describe('models/BaseResource', function() { expect(Backbone.Model.prototype.save) .not.toHaveBeenCalled(); + expect(RB.apiCall).not.toHaveBeenCalled(); expect($.ajax).not.toHaveBeenCalled(); expect(callbacks.success).not.toHaveBeenCalled(); expect(callbacks.error).toHaveBeenCalled(); + expect(model.trigger).not.toHaveBeenCalledWith('saved'); }); it('Without callbacks', function() { @@ -440,7 +449,9 @@ describe('models/BaseResource', function() { expect(Backbone.Model.prototype.save) .not.toHaveBeenCalled(); + expect(RB.apiCall).not.toHaveBeenCalled(); expect($.ajax).not.toHaveBeenCalled(); + expect(model.trigger).not.toHaveBeenCalledWith('saved'); }); }); @@ -463,11 +474,13 @@ describe('models/BaseResource', function() { expect(Backbone.Model.prototype.save).toHaveBeenCalled(); expect(callbacks.success).toHaveBeenCalled(); expect(callbacks.error).not.toHaveBeenCalled(); + expect(model.trigger).toHaveBeenCalledWith('saved'); }); it('Without callbacks', function() { model.save(); expect(Backbone.Model.prototype.save).toHaveBeenCalled(); + expect(model.trigger).toHaveBeenCalledWith('saved'); }); }); @@ -492,6 +505,7 @@ describe('models/BaseResource', function() { options.ready.call(context); }); + spyOn(RB, 'apiCall').andCallThrough(); spyOn($, 'ajax').andCallFake(function(request) { expect(request.type).toBe('PUT'); @@ -508,8 +522,10 @@ describe('models/BaseResource', function() { expect(parentObject.ready).toHaveBeenCalled(); expect(Backbone.Model.prototype.save).toHaveBeenCalled(); expect(callbacks.success).toHaveBeenCalled(); + expect(RB.apiCall).toHaveBeenCalled(); expect($.ajax).toHaveBeenCalled(); expect(callbacks.error).not.toHaveBeenCalled(); + expect(model.trigger).toHaveBeenCalledWith('saved'); }); it('Without callbacks', function() { @@ -517,7 +533,9 @@ describe('models/BaseResource', function() { expect(parentObject.ready).toHaveBeenCalled(); expect(Backbone.Model.prototype.save).toHaveBeenCalled(); + expect(RB.apiCall).toHaveBeenCalled(); expect($.ajax).toHaveBeenCalled(); + expect(model.trigger).toHaveBeenCalledWith('saved'); }); }); @@ -552,6 +570,7 @@ describe('models/BaseResource', function() { .not.toHaveBeenCalled(); expect(callbacks.success).not.toHaveBeenCalled(); expect(callbacks.error).toHaveBeenCalled(); + expect(model.trigger).not.toHaveBeenCalledWith('saved'); }); it('Without callbacks', function() { @@ -560,6 +579,7 @@ describe('models/BaseResource', function() { expect(parentObject.ready).toHaveBeenCalled(); expect(Backbone.Model.prototype.save) .not.toHaveBeenCalled(); + expect(model.trigger).not.toHaveBeenCalledWith('saved'); }); }); }); @@ -584,15 +604,16 @@ describe('models/BaseResource', function() { options.ready.call(context); }); + spyOn(RB, 'apiCall').andCallThrough(); spyOn($, 'ajax').andCallFake(function(request) { - var data = $.parseJSON(request.data); - expect(request.url).toBe(model.url); - expect(request.contentType).toBe('application/json'); + expect(request.contentType) + .toBe('application/x-www-form-urlencoded'); + expect(request.processData).toBe(true); - expect(data.a).toBe(10); - expect(data.b).toBe(20); - expect(data.c).toBe(30); + expect(request.data.a).toBe(10); + expect(request.data.b).toBe(20); + expect(request.data.c).toBe(30); request.success({ stat: 'ok', @@ -609,6 +630,7 @@ describe('models/BaseResource', function() { model.save(); expect(model.toJSON).toHaveBeenCalled(); + expect(RB.apiCall).toHaveBeenCalled(); expect($.ajax).toHaveBeenCalled(); }); }); diff --git a/reviewboard/templates/js/tests.html b/reviewboard/templates/js/tests.html index 585be0eeb931006c7c37a0b2b40221ce26292e6c..5f6749f66af942fd47bb0e5a9797ab7b8636978c 100644 --- a/reviewboard/templates/js/tests.html +++ b/reviewboard/templates/js/tests.html @@ -32,6 +32,9 @@ $(document).ready(function() { var jasmineEnv = jasmine.getEnv(), htmlReporter = new jasmine.HtmlReporter(); + RB.ajaxOptions.enableQueuing = false; + RB.ajaxOptions.enableIndicator = false; + jasmineEnv.specFilter = function(spec) { return htmlReporter.specFilter(spec); };