-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat: Configured subsection prerequisites persist during course olx/xml export/import #37475
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for the pull request, @haftamuk! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
c847977 to
d43f4b4
Compare
8a6be77 to
f675094
Compare
tecoholic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haftamuk Hi, the changes look good on the export side. I was able to verify that the <prerequisite> block is added to the output XML as expected. However, when I tried to reimport it in a new course, I ended up with this:
So, we are clearly missing the "import" part of the solution. Can you kindly add that functionality?
Additionally, I would recommend adding some unit test around the newly created functions. This is a very "core" part of the system and it's best to have unit tests in place.
Kindly see the import functionality is added! |
@tecoholic I will proceed to ensure existing unit tests pass and introduce new unit tests for the main changes made. |
|
@haftamuk Thanks for updating the PR with the import implementation. It looks like you have used a Generative AI / LLM for the implementation and hence can't be used as is. Kindly consult the [official policy on use of LLMs for acceptable usage guidelines. In short, you can use it to experiment and create solutions, but the final code should be from the developer. That doesn't mean we will have to discard this work. Let's use this as a base for the final solution. I have an important request - for future work on this PR, please do not force-push changes. This will allow us to work ahead incrementally and rollback things if we end up doing something we don't want in the final solution. Kindly see my inline comments in the diff for specific details. |
|
@haftamuk I realized, there is lot of things I have noted in comments. So, let's go step by step in addressing them
Ping me for a review after that. Let's move on to the import part of the changes after that. |
|
@tecoholic , would you be able to review this? I can review it as the CC in the next sprint, but I still have a few things to address in the current one. cc: @haftamuk |
|
@Agrendalath Absolutely. I will review it this sprint and you can do the CC in next. |
tecoholic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haftamuk This is looking great. I have a few more asks on the tests department. Can you kindly address those?
| self.mock_sequential = mock.Mock() | ||
| self.mock_sequential.location = self.sequential_location | ||
|
|
||
| def test_gating_api_calls(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tests only the happy path where the the mock_sequential has all the necessary values. More test cases convering the other code paths like the if conditional, the try...except block where the values are either missing or not in the expected format (int, string..etc) should be added to this test case.
| assert metadata["prev_url"] == "prev_url" | ||
|
|
||
| @patch("openedx.core.lib.gating.api.get_required_content") | ||
| def test_export_sequence_with_prerequisites(self, mock_get_required_content): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the importer testcase, this only checks the happy path of the code. Let's add test cases where the 'if' conditions in add_prerequisite_to_xml produce other resutls as well.
e8b582c to
2ff0c86
Compare
tecoholic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I tested this: I verified that the export and import of courses preserves pre-requisites in sequentials.
- I read through the code
- I checked for accessibility issues
- Includes documentation - Handled in a separate PR.
|
Hello @Agrendalath, Thank you |
|
@Agrendalath Hi, a gentle reminder on taking a look at this PR. |
|
Sorry for the delay, @haftamuk, @tecoholic. I will review this later this week. |
053784a to
d663324
Compare
|
Hello @Agrendalath , Thank you. |
Agrendalath
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @haftamuk, sorry for the delay. I reviewed non-test code and left some comments.
xmodule/seq_block.py
Outdated
| prereq_id is None | ||
| or not isinstance(prereq_id, str) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: the second condition already covers the first one.
| self.add_prerequisite_to_xml(xml_object) | ||
| return xml_object | ||
|
|
||
| def add_prerequisite_to_xml(self, xml_object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exports do not contain the information whether a subsection is available as a prerequisite: https://github.com/openedx/edx-platform/blob/d6633243a8397bb56eb21e38cc43efada4f05931/cms/templates/js/access-editor.underscore#L43-L55
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xmodule/modulestore/xml_importer.py
Outdated
|
|
||
| prerequisite_usage_key = course_key.make_usage_key('sequential', required_content) | ||
|
|
||
| gating_api.add_prerequisite(course_key, prerequisite_usage_key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's consider the opposite scenario: the existing sequential in the course has prerequisites, but its equivalent in the course export does not. Should we delete the prerequisites in such a case?
xmodule/modulestore/xml_importer.py
Outdated
| gating_api.set_required_content( | ||
| course_key, | ||
| block.location, | ||
| prerequisite_usage_key, | ||
| min_score, | ||
| min_completion | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will raise an exception if the subsection does not exist. It should be okay to mark the import as failed in this scenario (instead of silently ignoring the errors), but we should ensure that the error message is clear to the course author.
061b9d2 to
aaef363
Compare
3181af7 to
2c0ceab
Compare
|
@Agrendalath Thank you. |
…rt/import
* feat: adds prerequisite information attributes to exported OLX. e.g.
<sequential
required_content="91d0290972c4488db10d7ca3694e13ca"
min_score="100"
min_completion="100"
... >
<vertical url_name="some_vertical"/>
...
</sequential>
* feat: parse prerequisite information attributes during import and
create gating relationship.
Issue Link: openedx#36995
This commit contans fixes as per review recomendations. This commit changes will be merged later after review approval
|
@Agrendalath A gentle reminder for a review here. |
Agrendalath
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tecoholic, I did another pass. The redundant checks are gone, but the crucial edge cases from my previous review remain unaddressed.
cc: @haftamuk
| min_score = int(min_score) | ||
| min_completion = int(min_completion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (+catching the exceptions) is redundant. set_required_content already validates casting these variables to integers with _validate_min_score and raises GatingValidationError.
| self.add_prerequisite_to_xml(xml_object) | ||
| return xml_object | ||
|
|
||
| def add_prerequisite_to_xml(self, xml_object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| prereq_usage_key_str, min_score, min_completion = prereq_info | ||
| if not isinstance(prereq_usage_key_str, str): | ||
| return | ||
| prereq_usage_key = UsageKey.from_string(prereq_usage_key_str) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haftamuk, I did not test this one, but wouldn't it be sufficient to add prereq_usage_key_str to XML (without building the UsageKey?
| min_completion | ||
| ) | ||
| except (GatingValidationError) as e: | ||
| logging.debug('Gating validation error : %s', {e}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haftamuk, a reminder about this comment. Importing the course would still fail silently.
|
|
||
| prerequisite_usage_key = course_key.make_usage_key('sequential', required_content) | ||
| try: | ||
| gating_api.add_prerequisite(course_key, prerequisite_usage_key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haftamuk, a reminder about this comment.
Configured subsection prerequisites persist during course olx/xml export/import
Overview
This PR introduces support for subsection (sequential) prerequisites in the OLX course format. Course authors can define prerequisite relationships between subsections with configurable completion criteria including minimum score and completion percentage requirements. Currently this information is lost during course import/export. This PR ensures configured subsection prerequisites are persisted during course olx/xml export/import. It allows course authors to define prerequisite relationships, have them exported to OLX format, and finally, have them properly imported.
Problem Statement
Currently, Open edX supports to define prerequisite relationships between subsections in Studio. This enables course authors from enforcing learning pathways where students must complete certain subsections before accessing others, which is crucial for structured learning experiences. But when a course is exported (XML/OLX) all configured relationships of subsections are lost and authors has to do it again manually in studio. This is time consuming and error prone!
Proposed Solution
Key Files
Service Layer (
xmodule/seq_block.py)Import Processing Layer (
xmodule/modulestore/xml_importer.py)OLX Format
Prerequisite information attached to sequential xml element
Subsection prerequisite configuration how-to
Settings->Advanced->Enable subsection prerequisites->trueNote : Even after configuring a complete learning path for a course you can disable the gating feature by simply turning this in to false. Also, if you switch it back to true, all the previous configured learning path will appear with out further work. This means, switching this setting to true/false will not impact persisted learning path configuration, but will impact how the learner will interact with the course.
Note : When you export a course with this flag set to true, the exported XML will contain the information
Later during import this setting will be applied to the destination course. This is also the case when it is set to false (the destination course will be
enable_subsection_gatingset to false)How to test
python -Wd -m pytest -p no:randomly --ds=cms.envs.test ./xmodule/modulestore/tests/test_xml_importer.py::TestSequentialPrerequisitesImport::test_gating_api_callspython -Wd -m pytest -p no:randomly --ds=cms.envs.test ./xmodule/tests/test_sequence.py::SequenceBlockTestCase::test_export_sequence_with_prerequisitesLinked Issue: #36995
Supporting information