Skip to content

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.

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
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ttqureshi
Copy link

@ttqureshi ttqureshi commented Mar 24, 2025

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
Author

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

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

@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

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

@ttqureshi
Copy link
Author

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

No worries

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

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.

@ttqureshi ttqureshi force-pushed the ttqureshi/pointer-tag branch from 6139828 to 8f6f7e0 Compare May 8, 2025 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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