diff --git a/common/static/coffee/spec/discussion/content_spec.coffee b/common/static/coffee/spec/discussion/content_spec.coffee index 8f03349393..77b8fd41e2 100644 --- a/common/static/coffee/spec/discussion/content_spec.coffee +++ b/common/static/coffee/spec/discussion/content_spec.coffee @@ -41,11 +41,11 @@ describe 'All Content', -> it 'can update info', -> @content.updateInfo { - ability: 'can_endorse', + ability: {'can_edit': true}, voted: true, subscribed: true } - expect(@content.get 'ability').toEqual 'can_endorse' + expect(@content.get 'ability').toEqual {'can_edit': true} expect(@content.get 'voted').toEqual true expect(@content.get 'subscribed').toEqual true @@ -77,3 +77,39 @@ describe 'All Content', -> myComments = new Comments myComments.add @comment1 expect(myComments.find('123')).toBe @comment1 + + it 'can be endorsed', -> + + DiscussionUtil.loadRoles( + {"Moderator": [111], "Administrator": [222], "Community TA": [333]} + ) + @discussionThread = new Thread({id: 1, thread_type: "discussion", user_id: 99}) + @discussionResponse = new Comment({id: 1, thread: @discussionThread}) + @questionThread = new Thread({id: 1, thread_type: "question", user_id: 99}) + @questionResponse = new Comment({id: 1, thread: @questionThread}) + + # mod + window.user = new DiscussionUser({id: 111}) + expect(@discussionResponse.canBeEndorsed()).toBe(true) + expect(@questionResponse.canBeEndorsed()).toBe(true) + + # admin + window.user = new DiscussionUser({id: 222}) + expect(@discussionResponse.canBeEndorsed()).toBe(true) + expect(@questionResponse.canBeEndorsed()).toBe(true) + + # TA + window.user = new DiscussionUser({id: 333}) + expect(@discussionResponse.canBeEndorsed()).toBe(true) + expect(@questionResponse.canBeEndorsed()).toBe(true) + + # thread author + window.user = new DiscussionUser({id: 99}) + expect(@discussionResponse.canBeEndorsed()).toBe(false) + expect(@questionResponse.canBeEndorsed()).toBe(true) + + # anyone else + window.user = new DiscussionUser({id: 999}) + expect(@discussionResponse.canBeEndorsed()).toBe(false) + expect(@questionResponse.canBeEndorsed()).toBe(false) + diff --git a/common/static/coffee/spec/discussion/view/thread_response_show_view_spec.coffee b/common/static/coffee/spec/discussion/view/thread_response_show_view_spec.coffee index 9d132da7e3..786713c607 100644 --- a/common/static/coffee/spec/discussion/view/thread_response_show_view_spec.coffee +++ b/common/static/coffee/spec/discussion/view/thread_response_show_view_spec.coffee @@ -41,6 +41,7 @@ describe "ThreadResponseShowView", -> course_id: "TestOrg/TestCourse/TestRun", body: "this is a comment", created_at: "2013-04-03T20:08:39Z", + endorsed: false, abuse_flaggers: [], votes: {up_count: "42"} } @@ -99,8 +100,8 @@ describe "ThreadResponseShowView", -> expect(@view.$(".posted-details").text()).not.toMatch(" by ") it "re-renders correctly when endorsement changes", -> + DiscussionUtil.loadRoles({"Moderator": [parseInt(window.user.id)]}) @thread.set("thread_type", "question") - @comment.updateInfo({"ability": {"can_endorse": true}}) expect(@view.$(".posted-details").text()).not.toMatch("marked as answer") @view.$(".action-endorse").click() expect(@view.$(".posted-details").text()).toMatch( @@ -108,3 +109,56 @@ describe "ThreadResponseShowView", -> ) @view.$(".action-endorse").click() expect(@view.$(".posted-details").text()).not.toMatch("marked as answer") + + it "allows a moderator to mark an answer in a question thread", -> + DiscussionUtil.loadRoles({"Moderator": parseInt(window.user.id)}) + @thread.set({ + "thread_type": "question", + "user_id": (parseInt(window.user.id) + 1).toString() + }) + @view.render() + endorseButton = @view.$(".action-endorse") + expect(endorseButton.length).toEqual(1) + expect(endorseButton).not.toHaveCss({"display": "none"}) + expect(endorseButton).toHaveClass("is-clickable") + endorseButton.click() + expect(endorseButton).toHaveClass("is-endorsed") + + it "allows the author of a question thread to mark an answer", -> + @thread.set({ + "thread_type": "question", + "user_id": window.user.id + }) + @view.render() + endorseButton = @view.$(".action-endorse") + expect(endorseButton.length).toEqual(1) + expect(endorseButton).not.toHaveCss({"display": "none"}) + expect(endorseButton).toHaveClass("is-clickable") + endorseButton.click() + expect(endorseButton).toHaveClass("is-endorsed") + + it "does not allow the author of a discussion thread to endorse", -> + @thread.set({ + "thread_type": "discussion", + "user_id": window.user.id + }) + @view.render() + endorseButton = @view.$(".action-endorse") + expect(endorseButton.length).toEqual(1) + expect(endorseButton).toHaveCss({"display": "none"}) + expect(endorseButton).not.toHaveClass("is-clickable") + endorseButton.click() + expect(endorseButton).not.toHaveClass("is-endorsed") + + it "does not allow a student who is not the author of a question thread to mark an answer", -> + @thread.set({ + "thread_type": "question", + "user_id": (parseInt(window.user.id) + 1).toString() + }) + @view.render() + endorseButton = @view.$(".action-endorse") + expect(endorseButton.length).toEqual(1) + expect(endorseButton).toHaveCss({"display": "none"}) + expect(endorseButton).not.toHaveClass("is-clickable") + endorseButton.click() + expect(endorseButton).not.toHaveClass("is-endorsed") diff --git a/common/static/coffee/src/discussion/content.coffee b/common/static/coffee/src/discussion/content.coffee index c89f5e1d2e..56867ba877 100644 --- a/common/static/coffee/src/discussion/content.coffee +++ b/common/static/coffee/src/discussion/content.coffee @@ -9,7 +9,6 @@ if Backbone? actions: editable: '.admin-edit' can_reply: '.discussion-reply' - can_endorse: '.admin-endorse' can_delete: '.admin-delete' can_openclose: '.admin-openclose' @@ -21,6 +20,9 @@ if Backbone? can: (action) -> (@get('ability') || {})[action] + # Default implementation + canBeEndorsed: -> false + updateInfo: (info) -> if info @set('ability', info.ability) @@ -187,6 +189,13 @@ if Backbone? count += comment.getCommentsCount() + 1 count + canBeEndorsed: => + user_id = window.user.get("id") + user_id && ( + DiscussionUtil.isPrivilegedUser(user_id) || + (@get('thread').get('thread_type') == 'question' && @get('thread').get('user_id') == user_id) + ) + class @Comments extends Backbone.Collection model: Comment diff --git a/common/static/coffee/src/discussion/utils.coffee b/common/static/coffee/src/discussion/utils.coffee index 157852a8f6..b4ef80a7b1 100644 --- a/common/static/coffee/src/discussion/utils.coffee +++ b/common/static/coffee/src/discussion/utils.coffee @@ -41,6 +41,9 @@ class @DiscussionUtil ta = _.union(@roleIds['Community TA']) _.include(ta, parseInt(user_id)) + @isPrivilegedUser: (user_id) -> + @isStaff(user_id) || @isTA(user_id) + @bulkUpdateContentInfo: (infos) -> for id, info of infos Content.getContent(id).updateInfo(info) diff --git a/common/static/coffee/src/discussion/views/discussion_content_view.coffee b/common/static/coffee/src/discussion/views/discussion_content_view.coffee index 09e07564c8..ec555d500b 100644 --- a/common/static/coffee/src/discussion/views/discussion_content_view.coffee +++ b/common/static/coffee/src/discussion/views/discussion_content_view.coffee @@ -46,15 +46,6 @@ if Backbone? can_delete: enable: -> @$(".action-delete").closest("li").show() disable: -> @$(".action-delete").closest("li").hide() - can_endorse: - enable: -> - @$(".action-endorse").show().css("cursor", "auto") - disable: -> - @$(".action-endorse").css("cursor", "default") - if not @model.get('endorsed') - @$(".action-endorse").hide() - else - @$(".action-endorse").show() can_openclose: enable: -> @$(".action-openclose").closest("li").show() disable: -> @$(".action-openclose").closest("li").hide() diff --git a/common/static/coffee/src/discussion/views/thread_response_show_view.coffee b/common/static/coffee/src/discussion/views/thread_response_show_view.coffee index 8435cb4171..79cac11ed1 100644 --- a/common/static/coffee/src/discussion/views/thread_response_show_view.coffee +++ b/common/static/coffee/src/discussion/views/thread_response_show_view.coffee @@ -14,14 +14,10 @@ if Backbone? attrRenderer: $.extend({}, DiscussionContentView.prototype.attrRenderer, { endorsed: (endorsed) -> - if endorsed - @$(".action-endorse").show().addClass("is-endorsed") - else - if @model.get('ability')?.can_endorse - @$(".action-endorse").show() - else - @$(".action-endorse").hide() - @$(".action-endorse").removeClass("is-endorsed") + $endorseButton = @$(".action-endorse") + $endorseButton.toggleClass("is-clickable", @model.canBeEndorsed()) + $endorseButton.toggleClass("is-endorsed", endorsed) + $endorseButton.toggle(endorsed || @model.canBeEndorsed()) }) $: (selector) -> @@ -67,7 +63,7 @@ if Backbone? toggleEndorse: (event) -> event.preventDefault() - if not @model.can('can_endorse') + if not @model.canBeEndorsed() return $elem = $(event.target) url = @model.urlFor('endorse') diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index f90369941c..03a3b4025e 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/lms/djangoapps/django_comment_client/base/tests.py @@ -6,7 +6,7 @@ from django.test.utils import override_settings from django.contrib.auth.models import User from django.core.management import call_command from django.core.urlresolvers import reverse -from mock import patch, ANY +from mock import patch, ANY, Mock from nose.tools import assert_true, assert_equal # pylint: disable=E0611 from opaque_keys.edx.locations import SlashSeparatedCourseKey @@ -26,9 +26,11 @@ CS_PREFIX = "http://localhost:4567/api/v1" class MockRequestSetupMixin(object): + def _create_repsonse_mock(self, data): + return Mock(text=json.dumps(data), json=Mock(return_value=data))\ + def _set_mock_request_data(self, mock_request, data): - mock_request.return_value.text = json.dumps(data) - mock_request.return_value.json.return_value = data + mock_request.return_value = self._create_repsonse_mock(data) @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) @@ -620,6 +622,53 @@ class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSet ) self.assertEqual(response.status_code, 200) + def _set_mock_request_thread_and_comment(self, mock_request, thread_data, comment_data): + def handle_request(*args, **kwargs): + url = args[1] + if "/threads/" in url: + return self._create_repsonse_mock(thread_data) + elif "/comments/" in url: + return self._create_repsonse_mock(comment_data) + else: + raise ArgumentError("Bad url to mock request") + mock_request.side_effect = handle_request + + def test_endorse_response_as_staff(self, mock_request): + self._set_mock_request_thread_and_comment( + mock_request, + {"type": "thread", "thread_type": "question", "user_id": str(self.student.id)}, + {"type": "comment", "thread_id": "dummy"} + ) + self.client.login(username=self.moderator.username, password=self.password) + response = self.client.post( + reverse("endorse_comment", kwargs={"course_id": self.course.id.to_deprecated_string(), "comment_id": "dummy"}) + ) + self.assertEqual(response.status_code, 200) + + def test_endorse_response_as_student(self, mock_request): + self._set_mock_request_thread_and_comment( + mock_request, + {"type": "thread", "thread_type": "question", "user_id": str(self.moderator.id)}, + {"type": "comment", "thread_id": "dummy"} + ) + self.client.login(username=self.student.username, password=self.password) + response = self.client.post( + reverse("endorse_comment", kwargs={"course_id": self.course.id.to_deprecated_string(), "comment_id": "dummy"}) + ) + self.assertEqual(response.status_code, 401) + + def test_endorse_response_as_student_question_author(self, mock_request): + self._set_mock_request_thread_and_comment( + mock_request, + {"type": "thread", "thread_type": "question", "user_id": str(self.student.id)}, + {"type": "comment", "thread_id": "dummy"} + ) + self.client.login(username=self.student.username, password=self.password) + response = self.client.post( + reverse("endorse_comment", kwargs={"course_id": self.course.id.to_deprecated_string(), "comment_id": "dummy"}) + ) + self.assertEqual(response.status_code, 200) + @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) class CreateThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockRequestSetupMixin): diff --git a/lms/djangoapps/django_comment_client/permissions.py b/lms/djangoapps/django_comment_client/permissions.py index 17de48b198..0826e3d469 100644 --- a/lms/djangoapps/django_comment_client/permissions.py +++ b/lms/djangoapps/django_comment_client/permissions.py @@ -5,6 +5,7 @@ Module for checking permissions with the comment_client backend import logging from types import NoneType from django.core import cache +from lms.lib.comment_client import Thread from opaque_keys.edx.keys import CourseKey CACHE = cache.get_cache('default') @@ -34,7 +35,7 @@ def has_permission(user, permission, course_id=None): return False -CONDITIONS = ['is_open', 'is_author'] +CONDITIONS = ['is_open', 'is_author', 'is_question_author'] def _check_condition(user, condition, content): @@ -50,9 +51,22 @@ def _check_condition(user, condition, content): except KeyError: return False + def check_question_author(user, content): + if not content: + return False + try: + if content["type"] == "thread": + return content["thread_type"] == "question" and content["user_id"] == str(user.id) + else: + # N.B. This will trigger a comments service query + return check_question_author(user, Thread(id=content["thread_id"]).to_dict()) + except KeyError: + return False + handlers = { 'is_open': check_open, 'is_author': check_author, + 'is_question_author': check_question_author, } return handlers[condition](user, content) @@ -85,7 +99,7 @@ VIEW_PERMISSIONS = { 'create_comment': [["create_comment", "is_open"]], 'delete_thread': ['delete_thread', ['update_thread', 'is_author']], 'update_comment': ['edit_content', ['update_comment', 'is_open', 'is_author']], - 'endorse_comment': ['endorse_comment'], + 'endorse_comment': ['endorse_comment', 'is_question_author'], 'openclose_thread': ['openclose_thread'], 'create_sub_comment': [['create_sub_comment', 'is_open']], 'delete_comment': ['delete_comment', ['update_comment', 'is_open', 'is_author']], diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index b31caadf25..b18b4b820e 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -258,7 +258,6 @@ def get_ability(course_id, content, user): return { 'editable': check_permissions_by_view(user, course_id, content, "update_thread" if content['type'] == 'thread' else "update_comment"), 'can_reply': check_permissions_by_view(user, course_id, content, "create_comment" if content['type'] == 'thread' else "create_sub_comment"), - 'can_endorse': check_permissions_by_view(user, course_id, content, "endorse_comment") if content['type'] == 'comment' else False, 'can_delete': check_permissions_by_view(user, course_id, content, "delete_thread" if content['type'] == 'thread' else "delete_comment"), 'can_openclose': check_permissions_by_view(user, course_id, content, "openclose_thread") if content['type'] == 'thread' else False, 'can_vote': check_permissions_by_view(user, course_id, content, "vote_for_thread" if content['type'] == 'thread' else "vote_for_comment"), diff --git a/lms/static/sass/discussion/_discussion.scss b/lms/static/sass/discussion/_discussion.scss index 1821797b1b..84fd22491f 100644 --- a/lms/static/sass/discussion/_discussion.scss +++ b/lms/static/sass/discussion/_discussion.scss @@ -647,6 +647,11 @@ body.discussion { border: 1px solid #a0a0a0; @include linear-gradient(top, $white 35%, $gray-l4); box-shadow: 0 1px 1px $shadow-l1; + cursor: default; + + &.is-clickable { + cursor: auto; + } .check-icon { display: block; @@ -654,6 +659,7 @@ body.discussion { height: 12px; margin: 8px auto; background: url(../images/endorse-icon.png) no-repeat; + pointer-events: none; } &.mark-answer .check-icon { diff --git a/lms/templates/discussion/_underscore_templates.html b/lms/templates/discussion/_underscore_templates.html index 658a9905c2..80976bb42d 100644 --- a/lms/templates/discussion/_underscore_templates.html +++ b/lms/templates/discussion/_underscore_templates.html @@ -162,10 +162,9 @@ "}" - style="cursor: default; display: none;" data-tooltip="${tooltip_expr}" > - + ${"<% if (obj.username) { %>"} ${'<%- username %>'}