-
Notifications
You must be signed in to change notification settings - Fork 17
feat: add load process for components and their versions #390
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
feat: add load process for components and their versions #390
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. Where 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. |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why return this instead of raising an error?
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.
Yeah it was weird. I changed it. Thanks
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.
Is there a newer version of this that you haven't pushed up yet?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The draft_version should exist regardless of whether or not it's the same as the published version.
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.
Fixed. Thanks
- 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! |
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.
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. |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do non-TOML files end up here in the first place?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please write a test for this.
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.
Applied. Thanks




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:
