feat: remove unpublished from collections [FC-0062] #252
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @ChrisChV! What's next?Please work through the following steps to get your changes 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. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by 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. |
ChrisChV
changed the title
| def remove_unpublished_from_collections(learning_package_id: int) -> None: | ||
| """ | ||
| Removes all unpublished entities from collections for a | ||
| given learning package. |
There was a problem hiding this comment.
@ChrisChV Can you please add an explanation/example of why we need this API, and what it's used for?
There was a problem hiding this comment.
Also as part of the PR and commit message please :)
One use case for this function is when reverting to published. If a collection has unpublished entities and the learning package is reverted to the published version, the unpublished entities need to be removed.
ChrisChV
force-pushed
the
chris/FAL-3873-remove-unpublished-from-collections
branch
from
72f9463 to
3b940ce
Compare
|
|
||
| One use case for this function is when reverting to published. | ||
| If a collection has unpublished entities and the learning package is reverted | ||
| to the published version, the unpublished entities need to be removed. |
There was a problem hiding this comment.
I think this API call would have unintended side-effects, since there may be many Collections that have unpublished things in them. Doing a revert shouldn't empty them all out.
There was a problem hiding this comment.
Doing a revert shouldn't empty them all out.
I understand that, in the context of libraries, it should be empty if no component is published. When a library is published, all its components, including those belonging to collections, are published. Similarly, when reverting, all components, including those belonging to collections, are discarded.
Since it is in the context of libraries, this function is only called when reverting a library, and has not been added when reverting any learning_package
I updated the comment to specify that the use case is in the context of libraries: e16f6a2
There was a problem hiding this comment.
If a library has components that were never published, but they've been added to a collection: when you "revert changes" to the library, those components should be essentially deleted. In that case, the deletion should already be propagating to the collection model. If not, that's a bug we need to fix in here - but shouldn't require an explicit API call.
If a library has components that were published then modified and/or added to new collections: when you "revert changes" to the library, the changes to the components should be reset so they match the published version. But, they will still be in any new collections that they've been added to. This is because tags and collections are not part of the draft-publish workflow.
Does that make sense? If so, I don't think we need this proposed API.
There was a problem hiding this comment.
@bradenmacdonald: I think we worked this out in the issue thread: openedx/modular-learning#234 (comment)
|
Closed, It's no longer used |
Description
Add
remove_unpublished_from_collectionsinapi. Removes all unpublished entities from collections for a given learning package. One use case for this function is when reverting a library to published version. If a collection of a library has unpublished entities and the library is reverted to the published version, the unpublished entities need to be removed to reflect the published state of the librarySupport information
Github issue: openedx/modular-learning#234
Internal ticket: FAL-3921
Used on: openedx/edx-platform#35734
Testing instructions