Emit events when a learning package is modified [FC-0117]#543
Emit events when a learning package is modified [FC-0117]#543bradenmacdonald wants to merge 8 commits intomainfrom
Conversation
openedx-webhooks
added
open-source-contribution
|
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. 🔘 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. |
bradenmacdonald
force-pushed
the
braden/emit-events
branch
from
b92d224 to
67cb958
Compare
bradenmacdonald
changed the title
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bradenmacdonald
force-pushed
the
braden/emit-events
branch
2 times, most recently
from
19b76fa to
4aa66f5
Compare
|
|
||
| old_version: int | None | ||
| """ | ||
| The old version_num of this entity (not the PublishableEntityVersion ID). |
There was a problem hiding this comment.
Working on this PR has made me want to add fully-typed IDs for PublishableEntityVersion and some kind of VersionNum type, because we do mix and match those a bit in the code, but they're very different things.
(I made PublishableEntity.id fully-typed but hadn't yet done ___Version)
| assert event.signal is api.signals.LEARNING_PACKAGE_ENTITIES_CHANGED | ||
| assert event.kwargs["learning_package"].id == learning_package.id | ||
| assert event.kwargs["learning_package"].title == "Test LP 📦" | ||
| assert event.kwargs["changed_by"].user_id is None | ||
| assert event.kwargs["change_log"].draft_change_log_id == expected_draft_change_log_id | ||
| assert event.kwargs["change_log"].changes == [ | ||
| api.signals.ChangeLogRecord(entity_id=entity.id, old_version=None, new_version=NEW_VERSION_NUM), | ||
| ] | ||
| assert event.kwargs["metadata"].time == now_time |
There was a problem hiding this comment.
Unfortunately, openedx-events doesn't yet support typing so event.kwargs is just a generic dict.
bradenmacdonald
force-pushed
the
braden/emit-events
branch
from
4aa66f5 to
296b74c
Compare
| class ChangeLogRecordData: | ||
| """A single change that was made to a PublishableEntity""" | ||
|
|
||
| entity_id: PublishableEntity.ID |
There was a problem hiding this comment.
Should I include container_type, component_type, container_code, and component_code in these ChangeLogRecords? I think so, but I currently implemented this entirely within the publishing app, and doing so would require either making publishing aware of containers/components or moving the signals out of publishing to become generic openedx_content signals. Perhaps I could even make signals its own applet?
There was a problem hiding this comment.
@ormsbee @kdmccormick Any opinions on this question ^ ?
Edit: I think I have something in mind that'll work, so no worries / no rush here.
bradenmacdonald
force-pushed
the
braden/emit-events
branch
from
716354e to
88f3791
Compare
2 participants
Placeholder PR for #462 ; not much to see here yet.