Merge pull request #5577 from edx/jsa/tnl549

Fix topic-related errors when editing discussion threads in inline mode.

common/static/coffee/spec/discussion/discussion_spec_helper.coffee

@@ -19,6 +19,23 @@ class @DiscussionSpecHelper
                 {always: ->}
         )
 
+    @makeEventSpy = () ->
+        jasmine.createSpyObj('event', ['preventDefault', 'target'])
+
+    @makeCourseSettings = (is_cohorted=true) ->
+        new DiscussionCourseSettings(
+            category_map:
+                children: ['Test Topic', 'Other Topic']
+                entries:
+                    'Test Topic':
+                        is_cohorted: is_cohorted
+                        id: 'test_topic'
+                    'Other Topic':
+                        is_cohorted: is_cohorted
+                        id: 'other_topic'
+            is_cohorted: is_cohorted
+        )
+
     @setUnderscoreFixtures = ->
         for templateName in ['thread-show']
             templateFixture = readFixtures('templates/discussion/' + templateName + '.underscore')

common/static/coffee/spec/discussion/view/discussion_thread_edit_view_spec.js

 (function() {
     'use strict';
     describe('DiscussionThreadEditView', function() {
+        var testUpdate, testCancel;
+
         beforeEach(function() {
             DiscussionSpecHelper.setUpGlobals();
             DiscussionSpecHelper.setUnderscoreFixtures();
             spyOn(DiscussionUtil, 'makeWmdEditor');
-            this.threadData = DiscussionViewSpecHelper.makeThreadWithProps();
-            this.thread = new Thread(this.threadData);
-            this.course_settings = new DiscussionCourseSettings({
-                'category_map': {
-                    'children': ['Topic'],
-                    'entries': {
-                        'Topic': {
-                            'is_cohorted': true,
-                            'id': 'topic'
-                        }
-                    }
-                },
-                'is_cohorted': true
+            this.threadData = DiscussionViewSpecHelper.makeThreadWithProps({
+                'commentable_id': 'test_topic',
+                'title': 'test thread title'
             });
+            this.thread = new Thread(this.threadData);
+            this.course_settings = DiscussionSpecHelper.makeCourseSettings();
 
             this.createEditView = function (options) {
-              options = _.extend({
+                options = _.extend({
                     container: $('#fixture-element'),
                     model: this.thread,
                     mode: 'tab',
-                    topicId: 'dummy_id',
-                    threadType: 'question',
                     course_settings: this.course_settings
                 }, options);
                 this.view = new DiscussionThreadEditView(options);
@@ -34,36 +26,58 @@
             };
         });
 
-        it('can save new data correctly', function() {
-            var view;
+        testUpdate = function(view, thread) {
             spyOn($, 'ajax').andCallFake(function(params) {
                 expect(params.url.path()).toEqual(DiscussionUtil.urlFor('update_thread', 'dummy_id'));
-                expect(params.data.thread_type).toBe('discussion');
-                expect(params.data.commentable_id).toBe('topic');
-                expect(params.data.title).toBe('new_title');
+                if (view.isTabMode()) {
+                    // TODO remove the tabMode condition, depends on #5554 / TNL-606
+                    expect(params.data.thread_type).toBe('discussion');
+                }
+                expect(params.data.commentable_id).toBe('other_topic');
+                expect(params.data.title).toBe('changed thread title');
                 params.success();
                 return {always: function() {}};
             });
-            this.createEditView();
-            this.view.$el.find('a.topic-title').first().click(); // set new topic
-            this.view.$('.edit-post-title').val('new_title'); // set new title
-            this.view.$("label[for$='post-type-discussion']").click(); // set new thread type
-            this.view.$('.post-update').click();
+            view.$el.find('a.topic-title')[1].click(); // set new topic
+            view.$('.edit-post-title').val('changed thread title'); // set new title
+            view.$("label[for$='post-type-discussion']").click(); // set new thread type
+            view.$('.post-update').click();
             expect($.ajax).toHaveBeenCalled();
 
-            expect(this.thread.get('title')).toBe('new_title');
-            expect(this.thread.get('commentable_id')).toBe('topic');
-            expect(this.thread.get('thread_type')).toBe('discussion');
-            expect(this.thread.get('courseware_title')).toBe('Topic');
+            expect(thread.get('title')).toBe('changed thread title');
+            if (view.isTabMode()) {
+                // TODO remove the tabMode condition, depends on #5554 / TNL-606
+                expect(thread.get('thread_type')).toBe('discussion');
+            }
+            expect(thread.get('commentable_id')).toBe('other_topic');
+            expect(thread.get('courseware_title')).toBe('Other Topic');
+            expect(view.$('.edit-post-title')).toHaveValue('');
+            expect(view.$('.wmd-preview p')).toHaveText('');
+        }
 
-            expect(this.view.$('.edit-post-title')).toHaveValue('');
-            expect(this.view.$('.wmd-preview p')).toHaveText('');
+        it('can save new data correctly in tab mode', function() {
+            this.createEditView();
+            testUpdate(this.view, this.thread);
         });
 
-        it('can close the view', function() {
-            this.createEditView();
-            this.view.$('.post-cancel').click();
+        it('can save new data correctly in inline mode', function() {
+            this.createEditView({"mode": "inline"});
+            testUpdate(this.view, this.thread);
+        });
+
+        testCancel = function(view) {
+            view.$('.post-cancel').click();
             expect($('.edit-post-form')).not.toExist();
+        }
+
+        it('can close the view in tab mode', function() {
+            this.createEditView();
+            testCancel(this.view);
+        });
+
+        it('can close the view in inline mode', function() {
+            this.createEditView({"mode": "inline"});
+            testCancel(this.view);
         });
     });
 }).call(this);

common/static/coffee/spec/discussion/view/discussion_thread_view_spec.coffee

@@ -11,6 +11,7 @@ describe "DiscussionThreadView", ->
         # Avoid unnecessary boilerplate
         spyOn(DiscussionThreadShowView.prototype, "convertMath")
         spyOn(DiscussionContentView.prototype, "makeWmdEditor")
+        spyOn(DiscussionUtil, "makeWmdEditor")
         spyOn(ThreadResponseView.prototype, "renderShowView")
 
     renderWithContent = (view, content) ->
@@ -46,7 +47,12 @@ describe "DiscussionThreadView", ->
             threadData = DiscussionViewSpecHelper.makeThreadWithProps({closed: originallyClosed})
             thread = new Thread(threadData)
             discussion = new Discussion(thread)
-            view = new DiscussionThreadView({ model: thread, el: $("#fixture-element"), mode: mode})
+            view = new DiscussionThreadView(
+                model: thread
+                el: $("#fixture-element")
+                mode: mode
+                course_settings: DiscussionSpecHelper.makeCourseSettings()
+            )
             renderWithContent(view, {resp_total: 1, children: [{}]})
             if mode == "inline"
               view.expand()
@@ -70,7 +76,12 @@ describe "DiscussionThreadView", ->
 
     describe "tab mode", ->
         beforeEach ->
-            @view = new DiscussionThreadView({ model: @thread, el: $("#fixture-element"), mode: "tab"})
+            @view = new DiscussionThreadView(
+                model: @thread
+                el: $("#fixture-element")
+                mode: "tab"
+                course_settings: DiscussionSpecHelper.makeCourseSettings()
+            )
 
         describe "response count and pagination", ->
             it "correctly render for a thread with no responses", ->
@@ -111,7 +122,12 @@ describe "DiscussionThreadView", ->
 
     describe "inline mode", ->
         beforeEach ->
-            @view = new DiscussionThreadView({ model: @thread, el: $("#fixture-element"), mode: "inline"})
+            @view = new DiscussionThreadView(
+                model: @thread
+                el: $("#fixture-element")
+                mode: "inline"
+                course_settings: DiscussionSpecHelper.makeCourseSettings()
+            )
 
         describe "render", ->
             it "shows content that should be visible when collapsed", ->
@@ -178,11 +194,26 @@ describe "DiscussionThreadView", ->
                 expect($(".post-body").text()).toEqual(maliciousAbbreviation)
                 expect($(".post-body").html()).not.toContain("<script")
 
+            it "re-renders the show view correctly when leaving the edit view", ->
+                DiscussionViewSpecHelper.setNextResponseContent({resp_total: 0, children: []})
+                @view.render()
+                @view.expand()
+                assertExpandedContentVisible(@view, true)
+                @view.edit()
+                assertContentVisible(@view, ".edit-post-body", true)
+                expect(@view.$el.find(".post-actions-list").length).toBe(0)
+                @view.closeEditView(DiscussionSpecHelper.makeEventSpy())
+                expect(@view.$el.find(".edit-post-body").length).toBe(0)
+                assertContentVisible(@view, ".post-actions-list", true)
+
     describe "for question threads", ->
         beforeEach ->
             @thread.set("thread_type", "question")
             @view = new DiscussionThreadView(
-                {model: @thread, el: $("#fixture-element"), mode: "tab"}
+                model: @thread
+                el: $("#fixture-element")
+                mode: "tab"
+                course_settings: DiscussionSpecHelper.makeCourseSettings()
             )
 
         renderTestCase = (view, numEndorsed, numNonEndorsed) ->

common/static/coffee/spec/discussion/view/response_comment_view_spec.coffee

@@ -17,12 +17,10 @@ describe 'ResponseCommentView', ->
         spyOn(DiscussionUtil, "makeWmdEditor")
         @view.render()
 
-    makeEventSpy = () -> jasmine.createSpyObj('event', ['preventDefault', 'target'])
-
     describe '_delete', ->
         beforeEach ->
             @comment.updateInfo {ability: {can_delete: true}}
-            @event = makeEventSpy()
+            @event = DiscussionSpecHelper.makeEventSpy()
             spyOn(@comment, "remove")
             spyOn(@view.$el, "remove")
 
@@ -81,9 +79,9 @@ describe 'ResponseCommentView', ->
             # Without calling renderEditView first, renderShowView is a no-op
             @view.renderEditView()
             @view.renderShowView()
-            @view.showView.trigger "comment:_delete", makeEventSpy()
+            @view.showView.trigger "comment:_delete", DiscussionSpecHelper.makeEventSpy()
             expect(@view._delete).toHaveBeenCalled()
-            @view.showView.trigger "comment:edit", makeEventSpy()
+            @view.showView.trigger "comment:edit", DiscussionSpecHelper.makeEventSpy()
             expect(@view.edit).toHaveBeenCalled()
             expect(@view.$(".edit-post-form#comment_#{@comment.id}")).not.toHaveClass("edit-post-form")
 
@@ -92,9 +90,9 @@ describe 'ResponseCommentView', ->
             spyOn(@view, "update")
             spyOn(@view, "cancelEdit")
             @view.renderEditView()
-            @view.editView.trigger "comment:update", makeEventSpy()
+            @view.editView.trigger "comment:update", DiscussionSpecHelper.makeEventSpy()
             expect(@view.update).toHaveBeenCalled()
-            @view.editView.trigger "comment:cancel_edit", makeEventSpy()
+            @view.editView.trigger "comment:cancel_edit", DiscussionSpecHelper.makeEventSpy()
             expect(@view.cancelEdit).toHaveBeenCalled()
             expect(@view.$(".edit-post-form#comment_#{@comment.id}")).toHaveClass("edit-post-form")
 
@@ -138,7 +136,7 @@ describe 'ResponseCommentView', ->
 
             it 'calls the update endpoint correctly and displays the show view on success', ->
                 @ajaxSucceed = true
-                @view.update(makeEventSpy())
+                @view.update(DiscussionSpecHelper.makeEventSpy())
                 expect($.ajax).toHaveBeenCalled()
                 expect($.ajax.mostRecentCall.args[0].url._parts.path).toEqual('/courses/edX/999/test/discussion/comments/01234567/update')
                 expect($.ajax.mostRecentCall.args[0].data.body).toEqual(@updatedBody)
@@ -148,7 +146,7 @@ describe 'ResponseCommentView', ->
             it 'handles AJAX errors', ->
                 originalBody = @comment.get("body")
                 @ajaxSucceed = false
-                @view.update(makeEventSpy())
+                @view.update(DiscussionSpecHelper.makeEventSpy())
                 expect($.ajax).toHaveBeenCalled()
                 expect($.ajax.mostRecentCall.args[0].url._parts.path).toEqual('/courses/edX/999/test/discussion/comments/01234567/update')
                 expect($.ajax.mostRecentCall.args[0].data.body).toEqual(@updatedBody)

common/static/coffee/src/discussion/discussion_module_view.coffee

@@ -98,7 +98,7 @@ if Backbone?
         @$el.append($discussion)
 
       @newPostForm = $('.new-post-article')
-      @threadviews = @discussion.map (thread) ->
+      @threadviews = @discussion.map (thread) =>
         new DiscussionThreadView(
           el: @$("article#thread_#{thread.id}"),
           model: thread,

common/static/coffee/src/discussion/views/discussion_thread_edit_view.js

@@ -17,7 +17,7 @@
                 this.mode = options.mode || 'inline';
                 this.course_settings = options.course_settings;
                 this.threadType = this.model.get('thread_type');
-                this.topicId = options.topicId;
+                this.topicId = this.model.get('commentable_id');
                 _.bindAll(this);
                 return this;
             },
@@ -32,12 +32,12 @@
                     threadTypeTemplate = _.template($("#thread-type-template").html());
                     this.addField(threadTypeTemplate({form_id: formId}));
                     this.$("#" + formId + "-post-type-" + this.threadType).attr('checked', true);
-                    this.topicView = new DiscussionTopicMenuView({
-                        topicId: this.topicId,
-                        course_settings: this.course_settings
-                    });
-                    this.addField(this.topicView.render());
                 }
+                this.topicView = new DiscussionTopicMenuView({
+                    topicId: this.topicId,
+                    course_settings: this.course_settings
+                });
+                this.addField(this.topicView.render());
                 DiscussionUtil.makeWmdEditor(this.$el, $.proxy(this.$, this), 'edit-post-body');
                 return this;
             },
@@ -55,7 +55,7 @@
                 var title = this.$('.edit-post-title').val(),
                     threadType = this.$(".post-type-input:checked").val(),
                     body = this.$('.edit-post-body textarea').val(),
-                    commentableId = this.isTabMode() ? this.topicView.getCurrentTopicId() : null;
+                    commentableId = this.topicView.getCurrentTopicId();
 
                 return DiscussionUtil.safeAjax({
                     $elem: this.submitBtn,
@@ -74,19 +74,15 @@
                     success: function() {
                         var newAttrs = {
                             title: title,
-                            body: body
+                            body: body,
+                            thread_type: threadType,
+                            commentable_id: commentableId,
+                            courseware_title: this.topicView.getFullTopicName()
                         };
                         // @TODO: Move this out of the callback, this makes it feel sluggish
                         this.$('.edit-post-title').val('').attr('prev-text', '');
                         this.$('.edit-post-body textarea').val('').attr('prev-text', '');
                         this.$('.wmd-preview p').html('');
-                        if (this.isTabMode()) {
-                            _.extend(newAttrs, {
-                                thread_type: threadType,
-                                commentable_id: commentableId,
-                                courseware_title: this.topicView.getFullTopicName()
-                            });
-                        }
                         this.model.set(newAttrs).unset('abbreviatedBody');
                         this.trigger('thread:updated');
                         if (this.threadType !== threadType) {

common/static/coffee/src/discussion/views/discussion_thread_view.coffee

@@ -272,7 +272,6 @@ if Backbone?
         model: @model
         mode: @mode
         course_settings: @options.course_settings
-        topicId: @model.get('commentable_id')
       )
       @editView.bind "thread:updated thread:cancel_edit", @closeEditView
       @editView.bind "comment:endorse", @endorseThread
@@ -296,6 +295,9 @@ if Backbone?
     closeEditView: (event) =>
       @createShowView()
       @renderShowView()
+      # next call is necessary to re-render the post action controls after
+      # submitting or cancelling a thread edit in inline mode.
+      @$el.find(".post-extended-content").show()
 
     # If you use "delete" here, it will compile down into JS that includes the
     # use of DiscussionThreadView.prototype.delete, and that will break IE8

lms/djangoapps/django_comment_client/forum/views.py

@@ -40,7 +40,7 @@ def _attr_safe_json(obj):
     return saxutils.escape(json.dumps(obj), {'"': '"'})
 
 @newrelic.agent.function_trace()
-def make_course_settings(course, include_category_map=False):
+def make_course_settings(course):
     """
     Generate a JSON-serializable model for course settings, which will be used to initialize a
     DiscussionCourseSettings object on the client.
@@ -51,11 +51,9 @@ def make_course_settings(course, include_category_map=False):
         'allow_anonymous': course.allow_anonymous,
         'allow_anonymous_to_peers': course.allow_anonymous_to_peers,
         'cohorts': [{"id": str(g.id), "name": g.name} for g in get_course_cohorts(course)],
+        'category_map': utils.get_discussion_category_map(course)
     }
 
-    if include_category_map:
-        obj['category_map'] = utils.get_discussion_category_map(course)
-
     return obj
 
 @newrelic.agent.function_trace()
@@ -167,7 +165,7 @@ def forum_form_discussion(request, course_id):
     nr_transaction = newrelic.agent.current_transaction()
 
     course = get_course_with_access(request.user, 'load_forum', course_key, check_if_enrolled=True)
-    course_settings = make_course_settings(course, include_category_map=True)
+    course_settings = make_course_settings(course)
 
     user = cc.User.from_django_user(request.user)
     user_info = user.to_dict()
@@ -231,7 +229,7 @@ def single_thread(request, course_id, discussion_id, thread_id):
     nr_transaction = newrelic.agent.current_transaction()
 
     course = get_course_with_access(request.user, 'load_forum', course_key)
-    course_settings = make_course_settings(course, include_category_map=True)
+    course_settings = make_course_settings(course)
     cc_user = cc.User.from_django_user(request.user)
     user_info = cc_user.to_dict()
     is_moderator = cached_has_permission(request.user, "see_all_cohorts", course_key)

lms/static/sass/_shame.scss

@@ -262,3 +262,19 @@ footer .references {
 .dashboard {
   padding-top: 60px;
 }
+
+// ====================
+
+// poor definition/scope on ul elements inside .vert-mod element in courseware - override needed for inline discussion editing
+.course-content .discussion-post.edit-post-form .topic-menu {
+  padding-left: 0;
+  list-style: none;
+
+  .topic-menu-item {
+    margin-bottom: 0;
+  }
+}
+
+.course-content .discussion-post.edit-post-form .topic-submenu {
+  list-style: none;
+}