Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor: refactor discussions_xblock (#30636)

Browse Source 
JIRA: https://openedx.atlassian.net/browse/BOM-2580
This PR aims at refactoring the discussion xblock sub project and moving it within the xmodule directory effectively removing its position as a sub project within edx-platform
This commit is contained in:
Mohammad Ahtasham ul Hassan
2022-06-27 13:11:56 +01:00
committed by GitHub
parent 8ef261fe07
commit 44fa09eba5
18 changed files with 43 additions and 115 deletions
Download Patch File Download Diff File Expand all files Collapse all files

265
xmodule/discussion_block.py Normal file
View File

@@ -0,0 +1,265 @@
"""
Discussion XBlock
"""
import logging
import urllib
from django.contrib.staticfiles.storage import staticfiles_storage
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 xblockutils.resources import ResourceLoader
from xblockutils.studio_editable import StudioEditableXBlockMixin
from openedx.core.djangolib.markup import HTML, Text
from openedx.core.lib.xblock_utils import get_css_dependencies, get_js_dependencies
from xmodule.xml_module import XmlParserMixin
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') # pylint: disable=abstract-method
@XBlock.needs('i18n')
@XBlock.needs('mako')
class DiscussionXBlock(XBlock, StudioEditableXBlockMixin, XmlParserMixin): # lint-amnesty, pylint: disable=abstract-method
"""
Provides a discussion forum that is inline with other content in the courseware.
"""
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 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
@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
"""
# normal import causes the xmodule_assets command to fail due to circular import - hence importing locally
from lms.djangoapps.discussion.django_comment_client.permissions import has_permission
return has_permission(self.django_user, permission, self.course_key)
def student_view(self, context=None):
"""
Renders student view for LMS.
"""
fragment = Fragment()
self.add_resource_urls(fragment)
login_msg = ''
if not self.django_user.is_authenticated:
qs = urllib.parse.urlencode({
'course_id': self.course_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('<a href="{url}">{sign_in_label}</a>').format(
sign_in_label=_('sign in'),
url='{}?{}'.format(reverse('signin_user'), qs),
),
register_link=HTML('<a href="/{url}">{register_label}</a>').format(
register_label=_('register'),
url='{}?{}'.format(reverse('register_user'), qs),
),
)
context = {
'discussion_id': self.discussion_id,
'display_name': self.display_name if self.display_name else _("Discussion"),
'user': self.django_user,
'course_id': self.course_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,
}
fragment.add_content(self.runtime.service(self, 'mako').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.service(self, 'mako').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}
@classmethod
def parse_xml(cls, node, runtime, keys, id_generator):
"""
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, id_generator)
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)

186
xmodule/tests/test_discussion.py Normal file
View File

@@ -0,0 +1,186 @@
""" 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 name={self.name}>'
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={})
self.id_gen_mock = mock.Mock()
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 = """
<discussion
url_name="82bb87a2d22240b1adac2dfcc1e7e5e4" xblock-family="xblock.v1"
{id_attr}="{id_value}"
{category_attr}="{category_value}"
{target_attr}="{target_value}"
/>
""".format(
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, self.id_gen_mock)
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 = """<discussion url_name="82bb87a2d22240b1adac2dfcc1e7e5e4"/>"""
xblock_definition_xml = """
<discussion
{id_attr}="{id_value}"
{category_attr}="{category_value}"
{target_attr}="{target_value}"
/>""".format(
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, self.id_gen_mock)
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