Skip to content

Commit

Permalink
Revert "feat: remove field-data binding from the runtime [FC-0026]" (o…
Browse files Browse the repository at this point in the history
…penedx#32740)

* Revert "feat: remove `field-data` service from runtime initialization"

This reverts commit 6c435bb.

* Revert "feat: remove field data binding from the runtime"

This reverts commit 5f46ea5.
  • Loading branch information
Agrendalath committed Jul 13, 2023
1 parent 472104a commit fa06be1
Show file tree
Hide file tree
Showing 9 changed files with 65 additions and 23 deletions.
7 changes: 6 additions & 1 deletion cms/djangoapps/contentstore/tests/test_i18n.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,13 @@ def setUp(self):
self.test_language = 'dummy language'
self.request = mock.Mock()
self.course = CourseFactory.create()
self.field_data = mock.Mock()
self.block = BlockFactory(category="pure", parent=self.course)
_prepare_runtime_for_preview(self.request, self.block)
_prepare_runtime_for_preview(
self.request,
self.block,
self.field_data,
)
self.addCleanup(translation.deactivate)

def get_block_i18n_service(self, block):
Expand Down
8 changes: 6 additions & 2 deletions cms/djangoapps/contentstore/views/preview.py
Original file line number Diff line number Diff line change
Expand Up @@ -149,13 +149,14 @@ def preview_layout_asides(block, context, frag, view_name, aside_frag_fns, wrap_
return result


def _prepare_runtime_for_preview(request, block):
def _prepare_runtime_for_preview(request, block, field_data):
"""
Sets properties in the runtime of the specified block that is
required for rendering block previews.
request: The active django request
block: An XBlock
field_data: Wrapped field data for previews
"""

course_id = block.location.course_key
Expand Down Expand Up @@ -198,6 +199,7 @@ def _prepare_runtime_for_preview(request, block):
deprecated_anonymous_user_id = anonymous_id_for_user(request.user, None)

services = {
"field-data": field_data,
"i18n": XBlockI18nService,
'mako': mako_service,
"settings": SettingsService(),
Expand Down Expand Up @@ -264,7 +266,9 @@ def _load_preview_block(request: Request, block: XModuleMixin):
else:
wrapper = partial(LmsFieldData, student_data=student_data)

_prepare_runtime_for_preview(request, block)
# wrap the _field_data upfront to pass to _prepare_runtime_for_preview
wrapped_field_data = wrapper(block._field_data) # pylint: disable=protected-access
_prepare_runtime_for_preview(request, block, wrapped_field_data)

block.bind_for_student(
request.user.id,
Expand Down
27 changes: 23 additions & 4 deletions cms/djangoapps/contentstore/views/tests/test_preview.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ def test_block_branch_not_changed_by_preview_handler(self):
self.assertFalse(modulestore().has_changes(modulestore().get_item(block.location)))


@XBlock.needs("field-data")
@XBlock.needs("i18n")
@XBlock.needs("mako")
@XBlock.needs("replace_urls")
Expand Down Expand Up @@ -203,6 +204,7 @@ def setUp(self):
self.user = UserFactory()
self.course = CourseFactory.create()
self.request = mock.Mock()
self.field_data = mock.Mock()

@XBlock.register_temp_plugin(PureXBlock, identifier='pure')
@ddt.data("user", "i18n", "field-data", "teams_configuration", "replace_urls")
Expand All @@ -211,7 +213,11 @@ def test_expected_services_exist(self, expected_service):
Tests that the 'user' and 'i18n' services are provided by the Studio runtime.
"""
block = BlockFactory(category="pure", parent=self.course)
_prepare_runtime_for_preview(self.request, block)
_prepare_runtime_for_preview(
self.request,
block,
self.field_data,
)
service = block.runtime.service(block, expected_service)
self.assertIsNotNone(service)

Expand All @@ -235,9 +241,14 @@ def setUp(self):
self.request = RequestFactory().get('/dummy-url')
self.request.user = self.user
self.request.session = {}
self.field_data = mock.Mock()
self.contentstore = contentstore()
self.block = BlockFactory(category="problem", parent=course)
_prepare_runtime_for_preview(self.request, block=self.block)
_prepare_runtime_for_preview(
self.request,
block=self.block,
field_data=mock.Mock(),
)
self.course = self.store.get_item(course.location)

def test_get_user_role(self):
Expand Down Expand Up @@ -292,7 +303,11 @@ def test_anonymous_user_id_individual_per_student(self):
"""Test anonymous_user_id on a block which uses per-student anonymous IDs"""
# Create the runtime with the flag turned on.
block = BlockFactory(category="problem", parent=self.course)
_prepare_runtime_for_preview(self.request, block=block)
_prepare_runtime_for_preview(
self.request,
block=block,
field_data=mock.Mock(),
)
deprecated_anonymous_user_id = (
block.runtime.service(block, 'user').get_current_user().opt_attrs.get(ATTR_KEY_DEPRECATED_ANONYMOUS_USER_ID)
)
Expand All @@ -303,7 +318,11 @@ def test_anonymous_user_id_individual_per_course(self):
"""Test anonymous_user_id on a block which uses per-course anonymous IDs"""
# Create the runtime with the flag turned on.
block = BlockFactory(category="lti", parent=self.course)
_prepare_runtime_for_preview(self.request, block=block)
_prepare_runtime_for_preview(
self.request,
block=block,
field_data=mock.Mock(),
)

anonymous_user_id = (
block.runtime.service(block, 'user').get_current_user().opt_attrs.get(ATTR_KEY_ANONYMOUS_USER_ID)
Expand Down
13 changes: 12 additions & 1 deletion lms/djangoapps/courseware/block_render.py
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,9 @@ def prepare_runtime_for_user(
Arguments:
see arguments for get_block()
request_token (str): A token unique to the request use by xblock initialization
Returns:
KvsFieldData: student_data bound to, primarily, the user and block
"""

def inner_get_block(block):
Expand Down Expand Up @@ -511,10 +514,14 @@ def inner_get_block(block):
# We already know the user has staff access when masquerading is active.
block_wrappers.append(partial(add_staff_markup, user, disable_staff_debug_info))

field_data = DateLookupFieldData(block._field_data, course_id, user) # pylint: disable=protected-access
field_data = LmsFieldData(field_data, student_data)

store = modulestore()

services = {
'fs': FSService(),
'field-data': field_data,
'mako': mako_service,
'user': DjangoXBlockUserService(
user,
Expand Down Expand Up @@ -584,6 +591,8 @@ def inner_get_block(block):

block.runtime.set('position', position)

return field_data


# TODO: Find all the places that this method is called and figure out how to
# get a loaded course passed into it
Expand All @@ -600,7 +609,7 @@ def get_block_for_descriptor_internal(user, block, student_data, course_id, trac
request_token (str): A unique token for this request, used to isolate xblock rendering
"""

prepare_runtime_for_user(
student_data = prepare_runtime_for_user(
user=user,
student_data=student_data, # These have implicit user bindings, the rest of args are considered not to
block=block,
Expand All @@ -626,6 +635,8 @@ def get_block_for_descriptor_internal(user, block, student_data, course_id, trac
],
)

block.scope_ids = block.scope_ids._replace(user_id=user.id)

# Do not check access when it's a noauth request.
# Not that the access check needs to happen after the block is bound
# for the student, since there may be field override data for the student
Expand Down
21 changes: 11 additions & 10 deletions lms/djangoapps/courseware/tests/test_block_render.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@


@XBlock.needs('fs')
@XBlock.needs('field-data')
@XBlock.needs('mako')
@XBlock.needs('user')
@XBlock.needs('verification')
Expand Down Expand Up @@ -2282,7 +2283,7 @@ def _prepare_runtime(self):
"""
Instantiate the runtem.
"""
render.prepare_runtime_for_user(
_ = render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
Expand Down Expand Up @@ -2653,7 +2654,7 @@ def setUp(self):
self.track_function = Mock()
self.request_token = Mock()
self.contentstore = contentstore()
render.prepare_runtime_for_user(
_ = render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
Expand Down Expand Up @@ -2681,7 +2682,7 @@ def test_user_is_staff(self, is_staff, expected_role):
if is_staff:
self.user = StaffFactory(course_key=self.course.id)

render.prepare_runtime_for_user(
_ = render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
Expand All @@ -2703,7 +2704,7 @@ def test_user_is_admin(self, is_global_staff):
if is_global_staff:
self.user = GlobalStaffFactory.create()

render.prepare_runtime_for_user(
_ = render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
Expand All @@ -2723,7 +2724,7 @@ def test_user_is_beta_tester(self, is_beta_tester):
if is_beta_tester:
self.user = BetaTesterFactory(course_key=self.course.id)

render.prepare_runtime_for_user(
_ = render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
Expand All @@ -2744,7 +2745,7 @@ def test_get_user_role(self, is_instructor, expected_role):
if is_instructor:
self.user = InstructorFactory(course_key=self.course.id)

render.prepare_runtime_for_user(
_ = render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
Expand Down Expand Up @@ -2773,7 +2774,7 @@ def test_anonymous_student_id_bug(self):
anonymous_student_id value.
"""

render.prepare_runtime_for_user(
_ = render.prepare_runtime_for_user(
self.user,
self.student_data,
self.problem_block,
Expand All @@ -2787,7 +2788,7 @@ def test_anonymous_student_id_bug(self):
ATTR_KEY_DEPRECATED_ANONYMOUS_USER_ID
) == anonymous_id_for_user(self.user, None)

render.prepare_runtime_for_user(
_ = render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
Expand All @@ -2807,7 +2808,7 @@ def test_anonymous_student_id_bug(self):
) == anonymous_id_for_user(self.user, None)

def test_user_service_with_anonymous_user(self):
render.prepare_runtime_for_user(
_ = render.prepare_runtime_for_user(
AnonymousUser(),
self.student_data,
self.block,
Expand Down Expand Up @@ -2836,7 +2837,7 @@ def test_get_real_user(self):
Newer code should use the user service, which gets tested in test_user_service.py
"""
render.prepare_runtime_for_user(
_ = render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
Expand Down
2 changes: 1 addition & 1 deletion lms/djangoapps/courseware/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ def test_index_query_counts(self):
self.client.login(username=self.user.username, password=self.user_password)
CourseEnrollment.enroll(self.user, course.id)

with self.assertNumQueries(177, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST):
with self.assertNumQueries(203, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST):
with check_mongo_calls(3):
url = reverse(
'courseware_section',
Expand Down
2 changes: 1 addition & 1 deletion openedx/core/djangoapps/schedules/tests/test_resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ def create_resolver(self, user_start_date_offset=8):
def test_schedule_context(self):
resolver = self.create_resolver()
# using this to make sure the select_related stays intact
with self.assertNumQueries(30):
with self.assertNumQueries(40):
sc = resolver.get_schedules()
schedules = list(sc)
apple_logo_url = 'http://email-media.s3.amazonaws.com/edX/2021/store_apple_229x78.jpg'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def test_queries(self):

# Fetch the view and verify that the query counts haven't changed
# TODO: decrease query count as part of REVO-28
with self.assertNumQueries(52, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST):
with self.assertNumQueries(54, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST):
with check_mongo_calls(3):
url = course_updates_url(self.course)
self.client.get(url)
6 changes: 4 additions & 2 deletions xmodule/services.py
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ def rebind_noauth_module_to_user(self, block, real_user):
with modulestore().bulk_operations(self.course_id):
course = modulestore().get_course(course_key=self.course_id)

self._ref["prepare_runtime_for_user"](
inner_student_data = self._ref["prepare_runtime_for_user"](
user=real_user,
student_data=student_data_real_user, # These have implicit user bindings, rest of args considered not to
block=block,
Expand All @@ -215,10 +215,12 @@ def rebind_noauth_module_to_user(self, block, real_user):
[
partial(DateLookupFieldData, course_id=self.course_id, user=self.user),
partial(OverrideFieldData.wrap, real_user, course),
partial(LmsFieldData, student_data=student_data_real_user),
partial(LmsFieldData, student_data=inner_student_data),
],
)

block.scope_ids = block.scope_ids._replace(user_id=real_user.id)


class EventPublishingService(Service):
"""
Expand Down

0 comments on commit fa06be1

Please sign in to comment.