feat: add load process for components and their versions#390
feat: add load process for components and their versions#390ormsbee merged 6 commits intoopenedx:mainfrom
Conversation
|
Thanks for the pull request, @dwong2708! 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. |
ormsbee
left a comment
There was a problem hiding this comment.
First pass complete. Thank you!
| if "entity" not in pe_data: | ||
| raise ValueError("Invalid publishable entity TOML: missing 'entity' section") | ||
| if "version" not in pe_data: | ||
| raise ValueError("Invalid publishable entity TOML: missing 'version' section") | ||
| if "key" not in pe_data["entity"]: | ||
| raise ValueError("Invalid publishable entity TOML: missing 'key' field") | ||
| if "can_stand_alone" not in pe_data["entity"]: | ||
| raise ValueError("Invalid publishable entity TOML: missing 'can_stand_alone' field") |
There was a problem hiding this comment.
Assume that these error messages will go into a log file, and try to surface everything that's wrong with the publishable entity at once, not just one at a time. Also include identifying information for which publishable entity is missing this information if possible, so there's some way to identify it.
There was a problem hiding this comment.
I implemented serializers to collect those errors for a future log file. Thank you.
| component_content = self._read_file_from_zip(zipf, component_file_path) | ||
| component_data, component_version_data = parse_publishable_entity_toml(component_content) | ||
|
|
||
| with publishing_api.bulk_draft_changes_for(learning_package.id): |
There was a problem hiding this comment.
If it's straightforward to do, we probably want two big bulk_draft_changes_for blocks--the first loads the published versions of everything and publishes them. The second would create the draft versions of everything.
There was a problem hiding this comment.
Change applied, thanks!
| A tuple of (ComponentType, local_key) if valid, else (None, None). | ||
| """ | ||
| if not entity_key: | ||
| return None, None |
There was a problem hiding this comment.
Why return this instead of raising an error?
There was a problem hiding this comment.
Yeah it was weird. I changed it. Thanks
There was a problem hiding this comment.
Is there a newer version of this that you haven't pushed up yet?
There was a problem hiding this comment.
Oh, I see. I forgot to remove it. It’s no longer needed since I added this logic in the components API layer. I'll remove it now.
There was a problem hiding this comment.
Applied. Thank you
|
|
||
| # Extract component type information | ||
| namespace, component_type_name, filename = parts | ||
| local_key = filename.rsplit(".", 1)[0] # Remove .toml extension |
There was a problem hiding this comment.
The component key parts should be derived from the [entity] key, not the file path, since the entity key is the canonical source for that data. Also, the logic for deriving the key parts from the entity key is something that could go into the components app api.py file.
There was a problem hiding this comment.
It was already using the entity key. I applied a change to move it to the component API file. Thanks.
|
|
||
| # Only assign draft if it’s not the same as published | ||
| if version_num == draft_version_num and version_num != published_version_num: | ||
| draft_version = version |
There was a problem hiding this comment.
The draft_version should exist regardless of whether or not it's the same as the published version.
- Add Component and ComponentVersion serializers. The goal is to validate inputs and capture errors consistently. - Refactor the component saving process: it now uses two separate blocks with the bulk draft changes context manager.
|
First round of addressing comments is done. In a nutshell, I implemented these changes:
Results derived from these changes: Input: Output: Thank you! |
ormsbee
left a comment
There was a problem hiding this comment.
Can you please investigate using DRF Serializers to do the validation? It seems like if we're at the point where we're using serializer classes, we might as well use standard ones.
You'd probably want to subclass rest_framework.serializers.Serializer for this work.
Thank you.
|
I agree with using DRF Serializer, it was straightforward to implement here. |
ormsbee
left a comment
There was a problem hiding this comment.
Question and a small request for a test, but otherwise looks good to merge. Thank you.
| if not file.endswith(".toml"): | ||
| # Skip non-TOML files | ||
| continue |
There was a problem hiding this comment.
How do non-TOML files end up here in the first place?
There was a problem hiding this comment.
The non-TOML files are static files. Here’s an example list
file entities/xblock.v1/drag-and-drop-v2/4d1b2fac-8b30-42fb-872d-6b10ab580b27/component_versions/v2/block.xml
file entities/xblock.v1/html/e32d5479-9492-41f6-9222-550a7346bc37/component_versions/v5/static/me.png
file entities/xblock.v1/html/e32d5479-9492-41f6-9222-550a7346bc37/component_versions/v5/block.xml
file entities/xblock.v1/html/e32d5479-9492-41f6-9222-550a7346bc37/component_versions/v4/block.xml
file entities/xblock.v1/html/e32d5479-9492-41f6-9222-550a7346bc37/component_versions/v4/static/me.png
file entities/xblock.v1/openassessment/1ee38208-a585-4455-a27e-4930aa541f53/component_versions/v2/block.xml
file entities/xblock.v1/problem/256739e8-c2df-4ced-bd10-8156f6cfa90b/component_versions/v2/block.xml
file entities/xblock.v1/survey/6681da3f-b056-4c6e-a8f9-040967907471/component_versions/v1/block.xml
file entities/xblock.v1/video/22601ebd-9da8-430b-9778-cfe059a98568/component_versions/v3/block.xml
This has not been implemented yet, but it will be included in the next steps
| f"Invalid entity_key format: {entity_key!r}. " | ||
| "Expected format: '{namespace}:{type_name}:{local_key}'" | ||
| ) from exc | ||
| return get_or_create_component_type(namespace, type_name), local_key |
There was a problem hiding this comment.
Please write a test for this.




Resolves: #383
Changes
End-to-End Testing Output
Input:
Given this dump file:
test.zip
Result:
We obtained the following output:
Components:

Publish Logs:

Draft Change Logs:
