refactor: reuse services and wrappers between XBlocks (fixed) [FC-0026]

[Agrendalath]

Description

This is #32420, with a fix added to the xmodule/partitions/partitions_service.py file.

This re-applies commit 36cc415 (#32420) with handling an invalid context_key in the PartitionService (xmodule/partitions/partitions_service.py). 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.

Supporting information

Why didn't it happen before #32420?

In #31472, we unified the ModuleSystem and the DescriptorSystem. To simplify things, we introduced a _runtime_services attribute (a dict), which took precedence over the native _services attribute.
This way, we were never using the partitions service generated by the create_runtime method when the runtime was "bound" for a user by a previously rendered XBlock.

What alternatives did you consider?

I considered adding if not isinstance(course_entry.course_key, LibraryLocator): above https://github.com/openedx/edx-platform/blob/aa7370c773dcd96850fbda8e4f6ad40950c10547/xmodule/modulestore/split_mongo/split.py#L3290 and to other ModuleStores. However, the partition generation process supports the plugins, which don't seem to be open-sourced, so I cannot determine if it wouldn't introduce other regressions.

Why was it happening only in Studio, not LMS/Preview

That's because the unbound LibraryContentBlock is validated only in the studio_xblock_wrapper.html template. This invokes the LmsBlockMixin, which (for an unknown reason) is included in CMS configs. As mentioned in this comment, it was already the case over six years ago when the EnrollmentTrackUserPartition was implemented.

As an alternative solution, we could ensure that https://github.com/openedx/edx-platform/blob/fd191db332d01a47b1c79f684617eafd81abda3d/lms/djangoapps/lms_xblock/mixin.py#L156 is invoked only in Studio. However, the None value handling is easier to understand and less error-prone than this.

[openedx-webhooks]

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]

@ormsbee, I included the fix in a separate commit (7bd5d941c14940d0737f5be7d054d4d0a03ace76) to make it easy to locate.

[openedx-webhooks]

@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.

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX production environment.

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX production environment.

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX production environment.

[michaelroytman]

@Agrendalath @ormsbee Hey there 👋. However unfortunate, the LTI xblock code imports and uses the get_block_for_descriptor function. Each time the function signature changes, various features of the LTI xblock break, including grade passback.

I know there's a much larger discussion that has to be had about why the LTI library should not import this function and how we can refactor the library to no longer do so, but, in the interim, can you please give me a heads up before you merge future pull requests with changes to this function? I know this is likely a frustrating ask, but if we can get any advance notice that the LTI library will break, that would be very helpful as we consider next steps for addressing this dependency. I'm just trying to mitigate future problems while we search for a solution. Alternatively, if you could tell me whether you have additional planned work in this area, that would also be helpful.

Thank you for your understanding.

[ormsbee]

@michaelroytman: Sorry, I had completely forgotten that LtiConsumerXBlock used that function when we did the initial revert. Hopefully we're good now and won't have to change this again, but I will notify you it looks like we'll need to revert again. Thank you for your patience, and apologies for breaking your XBlock.

[Agrendalath]

@michaelroytman, ah, sorry for breaking this. Initially, I didn't plan to merge get_block_for_descriptor_internal into the get_block_for_descriptor, but after some simplifications, it turned out that we could do this.
I tested the set_user_module_score method (which uses the rebind_user runtime service) but missed the usages of the get_block_for_descriptor* after making this merge.