diff --git a/lms/djangoapps/courseware/management/commands/dump_course_structure.py b/lms/djangoapps/courseware/management/commands/dump_course_structure.py index d0e0f744c705..5c73a83dc7d6 100644 --- a/lms/djangoapps/courseware/management/commands/dump_course_structure.py +++ b/lms/djangoapps/courseware/management/commands/dump_course_structure.py @@ -24,8 +24,8 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey from xblock.fields import Scope +from xblocks_contrib.discussion import DiscussionXBlock -from xmodule.discussion_block import DiscussionXBlock from xmodule.modulestore.django import modulestore from xmodule.modulestore.inheritance import compute_inherited_metadata, own_metadata diff --git a/lms/djangoapps/courseware/tests/test_discussion_xblock.py b/lms/djangoapps/courseware/tests/test_discussion_xblock.py index fda37ef51b82..c7e92b9971cf 100644 --- a/lms/djangoapps/courseware/tests/test_discussion_xblock.py +++ b/lms/djangoapps/courseware/tests/test_discussion_xblock.py @@ -19,6 +19,7 @@ from opaque_keys.edx.keys import CourseKey from web_fragments.fragment import Fragment from xblock.field_data import DictFieldData +from xblocks_contrib.discussion import DiscussionXBlock from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory from lms.djangoapps.course_api.blocks.tests.helpers import deserialize_usage_key @@ -26,7 +27,6 @@ from lms.djangoapps.courseware.tests.helpers import XModuleRenderingTestBase from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration, Provider from openedx.core.djangoapps.discussions.services import DiscussionConfigService -from xmodule.discussion_block import DiscussionXBlock from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import BlockFactory, ToyCourseFactory from xmodule.tests.helpers import mock_render_template diff --git a/openedx/core/djangoapps/discussions/utils.py b/openedx/core/djangoapps/discussions/utils.py index 99b0f06a63e4..d358e1220e7f 100644 --- a/openedx/core/djangoapps/discussions/utils.py +++ b/openedx/core/djangoapps/discussions/utils.py @@ -5,6 +5,7 @@ from typing import Dict, List, Optional, Tuple # noqa: UP035 from opaque_keys.edx.keys import CourseKey +from xblocks_contrib.discussion import DiscussionXBlock # pylint: disable=wrong-import-order from lms.djangoapps.courseware.access import has_access from openedx.core.djangoapps.course_groups.cohorts import get_cohort_names, is_course_cohorted @@ -13,7 +14,6 @@ from openedx.core.lib.courses import get_course_by_id from openedx.core.types import User from xmodule.course_block import CourseBlock # pylint: disable=wrong-import-order -from xmodule.discussion_block import DiscussionXBlock from xmodule.modulestore.django import modulestore # pylint: disable=wrong-import-order from xmodule.partitions.partitions import ( # pylint: disable=wrong-import-order ENROLLMENT_TRACK_PARTITION_ID, diff --git a/openedx/envs/common.py b/openedx/envs/common.py index 31b0015d34d9..dcd77652e99c 100644 --- a/openedx/envs/common.py +++ b/openedx/envs/common.py @@ -2104,15 +2104,6 @@ def add_optional_apps(optional_apps, installed_apps): # .. toggle_target_removal_date: 2026-04-10 USE_EXTRACTED_HTML_BLOCK = True -# .. toggle_name: USE_EXTRACTED_DISCUSSION_BLOCK -# .. toggle_default: False -# .. toggle_implementation: DjangoSetting -# .. toggle_description: Enables the use of the extracted Discussion XBlock, which has been shifted to the 'openedx/xblocks-contrib' repo. -# .. toggle_use_cases: temporary -# .. toggle_warning: Not production-ready until relevant subtask https://github.com/openedx/edx-platform/issues/34827 is done. -# .. toggle_creation_date: 2024-11-10 -# .. toggle_target_removal_date: 2026-04-10 -USE_EXTRACTED_DISCUSSION_BLOCK = True # .. toggle_name: USE_EXTRACTED_PROBLEM_BLOCK # .. toggle_default: False diff --git a/pyproject.toml b/pyproject.toml index 44488938ec95..2076330f5b1f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,10 +22,10 @@ chapter = "xmodule.seq_block:SectionBlock" conditional = "xmodule.conditional_block:ConditionalBlock" course = "xmodule.course_block:CourseBlock" course_info = "xmodule.html_block:CourseInfoBlock" +discussion = "xblocks_contrib:DiscussionXBlock" customtag = "xmodule.template_block:CustomTagBlock" custom_tag_template = "xmodule.template_block:CustomTagTemplateBlock" discuss = "xmodule.template_block:TranslateCustomTagBlock" -discussion = "xmodule.discussion_block:DiscussionXBlock" error = "xmodule.error_block:ErrorBlock" hidden = "xmodule.hidden_block:HiddenBlock" html = "xmodule.html_block:HtmlBlock" diff --git a/xmodule/discussion_block.py b/xmodule/discussion_block.py deleted file mode 100644 index fbbe3298803a..000000000000 --- a/xmodule/discussion_block.py +++ /dev/null @@ -1,310 +0,0 @@ -""" -Discussion XBlock -""" - -import logging -import urllib - -from django.conf import settings -from django.contrib.staticfiles.storage import staticfiles_storage -from django.template.loader import render_to_string -from django.urls import reverse -from django.utils.translation import get_language_bidi -from web_fragments.fragment import Fragment -from xblock.completable import XBlockCompletionMode -from xblock.core import XBlock -from xblock.fields import UNIQUE_ID, Scope, String -from xblock.utils.resources import ResourceLoader -from xblock.utils.studio_editable import StudioEditableXBlockMixin -from xblocks_contrib.discussion import DiscussionXBlock as _ExtractedDiscussionXBlock - -from openedx.core.djangolib.markup import HTML, Text -from openedx.core.lib.xblock_utils import get_css_dependencies, get_js_dependencies -from xmodule.xml_block import XmlMixin - -log = logging.getLogger(__name__) -loader = ResourceLoader("lms") # pylint: disable=invalid-name - - -def _(text): - """ - A noop underscore function that marks strings for extraction. - """ - return text - - -@XBlock.needs('user') # pylint: disable=abstract-method -@XBlock.needs('i18n') -@XBlock.needs('mako') -@XBlock.wants('discussion_config_service') -class _BuiltInDiscussionXBlock(XBlock, StudioEditableXBlockMixin, - XmlMixin): # pylint: disable=abstract-method - """ - Provides a discussion forum that is inline with other content in the courseware. - """ - is_extracted = False - completion_mode = XBlockCompletionMode.EXCLUDED - - discussion_id = String(scope=Scope.settings, default=UNIQUE_ID) - display_name = String( - display_name=_("Display Name"), - help=_("The 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 discussion_config_service(self): - """ - Returns discussion configuration service. - """ - return self.runtime.service(self, 'discussion_config_service') - - @property - def is_visible(self): - """ - Discussion Xblock does not support new OPEN_EDX provider - """ - if self.discussion_config_service: - return self.discussion_config_service.is_discussion_visible(self.context_key) - return False - - @property - def is_discussion_enabled(self): - """ - Returns True if discussions are enabled; else False - """ - if self.discussion_config_service: - return self.discussion_config_service.is_discussion_enabled() - return False - - @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 - - @staticmethod - def vendor_js_dependencies(): - """ - Returns list of vendor JS files that this XBlock depends on. - - The helper function that it uses to obtain the list of vendor JS files - works in conjunction with the Django pipeline to ensure that in development mode - the files are loaded individually, but in production just the single bundle is loaded. - """ - return get_js_dependencies('discussion_vendor') - - @staticmethod - def js_dependencies(): - """ - Returns list of JS files that this XBlock depends on. - - The helper function that it uses to obtain the list of JS files - works in conjunction with the Django pipeline to ensure that in development mode - the files are loaded individually, but in production just the single bundle is loaded. - """ - return get_js_dependencies('discussion') - - @staticmethod - def css_dependencies(): - """ - Returns list of CSS files that this XBlock depends on. - - The helper function that it uses to obtain the list of CSS files - works in conjunction with the Django pipeline to ensure that in development mode - the files are loaded individually, but in production just the single bundle is loaded. - """ - if get_language_bidi(): - return get_css_dependencies('style-inline-discussion-rtl') - else: - return get_css_dependencies('style-inline-discussion') - - def add_resource_urls(self, fragment): - """ - Adds URLs for JS and CSS resources that this XBlock depends on to `fragment`. - """ - # Head dependencies - for vendor_js_file in self.vendor_js_dependencies(): - fragment.add_resource_url(staticfiles_storage.url(vendor_js_file), "application/javascript", "head") - - for css_file in self.css_dependencies(): - fragment.add_css_url(staticfiles_storage.url(css_file)) - - # Body dependencies - for js_file in self.js_dependencies(): - fragment.add_javascript_url(staticfiles_storage.url(js_file)) - - 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 - """ - if self.discussion_config_service: - return self.discussion_config_service.has_permission(self.django_user, permission, self.context_key) - return False - - def student_view(self, context=None): - """ - Renders student view for LMS. - """ - fragment = Fragment() - - if not self.is_visible: - return fragment - - self.add_resource_urls(fragment) - login_msg = '' - - if not self.django_user.is_authenticated: - qs = urllib.parse.urlencode({ - 'course_id': self.context_key, - 'enrollment_action': 'enroll', - 'email_opt_in': False, - }) - login_msg = Text(_("You are not signed in. To view the discussion content, {sign_in_link} or " - "{register_link}, and enroll in this course.")).format( - sign_in_link=HTML('{sign_in_label}').format( - sign_in_label=_('sign in'), - url='{}?{}'.format(reverse('signin_user'), qs), - ), - register_link=HTML('{register_label}').format( - register_label=_('register'), - url='{}?{}'.format(reverse('register_user'), qs), - ), - ) - if self.is_discussion_enabled: - context = { - 'discussion_id': self.discussion_id, - 'display_name': self.display_name if self.display_name else _("Discussion"), - 'user': self.django_user, - 'course_id': self.context_key, - 'discussion_category': self.discussion_category, - 'discussion_target': self.discussion_target, - 'can_create_thread': self.has_permission("create_thread"), - 'can_create_comment': self.has_permission("create_comment"), - 'can_create_subcomment': self.has_permission("create_sub_comment"), - 'login_msg': login_msg, - 'PLATFORM_NAME': settings.PLATFORM_NAME, - 'enable_discussion_home_panel': settings.FEATURES.get("ENABLE_DISCUSSION_HOME_PANEL", False), - } - fragment.add_content( - render_to_string('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() - # For historic reasons, this template is in the LMS templates folder: - context = { - 'discussion_id': self.discussion_id, - 'is_visible': self.is_visible, - } - fragment.add_content( - loader.render_django_template('templates/discussion/_discussion_inline_studio.html', context) - ) - return fragment - - def student_view_data(self): - """ - Returns a JSON representation of the student_view of this XBlock. - """ - return {'topic_id': self.discussion_id} - - @classmethod - def parse_xml(cls, node, runtime, keys): - """ - Parses OLX into XBlock. - - This method is overridden here to allow parsing legacy OLX, coming from discussion XModule. - XBlock stores all the associated data, fields and children in a XML element inlined into vertical XML file - XModule stored only minimal data on the element included into vertical XML and used a dedicated "discussion" - folder in OLX to store fields and children. Also, some info was put into "policy.json" file. - - If no external data sources are found (file in "discussion" folder), it is exactly equivalent to base method - XBlock.parse_xml. Otherwise this method parses file in "discussion" folder (known as definition_xml), applies - policy.json and updates fields accordingly. - """ - block = super().parse_xml(node, runtime, keys) - - cls._apply_metadata_and_policy(block, node, runtime) - - return block - - @classmethod - def _apply_metadata_and_policy(cls, block, node, runtime): - """ - Attempt to load definition XML from "discussion" folder in OLX, than parse it and update block fields - """ - if node.get('url_name') is None: - return # Newer/XBlock XML format - no need to load an additional file. - try: - definition_xml, _ = cls.load_definition_xml(node, runtime, block.scope_ids.def_id) - except Exception as err: # pylint: disable=broad-except - log.info( - "Exception %s when trying to load definition xml for block %s - assuming XBlock export format", - err, - block - ) - return - - metadata = cls.load_metadata(definition_xml) - cls.apply_policy(metadata, runtime.get_policy(block.scope_ids.usage_id)) - - for field_name, value in metadata.items(): - if field_name in block.fields: - setattr(block, field_name, value) - - -DiscussionXBlock = None - - -def reset_class(): - """Reset class as per django settings flag""" - global DiscussionXBlock - DiscussionXBlock = ( - _ExtractedDiscussionXBlock if settings.USE_EXTRACTED_DISCUSSION_BLOCK - else _BuiltInDiscussionXBlock - ) - return DiscussionXBlock - -reset_class() -DiscussionXBlock.__name__ = "DiscussionXBlock" diff --git a/xmodule/js/src/discussion/display.js b/xmodule/js/src/discussion/display.js deleted file mode 100644 index 64dfc685fef8..000000000000 --- a/xmodule/js/src/discussion/display.js +++ /dev/null @@ -1,21 +0,0 @@ -// Once generated by CoffeeScript 1.9.3, but now lives as pure JS -/* eslint-disable */ -(function() { - var extend = function(child, parent) { for (var key in parent) { if (hasProp.call(parent, key)) child[key] = parent[key]; } function ctor() { this.constructor = child; } ctor.prototype = parent.prototype; child.prototype = new ctor(); child.__super__ = parent.prototype; return child; }, - hasProp = {}.hasOwnProperty; - - this.InlineDiscussion = (function(superClass) { - extend(InlineDiscussion, superClass); - - function InlineDiscussion(element) { - this.el = $(element).find('.discussion-module'); - this.view = new DiscussionInlineView({ - el: this.el - }); - } - - return InlineDiscussion; - - })(XModule.Descriptor); - -}).call(this); diff --git a/xmodule/tests/test_discussion.py b/xmodule/tests/test_discussion.py deleted file mode 100644 index 5c1e8741ce29..000000000000 --- a/xmodule/tests/test_discussion.py +++ /dev/null @@ -1,185 +0,0 @@ -""" Tests for DiscussionXBLock""" - - -import itertools -import random -import string -from collections import namedtuple -from unittest import TestCase, mock - -import ddt -from xblock.field_data import DictFieldData -from xblock.fields import NO_CACHE_VALUE, UNIQUE_ID, ScopeIds -from xblock.runtime import Runtime - -from openedx.core.lib.safe_lxml import etree -from xmodule.discussion_block import DiscussionXBlock - - -def attribute_pair_repr(self): - """ - Custom string representation for the AttributePair namedtuple which is - consistent between test runs. - """ - return f'' - - -AttributePair = namedtuple("AttributePair", ["name", "value"]) -AttributePair.__repr__ = attribute_pair_repr - - -ID_ATTR_NAMES = ("discussion_id",) -CATEGORY_ATTR_NAMES = ("discussion_category",) -TARGET_ATTR_NAMES = ("discussion_target",) - - -def _random_string(): - """ - Generates random string - """ - return ''.join(random.choice(string.ascii_lowercase, ) for _ in range(12)) - - -def _make_attribute_test_cases(): - """ - Builds test cases for attribute-dependent tests - """ - attribute_names = itertools.product(ID_ATTR_NAMES, CATEGORY_ATTR_NAMES, TARGET_ATTR_NAMES) - for id_attr, category_attr, target_attr in attribute_names: - yield ( - AttributePair(id_attr, _random_string()), - AttributePair(category_attr, _random_string()), - AttributePair(target_attr, _random_string()) - ) - - -@ddt.ddt -class DiscussionXBlockImportExportTests(TestCase): - """ - Import and export tests - """ - DISCUSSION_XBLOCK_LOCATION = "xmodule.discussion_block.DiscussionXBlock" - - def setUp(self): - """ - Set up method - """ - super().setUp() - self.keys = ScopeIds("any_user", "discussion", "def_id", "usage_id") - self.runtime_mock = mock.Mock(spec=Runtime) - self.runtime_mock.construct_xblock_from_class = mock.Mock(side_effect=self._construct_xblock_mock) - self.runtime_mock.get_policy = mock.Mock(return_value={}) - - def _construct_xblock_mock(self, cls, keys): # pylint: disable=unused-argument - """ - Builds target xblock instance (DiscussionXBlock) - - Signature-compatible with runtime.construct_xblock_from_class - can be used as a mock side-effect - """ - return DiscussionXBlock(self.runtime_mock, scope_ids=keys, field_data=DictFieldData({})) - - @mock.patch(DISCUSSION_XBLOCK_LOCATION + ".load_definition_xml") - @ddt.unpack - @ddt.data(*list(_make_attribute_test_cases())) - def test_xblock_export_format(self, id_pair, category_pair, target_pair, patched_load_definition_xml): - """ - Test that xblock export XML format can be parsed preserving field values - """ - xblock_xml = """ - - """.format( # noqa: UP032 - id_attr=id_pair.name, id_value=id_pair.value, - category_attr=category_pair.name, category_value=category_pair.value, - target_attr=target_pair.name, target_value=target_pair.value, - ) - node = etree.fromstring(xblock_xml) - - patched_load_definition_xml.side_effect = Exception("Irrelevant") - - block = DiscussionXBlock.parse_xml(node, self.runtime_mock, self.keys) - try: - assert block.discussion_id == id_pair.value - assert block.discussion_category == category_pair.value - assert block.discussion_target == target_pair.value - except AssertionError: - print(xblock_xml) - raise - - @mock.patch(DISCUSSION_XBLOCK_LOCATION + ".load_definition_xml") - @ddt.unpack - @ddt.data(*(_make_attribute_test_cases())) - def test_legacy_export_format(self, id_pair, category_pair, target_pair, patched_load_definition_xml): - """ - Test that legacy export XML format can be parsed preserving field values - """ - xblock_xml = """""" - xblock_definition_xml = """ - """.format( # noqa: UP032 - id_attr=id_pair.name, id_value=id_pair.value, - category_attr=category_pair.name, category_value=category_pair.value, - target_attr=target_pair.name, target_value=target_pair.value, - ) - node = etree.fromstring(xblock_xml) - definition_node = etree.fromstring(xblock_definition_xml) - - patched_load_definition_xml.return_value = (definition_node, "irrelevant") - - block = DiscussionXBlock.parse_xml(node, self.runtime_mock, self.keys) - try: - assert block.discussion_id == id_pair.value - assert block.discussion_category == category_pair.value - assert block.discussion_target == target_pair.value - except AssertionError: - print(xblock_xml, xblock_definition_xml) - raise - - def test_export_default_discussion_id(self): - """ - Test that default discussion_id values are not exported. - - Historically, the OLX format allowed omitting discussion ID values; in such case, the IDs are generated - deterministically based on the course ID and the usage ID. Moreover, Studio does not allow course authors - to edit discussion_id, so all courses authored in Studio have discussion_id omitted in OLX. - - Forcing Studio to always export discussion_id can cause data loss when switching between an older and newer - export, in a course with a course ID different from the one from which the export was created - because the - discussion ID would be different. - """ - target_node = etree.Element('dummy') - - block = DiscussionXBlock(self.runtime_mock, scope_ids=self.keys, field_data=DictFieldData({})) - discussion_id_field = block.fields['discussion_id'] # pylint: disable=unsubscriptable-object - - # precondition checks - discussion_id does not have a value and uses UNIQUE_ID - assert discussion_id_field._get_cached_value(block) == NO_CACHE_VALUE # pylint: disable=W0212 - assert discussion_id_field.default == UNIQUE_ID - - block.add_xml_to_node(target_node) - assert target_node.tag == 'discussion' # pylint: disable=W0212 - assert 'discussion_id' not in target_node.attrib - - @ddt.data("jediwannabe", "iddqd", "itisagooddaytodie") - def test_export_custom_discussion_id(self, discussion_id): - """ - Test that custom discussion_id values are exported - """ - target_node = etree.Element('dummy') - - block = DiscussionXBlock(self.runtime_mock, scope_ids=self.keys, field_data=DictFieldData({})) - block.discussion_id = discussion_id - - # precondition check - assert block.discussion_id == discussion_id - - block.add_xml_to_node(target_node) - assert target_node.tag == 'discussion' - assert target_node.attrib['discussion_id'], discussion_id