Skip to content

feat:! Adds pointer tag parsing implementation in parse_xml #830

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.

Sign up for GitHub

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

Jump to bottom
Closed
ttqureshi wants to merge 8 commits into from
Closed

feat:! Adds pointer tag parsing implementation in parse_xml #830

ttqureshi wants to merge 8 commits into from

Conversation

@ttqureshi
Copy link
Member

@ttqureshi ttqureshi commented Mar 24, 2025 •
edited
Loading

The implementation has been taken from XmlMixin's pointer tags parsing logic.

Testing Notes:

  • Run all test cases
  • Check course import functionality with pointer tag syntax in CMS

Overall bug investigation ticket: openedx/edx-platform#36390

@ttqureshi ttqureshi force-pushed the ttqureshi/pointer-tag branch 2 times, most recently from b3eb22a to 5e0828f Compare March 26, 2025 10:30
@ttqureshi
Copy link
Member Author

ttqureshi commented Mar 26, 2025

This PR is ready for review.
cc: @kdmccormick @feanil

@kdmccormick kdmccormick self-requested a review March 26, 2025 14:01
@ttqureshi ttqureshi mentioned this pull request Apr 3, 2025
Merged
8 tasks
@farhan farhan moved this to 👀 In review in Aximprovements Team Apr 3, 2025
@ttqureshi ttqureshi mentioned this pull request Apr 3, 2025
Open
farhan
farhan requested changes Apr 7, 2025
View reviewed changes
@ttqureshi ttqureshi force-pushed the ttqureshi/pointer-tag branch from 15c9d82 to cd7ba89 Compare April 8, 2025 10:51
@ttqureshi ttqureshi requested a review from farhan April 8, 2025 11:01
farhan
farhan requested changes Apr 8, 2025
View reviewed changes
@ttqureshi ttqureshi force-pushed the ttqureshi/pointer-tag branch 5 times, most recently from fd3b0eb to d247d0d Compare April 9, 2025 09:53
@ttqureshi ttqureshi requested a review from farhan April 9, 2025 11:51
farhan
farhan reviewed Apr 9, 2025
View reviewed changes
farhan
farhan reviewed Apr 9, 2025
View reviewed changes
farhan
farhan reviewed Apr 9, 2025
View reviewed changes
farhan
farhan reviewed Apr 9, 2025
View reviewed changes
@ttqureshi ttqureshi force-pushed the ttqureshi/pointer-tag branch from bc0a1f5 to fe49706 Compare April 10, 2025 08:37
@ttqureshi ttqureshi requested a review from farhan April 10, 2025 08:38
@farhan
Copy link
Contributor

farhan commented Apr 10, 2025

I didn't test this PR manually, I am trusting Tayyab good domain knowledge and testing on it

@ttqureshi
Copy link
Member Author

ttqureshi commented Apr 15, 2025

@kdmccormick
A gentle reminder, your review is awaited for this PR. It's a blocker for LTIBlock Test PR.

@ttqureshi ttqureshi force-pushed the ttqureshi/pointer-tag branch from fe49706 to d8f02a9 Compare April 15, 2025 12:58
@kdmccormick
Copy link
Member

kdmccormick commented Apr 15, 2025

Yep, this is in my queue Tayyab, sorry for my slowness.

@ttqureshi
Copy link
Member Author

ttqureshi commented Apr 16, 2025

Yep, this is in my queue Tayyab, sorry for my slowness.

No worries

kdmccormick
kdmccormick requested changes May 6, 2025
View reviewed changes
xblock/utils/helpers.py Outdated
Comment on lines 64 to 81
def load_definition_xml(node, runtime, def_id):
"""
Parses and loads an XML definition file based on a given node, runtime
environment, and definition ID.
Arguments:
node: XML element containing attributes for definition loading.
runtime: The runtime environment that provides resource access.
def_id: Unique identifier for the definition being loaded.
Returns:
tuple: A tuple containing the loaded XML definition and the
corresponding file path.
"""
url_name = node.get('url_name')
filepath = format_filepath(node.tag, name_to_pathname(url_name))
definition_xml = load_file(filepath, runtime.resources_fs, def_id)
return definition_xml, filepath
Copy link
Member

@kdmccormick kdmccormick May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work in xblock-sdk as well, or is this logic specific to edx-platform?

If it is specific to edx-platform, then we want want to go through a Runtime method instead, so that edx-platform and xblock-sdk can implement it differently.

Copy link
Member Author

@ttqureshi ttqureshi May 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work in xblock-sdk as well, or is this logic specific to edx-platform?

If it is specific to edx-platform, then we want want to go through a Runtime method instead, so that edx-platform and xblock-sdk can implement it differently.

@kdmccormick
I've added the load_definition_xml function to the Runtime class so that it can be optionally overridden by other Runtimes like xblock-sdk. I haven't made it abstract because, as of now, xblock-sdk does not have a course import mechanism that handles the 'pointer-tag' syntax in XML definitions. Making it abstract would unnecessarily enforce implementation where it's not currently needed.

@ttqureshi @kdmccormick
chore: remove lint-amnesty
fa76371 
Co-authored-by: Kyle McCormick <[email protected]>
@ttqureshi ttqureshi force-pushed the ttqureshi/pointer-tag branch from 6139828 to 8f6f7e0 Compare May 8, 2025 08:41
@ttqureshi ttqureshi force-pushed the ttqureshi/pointer-tag branch from 8f6f7e0 to 12e8032 Compare May 9, 2025 08:08
@ttqureshi ttqureshi force-pushed the ttqureshi/pointer-tag branch from 2ad1be0 to ea7eddd Compare May 12, 2025 13:01
@ttqureshi ttqureshi requested a review from kdmccormick May 12, 2025 13:03
@ttqureshi ttqureshi changed the title Adds pointer tag parsing implementation in parse_xml feat:! Adds pointer tag parsing implementation in parse_xml May 13, 2025
@bradenmacdonald
Copy link
Contributor

bradenmacdonald commented May 14, 2025

I'm not so sure about this approach and wrote up some questions here on the related issue. I don't want to unnecessarily hold things up so don't take this as a definite blocker, but I do want to say that:

In general, the way that parse_xml (and the related methods like parse_xml_new_runtime, definition_from_xml, load_definition_xml, etc.) works in edx-platform is very messy and confusing. A big part of the problem is the inconsistent use of XmlMixin and the complex method resolution order of all our mixins in general, but the main problem is just the stupid API contract of parse_xml, add_node_as_child, etc. But given that we can't really change the latter anytime soon, I would strongly prefer that we find ways to make the platform's XML parsing simpler and more consistent, rather than adding more divergence and complexity to the core XBlock parsing APIs.

kdmccormick
kdmccormick requested changes Jun 3, 2025
View reviewed changes
Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are discussing alternative implementations on #823 (comment)

@ttqureshi
Copy link
Member Author

ttqureshi commented Aug 22, 2025

Closing this PR in favor of this PR in the edx-platform.

@ttqureshi ttqureshi closed this Aug 22, 2025
@ttqureshi ttqureshi deleted the ttqureshi/pointer-tag branch August 22, 2025 06:28
@ttqureshi ttqureshi mentioned this pull request Jan 1, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kdmccormick kdmccormick kdmccormick requested changes

@farhan farhan farhan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

4 participants

@ttqureshi @farhan @kdmccormick @bradenmacdonald