refactor: unify XBlock naming: item [BD-13]

[tecoholic]

Description

Currently, there are various terms used to refer to XBlocks: xblocks (or blocks), descriptors, modules, or items. This makes the code a lot less approachable and understandable. Since we are getting rid of a lot of tech debt it is a good time to do a quick update of the terminology in the edx-platform to make it consistent.

In this PR:

Convert references using item to refer to XBlocks to use block (e.g. in ItemFactory).

Supporting information

This ticket was created from the work started with #31113

Testing instructions

There is nothing specific to be tested, as we are not adding or changing any functionality. However, we can just perform a general testing of LMS and Studio using the following sandbox.

Sandbox LMS: https://pr31384.sandbox.opencraft.hosting/
Sandbox Studio: https://studio.pr31384.sandbox.opencraft.hosting/

Deadline

"None"

Other information

Related PR: mitodl/edx-sga#332 (it's not a blocker, as these integration tests do not seem to be used anywhere).

[openedx-webhooks]

Thanks for the pull request, @tecoholic!

When this pull request is ready, tag your edX technical lead.

[tecoholic]

@Agrendalath I was able to replace item the ItemFactory and the get_display_items and displayable_items of the XModuleMixin. Other uses of item like load_item seem to have links to the modulestore. Should we tackle these?

Other heavy use of item is from the modulestore functions themselves. I guess, we are not touching these.

[Agrendalath]

@tecoholic,

Other uses of item like load_item seem to have links to the modulestore. Should we tackle these?

We already have the get_block which uses self.load_item. We could replace all uses of *.load_item with runtime.get_block (or self.get_block in a few cases) and change self.load_item in DescriptorSystem to self._load_item, to avoid using it directly.

Regarding the name of the load_item argument of the DescriptorSystem: it makes sense to keep this name in sync with the methods passed to it. However, according to this TODO, it might be a good idea to rename these methods to get_instance. I'm specifically talking about the following occurrences:

1. xmodule.x_module.DescriptorSystem
2. xmodule.modulestore.mongo.base:CachingDescriptorSystem
3. xmodule.modulestore.split_mongo.split:SplitMongoModuleStore
4. xmodule.modulestore.xml:ImportSystem
5. xmodule.tests.xml.__init__:InMemorySystem

Other heavy use of item is from the modulestore functions themselves. I guess, we are not touching these.

There are external packages that use these methods. For example, ORA2 and LTI_Consumer use get_item directly. I don't think we can safely rename these without proper deprecation, so we can omit them.

@ormsbee, what do you think about these points?

[ormsbee]

@Agrendalath: I don't really have an opinion on the get_instance thing. That TODO was from 2012, and I honestly wouldn't read too much into it. I think it's fine to keep thing in sync with the methods passed to it.

Absolutely agree that we cannot break compatibility by changing the modulestore functions. We can add new methods and have the old ones call into them with deprecation warnings, but I'm honestly not sure if the long term simplification is worth having both methods around for the same thing for one or two release cycles.

[tecoholic]

@Agrendalath @ormsbee Thank you for your inputs. I have changes most of the system/runtime.load_item calls to get_block. But making the load_item private by renaming it to _load_item breaks the system due to:

1. CachingDescriptorSystem defining the load_item function with a different signature by adding extra kwargs. It's usage cannot be replaced with the get_block without changing its signature.
2. The private property overshadowing the CachingDescriptorSystem's private memeber - Lint Error

Venturing further into renaming the load_item argument and syncing all the referencing functions seems a bit unfeasible at the moment, given we can't fully get rid of the load_item calls. And also, there doesn't seem to be any references to runtime.load_item outside of the xmodule

$ ack "load_item\(" -l
xmodule/tests/test_conditional.py
xmodule/tests/xml/__init__.py
xmodule/modulestore/split_mongo/split.py
xmodule/modulestore/split_mongo/caching_descriptor_system.py
xmodule/modulestore/xml.py
xmodule/modulestore/mongo/base.py
xmodule/x_module.py

[Agrendalath]

👍

• I tested this: verified that XBlocks work correctly (especially the LibraryContentBlock)
• I read through the code
• I checked for accessibility issues: n/a
• Includes documentation: n/a
• I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

[Agrendalath]

@tecoholic, please rebase the PR and squash the commits into logical chunks.

@ormsbee, would you mind doing a sanity check on this PR? Unlike in #31113, we're not breaking any compatibility here.

The only external usage of the ItemFactory was in the edx-sga XBlock's integration tests that are not a part of any pipeline. We have already opened a PR to synchronize these names there.

[ormsbee]

@Agrendalath: Looks sane to me. It's only changing test-related files, right? Nothing in the modulestore itself? Aside from the load_item -> get_block change, which is equivalent...?

[ormsbee]

FWIW, this is another change that's worth broadcasting loudly, since it will likely break folks who are actively working on branches.

[Agrendalath]

@ormsbee, we have additionally (as described here):

1. Replaced iterations over displayable_items with self, as this method was returning [self].
2. Replaced displayable_items with get_children, as the result is identical.
3. Removed these two (already deprecated) methods, as they were not used outside of the edx-platform.

These changes do not affect the modulestore, though.

FWIW, this is another change that's worth broadcasting loudly, since it will likely break folks who are actively working on branches.

Good point. I will post an announcement once we merge this.

[tecoholic]

@Agrendalath Thanks for the review. I have squashed the commits and rebased it on the latest changes. This is ready for merging.

[tecoholic]

@Agrendalath Gentle reminder about merging this before some conflict with the master branch is introduced.

[Agrendalath]

@ormsbee, I'm just checking if you had a chance to see my previous comment and if there is anything else to do before we schedule the merge.

[ormsbee]

@Agrendalath: No, go ahead and schedule the merge, ty

[openedx-webhooks]

@tecoholic 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

[Agrendalath]

Forum announcement for this change: https://discuss.openedx.org/t/refactoring-xblock-references/9194.

[edx-pipeline-bot]

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

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.