From 8d1e10bd8a8c38c8e58109ef381741eed1b0bb2a Mon Sep 17 00:00:00 2001 From: "E. Kolpakov" Date: Wed, 29 Jun 2016 12:56:30 +0300 Subject: [PATCH] Fixes and improvements from solutions fork: * Removing jQuery.ajax.beforeSend - it might be set globally and this code overrides it (part of 62fa253af6ee384f0766d72cd2d67bcf43697692) * Avoid resetting Backbone history if already started (part of 9591b526bdd56163de0098099e46e44c566061f5) * goHome no longer triggers thread:removed event - already triggered up the call stack (part of 9591b526bdd56163de0098099e46e44c566061f5) * Avoid using beforeSend as it might be set globally (adapted from 2dfe104f3915efa546c85ef277eb75c5abb9b49b) * Refactor triggering navigation to use constant/method instead of magic strings (part of 4843facd507b1cee8712bf10e96ff6c25c98bca2) * Switching to using jQueyr.prop instead of jQuery.attr * Fixed email notifications enable/disable (adapted from 8a7f022) * Load thread if requested thread isn't loaded yet (adapted from e28dde32169b388235030dba945366f4c13e565f) * Added ability to set custom css classes on search messages (adapted from b2e4c18905d7c7560838e264d6b218a3c4c6dc1f) * Actually rerendering view with new model data when it is loaded/changed --- .../common/js/discussion/discussion_router.js | 64 +++++++++++++---- common/static/common/js/discussion/main.js | 9 +-- common/static/common/js/discussion/utils.js | 35 ++++----- .../views/discussion_content_view.js | 22 +++--- .../views/discussion_thread_list_view.js | 71 +++++++++---------- .../views/discussion_thread_view.js | 1 + .../common/js/spec/discussion/utils_spec.js | 35 +++++++-- .../view/discussion_thread_list_view_spec.js | 32 ++++++++- .../discussion/view/new_post_view_spec.js | 3 +- .../view/response_comment_view_spec.js | 17 +++-- .../discussion/search-alert.underscore | 2 +- 11 files changed, 195 insertions(+), 96 deletions(-) diff --git a/common/static/common/js/discussion/discussion_router.js b/common/static/common/js/discussion/discussion_router.js index 14ce7a6bf4..81905c3ff4 100644 --- a/common/static/common/js/discussion/discussion_router.js +++ b/common/static/common/js/discussion/discussion_router.js @@ -1,4 +1,4 @@ -/* globals DiscussionThreadListView, DiscussionThreadView, DiscussionUtil, NewPostView */ +/* globals DiscussionThreadListView, DiscussionThreadView, DiscussionUtil, NewPostView, Thread */ (function() { 'use strict'; var __hasProp = {}.hasOwnProperty, @@ -18,9 +18,20 @@ return child; }; + function getSingleThreadRoute(commentable_id, thread_id) { + return commentable_id + "/threads/" + thread_id; + } + if (typeof Backbone !== "undefined" && Backbone !== null) { this.DiscussionRouter = (function(_super) { + var allThreadsRoute = "", + singleThreadRoute = getSingleThreadRoute(":forum_name", ":thread_id"), // :forum_name/threads/:thread_id + routes = {}; + + routes[allThreadsRoute] = "allThreads"; + routes[singleThreadRoute] = "showThread"; + __extends(DiscussionRouter, _super); function DiscussionRouter() { @@ -40,16 +51,16 @@ this.showMain = function() { return DiscussionRouter.prototype.showMain.apply(self, arguments); }; + this.renderThreadView = function() { + return DiscussionRouter.prototype.renderThreadView.apply(self, arguments); + }; this.setActiveThread = function() { return DiscussionRouter.prototype.setActiveThread.apply(self, arguments); }; return DiscussionRouter.__super__.constructor.apply(this, arguments); } - DiscussionRouter.prototype.routes = { - "": "allThreads", - ":forum_name/threads/:thread_id": "showThread" - }; + DiscussionRouter.prototype.routes = routes; DiscussionRouter.prototype.initialize = function(options) { var self = this; @@ -89,16 +100,44 @@ if (this.thread) { return this.nav.setActiveThread(this.thread.get("id")); } else { - return this.nav.goHome; + return this.nav.goHome(); } }; DiscussionRouter.prototype.showThread = function(forum_name, thread_id) { + var self; + self = this; this.thread = this.discussion.get(thread_id); + + if (this.thread) { + this.renderThreadView(); + return; + } + + // if thread is not loaded yet for some reason - try loading it + DiscussionUtil.safeAjax({ + url: DiscussionUtil.urlFor('retrieve_single_thread', forum_name, thread_id) + }).done(function(data) { + // if succeded - proceed normally + self.thread = new Thread(data.content); + self.discussion.add(self.thread); + self.renderThreadView(); + }).fail(function(xhr) { + // otherwise display error message and navigate to all threads view + var errorMessage = (xhr.status === 404) ? + gettext("The thread you selected has been deleted. Please select another thread.") : + gettext("We had some trouble loading more responses. Please try again."); + + DiscussionUtil.discussionAlert(gettext("Sorry"), errorMessage); + this.allThreads(); + }); + }; + + DiscussionRouter.prototype.renderThreadView = function() { this.thread.set("unread_comments_count", 0); this.thread.set("read", true); this.setActiveThread(); - return this.showMain(); + this.showMain(); }; DiscussionRouter.prototype.showMain = function() { @@ -127,17 +166,14 @@ }; DiscussionRouter.prototype.navigateToThread = function(thread_id) { - var thread; + var thread, targetThreadRoute; thread = this.discussion.get(thread_id); - return this.navigate("" + (thread.get("commentable_id")) + "/threads/" + thread_id, { - trigger: true - }); + targetThreadRoute = getSingleThreadRoute(thread.get("commentable_id"), thread_id); + this.navigate(targetThreadRoute, { trigger: true}); }; DiscussionRouter.prototype.navigateToAllThreads = function() { - return this.navigate("", { - trigger: true - }); + this.navigate(allThreadsRoute, { trigger: true }); }; DiscussionRouter.prototype.showNewPost = function() { diff --git a/common/static/common/js/discussion/main.js b/common/static/common/js/discussion/main.js index 866a3491d1..5f34c6af9d 100644 --- a/common/static/common/js/discussion/main.js +++ b/common/static/common/js/discussion/main.js @@ -34,10 +34,11 @@ course_settings: course_settings }); /* jshint +W031*/ - return Backbone.history.start({ - pushState: true, - root: "/courses/" + $$course_id + "/discussion/forum/" - }); + if (!Backbone.History.started) { + Backbone.history.start({pushState: true, root: "/courses/" + $$course_id + "/discussion/forum/"}); + } else { + Backbone.history.loadUrl(window.location.pathname); + } } }; DiscussionProfileApp = { diff --git a/common/static/common/js/discussion/utils.js b/common/static/common/js/discussion/utils.js index be3066b079..a0b1345ccb 100644 --- a/common/static/common/js/discussion/utils.js +++ b/common/static/common/js/discussion/utils.js @@ -184,9 +184,9 @@ DiscussionUtil.safeAjax = function(params) { var $elem, deferred, request, - self = this; + self = this, deferred; $elem = params.$elem; - if ($elem && $elem.attr("disabled")) { + if ($elem && $elem.prop("disabled")) { deferred = $.Deferred(); deferred.reject(); return deferred.promise(); @@ -194,18 +194,6 @@ params.url = URI(params.url).addSearch({ ajax: 1 }); - params.beforeSend = function() { - if ($elem) { - $elem.attr("disabled", "disabled"); - } - if (params.$loading) { - if (params.loadingCallback) { - return params.loadingCallback.apply(params.$loading); - } else { - return self.showLoadingIndicator($(params.$loading), params.takeFocus); - } - } - }; if (!params.error) { params.error = function() { self.discussionAlert( @@ -216,9 +204,21 @@ ); }; } + + if ($elem) { + $elem.prop("disabled", true); + } + if (params.$loading) { + if (params.loadingCallback) { + params.loadingCallback.apply(params.$loading); + } else { + self.showLoadingIndicator($(params.$loading), params.takeFocus); + } + } + request = $.ajax(params).always(function() { if ($elem) { - $elem.removeAttr("disabled"); + $elem.prop("disabled", false); } if (params.$loading) { if (params.loadedCallback) { @@ -231,7 +231,7 @@ return request; }; - DiscussionUtil.updateWithUndo = function(model, updates, safeAjaxParams, errorMsg) { + DiscussionUtil.updateWithUndo = function(model, updates, safeAjaxParams, errorMsg, beforeSend) { var undo, self = this; if (errorMsg) { @@ -241,6 +241,9 @@ } undo = _.pick(model.attributes, _.keys(updates)); model.set(updates); + if (typeof beforeSend === 'function') { + beforeSend(); + } return this.safeAjax(safeAjaxParams).fail(function() { return model.set(undo); }); diff --git a/common/static/common/js/discussion/views/discussion_content_view.js b/common/static/common/js/discussion/views/discussion_content_view.js index 4623a49c45..76e9d8517f 100644 --- a/common/static/common/js/discussion/views/discussion_content_view.js +++ b/common/static/common/js/discussion/views/discussion_content_view.js @@ -389,18 +389,18 @@ msg = gettext("We had some trouble removing this endorsement. Please try again."); } } - beforeFunc = function() { - return self.trigger("comment:endorse"); - }; - return DiscussionUtil.updateWithUndo(this.model, updates, { - url: url, - type: "POST", - data: { - endorsed: is_endorsing + return DiscussionUtil.updateWithUndo( + this.model, + updates, + { + url: url, + type: "POST", + data: { endorsed: is_endorsing }, + $elem: $(event.currentTarget) }, - beforeSend: beforeFunc, - $elem: $(event.currentTarget) - }, msg).always(this.trigger("comment:endorse")); + msg, + function() { return self.trigger("comment:endorse"); } + ).always(this.trigger("comment:endorse")); }; DiscussionContentShowView.prototype.toggleVote = function(event) { diff --git a/common/static/common/js/discussion/views/discussion_thread_list_view.js b/common/static/common/js/discussion/views/discussion_thread_list_view.js index 7fe06e33e6..69b794d4b4 100644 --- a/common/static/common/js/discussion/views/discussion_thread_list_view.js +++ b/common/static/common/js/discussion/views/discussion_thread_list_view.js @@ -123,6 +123,7 @@ return self.displayedCollection.reset(discussion.models); }); this.collection.on("add", this.addAndSelectThread); + this.collection.on("thread:remove", this.threadRemoved); this.sidebar_padding = 10; this.boardName = null; this.template = _.template($("#thread-list-template").html()); @@ -135,7 +136,8 @@ var content; content = _.template($("#search-alert-template").html())({ 'message': searchAlert.attributes.message, - 'cid': searchAlert.cid + 'cid': searchAlert.cid, + 'css_class': searchAlert.attributes.css_class }); self.$(".search-alerts").append(content); return self.$("#search-alert-" + searchAlert.cid + " a.dismiss") @@ -151,11 +153,19 @@ }); }; - DiscussionThreadListView.prototype.addSearchAlert = function(message) { + /** + * Creates search alert model and adds it to collection + * @param message - alert message + * @param css_class - Allows setting custom css class for a message. This can be used to style messages + * of different types differently (i.e. other background, completely hide, etc.) + * @returns {Backbone.Model} + */ + DiscussionThreadListView.prototype.addSearchAlert = function(message, css_class) { var m; - m = new Backbone.Model({ - "message": message - }); + if (typeof css_class === 'undefined' || css_class === null) { + css_class = ""; + } + m = new Backbone.Model({"message": message, "css_class": css_class}); this.searchAlertCollection.add(m); return m; }; @@ -392,8 +402,8 @@ return false; }; - DiscussionThreadListView.prototype.threadRemoved = function(thread_id) { - return this.trigger("thread:removed", thread_id); + DiscussionThreadListView.prototype.threadRemoved = function(thread) { + this.trigger("thread:removed", thread); }; DiscussionThreadListView.prototype.setActiveThread = function(thread_id) { @@ -406,7 +416,7 @@ }; DiscussionThreadListView.prototype.goHome = function() { - var thread_id, url; + var url; this.template = _.template($("#discussion-home-template").html()); $(".forum-content").html(this.template); $(".forum-nav-thread-list a").removeClass("is-active").find(".sr").remove(); @@ -416,19 +426,9 @@ url: url, type: "GET", success: function(response) { - if (response.status) { - return $('input.email-setting').attr('checked', 'checked'); - } else { - return $('input.email-setting').removeAttr('checked'); - } + $('input.email-setting').prop('checked', response.status); } }); - thread_id = null; - return this.trigger("thread:removed"); - /* - select all threads - */ - }; DiscussionThreadListView.prototype.isBrowseMenuVisible = function() { @@ -731,8 +731,7 @@ url: DiscussionUtil.urlFor("users"), type: "GET", dataType: 'json', - error: function() { - }, + error: function() {}, success: function(response) { var message; if (response.users.length > 0) { @@ -742,7 +741,7 @@ username: response.users[0].username }) }, true); - return self.addSearchAlert(message); + return self.addSearchAlert(message, 'search-by-user'); } } }); @@ -765,23 +764,17 @@ }; DiscussionThreadListView.prototype.updateEmailNotifications = function() { - if ($('input.email-setting').attr('checked')) { - return DiscussionUtil.safeAjax({ - url: DiscussionUtil.urlFor("enable_notifications"), - type: "POST", - error: function() { - return $('input.email-setting').removeAttr('checked'); - } - }); - } else { - return DiscussionUtil.safeAjax({ - url: DiscussionUtil.urlFor("disable_notifications"), - type: "POST", - error: function() { - return $('input.email-setting').attr('checked', 'checked'); - } - }); - } + var checkbox, checked, urlName; + checkbox = $('input.email-setting'); + checked = checkbox.prop('checked'); + urlName = (checked) ? "enable_notifications" : "disable_notifications"; + DiscussionUtil.safeAjax({ + url: DiscussionUtil.urlFor(urlName), + type: "POST", + error: function() { + checkbox.prop('checked', !checked); + } + }); }; return DiscussionThreadListView; diff --git a/common/static/common/js/discussion/views/discussion_thread_view.js b/common/static/common/js/discussion/views/discussion_thread_view.js index 2854690369..d3b255f8be 100644 --- a/common/static/common/js/discussion/views/discussion_thread_view.js +++ b/common/static/common/js/discussion/views/discussion_thread_view.js @@ -91,6 +91,7 @@ id = self.model.get("id"); if (collection.get(id)) { self.model = collection.get(id); + self.rerender(); } }); this.createShowView(); diff --git a/common/static/common/js/spec/discussion/utils_spec.js b/common/static/common/js/spec/discussion/utils_spec.js index 22793d3e7a..fec3a84016 100644 --- a/common/static/common/js/spec/discussion/utils_spec.js +++ b/common/static/common/js/spec/discussion/utils_spec.js @@ -3,9 +3,10 @@ 'use strict'; describe('DiscussionUtil', function() { beforeEach(function() { - return DiscussionSpecHelper.setUpGlobals(); + DiscussionSpecHelper.setUpGlobals(); }); - return describe("updateWithUndo", function() { + + describe("updateWithUndo", function() { it("calls through to safeAjax with correct params, and reverts the model in case of failure", function() { var deferred, model, res, updates; deferred = $.Deferred(); @@ -45,13 +46,13 @@ updates = { hello: "world" }; - $elem = jasmine.createSpyObj('$elem', ['attr']); - $elem.attr.and.returnValue(true); + $elem = jasmine.createSpyObj('$elem', ['prop']); + $elem.prop.and.returnValue(true); res = DiscussionUtil.updateWithUndo(model, updates, { foo: "bar", $elem: $elem }, "error message"); - expect($elem.attr).toHaveBeenCalledWith("disabled"); + expect($elem.prop).toHaveBeenCalledWith("disabled"); expect(DiscussionUtil.safeAjax).toHaveBeenCalled(); expect(model.attributes).toEqual({ hello: false, @@ -64,6 +65,30 @@ return expect(failed).toBe(true); }); }); + + describe('safeAjax', function() { + function dismissAlert() { + $(".modal#discussion-alert").remove(); + } + + it('respects global beforeSend', function() { + var beforeSendSpy = jasmine.createSpy(); + $.ajaxSetup({beforeSend: beforeSendSpy}); + + var $elem = jasmine.createSpyObj('$elem', ['prop']); + + DiscussionUtil.safeAjax({ + $elem: $elem, + url: "/", + type: "GET", + dataType: "json" + }).always(function() { + dismissAlert(); + }); + expect($elem.prop).toHaveBeenCalledWith("disabled", true); + expect(beforeSendSpy).toHaveBeenCalled(); + }); + }); }); }).call(this); diff --git a/common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js b/common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js index 596c2c4c5d..6aa0eaac32 100644 --- a/common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js +++ b/common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js @@ -261,7 +261,7 @@ }); }); describe("search alerts", function() { - var testAlertMessages; + var testAlertMessages, getAlertMessagesAndClasses; testAlertMessages = function(expectedMessages) { return expect($(".search-alert .message").map(function() { @@ -269,6 +269,15 @@ }).get()).toEqual(expectedMessages); }; + getAlertMessagesAndClasses = function () { + return $(".search-alert").map(function() { + return { + text: $('.message', this).html(), + css_class: $(this).attr('class') + }; + }).get(); + }; + it("renders and removes search alerts", function() { var bar, foo; testAlertMessages([]); @@ -282,6 +291,27 @@ return testAlertMessages([]); }); + it("renders search alert with custom class", function() { + var foo, messages; + testAlertMessages([]); + + this.view.addSearchAlert("foo", "custom-class"); + messages = getAlertMessagesAndClasses(); + expect(messages.length).toEqual(1); + expect(messages[0].text).toEqual("foo"); + expect(messages[0].css_class).toEqual("search-alert custom-class"); + + foo = this.view.addSearchAlert("bar", "other-class"); + + messages = getAlertMessagesAndClasses(); + expect(messages.length).toEqual(2); + expect(messages[0].text).toEqual("foo"); + expect(messages[0].css_class).toEqual("search-alert custom-class"); + expect(messages[1].text).toEqual("bar"); + expect(messages[1].css_class).toEqual("search-alert other-class"); + }); + + it("clears all search alerts", function() { this.view.addSearchAlert("foo"); this.view.addSearchAlert("bar"); diff --git a/common/static/common/js/spec/discussion/view/new_post_view_spec.js b/common/static/common/js/spec/discussion/view/new_post_view_spec.js index 34e06fa12a..2f902d6052 100644 --- a/common/static/common/js/spec/discussion/view/new_post_view_spec.js +++ b/common/static/common/js/spec/discussion/view/new_post_view_spec.js @@ -94,7 +94,7 @@ this.view.render(); expectedGroupId = null; DiscussionSpecHelper.makeAjaxSpy(function(params) { - return expect(params.data.group_id).toEqual(expectedGroupId); + expect(params.data.group_id).toEqual(expectedGroupId); }); return _.each(["1", "2", ""], function(groupIdStr) { expectedGroupId = groupIdStr; @@ -103,6 +103,7 @@ self.view.$(".js-post-body textarea").val("dummy body"); self.view.$(".forum-new-post-form").submit(); expect($.ajax).toHaveBeenCalled(); + self.view.$(".forum-new-post-form").prop("disabled", false); return $.ajax.calls.reset(); }); }); diff --git a/common/static/common/js/spec/discussion/view/response_comment_view_spec.js b/common/static/common/js/spec/discussion/view/response_comment_view_spec.js index cc926df2db..5fc66d1ae9 100644 --- a/common/static/common/js/spec/discussion/view/response_comment_view_spec.js +++ b/common/static/common/js/spec/discussion/view/response_comment_view_spec.js @@ -33,8 +33,10 @@ } }); this.event = DiscussionSpecHelper.makeEventSpy(); + this.event.target = $("body"); spyOn(this.comment, "remove"); - return spyOn(this.view.$el, "remove"); + spyOn(this.view.$el, "remove"); + $(this.event.target).prop("disabled", false); }); setAjaxResult = function(isSuccess) { return spyOn($, "ajax").and.callFake(function(params) { @@ -151,7 +153,7 @@ this.view.$el.find(".edit-comment-body").html($("")); this.view.$el.find(".edit-comment-body textarea").val(this.updatedBody); spyOn(this.view, 'cancelEdit'); - return spyOn($, "ajax").and.callFake(function(params) { + spyOn($, "ajax").and.callFake(function(params) { if (self.ajaxSucceed) { params.success(); } else { @@ -164,10 +166,17 @@ } }; }); + + this.event = DiscussionSpecHelper.makeEventSpy(); + // All the way down in discussion/utils.js there's this line + // element.after(...); + // element is event.target in this case. This causes a JS exception, so we override the target + this.event.target = $("body"); + $(this.event.target).prop("disabled", false); }); it('calls the update endpoint correctly and displays the show view on success', function() { this.ajaxSucceed = true; - this.view.update(DiscussionSpecHelper.makeEventSpy()); + this.view.update(this.event); expect($.ajax).toHaveBeenCalled(); expect($.ajax.calls.mostRecent().args[0].url._parts.path) .toEqual('/courses/edX/999/test/discussion/comments/01234567/update'); @@ -179,7 +188,7 @@ var originalBody; originalBody = this.comment.get("body"); this.ajaxSucceed = false; - this.view.update(DiscussionSpecHelper.makeEventSpy()); + this.view.update(this.event); expect($.ajax).toHaveBeenCalled(); expect($.ajax.calls.mostRecent().args[0].url._parts.path) .toEqual('/courses/edX/999/test/discussion/comments/01234567/update'); diff --git a/common/static/common/templates/discussion/search-alert.underscore b/common/static/common/templates/discussion/search-alert.underscore index 74e908b9ee..1ba3ee2ab5 100644 --- a/common/static/common/templates/discussion/search-alert.underscore +++ b/common/static/common/templates/discussion/search-alert.underscore @@ -1,4 +1,4 @@ -
+

<%= message %>