fix: don't allow publishing within a draft change log context#580
fix: don't allow publishing within a draft change log context#580bradenmacdonald wants to merge 1 commit intomainfrom
Conversation
|
Thanks for the pull request, @bradenmacdonald! 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. |
I'm not sure if this is best described as a
fix:or afeat!:, but I'm opening this PR so we can consider re-landing this additional validation.This PR disallows any publish APIs within a bulk draft changes context.
For the record, before we merge it, we have to refactor openedx-platform's modulestore_migrator: openedx/openedx-platform#38508
Part of #463 . Originally merged as part of #521 but later reverted in #574 after we discovered the modulestore_migrator issue.
Motivation
Allowing publishes within a draft change context risks publishing snapshots that are in an intermediate draft state. A
DraftChangeLogis designed to collapse repeated edits to the same entity into a single record (old→final) at exit — see_add_to_existing_draft_change_log. If you doset_draft_version(A, v2); publish_all_drafts(...); set_draft_version(A, v3)inside the same context,Published.versionends up at v2 (a transient mid-bulk state), while theDraftChangeLogrecords A: v1 → v3. The publish captures a state that was never a settled state of the bulk change. (particularly relevant with events that get emitted, as published events would be emitted before draft events.)Furthermore, in the specific case of modulestore_migrator it currently does something we definitely don't want: it groups all the migrated drafts into a single draft change log, but it creates many separate publish logs - one for each imported component and container.
Alternative
An alternative validation could be to allow publishes within a bulk draft changes context, but then disallow any further draft changes to those entities which are published. I don't really like this approach though as it requires a lot more bookkeeping. It would allow us to keep the modulestore_migrator code unchanged, but I think that's not good because it still has this problematic creation of many publish logs.