Skip to content

Conversation

@Faraz32123
Copy link

@Faraz32123 Faraz32123 commented Jun 3, 2025

In this PR,

  • fix parse_xml error that occurs during importing a course that contains problem-builder component or during copying pasting the problem-builder component by removing unused & unsupported positional argument i.e. "id_generator".
  • upgrade requirements especially "xblock[django]" to version "2.0.0" where this extra argument "id_generator" was removed.
    • With this requirement upgrade, also dropped support for django 3.2. due to conflicts in dependencies in CI.
      • ERROR: Cannot install Django<5.0 and >=4.2 and django==3.2.25 because these package versions have conflicting dependencies.
  • update ubuntu version ro ubuntu-latest.
  • update codecov actions to v5 in CI.

Note: This Change will be incompatible with Palm release of openedx.

Traceback (most recent call last):
  File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py", line 596, in _create_block
    created_xblock, notices = import_staged_content_from_user_clipboard(
                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/helpers.py", line 373, in import_staged_content_from_user_clipboard
    new_xblock = _import_xml_node_to_parent(
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/helpers.py", line 586, in _import_xml_node_to_parent
    _import_xml_node_to_parent(
  File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/helpers.py", line 586, in _import_xml_node_to_parent
    _import_xml_node_to_parent(
  File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/helpers.py", line 546, in _import_xml_node_to_parent
    temp_xblock = xblock_class.parse_xml(node, runtime, keys)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: ChoiceBlock.parse_xml() missing 1 required positional argument: 'id_generator'

@Faraz32123
Copy link
Author

Hi @Agrendalath, Can you look into this and merge this for me! before it's deprecated as it's deprecation PR isn't merged yet.

Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

@Faraz32123,

  1. Please indicate (in the commit message) that this is incompatible with Palm.
  2. tox -e py38-django42 fails with:
FAILED problem_builder/v1/tests/test_upgrade.py::TestUpgrade::test_xml_upgrade_1_v1_upgrade_a - TypeError: parse_xml() takes 4 positional arguments but 5 were given
FAILED problem_builder/v1/tests/test_upgrade.py::TestUpgrade::test_xml_upgrade_2_v1_upgrade_b - TypeError: parse_xml() takes 4 positional arguments but 5 were given

@Faraz32123 Faraz32123 force-pushed the fix/parse_xml_error_during_import branch from e381d58 to 9b04644 Compare June 4, 2025 05:28
@Faraz32123
Copy link
Author

@Agrendalath I'll ping you once it's ready.

@Faraz32123 Faraz32123 force-pushed the fix/parse_xml_error_during_import branch from 9b04644 to f43f9ab Compare June 4, 2025 07:46
@Faraz32123
Copy link
Author

@Agrendalath I have added details in commit message. Can you run the workflows again now.

@Agrendalath
Copy link
Member

@Faraz32123 , the worklogs will not run without changing the os to ubuntu-latest.

@Faraz32123 Faraz32123 force-pushed the fix/parse_xml_error_during_import branch from 4d42628 to f43f9ab Compare June 4, 2025 07:55
@Faraz32123
Copy link
Author

@Agrendalath let me create a separate PR for that.

@Faraz32123
Copy link
Author

Faraz32123 commented Jun 4, 2025

@Agrendalath I have created a simple PR here: #459 to update ubuntu version. But I think maintainers can also approve workflows , I am not sure if you are maintainer of this repo or not. But thanks for assisting me here.
Screenshot 2025-06-04 at 1 07 56 PM

41e1795
In this commit:
- fix parse_xml error that occurs during importing a course that contains problem-builder component or during copying pasting the problem-builder component by removing unused & unsupported positional argument i.e. "id_generator".
- upgrade requirements especially "xblock[django]" to version "2.0.0" where this extra argument "id_generator" was removed.
    - With this requirement upgrade, also dropped support for django 3.2. due to conflicts in dependencies in CI.
        - ERROR: Cannot install Django<5.0 and >=4.2 and django==3.2.25 because these package versions have conflicting dependencies.
- update ubuntu version ro ubuntu-latest.
- update codecov actions to v5 in CI.

Note: This Change will be incompatible with Palm release of openedx.
@Faraz32123 Faraz32123 force-pushed the fix/parse_xml_error_during_import branch from f43f9ab to f833c37 Compare June 5, 2025 09:22
integration tests are failing with below error, This error comes from xblock-utils package which is already deprecated. So, disabling these tests for now.

______ ERROR collecting problem_builder/tests/integration/test_titles.py _______
ImportError while importing test module '/home/runner/work/problem-builder/problem-builder/problem_builder/tests/integration/test_titles.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
problem_builder/tests/integration/test_titles.py:27: in <module>
    from xblockutils.base_test import SeleniumXBlockTest
.tox/integration42/lib/python3.8/site-packages/xblockutils/base_test.py:28: in <module>
    from workbench.test.selenium_test import SeleniumTest
E   ModuleNotFoundError: No module named 'workbench.test.selenium_test'
@Faraz32123
Copy link
Author

After first commit, integration42 test still fails with below errors. This error comes from xblock-utils package which is already deprecated. So, disabling these integration workflow for now.

___ ERROR collecting problem_builder/tests/integration/test_step_builder.py ____
ImportError while importing test module '/home/runner/work/problem-builder/problem-builder/problem_builder/tests/integration/test_step_builder.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
problem_builder/tests/integration/test_step_builder.py:9: in <module>
    from .base_test import (CORRECT, INCORRECT, PARTIAL, GetChoices,
problem_builder/tests/integration/base_test.py:23: in <module>
    from xblockutils.base_test import SeleniumBaseTest, SeleniumXBlockTest
.tox/integration42/lib/python3.8/site-packages/xblockutils/base_test.py:28: in <module>
    from workbench.test.selenium_test import SeleniumTest
E   ModuleNotFoundError: No module named 'workbench.test.selenium_test'

Detailed error trace can be seen here.

@Faraz32123
Copy link
Author

Faraz32123 commented Jun 5, 2025

@Agrendalath Can you look into it now!
There'll be only 3 workflows now commitlint, quality & py38-django42, these will be passing hopefully.

@codecov
Copy link

codecov bot commented Jun 5, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 45.04%. Comparing base (5602b7b) to head (edde9a6).
Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
problem_builder/v1/studio_xml_utils.py 0.00% 1 Missing ⚠️
problem_builder/v1/upgrade.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #458      +/-   ##
==========================================
- Coverage   45.08%   45.04%   -0.04%     
==========================================
  Files          40       40              
  Lines        3309     3303       -6     
  Branches      463      401      -62     
==========================================
- Hits         1492     1488       -4     
+ Misses       1769     1768       -1     
+ Partials       48       47       -1     
Flag Coverage Δ
unittests 45.04% <66.66%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

@Faraz32123, just a few final notes.

- bump minor version instead of patch one
- remove XBlock[django]==2.0.0 constraint from test.in file as it's already in constraints.txt
@Faraz32123 Faraz32123 force-pushed the fix/parse_xml_error_during_import branch from 797071e to 84352ea Compare June 10, 2025 07:08
Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

👍

  • I tested this: checked that the unit tests are passing
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: n/a

@Agrendalath Agrendalath merged commit a4cfa28 into open-craft:master Jun 16, 2025
9 checks passed
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.

2 participants