Skip to content

Add logic to parse XBlock's XML with the pointer tag correctly. #823

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

Open
ttqureshi opened this issue Mar 12, 2025 · 9 comments · May be fixed by #830
Open

Add logic to parse XBlock's XML with the pointer tag correctly. #823

ttqureshi opened this issue Mar 12, 2025 · 9 comments · May be fixed by #830
Assignees

Comments

@ttqureshi
Copy link

Currently parse_xml function in xblock/core.py doesn't support the implementation to parse the pointer tags within an XBlock's XML file. It instantiates the XBlock without picking fields data (and other metadata, if any) if there is a pointer tag.

Example XML with pointer tag

<vertical display_name="LTI">
  <lti url_name="lti"/>
</vertical>

Why need it?

We are extracting BuiltIn XBlocks from edx-platform, in order for the extracted XBlocks to work properly we want this pointer tag logic to be implemented in xblock/core.py. This will prevent breaking any functionality, ensuring that XBlock's definition is loaded correctly.

Here's the reference from Demo course, which was exported from Studio as recently as Redwood:

@ttqureshi
Copy link
Author

FYI @kdmccormick

cc: @farhan @salman2013 @irtazaakram

@kdmccormick
Copy link
Member

TY. Out on vacation currently but I will add some notes when I'm back on Friday.

@kdmccormick
Copy link
Member

I have created this ticket for the overall bug investigation:

We can keep this issue (xblock/823) focused on fixing the immediate bug, and use edx-platform/36390 for the big-picture investigation of what happened.

@kdmccormick
Copy link
Member

kdmccormick commented Mar 17, 2025

There is one big-picture thing we need to worry about for this ticket:

In order to keep Teak course exports compatible with the broken releases (which are most likely Redwood and Sumac), we should continue to export the currently-external blocks using inline syntax, even though we will restore the ability to import them with pointer syntax. I think we could do this by adding a new class variable to XBlock called export_with_pointer_tag, defaulting to False. The currently-built-in blocks and their extracted counterparts can override this variable to True. The export process can look at the variable to determine whether to export that block using inline or pointer syntax.

For example: Because drag-and-drop-v2 fails to import with pointer syntax in Redwood and Sumac, we should make sure to export it via inline syntax going forward, so that if an author exports drag-and-drop-v2 from Teak, it will load correctly into Redwood and Sumac.

Another example: Because word_cloud imports OK with pointer syntax in all releases, we can export it using pointer syntax even after we extract it, because it pointer syntax will work fine for it in Redwood and Sumac. So, we would set export_with_pointer_syntax = True in its class definition.

Alternatively, if we can find out when and why pointer syntax stopped working and backport a bugfix to all affected releases, then we can instead return to the behavior of always exporting every block with pointer syntax.

@ttqureshi
Copy link
Author

ttqureshi commented Mar 18, 2025

Thanks @kdmccormick for the introspective look into the issue.
I will be starting to work on this issue (XBlock/823) as it is blocking the LTI XBlock Extraction.

@bradenmacdonald
Copy link
Contributor

We have previously made the decision to move away from XBlocks having children, and move toward a system where we use Containers for hierarchy above the Component level. So I'm a little bit hesitant to add this logic (which is strictly related to parsing/encoding children) into XBlock core.

When you see something like the example linked above:

<vertical display_name="Code Grading">
   ...
  <lti url_name="8e71d278e94f4913bc564cc6141fde53"/>
   ...
</vertical>

It's true that we could put logic into the XBlock core so that the LTI block can parse that itself, but it's equally possible (and I would argue, better) to shift this up one level and have the vertical block in this case realize that the child is a pointer node and load the full definition before telling the LTI block to parse it.

In fact, I don't think we need to make any XBlocks aware of pointer nodes. With the example code here, when the vertical block encounters <lti url_name="..." />, it should call runtime.add_node_as_child(block, child). Why can't we just make it so that that runtime function understands pointer nodes and substitutes in the full XML before calling the child (LTI in this example) to parse_xml ?

@ttqureshi
Copy link
Author

ttqureshi commented May 19, 2025

Thanks @bradenmacdonald for the input.

but it's equally possible (and I would argue, better) to shift this up one level and have the vertical block in this case realize that the child is a pointer node and load the full definition before telling the LTI block to parse it.

After brainstorming and connecting a few dots, I think that we might need to define parse_xml method inside the VerticalBlock. This parse_xml method will:

  • load the vertical block definition from the file
  • check if the child node is a pointer node then load it's full definition and
  • return the block

Please share your thoughts on this. Would this serve the purpose without touching the xblock core functionality?

@ttqureshi
Copy link
Author

ttqureshi commented May 19, 2025

I want to highlight one more point here, the edx-platform exports BuiltIn XBlocks in the pointer format, examples can be seen from the Demo course linked in the description, while all other XBlocks that reside outside the edx-platform are exported in the inline format by default (e.g., a, b).

The whole point of creating this issue is to support the previously exported BuiltIn XBlocks in the pointer tag format after extracting them in the xblocks-contrib.

Also, there is one test case that is failing in the test PR for extracted LTIBlock which is because of the pointer tag format in which the LTI block is present inside the edx-platform's test course, scoreable.

So, in any way, I believe we've to come up with a solution (either by adding a parse_xml in VerticalBlock or something better) that could support the parsing of the blocks exported in the pointer tag format.

@bradenmacdonald
Copy link
Contributor

Adding the support to Vertical Block is fine with me, though I would prefer to keep it in the runtime itself.

Also, just as a side note, I find the pointer form is annoying to work with and much prefer the inline form wherever possible. Manually editing a course tarball that uses the pointer form extensively involves much more tedious work correlating files with each other compared to simple tarballs where each unit contains the vertical with all children inline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging a pull request may close this issue.

3 participants