feat: allow block publish if was published before Library v2 migration#38865
feat: allow block publish if was published before Library v2 migration#38865asadali145 wants to merge 1 commit into
Conversation
|
Thanks for the pull request, @asadali145! 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. |
4db4e90 to
4f8a913
Compare
4f8a913 to
b59b9e4
Compare
b59b9e4 to
1962ae8
Compare
|
@arslanashraf7 this should be good for another look. |
|
Does this need Product review? I feel like @jmakowski1123 told me once that there was some concern about having blocks published as part of the migration process. |
@pdpinch, we are not changing the default behavior. It is still the same after the changes. I would like to know more about the concerns about publishing the migrated blocks. |
|
Also FYI @kdmccormick |
kdmccormick
left a comment
There was a problem hiding this comment.
I can see the value in reducing number of clicks authors need to do in order to migrate, but I am wary of introducing a bug by publishing in non-standard way.
| from cms.djangoapps.modulestore_migrator import api as migrator_api | ||
| store = modulestore() | ||
| with store.bulk_operations(self.course_id): | ||
| was_published = persist_publish_state and store.has_published_version(self) |
There was a problem hiding this comment.
I think this will check if there is any published version, not whether the current version is published, right?
def has_published_version(self, xblock):
"""
Returns whether this xblock has a published version (whether it's up to date or not).
"""
return self._get_head(xblock, ModuleStoreEnum.BranchName.published) is not NoneThere was a problem hiding this comment.
Yes, that's right. Let me update it to check the current version is published.
There was a problem hiding this comment.
I have updated the PR and used has_changes to detect if there are any draft changes.
| self.save() | ||
| store.update_item(self, user_id) | ||
| if was_published: | ||
| store.publish(self.location, user_id) |
There was a problem hiding this comment.
@ormsbee Is there prior art for publishing a single course component rather than a unit?
There was a problem hiding this comment.
The only reason behind not publishing a unit was that a unit can contain draft and published blocks.
There was a problem hiding this comment.
Not that I'm aware of, though I think it should work. We do auto-publish some standalone HTMLBlock sub-types like for course-tab content.
| @shared_task(base=LegacyLibraryContentToItemBank, bind=True) | ||
| def migrate_course_legacy_library_blocks_to_item_bank(self, user_id: int, course_key: str): | ||
| def migrate_course_legacy_library_blocks_to_item_bank( | ||
| self, user_id: int, course_key: str, persist_publish_state: bool = False, |
There was a problem hiding this comment.
what code would ever specify persist_publish_state = True?
There was a problem hiding this comment.
The idea is to provide a management command to do this.
There was a problem hiding this comment.
ah, would the management command be an alternative to migrating the legacy library references from within the Studio UI?
could you describe the proposed CLI a bit more?
There was a problem hiding this comment.
When I explored the code around migration, I saw replace_v1_lib_refs_with_v2_in_courses. It looked like it should do what the migration API does when we call from the authoring MFE, but this command replaces the existing field references with V2, and that field is not even compatible with V2 Library locator. I don't know why it still exists there.
Then I found the API, and it had this publishing issue. My idea is to keep the current API as is (Keep the blocks in draft). Create a new command that does the same but also provides an optional argument to publish the blocks using this flag. So, for people who really know what they are doing, they can use this command.
Another option is to maybe give another button in the authoring MFE that hits the same API but also publishes the blocks using this flag.
Let me know what you think of the 2 options, and I can create a follow-up PRs or maybe if we go with the command then I can add it in this PR.
1962ae8 to
3b9c887
Compare
Description
This PR adds the ability to publish a library content block if it was published before the V2 migration. This would be helpful for anyone having too many courses and libraries.
Testing instructions
content -> library update -> Review content updatesand update the library references.