feat: New history log api functions [FC-0123]#501
feat: New history log api functions [FC-0123]#501ChrisChV wants to merge 7 commits intoopenedx:mainfrom
history log api functions [FC-0123]#501Conversation
|
Thanks for the pull request, @ChrisChV! 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. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. 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. |
history log api functionshistory log api functions [FC-0123]
| Return DraftChangeLogRecords for a PublishableEntity since its last publication, | ||
| ordered from most recent to oldest. | ||
|
|
||
| If the entity has never been published, all DraftChangeLogRecords are returned. |
There was a problem hiding this comment.
What is the intended behavior of this function when the last "publish" was a deletion?
There was a problem hiding this comment.
I think the current code does work in this case, to be clear. And I realize that it's not something that's likely to come up in the UX. I just want to make sure that the behavior is well defined and tested for in that edge case.
There was a problem hiding this comment.
Thanks! Yes, I'll add tests for that case.
|
High level comment: please note that it's possible to reset a draft to the published state (i.e. discard changes). When that happens, the Draft version is reset to point to the same version as Published, but all the versions created still exist and are reflected in the draft change log. |
| .values_list("draft_change_log__changed_by", flat=True) | ||
| .distinct() | ||
| ) | ||
| return get_user_model().objects.filter(pk__in=contributor_ids) |
There was a problem hiding this comment.
Do you think it is worth adding an order_by here to assert consistent results?
|
Per this slack thread, I think we're going to want to extend the Does this sound correct to you @sdaitzman, @marcotuts? |
|
I made a separate issue to capture user publish intent (my comment above), so that we can discuss in more detail: http://github.com/openedx/openedx-core/issues/533 |
| # If reset_drafts_to_published() was called within this publish window, there | ||
| # will be a DraftChangeLogRecord where new_version == baseline. Use the most | ||
| # recent such record as the new lower bound so discarded entries are excluded. |
There was a problem hiding this comment.
I know I was the one who flagged this because I wanted to make sure the code noted it, but I actually don't think we should filter out discarded changes. It's a log, and "discard changes" is a legit thing that someone in that chain of edits did. I think we should be explicit that it happened, especially if people are going here because they're thinking, "I know I was working on this, but what happened to my edits!?!" Then they could see "Dave discarded changes" and know who to strangle talk to.
|
|
||
| def get_descendant_component_entity_ids(container: Container) -> list[int]: | ||
| """ | ||
| Return the entity IDs of all leaf (non-Container) descendants of ``container``. |
There was a problem hiding this comment.
Code in the containers applet shouldn't be querying Draft models directly, or ideally even be aware of components.
Where is this going to get called from?
There was a problem hiding this comment.
Code in the containers applet shouldn't be querying Draft models directly, or ideally even be aware of components.
I've refactored the function to not use Draft
Where is this going to get called from?
This function is called in edx-platform, in get_library_container_draft_history and in get_library_container_publish_history. This is what I have implemented, as I mentioned in Slack. It might change later after implement the direct field.
Description
get_entity_draft_history: ReturnDraftChangeLogRecordsfor aPublishableEntitysince its last publication.get_entity_publish_history: Return allPublishLogRecordsfor aPublishableEntityget_entity_publish_history_entries: Return theDraftChangeLogRecordsassociated with a specificPublishLogList of features (TODOs): See openedx/openedx-platform#38178 (comment)
Supporting information
Testing instructions
Follow the testing instructions in openedx/frontend-app-authoring#2948
Deadline
Before the Verawood cut.
Other information
N/A