feat: remove field-data binding from the runtime [FC-0026] - #32357
Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @Agrendalath! As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion. |
Agrendalath
changed the title
There was a problem hiding this comment.
We're removing lots of these block-agnostic queries by moving out the DateLookupFieldData initialization out of the runtime preparation:
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
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
4254929 to
3413277
Compare
There was a problem hiding this comment.
We already add these as wrappers in https://github.com/openedx/edx-platform/blob/3413277a63860a84e5fc5b6dea70a52efbd45296/lms/djangoapps/courseware/block_render.py#L658-L665.
There was a problem hiding this comment.
I did not find any direct usage of this service.
We bind the field_data in the XModuleMixin: https://github.com/openedx/edx-platform/blob/0b455e0336fa54199fd992c2ce83a81c07dd07c2/xmodule/x_module.py#L649-L659
Then, we have the following wrappers for accessing field-data; none of them uses the service.
There was a problem hiding this comment.
@Agrendalath, since the service is being removed, do we need @XBlock.needs("field-data")?
There was a problem hiding this comment.
Regarding removing @XBlock.needs('field-data'): The field-data service in general is still used; it's just this particular way of setting the student-bound wrapped field data that is no longer used. I think :)
Though I also think that the field-data service is always "needed" so it's probably not necessary to use the @XBlock.needs decorator.
There was a problem hiding this comment.
Nice catch! We use field-data as a service but never reach the service declaration checks because of its special handling.
I removed these decorators in cca07d635fcc8001515704647fd713f46626c8b5.
There was a problem hiding this comment.
We don't need this - it's already handled in bind_for_student: https://github.com/openedx/edx-platform/blob/0b455e0336fa54199fd992c2ce83a81c07dd07c2/xmodule/x_module.py#L630-L631
There was a problem hiding this comment.
We don't need this - it's already handled in bind_for_student: https://github.com/openedx/edx-platform/blob/0b455e0336fa54199fd992c2ce83a81c07dd07c2/xmodule/x_module.py#L630-L631
|
@0x29a, @bradenmacdonald, this is ready for review. I did plenty of regression checks, but please let me know if you are aware of some specific edge cases we should verify. |
3413277 to
4719ca9
Compare
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
I just read through the code and will rely on you two for testing... This looks great though. A very nice cleanup and I'm pleasantly surprised that we can just remove and simplify things like this, with no other changes. Excellent.
There was a problem hiding this comment.
Regarding removing @XBlock.needs('field-data'): The field-data service in general is still used; it's just this particular way of setting the student-bound wrapped field data that is no longer used. I think :)
Though I also think that the field-data service is always "needed" so it's probably not necessary to use the @XBlock.needs decorator.
|
Thank you for your reviews, @0x29a and @bradenmacdonald! @ormsbee, would you like to take a look at this before we schedule the merge? |
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
I have pretty much exactly @bradenmacdonald's reaction to this.
0ffd938 to
d492163
Compare
d492163 to
6c435bb
Compare
|
@Agrendalath 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
6 participants
Description
The goal of FC-0026 is to remove the XBlock-specific handling from the prepare_runtime_for_user function. This part handles the
field-data. Please take a look at the review comments that explain each significant change.Supporting information
Private-ref: BB-7448
Testing instructions
Ensure that XBlocks work correctly in the LMS, Preview, and Studio. Check that importing and exporting the course works.