-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat: remove field-data binding from the runtime [FC-0026] #32357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -433,9 +433,6 @@ 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): | ||
|
|
@@ -524,14 +521,10 @@ def inner_get_block(block): | |
| if staff_access: | ||
| 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) | ||
|
Comment on lines
527
to
528
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already add these as wrappers in https://github.com/openedx/edx-platform/blob/3413277a63860a84e5fc5b6dea70a52efbd45296/lms/djangoapps/courseware/block_render.py#L658-L665. |
||
|
|
||
| store = modulestore() | ||
|
|
||
| services = { | ||
| 'fs': FSService(), | ||
| 'field-data': field_data, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did not find any direct usage of this service. We bind the field_data in the Then, we have the following wrappers for accessing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Agrendalath, since the service is being removed, do we need @XBlock.needs("field-data")?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding removing Though I also think that the field-data service is always "needed" so it's probably not necessary to use the @XBlock.needs decorator.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice catch! We use I removed these decorators in cca07d635fcc8001515704647fd713f46626c8b5. |
||
| 'mako': mako_service, | ||
| 'user': DjangoXBlockUserService( | ||
| user, | ||
|
|
@@ -601,8 +594,6 @@ 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 | ||
|
|
@@ -619,7 +610,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 | ||
| """ | ||
|
|
||
| student_data = prepare_runtime_for_user( | ||
| 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, | ||
|
|
@@ -645,8 +636,6 @@ def get_block_for_descriptor_internal(user, block, student_data, course_id, trac | |
| ], | ||
| ) | ||
|
|
||
| block.scope_ids = block.scope_ids._replace(user_id=user.id) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need this - it's already handled in |
||
|
|
||
| # 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(203, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST): | ||
| with self.assertNumQueries(177, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST): | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're removing lots of these block-agnostic queries by moving out the SELECT "schedules_schedule"."id", "schedules_schedule"."created", "schedules_schedule"."modified", "schedules_schedule"."enrollment_id", "schedules_schedule"."active", "schedules_schedule"."start_date", "schedules_schedule"."upgrade_deadline" FROM "schedules_schedule" INNER JOIN "student_courseenrollment" ON ("schedules_schedule"."enrollment_id" = "student_courseenrollment"."id") WHERE ("student_courseenrollment"."course_id" = 'course-v1:org.0+course_0+Run_0' AND "student_courseenrollment"."user_id" = 1) LIMIT 21 |
||
| with check_mongo_calls(3): | ||
| url = reverse( | ||
| 'courseware_section', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
|
||
| inner_student_data = self._ref["prepare_runtime_for_user"]( | ||
| 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,12 +215,10 @@ 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=inner_student_data), | ||
| partial(LmsFieldData, student_data=student_data_real_user), | ||
| ], | ||
| ) | ||
|
|
||
| block.scope_ids = block.scope_ids._replace(user_id=real_user.id) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need this - it's already handled in |
||
|
|
||
|
|
||
| class EventPublishingService(Service): | ||
| """ | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.