feat: add component versioning directory structure #356
Conversation
In the lp_dump file, only the draft and publishable versions are serialized, rather than all versions. Static files remain out of scope for this change.
openedx-webhooks
added
the
open-source-contribution
|
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
ormsbee
left a comment
There was a problem hiding this comment.
Just a couple of requests. I had one other high level concern though: one of the issues with export is going to be performance. In particular, we're going to want to pull as much of this stuff back in one query as possible while doing iteration across all the publishable entities.
So for instance, grabbing all publishable entities in this case should include doing select related for the draft version and published version, so that we're not doing n+1 queries for those.
I think it's okay to do a separate query for each ComponentVersion for now, though this is something I also think we'll eventually want to further optimize.
Can you please make sure the correct select_relateds are happening to make it so that the for loop through PublishableEntities doesn't make extra queries?
Thank you.
| current_draft: Optional[Draft] = getattr(entity, "draft", None) | ||
| current_published: Optional[Published] = getattr(entity, "published", None) | ||
|
|
||
| draft_version: Optional[PublishableEntityVersion] = getattr(current_draft, "version", None) | ||
| published_version: Optional[PublishableEntityVersion] = getattr(current_published, "version", None) |
There was a problem hiding this comment.
I created an issue for making it so that this code isn't necessary: #362
But I came to realize that doing that would introduce more problems.
Then I thought to ask you to please replace this with calls to get_published_version and get_draft_version, but I realize that those methods are actually going to be really inefficient for this iteration, because they query the Published and Draft tables directly, instead of using potentially pre-loaded data.
So my request for this PR is:
- Please use
get_published_versionandget_draft_versionhere, but... - Please modify
get_published_versionandget_draft_versionso that they can take either the integer primary key (as they do today), or thePublishableEntitymodel object. You can see an example of this sort of pattern inset_draft_version.
If a PublishableEntity model is passed into those functions, the code should operate very much like how you have here, where it checks for whether the attribute exists and returns None if there is no current published/draft version.
tests/openedx_learning/apps/authoring/backup_restore/test_backup.py
Outdated
Show resolved
Hide resolved
All suggestions have been addressed. Thanks a lot for the helpful input! |
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
Looking good. Just some low level requests. Thank you!
| "component", | ||
| "container", | ||
| "component__component_type", |
There was a problem hiding this comment.
Things in the publishing app shouldn't show an awareness of components, because that's a higher-level abstraction. Also, we're eventually going to pull containers out of publishing, so please remove that as well. If we need those for performance reasons, please add those select_related from the calling app (since we can chain select_related calls together).
There was a problem hiding this comment.
Understood. Changes applied. Thanks
| # The following code retrieves the draft version for a given PublishableEntity. | ||
| # Useful for preloading the draft version in certain contexts. |
There was a problem hiding this comment.
I think the most useful part of this is that it gracefully handles the edge cases when there is no draft version, not really pre-loading.
| # The following code retrieves the draft version for a given PublishableEntity. | ||
| # Useful for preloading the draft version in certain contexts. | ||
| draft: Optional[Draft] = getattr(publishable_entity_id, "draft", None) | ||
| return getattr(draft, "version", None) |
There was a problem hiding this comment.
Please explicitly check to see if draft is None and then return draft.version if it's not. Something like:
draft: Optional[Draft] = getattr(publishable_entity_or_id, "draft", None)
if draft is None:
return None
return draft.versionI know it's basically equivalent at the moment, but getattr is a dangerous tool that I think we should minimize our use of. We have no choice but to use it when getting the draft because that's just how Django 1:1 relations work. But if for some terrible reason we decided to change the attribute from draft.version to draft.current, we want it to explode in an error and not silently return None.
There was a problem hiding this comment.
Nice!, I get it now. Thanks.
…d misc improvements
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
Very minor question/suggestion.
| api.create_component_version( | ||
| cls.published_component.pk, | ||
| version_num=cls.published_component.versioning.draft.version_num + 1, | ||
| title="My published problem draft v2", | ||
| created=cls.now, | ||
| created_by=cls.user.id, | ||
| ) |
There was a problem hiding this comment.
This use case comes up so often that we made a create_next_component_version() function for it. Please use that instead.
| for e in entities: | ||
| draft = getattr(e, 'draft', None) | ||
| published = getattr(e, 'published', None) | ||
|
|
||
| _ = getattr(draft, 'version', None) | ||
| _ = getattr(published, 'version', None) |
There was a problem hiding this comment.
It's not clear whether we expect the draft/published versions to exist or not. Please make these regular access calls to draft.version and published.version and do something like check an attribute's value, e.g. assert draft.version.version_num == 2
3 participants
Description
Resolves: #352
This change updates the
lp_dumpfile to serialize only the draft and publishable versions, instead of all available versions.In addition, TOML component files are now excluded from the top level of the entities directory.
Static files remain out of scope for this change.
Directory Output Example
Given these components:
v1,v2v2v1v1,v2v2NoneDump File Output