Skip to content

Commit

Permalink
refactor: reuse services and wrappers between XBlocks (fixed)
Browse files Browse the repository at this point in the history
This re-applies commit 36cc415 with handling an invalid context_key in the
`PartitionService`. It can happen when rendering a `LibraryContentBlock` in
Studio because this service is initialized by the modulestore when validating
an XBlock to gather its error messages in the `studio_xblock_wrapper`.
  • Loading branch information
Agrendalath committed Jul 19, 2023
1 parent d79625e commit 92b6840
Show file tree
Hide file tree
Showing 20 changed files with 259 additions and 251 deletions.
2 changes: 1 addition & 1 deletion cms/djangoapps/contentstore/views/preview.py
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ def _prepare_runtime_for_preview(request, block):
# Set up functions to modify the fragment produced by student_view
block.runtime.wrappers = wrappers
block.runtime.wrappers_asides = wrappers_asides
block.runtime._runtime_services.update(services) # lint-amnesty, pylint: disable=protected-access
block.runtime._services.update(services) # pylint: disable=protected-access

# xmodules can check for this attribute during rendering to determine if
# they are being rendered for preview (i.e. in Studio)
Expand Down
282 changes: 140 additions & 142 deletions lms/djangoapps/courseware/block_render.py

Large diffs are not rendered by default.

29 changes: 15 additions & 14 deletions lms/djangoapps/courseware/tests/test_block_render.py
Original file line number Diff line number Diff line change
Expand Up @@ -949,7 +949,7 @@ def test_will_recheck_access_handler_attribute(self, handler, will_recheck_acces
request.user = self.mock_user

render.handle_xblock_callback(request, str(course.id), usage_id, handler)
assert mock_get_block.call_count == 1
assert mock_get_block.call_count == 2
assert mock_get_block.call_args[1]['will_recheck_access'] == will_recheck_access


Expand Down Expand Up @@ -1922,14 +1922,16 @@ def _get_anonymous_id(self, course_id, xblock_class, should_get_deprecated_id: b
if hasattr(xblock_class, 'module_class'):
block.module_class = xblock_class.module_class

rendered_block = render.get_block_for_descriptor_internal(
rendered_block = render.get_block_for_descriptor(
user=self.user,
block=block,
student_data=Mock(spec=FieldData, name='student_data'),
course_id=course_id,
course_key=course_id,
track_function=Mock(name='track_function'), # Track Function
request_token='request_token',
course=self.course,
request=None,
field_data_cache=None,
)
current_user = rendered_block.runtime.service(rendered_block, 'user').get_current_user()

Expand Down Expand Up @@ -2285,7 +2287,7 @@ def _prepare_runtime(self):
render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
self.block.runtime,
self.course.id,
self.track_function,
self.request_token,
Expand Down Expand Up @@ -2439,7 +2441,6 @@ def test_no_service_exception_(self):
Test: NoSuchServiceError should be raised if i18n service is none.
"""
i18nService = self.block.runtime._services['i18n'] # pylint: disable=protected-access
self.block.runtime._runtime_services['i18n'] = None # pylint: disable=protected-access
self.block.runtime._services['i18n'] = None # pylint: disable=protected-access
with pytest.raises(NoSuchServiceError):
self.block.runtime.service(self.block, 'i18n')
Expand Down Expand Up @@ -2656,7 +2657,7 @@ def setUp(self):
render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
self.block.runtime,
self.course.id,
self.track_function,
self.request_token,
Expand Down Expand Up @@ -2684,7 +2685,7 @@ def test_user_is_staff(self, is_staff, expected_role):
render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
self.block.runtime,
self.course.id,
self.track_function,
self.request_token,
Expand All @@ -2706,7 +2707,7 @@ def test_user_is_admin(self, is_global_staff):
render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
self.block.runtime,
self.course.id,
self.track_function,
self.request_token,
Expand All @@ -2726,7 +2727,7 @@ def test_user_is_beta_tester(self, is_beta_tester):
render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
self.block.runtime,
self.course.id,
self.track_function,
self.request_token,
Expand All @@ -2747,7 +2748,7 @@ def test_get_user_role(self, is_instructor, expected_role):
render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
self.block.runtime,
self.course.id,
self.track_function,
self.request_token,
Expand Down Expand Up @@ -2776,7 +2777,7 @@ def test_anonymous_student_id_bug(self):
render.prepare_runtime_for_user(
self.user,
self.student_data,
self.problem_block,
self.problem_block.runtime,
self.course.id,
self.track_function,
self.request_token,
Expand All @@ -2790,7 +2791,7 @@ def test_anonymous_student_id_bug(self):
render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
self.block.runtime,
self.course.id,
self.track_function,
self.request_token,
Expand All @@ -2810,7 +2811,7 @@ def test_user_service_with_anonymous_user(self):
render.prepare_runtime_for_user(
AnonymousUser(),
self.student_data,
self.block,
self.block.runtime,
self.course.id,
self.track_function,
self.request_token,
Expand Down Expand Up @@ -2839,7 +2840,7 @@ def test_get_real_user(self):
render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
self.block.runtime,
self.course.id,
self.track_function,
self.request_token,
Expand Down
26 changes: 17 additions & 9 deletions lms/djangoapps/courseware/tests/test_discussion_xblock.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
from xmodule.modulestore.tests.factories import BlockFactory, ToyCourseFactory

from lms.djangoapps.course_api.blocks.tests.helpers import deserialize_usage_key
from lms.djangoapps.courseware.block_render import get_block_for_descriptor_internal
from lms.djangoapps.courseware.block_render import get_block_for_descriptor
from lms.djangoapps.courseware.tests.helpers import XModuleRenderingTestBase
from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration, Provider
from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory
Expand Down Expand Up @@ -293,13 +293,15 @@ def test_html_with_user(self):
"""
Test rendered DiscussionXBlock permissions.
"""
discussion_xblock = get_block_for_descriptor_internal(
discussion_xblock = get_block_for_descriptor(
user=self.user,
block=self.discussion,
student_data=mock.Mock(name='student_data'),
course_id=self.course.id,
course_key=self.course.id,
track_function=mock.Mock(name='track_function'),
request_token='request_token',
request=None,
field_data_cache=None,
)

fragment = discussion_xblock.render('student_view')
Expand Down Expand Up @@ -336,13 +338,15 @@ def test_discussion_render_successfully_with_orphan_parent(self):
assert orphan_sequential.location.block_id == root.location.block_id

# Get xblock bound to a user and a block.
discussion_xblock = get_block_for_descriptor_internal(
discussion_xblock = get_block_for_descriptor(
user=self.user,
block=discussion,
student_data=mock.Mock(name='student_data'),
course_id=self.course.id,
course_key=self.course.id,
track_function=mock.Mock(name='track_function'),
request_token='request_token',
request=None,
field_data_cache=None,
)

fragment = discussion_xblock.render('student_view')
Expand Down Expand Up @@ -386,13 +390,15 @@ def test_discussion_xblock_visibility(self):
provider_type=Provider.OPEN_EDX,
)

discussion_xblock = get_block_for_descriptor_internal(
discussion_xblock = get_block_for_descriptor(
user=self.user,
block=self.discussion,
student_data=mock.Mock(name='student_data'),
course_id=self.course.id,
course_key=self.course.id,
track_function=mock.Mock(name='track_function'),
request_token='request_token',
request=None,
field_data_cache=None,
)

fragment = discussion_xblock.render('student_view')
Expand Down Expand Up @@ -436,13 +442,15 @@ def test_permissions_query_load(self):
num_queries = 6

for discussion in discussions:
discussion_xblock = get_block_for_descriptor_internal(
discussion_xblock = get_block_for_descriptor(
user=user,
block=discussion,
student_data=mock.Mock(name='student_data'),
course_id=course.id,
course_key=course.id,
track_function=mock.Mock(name='track_function'),
request_token='request_token',
request=None,
field_data_cache=None,
)
with self.assertNumQueries(num_queries):
fragment = discussion_xblock.render('student_view')
Expand Down
18 changes: 9 additions & 9 deletions lms/djangoapps/courseware/testutils.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,8 @@ class RenderXBlockTestMixin(MasqueradeMixin, metaclass=ABCMeta):
'<div class="container"',
]

# DOM elements that appear in an xBlock,
# but are excluded from the xBlock-only rendering.
XBLOCK_REMOVED_HTML_ELEMENTS = [
# DOM elements that should only be present when viewing the XBlock as staff.
XBLOCK_STAFF_DEBUG_INFO = [
'<div class="wrap-instructor-info"',
]

Expand Down Expand Up @@ -143,23 +142,24 @@ def setup_user(self, admin=False, enroll=False, login=False):
if login:
self.login()

def verify_response(self, expected_response_code=200, url_params=None):
def verify_response(self, expected_response_code=200, url_params=None, is_staff=False):
"""
Helper method that calls the endpoint, verifies the expected response code, and returns the response.
Arguments:
expected_response_code: The expected response code.
url_params: URL parameters that will be encoded and passed to the request.
is_staff: Whether the user has staff permissions in the course.
"""
if url_params:
url_params = urlencode(url_params)

response = self.get_response(self.block_to_be_tested.location, url_params)
if expected_response_code == 200:
self.assertContains(response, self.html_block.data, status_code=expected_response_code)
unexpected_elements = self.block_specific_chrome_html_elements
unexpected_elements += self.COURSEWARE_CHROME_HTML_ELEMENTS + self.XBLOCK_REMOVED_HTML_ELEMENTS
unexpected_elements = self.block_specific_chrome_html_elements + self.COURSEWARE_CHROME_HTML_ELEMENTS
if not is_staff:
unexpected_elements += self.XBLOCK_STAFF_DEBUG_INFO
for chrome_element in unexpected_elements:
self.assertNotContains(response, chrome_element)
else:
Expand Down Expand Up @@ -202,12 +202,12 @@ def test_success_enrolled_staff(self):
# (3) definition - HTML block
# (4) definition - edx_notes decorator (original_get_html)
with check_mongo_calls(4):
self.verify_response()
self.verify_response(is_staff=True)

def test_success_unenrolled_staff(self):
self.setup_course()
self.setup_user(admin=True, enroll=False, login=True)
self.verify_response()
self.verify_response(is_staff=True)

def test_success_unenrolled_staff_masquerading_as_student(self):
self.setup_course()
Expand Down
10 changes: 7 additions & 3 deletions lms/djangoapps/courseware/views/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -1521,7 +1521,7 @@ def _check_sequence_exam_access(request, location):
@xframe_options_exempt
@transaction.non_atomic_requests
@ensure_csrf_cookie
def render_xblock(request, usage_key_string, check_if_enrolled=True):
def render_xblock(request, usage_key_string, check_if_enrolled=True, disable_staff_debug_info=False):
"""
Returns an HttpResponse with HTML content for the xBlock with the given usage_key.
The returned HTML is a chromeless rendering of the xBlock (excluding content of the containing courseware).
Expand Down Expand Up @@ -1571,8 +1571,12 @@ def render_xblock(request, usage_key_string, check_if_enrolled=True):
# get the block, which verifies whether the user has access to the block.
recheck_access = request.GET.get('recheck_access') == '1'
block, _ = get_block_by_usage_id(
request, str(course_key), str(usage_key), disable_staff_debug_info=True, course=course,
will_recheck_access=recheck_access
request,
str(course_key),
str(usage_key),
disable_staff_debug_info=disable_staff_debug_info,
course=course,
will_recheck_access=recheck_access,
)

student_view_context = request.GET.dict()
Expand Down
24 changes: 9 additions & 15 deletions lms/djangoapps/instructor_task/tasks_helper/module_state.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

from django.utils.translation import gettext_noop
from opaque_keys.edx.keys import UsageKey
from xblock.runtime import KvsFieldData
from xblock.scorable import Score

from xmodule.capa.responsetypes import LoncapaProblemError, ResponseError, StudentInputError
Expand All @@ -18,9 +17,9 @@
from common.djangoapps.track.views import task_track
from common.djangoapps.util.db import outer_atomic
from lms.djangoapps.courseware.courses import get_problems_in_section
from lms.djangoapps.courseware.model_data import DjangoKeyValueStore, FieldDataCache
from lms.djangoapps.courseware.model_data import FieldDataCache
from lms.djangoapps.courseware.models import StudentModule
from lms.djangoapps.courseware.block_render import get_block_for_descriptor_internal
from lms.djangoapps.courseware.block_render import get_block_for_descriptor
from lms.djangoapps.grades.api import events as grades_events
from openedx.core.lib.courses import get_course_by_id
from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order
Expand Down Expand Up @@ -334,16 +333,10 @@ def _get_module_instance_for_task(course_id, student, block, xblock_instance_arg
"""
Fetches a StudentModule instance for a given `course_id`, `student` object, and `block`.
`xblock_instance_args` is used to provide information for creating a track function and an XQueue callback.
These are passed, along with `grade_bucket_type`, to get_block_for_descriptor_internal, which sidesteps
the need for a Request object when instantiating an xblock instance.
`xblock_instance_args` is used to provide information for creating a track function.
It is passed, along with `grade_bucket_type`, to get_block_for_descriptor.
"""
# reconstitute the problem's corresponding XBlock:
field_data_cache = FieldDataCache.cache_for_block_descendents(course_id, student, block)
student_data = KvsFieldData(DjangoKeyValueStore(field_data_cache))

# get request-related tracking information from args passthrough, and supplement with task-specific
# information:
# get request-related tracking information from args passthrough, and supplement with task-specific information:
request_info = xblock_instance_args.get('request_info', {}) if xblock_instance_args is not None else {}
task_info = {"student": student.username, "task_id": _get_task_id_from_xblock_args(xblock_instance_args)}

Expand All @@ -357,11 +350,12 @@ def make_track_function():
'''
return lambda event_type, event: task_track(request_info, task_info, event_type, event, page='x_module_task')

return get_block_for_descriptor_internal(
return get_block_for_descriptor(
user=student,
request=None,
block=block,
student_data=student_data,
course_id=course_id,
field_data_cache=FieldDataCache.cache_for_block_descendents(course_id, student, block),
course_key=course_id,
track_function=make_track_function(),
grade_bucket_type=grade_bucket_type,
# This module isn't being used for front-end rendering
Expand Down
Loading

0 comments on commit 92b6840

Please sign in to comment.