-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat: copy/paste containers #37008
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: copy/paste containers #37008
Conversation
|
Thanks for the pull request, @rpenido! 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. |
ae3c3dd to
8da2d48
Compare
6fdce69 to
e61a47e
Compare
e61a47e to
4ee9ce7
Compare
navinkarkera
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.
@rpenido Nice work! It mostly works with some issues like
- I had to remove these two lines to make copy work with units containing html blocks.
- And we need to handle static content while copying.
| from openedx.core.djangoapps.content_libraries import api as lib_api | ||
|
|
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.
Any reason for moving this 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.
We should stop importing the content_libraries here in the future, but we probably need to import the other way around. I moved it here to avoid circular imports.
4e705eb to
50de6b0
Compare
bradenmacdonald
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.
Great work! I think this is almost ready.
Co-authored-by: Braden MacDonald <mail@bradenm.com>
07ec126 to
78fca3b
Compare
78fca3b to
9b4efbd
Compare
|
Hi @bradenmacdonald @kdmccormick! Adding the |
|
@rpenido If you set |
| olx.attrib["source_key"] = str(container_key) | ||
| olx.attrib["source_version"] = str(container_metadata.draft_version_num) |
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.
Sorry for the drive-by comment, but is there a reason not to use the established attributes copied_from_block{_version} and/or upstream{_version} which we use for components that are copy-pasted or linked?
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 wait, source_key is for the container itself, not for the container it was copied from, right? Sorry, I'm out of the loop, if there's a thread or something where source_key was discussed I'm happy to read through that and catch myself up.
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.
Sorry for the drive-by comment, but is there a reason not to use the established attributes copied_from_block{_version}
It could use copied_from_block (instead of souce_key). While working on this, I don't remember finding a copy_from_block in the OLX, but it makes sense since we are using it on the (pasted) XBlock. I was trying to avoid referencing as blocks since these are Containers from libraries.
and/or upstream{_version}, which we use for components that are copy-pasted or linked?
Oh wait, source_key is for the container itself, not for the container it was copied from, right?
Exactly. This serializer is used when copying items from the library, so we probably won't have the upstream field here. But, if we expand the current code that copies from courses to use it, we will probably have both upstream (meaning that the source item is linked to an upstream library item) and souce_key, referencing the items that is being copied.
Sorry, I'm out of the loop, if there's a thread or something where source_key was discussed
I don't think we have had a thorough discussion beyond what we've included in this PR. Please let me know if you have any concerns!
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 could use copied_from_block (instead of souce_key). While working on this, I don't remember finding a copy_from_block in the OLX, but it makes sense since we are using it on the (pasted) XBlock. I was trying to avoid referencing as blocks since these are Containers from libraries.
Ah, I see. Currently, copied_from_block is mixed into CMS xblocks, and it is set when pasting an xblock. I agree that it'd be ideal if the field didn't include the word "block" (I wish it were just copied_from) but IMO consistency beats semantic correctness here. So, personally I'd prefer copied_from_block over source_key, but let me know if you disagree.
Exactly. This serializer is used when copying items from the library, so we probably won't have the upstream field here. But, if we expand the current code that copies from courses to use it, we will probably have both upstream (meaning that the source item is linked to an upstream library item) and souce_key, referencing the items that is being copied.
Yes, I think that matches the current behavior, which is:
upstreamis set iff it's linked to the librarycopied_from_blockis set iff it was pasted without link OR linked and then unlinked.
I don't think we need to set both upstream and copied_from_block at the same time.
9eabb47 to
9a267ec
Compare
9a267ec to
0754d08
Compare
0754d08 to
6f1fecc
Compare
|
Hi @bradenmacdonald! Do you think we are ready to merge now? |
bradenmacdonald
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.
Yep, I think this looks good. Thanks!
| if node_copied_from: | ||
| _fetch_and_set_upstream_link(node_copied_from, node_copied_version, temp_xblock, user) | ||
| elif copied_from_block: | ||
| # Use the copied_from_block param only if the copied_from_block from the OLX is not set. |
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.
Should we explain this slightly more? This says what we're doing but not why.
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.
Removed this fallback elif here 4f3e0bc
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
…penedx#37008) * feat: copy endpoint for Library Containers * fix: make source_usage_key optional and removing upstram info for xblock olx * test: add tests * refactor: remove unecessary changes to reduce diff * fix: change assert * feat: add `write_upstream` field to ContainerSerializer * fix: remove comment * refactor: change `source_usage_key` type and more * fix: try to infer the source version * fix: InvalidKeyError while copying container with assets * fix: read source_version from OLX * fix: remove store check * fix: change ident Co-authored-by: Braden MacDonald <mail@bradenm.com> * feat: fill source_version and make get_component_version_from_block public * refactor: rename `source_key` to `copied_from_block` * test: add test to `write_copied_from=false` * fix: removing unused fallback elif * fix: remove `copied_from_block` param --------- Co-authored-by: Braden MacDonald <mail@bradenm.com>
Description
This PR adds support for copying (any container) and pasting (for now, only units) on a course.
Supporting information
Testing instructions
POSTcall tohttp://studio.local.openedx.io:8001/api/libraries/v2/containers/{key}/copy/. You could use a Unit with an HTML component containing some images to test the static file copy.Tip: To get it right from your browser, you can rename a container and edit the
PATCHURL, changing the method toPOSTand appendingcopy/to the end of the URL.Units. Try to paste a copied unit into a section using the current UIDeadline
None
Private ref: FAL-4221