From 33b3dfcc9279ce59a1b8adf49adb638202d8680f Mon Sep 17 00:00:00 2001 From: "E. Kolpakov" Date: Wed, 18 May 2016 18:58:40 +0300 Subject: [PATCH] Converts Discussion XModule to Discussion XBlock * Renames discussion_module to discussion_xblock * Moves common/lib/xmodule/xmodule_discussion to openedx/core/lib/xblock_builtin/xblock_discussion --- .../component_settings_editor_helpers.py | 14 +- .../features/discussion-editor.feature | 2 +- .../features/discussion-editor.py | 4 +- common/lib/xmodule/setup.py | 1 - .../lib/xmodule/xmodule/discussion_module.py | 132 ------ .../xmodule/tests/test_xblock_wrappers.py | 2 - .../xmodule/video_module/video_module.py | 3 +- .../tests/discussion/test_discussion.py | 24 +- .../commands/dump_course_structure.py | 5 +- .../tests/test_discussion_module.py | 131 ------ .../tests/test_discussion_xblock.py | 426 ++++++++++++++++++ .../tests/test_self_paced_overrides.py | 34 +- lms/djangoapps/discussion_api/api.py | 40 +- .../discussion_api/tests/test_api.py | 48 +- .../discussion_api/tests/test_views.py | 14 +- .../django_comment_client/forum/tests.py | 12 +- .../django_comment_client/forum/views.py | 12 +- .../django_comment_client/tests/test_utils.py | 24 +- lms/djangoapps/django_comment_client/utils.py | 73 +-- .../discussion/_discussion_inline.html | 28 ++ ...io.html => _discussion_inline_studio.html} | 3 +- .../discussion/_discussion_module.html | 14 - .../content/course_structures/tests.py | 4 +- openedx/core/lib/xblock_builtin/README.rst | 17 + openedx/core/lib/xblock_builtin/__init__.py | 0 .../xblock_discussion/__init__.py | 0 .../xblock_builtin/xblock_discussion/setup.py | 35 ++ .../xblock_discussion/xblock_discussion.py | 136 ++++++ requirements/edx/local.txt | 2 + 29 files changed, 806 insertions(+), 434 deletions(-) delete mode 100644 common/lib/xmodule/xmodule/discussion_module.py delete mode 100644 lms/djangoapps/courseware/tests/test_discussion_module.py create mode 100644 lms/djangoapps/courseware/tests/test_discussion_xblock.py create mode 100644 lms/templates/discussion/_discussion_inline.html rename lms/templates/discussion/{_discussion_module_studio.html => _discussion_inline_studio.html} (92%) delete mode 100644 lms/templates/discussion/_discussion_module.html create mode 100644 openedx/core/lib/xblock_builtin/README.rst create mode 100644 openedx/core/lib/xblock_builtin/__init__.py create mode 100644 openedx/core/lib/xblock_builtin/xblock_discussion/__init__.py create mode 100644 openedx/core/lib/xblock_builtin/xblock_discussion/setup.py create mode 100644 openedx/core/lib/xblock_builtin/xblock_discussion/xblock_discussion.py diff --git a/cms/djangoapps/contentstore/features/component_settings_editor_helpers.py b/cms/djangoapps/contentstore/features/component_settings_editor_helpers.py index aebe907468..6f572e9ae6 100644 --- a/cms/djangoapps/contentstore/features/component_settings_editor_helpers.py +++ b/cms/djangoapps/contentstore/features/component_settings_editor_helpers.py @@ -27,6 +27,8 @@ def create_component_instance(step, category, component_type=None, is_advanced=F module_css = 'div.xmodule_CapaModule' elif category == 'advanced': module_css = 'div.xmodule_{}Module'.format(advanced_component.title()) + elif category == 'discussion': + module_css = 'div.xblock-author_view-{}'.format(category.lower()) else: module_css = 'div.xmodule_{}Module'.format(category.title()) @@ -168,7 +170,9 @@ def verify_setting_entry(setting, display_name, value, explicitly_set): for the problem, rather than derived from the defaults. This is verified by the existence of a "Clear" button next to the field value. """ - assert_equal(display_name, setting.find_by_css('.setting-label')[0].html.strip()) + label_element = setting.find_by_css('.setting-label')[0] + assert_equal(display_name, label_element.html.strip()) + label_for = label_element['for'] # Check if the web object is a list type # If so, we use a slightly different mechanism for determining its value @@ -179,7 +183,7 @@ def verify_setting_entry(setting, display_name, value, explicitly_set): list_value = ', '.join(ele.find_by_css('input')[0].value for ele in setting.find_by_css('.videolist-settings-item')) assert_equal(value, list_value) else: - assert_equal(value, setting.find_by_css('.setting-input')[0].value) + assert_equal(value, setting.find_by_id(label_for).value) # VideoList doesn't have clear button if not setting.has_class('metadata-videolist-enum'): @@ -201,7 +205,7 @@ def verify_all_setting_entries(expected_entries): @world.absorb def save_component(): - world.css_click("a.action-save") + world.css_click("a.action-save,a.save-button") world.wait_for_ajax_complete() @@ -241,7 +245,7 @@ def get_setting_entry(label): @world.absorb def get_setting_entry_index(label): def get_index(): - settings = world.css_find('.metadata_edit .wrapper-comp-setting') + settings = world.css_find('.wrapper-comp-setting') for index, setting in enumerate(settings): if setting.find_by_css('.setting-label')[0].value == label: return index @@ -259,6 +263,6 @@ def set_field_value(index, value): Instead we will find the element, set its value, then hit the Tab key to get to the next field. """ - elem = world.css_find('.metadata_edit div.wrapper-comp-setting input.setting-input')[index] + elem = world.css_find('div.wrapper-comp-setting input')[index] elem.value = value elem.type(Keys.TAB) diff --git a/cms/djangoapps/contentstore/features/discussion-editor.feature b/cms/djangoapps/contentstore/features/discussion-editor.feature index 2275da8706..53f32eddb7 100644 --- a/cms/djangoapps/contentstore/features/discussion-editor.feature +++ b/cms/djangoapps/contentstore/features/discussion-editor.feature @@ -5,7 +5,7 @@ Feature: CMS.Discussion Component Editor Scenario: User can view discussion component metadata Given I have created a Discussion Tag And I edit the component - Then I see three alphabetized settings and their expected values + Then I see three settings and their expected values # Safari doesn't save the name properly @skip_safari diff --git a/cms/djangoapps/contentstore/features/discussion-editor.py b/cms/djangoapps/contentstore/features/discussion-editor.py index 1a04ec0e09..e7f52df8e6 100644 --- a/cms/djangoapps/contentstore/features/discussion-editor.py +++ b/cms/djangoapps/contentstore/features/discussion-editor.py @@ -13,12 +13,12 @@ def i_created_discussion_tag(step): ) -@step('I see three alphabetized settings and their expected values$') +@step('I see three settings and their expected values$') def i_see_only_the_settings_and_values(step): world.verify_all_setting_entries( [ - ['Category', "Week 1", False], ['Display Name', "Discussion", False], + ['Category', "Week 1", False], ['Subcategory', "Topic-Level Student-Visible Label", False] ]) diff --git a/common/lib/xmodule/setup.py b/common/lib/xmodule/setup.py index 73b4d4e168..ab5ef7a687 100644 --- a/common/lib/xmodule/setup.py +++ b/common/lib/xmodule/setup.py @@ -23,7 +23,6 @@ XMODULES = [ "videoalpha = xmodule.video_module:VideoDescriptor", "videodev = xmodule.backcompat_module:TranslateCustomTagDescriptor", "videosequence = xmodule.seq_module:SequenceDescriptor", - "discussion = xmodule.discussion_module:DiscussionDescriptor", "course_info = xmodule.html_module:CourseInfoDescriptor", "static_tab = xmodule.html_module:StaticTabDescriptor", "custom_tag_template = xmodule.raw_module:RawDescriptor", diff --git a/common/lib/xmodule/xmodule/discussion_module.py b/common/lib/xmodule/xmodule/discussion_module.py deleted file mode 100644 index 3d2a47941f..0000000000 --- a/common/lib/xmodule/xmodule/discussion_module.py +++ /dev/null @@ -1,132 +0,0 @@ -""" -Definition of the Discussion module. -""" -import json -from pkg_resources import resource_string - -from xblock.core import XBlock -from xmodule.x_module import XModule -from xmodule.raw_module import RawDescriptor -from xmodule.editing_module import MetadataOnlyEditingDescriptor -from xblock.fields import String, Scope, UNIQUE_ID - -# Make '_' a no-op so we can scrape strings. Using lambda instead of -# `django.utils.translation.ugettext_noop` because Django cannot be imported in this file -_ = lambda text: text - - -class DiscussionFields(object): - discussion_id = String( - display_name=_("Discussion Id"), - help=_("The id is a unique identifier for the discussion. It is non editable."), - scope=Scope.settings, - default=UNIQUE_ID) - display_name = String( - display_name=_("Display Name"), - help=_("Display name for this module"), - default="Discussion", - scope=Scope.settings - ) - data = String( - help=_("XML data for the problem"), - scope=Scope.content, - default="" - ) - discussion_category = String( - display_name=_("Category"), - default="Week 1", - help=_("A category name for the discussion. This name appears in the left pane of the discussion forum for the course."), - scope=Scope.settings - ) - discussion_target = String( - display_name=_("Subcategory"), - default="Topic-Level Student-Visible Label", - help=_("A subcategory name for the discussion. This name appears in the left pane of the discussion forum for the course."), - scope=Scope.settings - ) - sort_key = String(scope=Scope.settings) - - -def has_permission(user, permission, course_id): - """ - Copied from django_comment_client/permissions.py because I can't import - that file from here. It causes the xmodule_assets command to fail. - """ - return any(role.has_permission(permission) - for role in user.roles.filter(course_id=course_id)) - - -@XBlock.wants('user') -class DiscussionModule(DiscussionFields, XModule): - """ - XModule for discussion forums. - """ - js = { - 'coffee': [ - resource_string(__name__, 'js/src/discussion/display.coffee') - ], - 'js': [ - resource_string(__name__, 'js/src/time.js') - ] - } - js_module_name = "InlineDiscussion" - - def get_html(self): - course = self.get_course() - user = None - user_service = self.runtime.service(self, 'user') - if user_service: - user = user_service._django_user # pylint: disable=protected-access - if user: - course_key = course.id - can_create_comment = has_permission(user, "create_comment", course_key) - can_create_subcomment = has_permission(user, "create_sub_comment", course_key) - can_create_thread = has_permission(user, "create_thread", course_key) - else: - can_create_comment = False - can_create_subcomment = False - can_create_thread = False - context = { - 'discussion_id': self.discussion_id, - 'course': course, - 'can_create_comment': json.dumps(can_create_comment), - 'can_create_subcomment': json.dumps(can_create_subcomment), - 'can_create_thread': can_create_thread, - } - if getattr(self.system, 'is_author_mode', False): - template = 'discussion/_discussion_module_studio.html' - else: - template = 'discussion/_discussion_module.html' - return self.system.render_template(template, context) - - def get_course(self): - """ - Return CourseDescriptor by course id. - """ - course = self.runtime.modulestore.get_course(self.course_id) - return course - - -class DiscussionDescriptor(DiscussionFields, MetadataOnlyEditingDescriptor, RawDescriptor): - module_class = DiscussionModule - resources_dir = None - - # The discussion XML format uses `id` and `for` attributes, - # but these would overload other module attributes, so we prefix them - # for actual use in the code - metadata_translations = dict(RawDescriptor.metadata_translations) - metadata_translations['id'] = 'discussion_id' - metadata_translations['for'] = 'discussion_target' - - @property - def non_editable_metadata_fields(self): - non_editable_fields = super(DiscussionDescriptor, self).non_editable_metadata_fields - # We may choose to enable sort_keys in the future, but while Kevin is investigating.... - non_editable_fields.extend([DiscussionDescriptor.discussion_id, DiscussionDescriptor.sort_key]) - return non_editable_fields - - def student_view_data(self): - """ - Returns a JSON representation of the student_view of this XModule. - """ - return {'topic_id': self.discussion_id} diff --git a/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py b/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py index 7dc06e011d..6cf57b43b0 100644 --- a/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py +++ b/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py @@ -31,7 +31,6 @@ from xmodule.x_module import ModuleSystem, XModule, XModuleDescriptor, Descripto from xmodule.annotatable_module import AnnotatableDescriptor from xmodule.capa_module import CapaDescriptor from xmodule.course_module import CourseDescriptor -from xmodule.discussion_module import DiscussionDescriptor from xmodule.html_module import HtmlDescriptor from xmodule.poll_module import PollDescriptor from xmodule.word_cloud_module import WordCloudDescriptor @@ -50,7 +49,6 @@ from xmodule.tests import get_test_descriptor_system, get_test_system LEAF_XMODULES = { AnnotatableDescriptor: [{}], CapaDescriptor: [{}], - DiscussionDescriptor: [{}], HtmlDescriptor: [{}], PollDescriptor: [{'display_name': 'Poll Display Name'}], WordCloudDescriptor: [{}], diff --git a/common/lib/xmodule/xmodule/video_module/video_module.py b/common/lib/xmodule/xmodule/video_module/video_module.py index 31f9ecc4ad..c4143506c1 100644 --- a/common/lib/xmodule/xmodule/video_module/video_module.py +++ b/common/lib/xmodule/xmodule/video_module/video_module.py @@ -112,6 +112,7 @@ class VideoModule(VideoFields, VideoTranscriptsMixin, VideoStudentViewHandlers, #TODO: For each of the following, ensure that any generated html is properly escaped. js = { 'js': [ + resource_string(module, 'js/src/time.js'), resource_string(module, 'js/src/video/00_component.js'), resource_string(module, 'js/src/video/00_video_storage.js'), resource_string(module, 'js/src/video/00_resizer.js'), @@ -350,7 +351,7 @@ class VideoModule(VideoFields, VideoTranscriptsMixin, VideoStudentViewHandlers, 'cdn_eval': cdn_eval, 'cdn_exp_group': cdn_exp_group, 'id': self.location.html_id(), - 'display_name': self.display_name_with_default_escaped, + 'display_name': self.display_name_with_default, 'handout': self.handout, 'download_video_link': download_video_link, 'track': track_url, diff --git a/common/test/acceptance/tests/discussion/test_discussion.py b/common/test/acceptance/tests/discussion/test_discussion.py index 73858ac547..5cd156e43a 100644 --- a/common/test/acceptance/tests/discussion/test_discussion.py +++ b/common/test/acceptance/tests/discussion/test_discussion.py @@ -966,22 +966,22 @@ class InlineDiscussionTest(UniqueCourseTest, DiscussionResponsePaginationTestMix self.assertFalse(self.thread_page.is_comment_deletable("comment1")) self.assertFalse(self.thread_page.is_comment_deletable("comment2")) - def test_dual_discussion_module(self): + def test_dual_discussion_xblock(self): """ - Scenario: Two discussion module in one unit shouldn't override their actions + Scenario: Two discussion xblocks in one unit shouldn't override their actions Given that I'm on courseware page where there are two inline discussion - When I click on one discussion module new post button - Then it should add new post form of that module in DOM - And I should be shown new post form of that module - And I shouldn't be shown second discussion module new post form - And I click on second discussion module new post button - Then it should add new post form of second module in DOM + When I click on one discussion xblock new post button + Then it should add new post form of that xblock in DOM + And I should be shown new post form of that xblock + And I shouldn't be shown second discussion xblock new post form + And I click on second discussion xblock new post button + Then it should add new post form of second xblock in DOM And I should be shown second discussion new post form - And I shouldn't be shown first discussion module new post form + And I shouldn't be shown first discussion xblock new post form And I have two new post form in the DOM - When I click back on first module new post button - And I should be shown new post form of that module - And I shouldn't be shown second discussion module new post form + When I click back on first xblock new post button + And I should be shown new post form of that xblock + And I shouldn't be shown second discussion xblock new post form """ self.discussion_page.wait_for_page() self.additional_discussion_page.wait_for_page() diff --git a/lms/djangoapps/courseware/management/commands/dump_course_structure.py b/lms/djangoapps/courseware/management/commands/dump_course_structure.py index ea308bf4b9..874133a074 100644 --- a/lms/djangoapps/courseware/management/commands/dump_course_structure.py +++ b/lms/djangoapps/courseware/management/commands/dump_course_structure.py @@ -22,9 +22,10 @@ from textwrap import dedent from django.core.management.base import BaseCommand, CommandError -from xmodule.discussion_module import DiscussionDescriptor from xmodule.modulestore.django import modulestore from xmodule.modulestore.inheritance import own_metadata, compute_inherited_metadata + +from xblock_discussion import DiscussionXBlock from xblock.fields import Scope from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey @@ -97,7 +98,7 @@ def dump_module(module, destination=None, inherited=False, defaults=False): items = own_metadata(module) # HACK: add discussion ids to list of items to export (AN-6696) - if isinstance(module, DiscussionDescriptor) and 'discussion_id' not in items: + if isinstance(module, DiscussionXBlock) and 'discussion_id' not in items: items['discussion_id'] = module.discussion_id filtered_metadata = {k: v for k, v in items.iteritems() if k not in FILTER_LIST} diff --git a/lms/djangoapps/courseware/tests/test_discussion_module.py b/lms/djangoapps/courseware/tests/test_discussion_module.py deleted file mode 100644 index e4ae915992..0000000000 --- a/lms/djangoapps/courseware/tests/test_discussion_module.py +++ /dev/null @@ -1,131 +0,0 @@ -# -*- coding: utf-8 -*- -"""Test for Discussion Xmodule functional logic.""" -import ddt -from django.core.urlresolvers import reverse -from mock import Mock -from . import BaseTestXmodule -from course_api.blocks.tests.helpers import deserialize_usage_key -from courseware.module_render import get_module_for_descriptor_internal -from student.tests.factories import UserFactory, CourseEnrollmentFactory -from xmodule.discussion_module import DiscussionModule -from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.django import modulestore -from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase -from xmodule.modulestore.tests.factories import ToyCourseFactory, ItemFactory - - -@ddt.ddt -class DiscussionModuleTest(BaseTestXmodule, SharedModuleStoreTestCase): - """Logic tests for Discussion Xmodule.""" - CATEGORY = "discussion" - - def test_html_with_user(self): - discussion = get_module_for_descriptor_internal( - user=self.users[0], - descriptor=self.item_descriptor, - student_data=Mock(name='student_data'), - course_id=self.course.id, - track_function=Mock(name='track_function'), - xqueue_callback_url_prefix=Mock(name='xqueue_callback_url_prefix'), - request_token='request_token', - ) - - fragment = discussion.render('student_view') - html = fragment.content - self.assertIn('data-user-create-comment="false"', html) - self.assertIn('data-user-create-subcomment="false"', html) - - @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) - def test_discussion_render_successfully_with_orphan_parent(self, default_store): - """ - Test that discussion module render successfully - if discussion module is child of an orphan. - """ - user = UserFactory.create() - store = modulestore() - with store.default_store(default_store): - course = store.create_course('testX', 'orphan', '123X', user.id) - orphan_sequential = store.create_item(self.user.id, course.id, 'sequential') - - vertical = store.create_child( - user.id, - orphan_sequential.location, - 'vertical', - block_id=course.location.block_id - ) - - discussion = store.create_child( - user.id, - vertical.location, - 'discussion', - block_id=course.location.block_id - ) - - discussion = store.get_item(discussion.location) - - root = self.get_root(discussion) - # Assert that orphan sequential is root of the discussion module. - self.assertEqual(orphan_sequential.location.block_type, root.location.block_type) - self.assertEqual(orphan_sequential.location.block_id, root.location.block_id) - - # Get module system bound to a user and a descriptor. - discussion_module = get_module_for_descriptor_internal( - user=user, - descriptor=discussion, - student_data=Mock(name='student_data'), - course_id=course.id, - track_function=Mock(name='track_function'), - xqueue_callback_url_prefix=Mock(name='xqueue_callback_url_prefix'), - request_token='request_token', - ) - - fragment = discussion_module.render('student_view') - html = fragment.content - - self.assertIsInstance(discussion_module._xmodule, DiscussionModule) # pylint: disable=protected-access - self.assertIn('data-user-create-comment="false"', html) - self.assertIn('data-user-create-subcomment="false"', html) - - def get_root(self, block): - """ - Return root of the block. - """ - while block.parent: - block = block.get_parent() - - return block - - def test_discussion_student_view_data(self): - """ - Tests that course block api returns student_view_data for discussion module - """ - course_key = ToyCourseFactory.create().id - course_usage_key = self.store.make_course_usage_key(course_key) - user = UserFactory.create() - self.client.login(username=user.username, password='test') - CourseEnrollmentFactory.create(user=user, course_id=course_key) - discussion_id = "test_discussion_module_id" - ItemFactory.create( - parent_location=course_usage_key, - category='discussion', - discussion_id=discussion_id, - discussion_category='Category discussion', - discussion_target='Target Discussion', - ) - - url = reverse('blocks_in_block_tree', kwargs={'usage_key_string': unicode(course_usage_key)}) - query_params = { - 'depth': 'all', - 'username': user.username, - 'block_types_filter': 'discussion', - 'student_view_data': 'discussion' - } - response = self.client.get(url, query_params) - self.assertEquals(response.status_code, 200) - self.assertEquals(response.data['root'], unicode(course_usage_key)) # pylint: disable=no-member - for block_key_string, block_data in response.data['blocks'].iteritems(): # pylint: disable=no-member - block_key = deserialize_usage_key(block_key_string, course_key) - self.assertEquals(block_data['id'], block_key_string) - self.assertEquals(block_data['type'], block_key.block_type) - self.assertEquals(block_data['display_name'], self.store.get_item(block_key).display_name or '') - self.assertEqual(block_data['student_view_data'], {"topic_id": discussion_id}) diff --git a/lms/djangoapps/courseware/tests/test_discussion_xblock.py b/lms/djangoapps/courseware/tests/test_discussion_xblock.py new file mode 100644 index 0000000000..34ca9bd406 --- /dev/null +++ b/lms/djangoapps/courseware/tests/test_discussion_xblock.py @@ -0,0 +1,426 @@ +""" +Tests for the discussion xblock. + +Most of the tests are in common/xblock/xblock_discussion, here are only +tests for functionalities that require django API, and lms specific +functionalities. +""" + +import uuid + +import ddt +import json +import mock + +from django.core.urlresolvers import reverse +from course_api.blocks.tests.helpers import deserialize_usage_key +from courseware.module_render import get_module_for_descriptor_internal +from student.tests.factories import UserFactory, CourseEnrollmentFactory +from xblock.field_data import DictFieldData +from xblock.fragment import Fragment +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.tests.factories import ToyCourseFactory, ItemFactory +from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase +from lms.djangoapps.courseware.tests import XModuleRenderingTestBase + +from xblock_discussion import DiscussionXBlock, loader + + +@ddt.ddt +class TestDiscussionXBlock(XModuleRenderingTestBase): + """ + Base class for tests + """ + + PATCH_DJANGO_USER = True + + def setUp(self): + """ + Set up the xblock runtime, test course, discussion, and user. + """ + super(TestDiscussionXBlock, self).setUp() + self.patchers = [] + self.course_id = "test_course" + self.runtime = self.new_module_runtime() + self.runtime.modulestore = mock.Mock() + + self.discussion_id = str(uuid.uuid4()) + self.data = DictFieldData({ + 'discussion_id': self.discussion_id + }) + scope_ids = mock.Mock() + scope_ids.usage_id.course_key = self.course_id + self.block = DiscussionXBlock( + self.runtime, + field_data=self.data, + scope_ids=scope_ids + ) + self.block.xmodule_runtime = mock.Mock() + + if self.PATCH_DJANGO_USER: + self.django_user_canary = object() + self.django_user_mock = self.add_patcher( + mock.patch.object(DiscussionXBlock, "django_user", new_callable=mock.PropertyMock) + ) + self.django_user_mock.return_value = self.django_user_canary + + def add_patcher(self, patcher): + """ + Registers a patcher object, and returns mock. This patcher will be disabled after the test. + """ + self.patchers.append(patcher) + return patcher.start() + + def tearDown(self): + """ + Tears down any patchers added during tests. + """ + super(TestDiscussionXBlock, self).tearDown() + for patcher in self.patchers: + patcher.stop() + + +class TestGetDjangoUser(TestDiscussionXBlock): + """ + Tests for the django_user property. + """ + + PATCH_DJANGO_USER = False + + def setUp(self): + """ + Mock the user service and runtime. + """ + super(TestGetDjangoUser, self).setUp() + self.django_user = object() + self.user_service = mock.Mock() + self.add_patcher( + mock.patch.object(self.runtime, "service", return_value=self.user_service) + ) + self.user_service._django_user = self.django_user # pylint: disable=protected-access + + def test_django_user(self): + """ + Tests that django_user users returns _django_user attribute + of the user service. + """ + actual_user = self.block.django_user + self.runtime.service.assert_called_once_with( + self.block, 'user') + self.assertEqual(actual_user, self.django_user) + + def test_django_user_handles_missing_service(self): + """ + Tests that get_django gracefully handles missing user service. + """ + self.runtime.service.return_value = None + self.assertEqual(self.block.django_user, None) + + +@ddt.ddt +class TestViews(TestDiscussionXBlock): + """ + Tests for student_view and author_view. + """ + + def setUp(self): + """ + Mock the methods needed for these tests. + """ + super(TestViews, self).setUp() + self.template_canary = u'canary' + self.render_template = mock.Mock() + self.render_template.return_value = self.template_canary + self.block.runtime.render_template = self.render_template + self.has_permission_mock = mock.Mock() + self.has_permission_mock.return_value = False + self.block.has_permission = self.has_permission_mock + + def get_template_context(self): + """ + Returns context passed to rendering of the django template + (rendered by runtime). + """ + self.assertEqual(self.render_template.call_count, 1) + return self.render_template.call_args_list[0][0][1] + + def get_rendered_template(self): + """ + Returns the name of the template rendered by runtime. + """ + self.assertEqual(self.render_template.call_count, 1) + return self.render_template.call_args_list[0][0][0] + + def test_studio_view(self): + """ + Test for the studio view. + """ + fragment = self.block.author_view() + self.assertIsInstance(fragment, Fragment) + self.assertEqual(fragment.content, self.template_canary) + self.render_template.assert_called_once_with( + 'discussion/_discussion_inline_studio.html', + {'discussion_id': self.discussion_id} + ) + + @ddt.data( + (False, False, False), + (True, False, False), + (False, True, False), + (False, False, True), + ) + def test_student_perms_are_correct(self, permissions): + """ + Test that context will get proper permissions. + """ + permission_dict = { + 'create_thread': permissions[0], + 'create_comment': permissions[1], + 'create_subcomment': permissions[2] + } + + expected_permissions = { + 'can_create_thread': permission_dict['create_thread'], + 'can_create_comment': permission_dict['create_comment'], + 'can_create_subcomment': permission_dict['create_subcomment'], + } + + self.block.has_permission = lambda perm: permission_dict[perm] + with mock.patch.object(loader, 'render_template', mock.Mock): + self.block.student_view() + + context = self.get_template_context() + + for permission_name, expected_value in expected_permissions.items(): + self.assertEqual(expected_value, context[permission_name]) + + def test_js_init(self): + """ + Test proper js init function is called. + """ + with mock.patch.object(loader, 'render_template', mock.Mock): + fragment = self.block.student_view() + self.assertEqual(fragment.js_init_fn, 'DiscussionInlineBlock') + + +@ddt.ddt +class TestTemplates(TestDiscussionXBlock): + """ + Tests rendering of templates. + """ + + def test_has_permission(self): + """ + Test for has_permission method. + """ + permission_canary = object() + with mock.patch('django_comment_client.permissions.has_permission', return_value=permission_canary) as has_perm: + actual_permission = self.block.has_permission("test_permission") + self.assertEqual(actual_permission, permission_canary) + has_perm.assert_called_once_with(self.django_user_canary, 'test_permission', 'test_course') + + def test_studio_view(self): + """Test for studio view.""" + fragment = self.block.author_view({}) + self.assertIn('data-discussion-id="{}"'.format(self.discussion_id), fragment.content) + + @ddt.data( + (True, False, False), + (False, True, False), + (False, False, True), + ) + def test_student_perms_are_correct(self, permissions): + """ + Test for lms view. + """ + permission_dict = { + 'create_thread': permissions[0], + 'create_comment': permissions[1], + 'create_subcomment': permissions[2] + } + + self.block.has_permission = lambda perm: permission_dict[perm] + fragment = self.block.student_view() + + self.assertIn('data-discussion-id="{}"'.format(self.discussion_id), fragment.content) + self.assertIn('data-user-create-comment="{}"'.format(json.dumps(permissions[1])), fragment.content) + self.assertIn('data-user-create-subcomment="{}"'.format(json.dumps(permissions[2])), fragment.content) + if permissions[0]: + self.assertIn("Add a Post", fragment.content) + else: + self.assertNotIn("Add a Post", fragment.content) + + +@ddt.ddt +class TestXBlockInCourse(SharedModuleStoreTestCase): + """ + Test the discussion xblock as rendered in the course and course API. + """ + + @classmethod + def setUpClass(cls): + """ + Set up a user, course, and discussion XBlock for use by tests. + """ + super(TestXBlockInCourse, cls).setUpClass() + cls.user = UserFactory.create() + cls.course = ToyCourseFactory.create() + cls.course_key = cls.course.id + cls.course_usage_key = cls.store.make_course_usage_key(cls.course_key) + cls.discussion_id = "test_discussion_xblock_id" + cls.discussion = ItemFactory.create( + parent_location=cls.course_usage_key, + category='discussion', + discussion_id=cls.discussion_id, + discussion_category='Category discussion', + discussion_target='Target Discussion', + ) + CourseEnrollmentFactory.create(user=cls.user, course_id=cls.course_key) + + def get_root(self, block): + """ + Return root of the block. + """ + while block.parent: + block = block.get_parent() + return block + + def test_html_with_user(self): + """ + Test rendered DiscussionXBlock permissions. + """ + discussion_xblock = get_module_for_descriptor_internal( + user=self.user, + descriptor=self.discussion, + student_data=mock.Mock(name='student_data'), + course_id=self.course.id, + track_function=mock.Mock(name='track_function'), + xqueue_callback_url_prefix=mock.Mock(name='xqueue_callback_url_prefix'), + request_token='request_token', + ) + + fragment = discussion_xblock.render('student_view') + html = fragment.content + self.assertIn('data-user-create-comment="false"', html) + self.assertIn('data-user-create-subcomment="false"', html) + + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + def test_discussion_render_successfully_with_orphan_parent(self, default_store): + """ + Test that discussion xblock render successfully + if discussion xblock is child of an orphan. + """ + with self.store.default_store(default_store): + orphan_sequential = self.store.create_item(self.user.id, self.course.id, 'sequential') + + vertical = self.store.create_child( + self.user.id, + orphan_sequential.location, + 'vertical', + block_id=self.course.location.block_id + ) + + discussion = self.store.create_child( + self.user.id, + vertical.location, + 'discussion', + block_id=self.course.location.block_id + ) + + discussion = self.store.get_item(discussion.location) + + root = self.get_root(discussion) + # Assert that orphan sequential is root of the discussion xblock. + self.assertEqual(orphan_sequential.location.block_type, root.location.block_type) + self.assertEqual(orphan_sequential.location.block_id, root.location.block_id) + + # Get xblock bound to a user and a descriptor. + discussion_xblock = get_module_for_descriptor_internal( + user=self.user, + descriptor=discussion, + student_data=mock.Mock(name='student_data'), + course_id=self.course.id, + track_function=mock.Mock(name='track_function'), + xqueue_callback_url_prefix=mock.Mock(name='xqueue_callback_url_prefix'), + request_token='request_token', + ) + + fragment = discussion_xblock.render('student_view') + html = fragment.content + + self.assertIsInstance(discussion_xblock, DiscussionXBlock) + self.assertIn('data-user-create-comment="false"', html) + self.assertIn('data-user-create-subcomment="false"', html) + + def test_discussion_student_view_data(self): + """ + Tests that course block api returns student_view_data for discussion xblock + """ + self.client.login(username=self.user.username, password='test') + url = reverse('blocks_in_block_tree', kwargs={'usage_key_string': unicode(self.course_usage_key)}) + query_params = { + 'depth': 'all', + 'username': self.user.username, + 'block_types_filter': 'discussion', + 'student_view_data': 'discussion' + } + response = self.client.get(url, query_params) + self.assertEquals(response.status_code, 200) + self.assertEquals(response.data['root'], unicode(self.course_usage_key)) # pylint: disable=no-member + for block_key_string, block_data in response.data['blocks'].iteritems(): # pylint: disable=no-member + block_key = deserialize_usage_key(block_key_string, self.course_key) + self.assertEquals(block_data['id'], block_key_string) + self.assertEquals(block_data['type'], block_key.block_type) + self.assertEquals(block_data['display_name'], self.store.get_item(block_key).display_name or '') + self.assertEqual(block_data['student_view_data'], {"topic_id": self.discussion_id}) + + +class TestXBlockQueryLoad(SharedModuleStoreTestCase): + """ + Test the number of queries executed when rendering the XBlock. + """ + + def test_permissions_query_load(self): + """ + Tests that the permissions queries are cached when rendering numerous discussion XBlocks. + """ + user = UserFactory.create() + course = ToyCourseFactory.create() + course_key = course.id + course_usage_key = self.store.make_course_usage_key(course_key) + discussions = [] + + for counter in range(5): + discussion_id = 'test_discussion_{}'.format(counter) + discussions.append(ItemFactory.create( + parent_location=course_usage_key, + category='discussion', + discussion_id=discussion_id, + discussion_category='Category discussion', + discussion_target='Target Discussion', + )) + + # 3 queries are required to do first discussion xblock render: + # * django_comment_client_role + # * django_comment_client_permission + # * lms_xblock_xblockasidesconfig + num_queries = 3 + for discussion in discussions: + discussion_xblock = get_module_for_descriptor_internal( + user=user, + descriptor=discussion, + student_data=mock.Mock(name='student_data'), + course_id=course.id, + track_function=mock.Mock(name='track_function'), + xqueue_callback_url_prefix=mock.Mock(name='xqueue_callback_url_prefix'), + request_token='request_token', + ) + with self.assertNumQueries(num_queries): + fragment = discussion_xblock.render('student_view') + + # Permissions are cached, so no queries required for subsequent renders + num_queries = 0 + + html = fragment.content + self.assertIn('data-user-create-comment="false"', html) + self.assertIn('data-user-create-subcomment="false"', html) diff --git a/lms/djangoapps/courseware/tests/test_self_paced_overrides.py b/lms/djangoapps/courseware/tests/test_self_paced_overrides.py index 467459f35d..b2e166fb3d 100644 --- a/lms/djangoapps/courseware/tests/test_self_paced_overrides.py +++ b/lms/djangoapps/courseware/tests/test_self_paced_overrides.py @@ -9,7 +9,7 @@ from mock import patch from courseware.tests.factories import BetaTesterFactory from courseware.access import has_access from lms.djangoapps.ccx.tests.test_overrides import inject_field_overrides -from lms.djangoapps.django_comment_client.utils import get_accessible_discussion_modules +from lms.djangoapps.django_comment_client.utils import get_accessible_discussion_xblocks from lms.djangoapps.courseware.field_overrides import OverrideFieldData, OverrideModulestoreFieldData from openedx.core.djangoapps.self_paced.models import SelfPacedConfiguration from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -59,8 +59,8 @@ class SelfPacedDateOverrideTest(ModuleStoreTestCase): inject_field_overrides((course, section), course, self.user) return (course, section) - def create_discussion_modules(self, parent): - # Create a released discussion module + def create_discussion_xblocks(self, parent): + # Create a released discussion xblock ItemFactory.create( parent=parent, category='discussion', @@ -68,7 +68,7 @@ class SelfPacedDateOverrideTest(ModuleStoreTestCase): start=self.now, ) - # Create a scheduled discussion module + # Create a scheduled discussion xblock ItemFactory.create( parent=parent, category='discussion', @@ -118,32 +118,32 @@ class SelfPacedDateOverrideTest(ModuleStoreTestCase): self.assertTrue(has_access(beta_tester, 'load', self_paced_section, self_paced_course.id)) @patch.dict('courseware.access.settings.FEATURES', {'DISABLE_START_DATES': False}) - def test_instructor_paced_discussion_module_visibility(self): + def test_instructor_paced_discussion_xblock_visibility(self): """ - Verify that discussion modules scheduled for release in the future are + Verify that discussion xblocks scheduled for release in the future are not visible to students in an instructor-paced course. """ course, section = self.setup_course(start=self.now, self_paced=False) - self.create_discussion_modules(section) + self.create_discussion_xblocks(section) - # Only the released module should be visible when the course is instructor-paced. - modules = get_accessible_discussion_modules(course, self.non_staff_user) + # Only the released xblocks should be visible when the course is instructor-paced. + xblocks = get_accessible_discussion_xblocks(course, self.non_staff_user) self.assertTrue( - all(module.display_name == 'released' for module in modules) + all(xblock.display_name == 'released' for xblock in xblocks) ) @patch.dict('courseware.access.settings.FEATURES', {'DISABLE_START_DATES': False}) - def test_self_paced_discussion_module_visibility(self): + def test_self_paced_discussion_xblock_visibility(self): """ - Regression test. Verify that discussion modules scheduled for release + Regression test. Verify that discussion xblocks scheduled for release in the future are visible to students in a self-paced course. """ course, section = self.setup_course(start=self.now, self_paced=True) - self.create_discussion_modules(section) + self.create_discussion_xblocks(section) - # The scheduled module should be visible when the course is self-paced. - modules = get_accessible_discussion_modules(course, self.non_staff_user) - self.assertEqual(len(modules), 2) + # The scheduled xblocks should be visible when the course is self-paced. + xblocks = get_accessible_discussion_xblocks(course, self.non_staff_user) + self.assertEqual(len(xblocks), 2) self.assertTrue( - any(module.display_name == 'scheduled' for module in modules) + any(xblock.display_name == 'scheduled' for xblock in xblocks) ) diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index 13376a043d..435a0c2508 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/lms/djangoapps/discussion_api/api.py @@ -42,7 +42,7 @@ from django_comment_common.signals import ( comment_voted, comment_deleted, ) -from django_comment_client.utils import get_accessible_discussion_modules, is_commentable_cohorted +from django_comment_client.utils import get_accessible_discussion_xblocks, is_commentable_cohorted from lms.djangoapps.discussion_api.pagination import DiscussionAPIPagination from lms.lib.comment_client.comment import Comment from lms.lib.comment_client.thread import Thread @@ -215,41 +215,41 @@ def get_courseware_topics(request, course_key, course, topic_ids): courseware_topics = [] existing_topic_ids = set() - def get_module_sort_key(module): + def get_xblock_sort_key(xblock): """ - Get the sort key for the module (falling back to the discussion_target + Get the sort key for the xblock (falling back to the discussion_target setting if absent) """ - return module.sort_key or module.discussion_target + return xblock.sort_key or xblock.discussion_target - def get_sorted_modules(category): - """Returns key sorted modules by category""" - return sorted(modules_by_category[category], key=get_module_sort_key) + def get_sorted_xblocks(category): + """Returns key sorted xblocks by category""" + return sorted(xblocks_by_category[category], key=get_xblock_sort_key) - discussion_modules = get_accessible_discussion_modules(course, request.user) - modules_by_category = defaultdict(list) - for module in discussion_modules: - modules_by_category[module.discussion_category].append(module) + discussion_xblocks = get_accessible_discussion_xblocks(course, request.user) + xblocks_by_category = defaultdict(list) + for xblock in discussion_xblocks: + xblocks_by_category[xblock.discussion_category].append(xblock) - for category in sorted(modules_by_category.keys()): + for category in sorted(xblocks_by_category.keys()): children = [] - for module in get_sorted_modules(category): - if not topic_ids or module.discussion_id in topic_ids: + for xblock in get_sorted_xblocks(category): + if not topic_ids or xblock.discussion_id in topic_ids: discussion_topic = DiscussionTopic( - module.discussion_id, - module.discussion_target, - get_thread_list_url(request, course_key, [module.discussion_id]), + xblock.discussion_id, + xblock.discussion_target, + get_thread_list_url(request, course_key, [xblock.discussion_id]), ) children.append(discussion_topic) - if topic_ids and module.discussion_id in topic_ids: - existing_topic_ids.add(module.discussion_id) + if topic_ids and xblock.discussion_id in topic_ids: + existing_topic_ids.add(xblock.discussion_id) if not topic_ids or children: discussion_topic = DiscussionTopic( None, category, - get_thread_list_url(request, course_key, [item.discussion_id for item in get_sorted_modules(category)]), + get_thread_list_url(request, course_key, [item.discussion_id for item in get_sorted_xblocks(category)]), children, ) courseware_topics.append(DiscussionTopicSerializer(discussion_topic).data) diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index b778313726..e3b374bb13 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/lms/djangoapps/discussion_api/tests/test_api.py @@ -65,7 +65,7 @@ def _remove_discussion_tab(course, user_id): """ Remove the discussion tab for the course. - user_id is passed to the modulestore as the editor of the module. + user_id is passed to the modulestore as the editor of the xblock. """ course.tabs = [tab for tab in course.tabs if not tab.type == 'discussion'] modulestore().update_item(course, user_id) @@ -206,8 +206,10 @@ class GetCourseTopicsTest(UrlResetMixin, ModuleStoreTestCase): self.request.user = self.user CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id) - def make_discussion_module(self, topic_id, category, subcategory, **kwargs): - """Build a discussion module in self.course""" + def make_discussion_xblock(self, topic_id, category, subcategory, **kwargs): + """ + Build a discussion xblock in self.course. + """ ItemFactory.create( parent_location=self.course.location, category="discussion", @@ -274,7 +276,7 @@ class GetCourseTopicsTest(UrlResetMixin, ModuleStoreTestCase): self.assertEqual(actual, expected) def test_with_courseware(self): - self.make_discussion_module("courseware-topic-id", "Foo", "Bar") + self.make_discussion_xblock("courseware-topic-id", "Foo", "Bar") actual = self.get_course_topics() expected = { "courseware_topics": [ @@ -297,11 +299,11 @@ class GetCourseTopicsTest(UrlResetMixin, ModuleStoreTestCase): "B": {"id": "non-courseware-2"}, } self.store.update_item(self.course, self.user.id) - self.make_discussion_module("courseware-1", "A", "1") - self.make_discussion_module("courseware-2", "A", "2") - self.make_discussion_module("courseware-3", "B", "1") - self.make_discussion_module("courseware-4", "B", "2") - self.make_discussion_module("courseware-5", "C", "1") + self.make_discussion_xblock("courseware-1", "A", "1") + self.make_discussion_xblock("courseware-2", "A", "2") + self.make_discussion_xblock("courseware-3", "B", "1") + self.make_discussion_xblock("courseware-4", "B", "2") + self.make_discussion_xblock("courseware-5", "C", "1") actual = self.get_course_topics() expected = { "courseware_topics": [ @@ -343,13 +345,13 @@ class GetCourseTopicsTest(UrlResetMixin, ModuleStoreTestCase): "Z": {"id": "non-courseware-4", "sort_key": "W"}, } self.store.update_item(self.course, self.user.id) - self.make_discussion_module("courseware-1", "First", "A", sort_key="D") - self.make_discussion_module("courseware-2", "First", "B", sort_key="B") - self.make_discussion_module("courseware-3", "First", "C", sort_key="E") - self.make_discussion_module("courseware-4", "Second", "A", sort_key="F") - self.make_discussion_module("courseware-5", "Second", "B", sort_key="G") - self.make_discussion_module("courseware-6", "Second", "C") - self.make_discussion_module("courseware-7", "Second", "D", sort_key="A") + self.make_discussion_xblock("courseware-1", "First", "A", sort_key="D") + self.make_discussion_xblock("courseware-2", "First", "B", sort_key="B") + self.make_discussion_xblock("courseware-3", "First", "C", sort_key="E") + self.make_discussion_xblock("courseware-4", "Second", "A", sort_key="F") + self.make_discussion_xblock("courseware-5", "Second", "B", sort_key="G") + self.make_discussion_xblock("courseware-6", "Second", "C") + self.make_discussion_xblock("courseware-7", "Second", "D", sort_key="A") actual = self.get_course_topics() expected = { @@ -411,21 +413,21 @@ class GetCourseTopicsTest(UrlResetMixin, ModuleStoreTestCase): ) with self.store.bulk_operations(self.course.id, emit_signals=False): - self.make_discussion_module("courseware-1", "First", "Everybody") - self.make_discussion_module( + self.make_discussion_xblock("courseware-1", "First", "Everybody") + self.make_discussion_xblock( "courseware-2", "First", "Cohort A", group_access={self.partition.id: [self.partition.groups[0].id]} ) - self.make_discussion_module( + self.make_discussion_xblock( "courseware-3", "First", "Cohort B", group_access={self.partition.id: [self.partition.groups[1].id]} ) - self.make_discussion_module("courseware-4", "Second", "Staff Only", visible_to_staff_only=True) - self.make_discussion_module( + self.make_discussion_xblock("courseware-4", "Second", "Staff Only", visible_to_staff_only=True) + self.make_discussion_xblock( "courseware-5", "Second", "Future Start Date", @@ -507,8 +509,8 @@ class GetCourseTopicsTest(UrlResetMixin, ModuleStoreTestCase): """ topic_id_1 = "topic_id_1" topic_id_2 = "topic_id_2" - self.make_discussion_module(topic_id_1, "test_category_1", "test_target_1") - self.make_discussion_module(topic_id_2, "test_category_2", "test_target_2") + self.make_discussion_xblock(topic_id_1, "test_category_1", "test_target_1") + self.make_discussion_xblock(topic_id_2, "test_category_2", "test_target_2") actual = get_course_topics(self.request, self.course.id, {"topic_id_1", "topic_id_2"}) self.assertEqual( actual, diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index 9fc884bd48..acc1794ff4 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -164,7 +164,7 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): def create_course(self, modules_count, module_store, topics): """ - Create a course in a specified module store with discussion module and topics + Create a course in a specified module store with discussion xblocks and topics """ course = CourseFactory.create( org="a", @@ -176,7 +176,7 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): ) CourseEnrollmentFactory.create(user=self.user, course_id=course.id) course_url = reverse("course_topics", kwargs={"course_id": unicode(course.id)}) - # add some discussion modules + # add some discussion xblocks for i in range(modules_count): ItemFactory.create( parent_location=course.location, @@ -188,9 +188,9 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): ) return course_url - def make_discussion_module(self, topic_id, category, subcategory, **kwargs): + def make_discussion_xblock(self, topic_id, category, subcategory, **kwargs): """ - Build a discussion module in self.course + Build a discussion xblock in self.course """ ItemFactory.create( parent_location=self.course.location, @@ -249,7 +249,7 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): Tests discussion topic does not exist for the given topic id. """ topic_id = "courseware-topic-id" - self.make_discussion_module(topic_id, "test_category", "test_target") + self.make_discussion_xblock(topic_id, "test_category", "test_target") url = "{}?topic_id=invalid_topic_id".format(self.url) response = self.client.get(url) self.assert_response_correct( @@ -264,8 +264,8 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): """ topic_id_1 = "topic_id_1" topic_id_2 = "topic_id_2" - self.make_discussion_module(topic_id_1, "test_category_1", "test_target_1") - self.make_discussion_module(topic_id_2, "test_category_2", "test_target_2") + self.make_discussion_xblock(topic_id_1, "test_category_1", "test_target_1") + self.make_discussion_xblock(topic_id_2, "test_category_2", "test_target_2") url = "{}?topic_id=topic_id_1,topic_id_2".format(self.url) response = self.client.get(url) self.assert_response_correct( diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index abb8a0f47d..813066f3e9 100644 --- a/lms/djangoapps/django_comment_client/forum/tests.py +++ b/lms/djangoapps/django_comment_client/forum/tests.py @@ -623,8 +623,8 @@ class SingleThreadContentGroupTestCase(UrlResetMixin, ContentGroupTestCase): thread_id = "test_thread_id" mock_request.side_effect = make_mock_request_impl(course=self.course, text="dummy content", thread_id=thread_id) - for discussion_module in [self.alpha_module, self.beta_module, self.global_module]: - self.assert_can_access(self.staff_user, discussion_module.discussion_id, thread_id, True) + for discussion_xblock in [self.alpha_module, self.beta_module, self.global_module]: + self.assert_can_access(self.staff_user, discussion_xblock.discussion_id, thread_id, True) def test_alpha_user(self, mock_request): """ @@ -634,8 +634,8 @@ class SingleThreadContentGroupTestCase(UrlResetMixin, ContentGroupTestCase): thread_id = "test_thread_id" mock_request.side_effect = make_mock_request_impl(course=self.course, text="dummy content", thread_id=thread_id) - for discussion_module in [self.alpha_module, self.global_module]: - self.assert_can_access(self.alpha_user, discussion_module.discussion_id, thread_id, True) + for discussion_xblock in [self.alpha_module, self.global_module]: + self.assert_can_access(self.alpha_user, discussion_xblock.discussion_id, thread_id, True) self.assert_can_access(self.alpha_user, self.beta_module.discussion_id, thread_id, False) @@ -647,8 +647,8 @@ class SingleThreadContentGroupTestCase(UrlResetMixin, ContentGroupTestCase): thread_id = "test_thread_id" mock_request.side_effect = make_mock_request_impl(course=self.course, text="dummy content", thread_id=thread_id) - for discussion_module in [self.beta_module, self.global_module]: - self.assert_can_access(self.beta_user, discussion_module.discussion_id, thread_id, True) + for discussion_xblock in [self.beta_module, self.global_module]: + self.assert_can_access(self.beta_user, discussion_xblock.discussion_id, thread_id, True) self.assert_can_access(self.beta_user, self.alpha_module.discussion_id, thread_id, False) diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index b808b562bc..2ba8f558a9 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -269,10 +269,8 @@ def forum_form_discussion(request, course_key): 'threads': json.dumps(threads), 'thread_pages': query_params['num_pages'], 'user_info': json.dumps(user_info, default=lambda x: None), - 'can_create_comment': json.dumps( - has_permission(request.user, "create_comment", course.id)), - 'can_create_subcomment': json.dumps( - has_permission(request.user, "create_sub_comment", course.id)), + 'can_create_comment': has_permission(request.user, "create_comment", course.id), + 'can_create_subcomment': has_permission(request.user, "create_sub_comment", course.id), 'can_create_thread': has_permission(request.user, "create_thread", course.id), 'flag_moderator': bool( has_permission(request.user, 'openclose_thread', course.id) or @@ -381,10 +379,8 @@ def single_thread(request, course_key, discussion_id, thread_id): 'csrf': csrf(request)['csrf_token'], 'init': '', # TODO: What is this? 'user_info': json.dumps(user_info), - 'can_create_comment': json.dumps( - has_permission(request.user, "create_comment", course.id)), - 'can_create_subcomment': json.dumps( - has_permission(request.user, "create_sub_comment", course.id)), + 'can_create_comment': has_permission(request.user, "create_comment", course.id), + 'can_create_subcomment': has_permission(request.user, "create_sub_comment", course.id), 'can_create_thread': has_permission(request.user, "create_thread", course.id), 'annotated_content_info': json.dumps(annotated_content_info), 'course': course, diff --git a/lms/djangoapps/django_comment_client/tests/test_utils.py b/lms/djangoapps/django_comment_client/tests/test_utils.py index 3726195886..968beb4025 100644 --- a/lms/djangoapps/django_comment_client/tests/test_utils.py +++ b/lms/djangoapps/django_comment_client/tests/test_utils.py @@ -177,29 +177,29 @@ class CoursewareContextTestCase(ModuleStoreTestCase): @ddt.data((ModuleStoreEnum.Type.mongo, 2), (ModuleStoreEnum.Type.split, 1)) @ddt.unpack - def test_get_accessible_discussion_modules(self, modulestore_type, expected_discussion_modules): + def test_get_accessible_discussion_xblocks(self, modulestore_type, expected_discussion_xblocks): """ - Tests that the accessible discussion modules having no parents do not get fetched for split modulestore. + Tests that the accessible discussion xblocks having no parents do not get fetched for split modulestore. """ course = CourseFactory.create(default_store=modulestore_type) - # Create a discussion module. + # Create a discussion xblock. test_discussion = self.store.create_child(self.user.id, course.location, 'discussion', 'test_discussion') - # Assert that created discussion module is not an orphan. + # Assert that created discussion xblock is not an orphan. self.assertNotIn(test_discussion.location, self.store.get_orphans(course.id)) - # Assert that there is only one discussion module in the course at the moment. - self.assertEqual(len(utils.get_accessible_discussion_modules(course, self.user)), 1) + # Assert that there is only one discussion xblock in the course at the moment. + self.assertEqual(len(utils.get_accessible_discussion_xblocks(course, self.user)), 1) - # Add an orphan discussion module to that course + # Add an orphan discussion xblock to that course orphan = course.id.make_usage_key('discussion', 'orphan_discussion') self.store.create_item(self.user.id, orphan.course_key, orphan.block_type, block_id=orphan.block_id) - # Assert that the discussion module is an orphan. + # Assert that the discussion xblock is an orphan. self.assertIn(orphan, self.store.get_orphans(course.id)) - self.assertEqual(len(utils.get_accessible_discussion_modules(course, self.user)), expected_discussion_modules) + self.assertEqual(len(utils.get_accessible_discussion_xblocks(course, self.user)), expected_discussion_xblocks) @attr('shard_3') @@ -262,7 +262,7 @@ class CachedDiscussionIdMapTestCase(ModuleStoreTestCase): with self.assertRaises(utils.DiscussionIdMapIsNotCached): utils.get_cached_discussion_key(self.course, 'test_discussion_id') - def test_module_does_not_have_required_keys(self): + def test_xblock_does_not_have_required_keys(self): self.assertTrue(utils.has_required_keys(self.discussion)) self.assertFalse(utils.has_required_keys(self.bad_discussion)) @@ -505,7 +505,7 @@ class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase): cohorted_if_in_list=True ) - def test_get_unstarted_discussion_modules(self): + def test_get_unstarted_discussion_xblocks(self): later = datetime.datetime(datetime.MAXYEAR, 1, 1, tzinfo=django_utc()) self.create_discussion("Chapter 1", "Discussion 1", start=later) @@ -1026,7 +1026,7 @@ class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase): @attr('shard_1') class ContentGroupCategoryMapTestCase(CategoryMapTestMixin, ContentGroupTestCase): """ - Tests `get_discussion_category_map` on discussion modules which are + Tests `get_discussion_category_map` on discussion xblocks which are only visible to some content groups. """ def test_staff_user(self): diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 54a1fc2f78..e713ce05e3 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -108,44 +108,44 @@ def has_forum_access(uname, course_id, rolename): return role.users.filter(username=uname).exists() -def has_required_keys(module): +def has_required_keys(xblock): """ - Returns True iff module has the proper attributes for generating metadata + Returns True iff xblock has the proper attributes for generating metadata with get_discussion_id_map_entry() """ for key in ('discussion_id', 'discussion_category', 'discussion_target'): - if getattr(module, key, None) is None: + if getattr(xblock, key, None) is None: log.debug( "Required key '%s' not in discussion %s, leaving out of category map", key, - module.location + xblock.location ) return False return True -def get_accessible_discussion_modules(course, user, include_all=False): # pylint: disable=invalid-name +def get_accessible_discussion_xblocks(course, user, include_all=False): # pylint: disable=invalid-name """ - Return a list of all valid discussion modules in this course that + Return a list of all valid discussion xblocks in this course that are accessible to the given user. """ - all_modules = modulestore().get_items(course.id, qualifiers={'category': 'discussion'}, include_orphans=False) + all_xblocks = modulestore().get_items(course.id, qualifiers={'category': 'discussion'}, include_orphans=False) return [ - module for module in all_modules - if has_required_keys(module) and (include_all or has_access(user, 'load', module, course.id)) + xblock for xblock in all_xblocks + if has_required_keys(xblock) and (include_all or has_access(user, 'load', xblock, course.id)) ] -def get_discussion_id_map_entry(module): +def get_discussion_id_map_entry(xblock): """ Returns a tuple of (discussion_id, metadata) suitable for inclusion in the results of get_discussion_id_map(). """ return ( - module.discussion_id, + xblock.discussion_id, { - "location": module.location, - "title": module.discussion_category.split("/")[-1].strip() + " / " + module.discussion_target + "location": xblock.location, + "title": xblock.discussion_category.split("/")[-1].strip() + " / " + xblock.discussion_target } ) @@ -157,7 +157,7 @@ class DiscussionIdMapIsNotCached(Exception): def get_cached_discussion_key(course, discussion_id): """ - Returns the usage key of the discussion module associated with discussion_id if it is cached. If the discussion id + Returns the usage key of the discussion xblock associated with discussion_id if it is cached. If the discussion id map is cached but does not contain discussion_id, returns None. If the discussion id map is not cached for course, raises a DiscussionIdMapIsNotCached exception. """ @@ -172,7 +172,7 @@ def get_cached_discussion_key(course, discussion_id): def get_cached_discussion_id_map(course, discussion_ids, user): """ - Returns a dict mapping discussion_ids to respective discussion module metadata if it is cached and visible to the + Returns a dict mapping discussion_ids to respective discussion xblock metadata if it is cached and visible to the user. If not, returns the result of get_discussion_id_map """ try: @@ -181,10 +181,10 @@ def get_cached_discussion_id_map(course, discussion_ids, user): key = get_cached_discussion_key(course, discussion_id) if not key: continue - module = modulestore().get_item(key) - if not (has_required_keys(module) and has_access(user, 'load', module, course.id)): + xblock = modulestore().get_item(key) + if not (has_required_keys(xblock) and has_access(user, 'load', xblock, course.id)): continue - entries.append(get_discussion_id_map_entry(module)) + entries.append(get_discussion_id_map_entry(xblock)) return dict(entries) except DiscussionIdMapIsNotCached: return get_discussion_id_map(course, user) @@ -192,10 +192,10 @@ def get_cached_discussion_id_map(course, discussion_ids, user): def get_discussion_id_map(course, user): """ - Transform the list of this course's discussion modules (visible to a given user) into a dictionary of metadata keyed + Transform the list of this course's discussion xblocks (visible to a given user) into a dictionary of metadata keyed by discussion_id. """ - return dict(map(get_discussion_id_map_entry, get_accessible_discussion_modules(course, user))) + return dict(map(get_discussion_id_map_entry, get_accessible_discussion_xblocks(course, user))) def _filter_unstarted_categories(category_map, course): @@ -256,7 +256,7 @@ def _sort_map_entries(category_map, sort_alpha): def get_discussion_category_map(course, user, cohorted_if_in_list=False, exclude_unstarted=True): """ - Transform the list of this course's discussion modules into a recursive dictionary structure. This is used + Transform the list of this course's discussion xblocks into a recursive dictionary structure. This is used to render the discussion category map in the discussion tab sidebar for a given user. Args: @@ -301,18 +301,21 @@ def get_discussion_category_map(course, user, cohorted_if_in_list=False, exclude """ unexpanded_category_map = defaultdict(list) - modules = get_accessible_discussion_modules(course, user) + xblocks = get_accessible_discussion_xblocks(course, user) course_cohort_settings = get_course_cohort_settings(course.id) - for module in modules: - id = module.discussion_id - title = module.discussion_target - sort_key = module.sort_key - category = " / ".join([x.strip() for x in module.discussion_category.split("/")]) - # Handle case where module.start is None - entry_start_date = module.start if module.start else datetime.max.replace(tzinfo=pytz.UTC) - unexpanded_category_map[category].append({"title": title, "id": id, "sort_key": sort_key, "start_date": entry_start_date}) + for xblock in xblocks: + discussion_id = xblock.discussion_id + title = xblock.discussion_target + sort_key = xblock.sort_key + category = " / ".join([x.strip() for x in xblock.discussion_category.split("/")]) + # Handle case where xblock.start is None + entry_start_date = xblock.start if xblock.start else datetime.max.replace(tzinfo=pytz.UTC) + unexpanded_category_map[category].append({"title": title, + "id": discussion_id, + "sort_key": sort_key, + "start_date": entry_start_date}) category_map = {"entries": defaultdict(dict), "subcategories": defaultdict(dict)} for category_path, entries in unexpanded_category_map.items(): @@ -385,7 +388,7 @@ def get_discussion_category_map(course, user, cohorted_if_in_list=False, exclude return _filter_unstarted_categories(category_map, course) if exclude_unstarted else category_map -def discussion_category_id_access(course, user, discussion_id, module=None): +def discussion_category_id_access(course, user, discussion_id, xblock=None): """ Returns True iff the given discussion_id is accessible for user in course. Assumes that the commentable identified by discussion_id has a null or 'course' context. @@ -395,12 +398,12 @@ def discussion_category_id_access(course, user, discussion_id, module=None): if discussion_id in course.top_level_discussion_topic_ids: return True try: - if not module: + if not xblock: key = get_cached_discussion_key(course, discussion_id) if not key: return False - module = modulestore().get_item(key) - return has_required_keys(module) and has_access(user, 'load', module, course.id) + xblock = modulestore().get_item(key) + return has_required_keys(xblock) and has_access(user, 'load', xblock, course.id) except DiscussionIdMapIsNotCached: return discussion_id in get_discussion_categories_ids(course, user) @@ -417,7 +420,7 @@ def get_discussion_categories_ids(course, user, include_all=False): """ accessible_discussion_ids = [ - module.discussion_id for module in get_accessible_discussion_modules(course, user, include_all=include_all) + xblock.discussion_id for xblock in get_accessible_discussion_xblocks(course, user, include_all=include_all) ] return course.top_level_discussion_topic_ids + accessible_discussion_ids diff --git a/lms/templates/discussion/_discussion_inline.html b/lms/templates/discussion/_discussion_inline.html new file mode 100644 index 0000000000..95eba3477b --- /dev/null +++ b/lms/templates/discussion/_discussion_inline.html @@ -0,0 +1,28 @@ +<%page expression_filter="h"/> +<%include file="_underscore_templates.html" /> +<%! +from django.utils.translation import ugettext as _ +from json import dumps as json_dumps +from openedx.core.djangolib.js_utils import js_escaped_string +%> + +
+ + ${_("Show Discussion")} + + % if can_create_thread: + + % endif +
+ diff --git a/lms/templates/discussion/_discussion_module_studio.html b/lms/templates/discussion/_discussion_inline_studio.html similarity index 92% rename from lms/templates/discussion/_discussion_module_studio.html rename to lms/templates/discussion/_discussion_inline_studio.html index d1cba06449..be1fe33533 100644 --- a/lms/templates/discussion/_discussion_module_studio.html +++ b/lms/templates/discussion/_discussion_inline_studio.html @@ -1,6 +1,7 @@ <%! from django.utils.translation import ugettext as _ %> +<%page expression_filter="h"/> -
+

diff --git a/lms/templates/discussion/_discussion_module.html b/lms/templates/discussion/_discussion_module.html deleted file mode 100644 index 6a8cb9785a..0000000000 --- a/lms/templates/discussion/_discussion_module.html +++ /dev/null @@ -1,14 +0,0 @@ -<%page expression_filter="h"/> -<%include file="_underscore_templates.html" /> -<%! -from django.utils.translation import ugettext as _ -%> - -

- - ${_("Show Discussion")} - - % if can_create_thread: - - % endif -
diff --git a/openedx/core/djangoapps/content/course_structures/tests.py b/openedx/core/djangoapps/content/course_structures/tests.py index b87385a9df..78e673dd5b 100644 --- a/openedx/core/djangoapps/content/course_structures/tests.py +++ b/openedx/core/djangoapps/content/course_structures/tests.py @@ -32,12 +32,12 @@ class CourseStructureTaskTests(ModuleStoreTestCase): super(CourseStructureTaskTests, self).setUp() self.course = CourseFactory.create(org='TestX', course='TS101', run='T1') self.section = ItemFactory.create(parent=self.course, category='chapter', display_name='Test Section') - self.discussion_module_1 = ItemFactory.create( + self.discussion_xblock_1 = ItemFactory.create( parent=self.course, category='discussion', discussion_id='test_discussion_id_1' ) - self.discussion_module_2 = ItemFactory.create( + self.discussion_xblock_2 = ItemFactory.create( parent=self.course, category='discussion', discussion_id='test_discussion_id_2' diff --git a/openedx/core/lib/xblock_builtin/README.rst b/openedx/core/lib/xblock_builtin/README.rst new file mode 100644 index 0000000000..ce8d58b767 --- /dev/null +++ b/openedx/core/lib/xblock_builtin/README.rst @@ -0,0 +1,17 @@ +Open edX: Built-in XBlocks +-------------------------- + +This area is meant for exceptional and hopefully temporary cases where an +XBlock is integral to the functionality of the Open edX platform. + +This is not a pattern we wish for normal XBlocks to follow; they should live in +their own repo. + +Discussion XBlock +================= + +This XBlock was converted from an XModule, and will hopefully be pulled out of +edx-platform into its own repo at some point. From discussions, it's not too +difficult to move the server-side code , but the client-side code is used by +the discussion board tab and the team discussion, so for now, must remain in +edx-platform. diff --git a/openedx/core/lib/xblock_builtin/__init__.py b/openedx/core/lib/xblock_builtin/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/core/lib/xblock_builtin/xblock_discussion/__init__.py b/openedx/core/lib/xblock_builtin/xblock_discussion/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/core/lib/xblock_builtin/xblock_discussion/setup.py b/openedx/core/lib/xblock_builtin/xblock_discussion/setup.py new file mode 100644 index 0000000000..215d21b6fb --- /dev/null +++ b/openedx/core/lib/xblock_builtin/xblock_discussion/setup.py @@ -0,0 +1,35 @@ +""" +Setup for discussion-forum XBlock. +""" + +import os +from setuptools import setup + + +def package_data(pkg, root_list): + """ + Generic function to find package_data for `pkg` under `root`. + """ + data = [] + for root in root_list: + for dirname, _, files in os.walk(os.path.join(pkg, root)): + for fname in files: + data.append(os.path.relpath(os.path.join(dirname, fname), pkg)) + + return {pkg: data} + + +setup( + name='xblock-discussion', + version='0.1', + description='XBlock - Discussion', + install_requires=[ + 'XBlock', + ], + entry_points={ + 'xblock.v1': [ + 'discussion = xblock_discussion:DiscussionXBlock' + ] + }, + package_data=package_data("xblock_discussion", ["static"]), +) diff --git a/openedx/core/lib/xblock_builtin/xblock_discussion/xblock_discussion.py b/openedx/core/lib/xblock_builtin/xblock_discussion/xblock_discussion.py new file mode 100644 index 0000000000..50e1c57840 --- /dev/null +++ b/openedx/core/lib/xblock_builtin/xblock_discussion/xblock_discussion.py @@ -0,0 +1,136 @@ +# -*- coding: utf-8 -*- +""" +Discussion XBlock +""" +import logging + +from xblockutils.resources import ResourceLoader +from xblockutils.studio_editable import StudioEditableXBlockMixin + +from xblock.core import XBlock +from xblock.fields import Scope, String, UNIQUE_ID +from xblock.fragment import Fragment + +log = logging.getLogger(__name__) +loader = ResourceLoader(__name__) # pylint: disable=invalid-name + + +def _(text): + """ + A noop underscore function that marks strings for extraction. + """ + return text + + +@XBlock.needs('user') +@XBlock.needs('i18n') +class DiscussionXBlock(XBlock, StudioEditableXBlockMixin): + """ + Provides a discussion forum that is inline with other content in the courseware. + """ + discussion_id = String(scope=Scope.settings, default=UNIQUE_ID, force_export=True) + display_name = String( + display_name=_("Display Name"), + help=_("Display name for this component"), + default="Discussion", + scope=Scope.settings + ) + discussion_category = String( + display_name=_("Category"), + default=_("Week 1"), + help=_( + "A category name for the discussion. " + "This name appears in the left pane of the discussion forum for the course." + ), + scope=Scope.settings + ) + discussion_target = String( + display_name=_("Subcategory"), + default="Topic-Level Student-Visible Label", + help=_( + "A subcategory name for the discussion. " + "This name appears in the left pane of the discussion forum for the course." + ), + scope=Scope.settings + ) + sort_key = String(scope=Scope.settings) + + editable_fields = ["display_name", "discussion_category", "discussion_target"] + + has_author_view = True # Tells Studio to use author_view + + @property + def course_key(self): + """ + :return: int course id + + NB: The goal is to move this XBlock out of edx-platform, and so we use + scope_ids.usage_id instead of runtime.course_id so that the code will + continue to work with workbench-based testing. + """ + return getattr(self.scope_ids.usage_id, 'course_key', None) + + @property + def django_user(self): + """ + Returns django user associated with user currently interacting + with the XBlock. + """ + user_service = self.runtime.service(self, 'user') + if not user_service: + return None + return user_service._django_user # pylint: disable=protected-access + + def has_permission(self, permission): + """ + Encapsulates lms specific functionality, as `has_permission` is not + importable outside of lms context, namely in tests. + + :param user: + :param str permission: Permission + :rtype: bool + """ + # normal import causes the xmodule_assets command to fail due to circular import - hence importing locally + from django_comment_client.permissions import has_permission # pylint: disable=import-error + + return has_permission(self.django_user, permission, self.course_key) + + def student_view(self, context=None): + """ + Renders student view for LMS. + """ + fragment = Fragment() + + course = self.runtime.modulestore.get_course(self.course_key) + + context = { + 'discussion_id': self.discussion_id, + 'user': self.django_user, + 'course': course, + 'course_id': self.course_key, + 'can_create_thread': self.has_permission("create_thread"), + 'can_create_comment': self.has_permission("create_comment"), + 'can_create_subcomment': self.has_permission("create_subcomment"), + } + + fragment.add_content(self.runtime.render_template('discussion/_discussion_inline.html', context)) + fragment.initialize_js('DiscussionInlineBlock') + + return fragment + + def author_view(self, context=None): # pylint: disable=unused-argument + """ + Renders author view for Studio. + """ + fragment = Fragment() + fragment.add_content(self.runtime.render_template( + 'discussion/_discussion_inline_studio.html', + {'discussion_id': self.discussion_id} + )) + return fragment + + def student_view_data(self): + """ + Returns a JSON representation of the student_view of this XBlock. + """ + return {'topic_id': self.discussion_id} diff --git a/requirements/edx/local.txt b/requirements/edx/local.txt index 48bc8294da..4bd03e4049 100644 --- a/requirements/edx/local.txt +++ b/requirements/edx/local.txt @@ -8,3 +8,5 @@ -e common/lib/sandbox-packages -e common/lib/symmath -e common/lib/xmodule + +-e openedx/core/lib/xblock_builtin/xblock_discussion