From fa06be106eae56b7e6427f947949b32a5d544d16 Mon Sep 17 00:00:00 2001 From: Piotr Surowiec Date: Thu, 13 Jul 2023 19:04:02 +0200 Subject: [PATCH] Revert "feat: remove field-data binding from the runtime [FC-0026]" (#32740) * Revert "feat: remove `field-data` service from runtime initialization" This reverts commit 6c435bb68cf05dfde8258089a76dbae7280b5cfa. * Revert "feat: remove field data binding from the runtime" This reverts commit 5f46ea52cd26a0db3b517ff2803d8a54da19a6ee. --- .../contentstore/tests/test_i18n.py | 7 ++++- cms/djangoapps/contentstore/views/preview.py | 8 ++++-- .../contentstore/views/tests/test_preview.py | 27 ++++++++++++++++--- lms/djangoapps/courseware/block_render.py | 13 ++++++++- .../courseware/tests/test_block_render.py | 21 ++++++++------- lms/djangoapps/courseware/tests/test_views.py | 2 +- .../schedules/tests/test_resolvers.py | 2 +- .../tests/views/test_course_updates.py | 2 +- xmodule/services.py | 6 +++-- 9 files changed, 65 insertions(+), 23 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_i18n.py b/cms/djangoapps/contentstore/tests/test_i18n.py index db0ed7e04d43..57e8626d4361 100644 --- a/cms/djangoapps/contentstore/tests/test_i18n.py +++ b/cms/djangoapps/contentstore/tests/test_i18n.py @@ -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): diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 37c788463ae9..c633ded93907 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -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 @@ -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(), @@ -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, diff --git a/cms/djangoapps/contentstore/views/tests/test_preview.py b/cms/djangoapps/contentstore/views/tests/test_preview.py index 0f6786ca6237..0908a2eb8fe4 100644 --- a/cms/djangoapps/contentstore/views/tests/test_preview.py +++ b/cms/djangoapps/contentstore/views/tests/test_preview.py @@ -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") @@ -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") @@ -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) @@ -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): @@ -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) ) @@ -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) diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index 973f9b774247..946c1b5186f1 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -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): @@ -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, @@ -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 @@ -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, @@ -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 diff --git a/lms/djangoapps/courseware/tests/test_block_render.py b/lms/djangoapps/courseware/tests/test_block_render.py index 1654196b47d2..33ed581c6850 100644 --- a/lms/djangoapps/courseware/tests/test_block_render.py +++ b/lms/djangoapps/courseware/tests/test_block_render.py @@ -105,6 +105,7 @@ @XBlock.needs('fs') +@XBlock.needs('field-data') @XBlock.needs('mako') @XBlock.needs('user') @XBlock.needs('verification') @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index c7d7ce7b177e..02419c7f9497 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -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', diff --git a/openedx/core/djangoapps/schedules/tests/test_resolvers.py b/openedx/core/djangoapps/schedules/tests/test_resolvers.py index 5881861b2282..8bc7f9b4e717 100644 --- a/openedx/core/djangoapps/schedules/tests/test_resolvers.py +++ b/openedx/core/djangoapps/schedules/tests/test_resolvers.py @@ -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' diff --git a/openedx/features/course_experience/tests/views/test_course_updates.py b/openedx/features/course_experience/tests/views/test_course_updates.py index 379be52ed40f..851d4d86e6da 100644 --- a/openedx/features/course_experience/tests/views/test_course_updates.py +++ b/openedx/features/course_experience/tests/views/test_course_updates.py @@ -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) diff --git a/xmodule/services.py b/xmodule/services.py index 88cbc183dc47..595c8082fb5a 100644 --- a/xmodule/services.py +++ b/xmodule/services.py @@ -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, @@ -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): """