Events refinements: 'restored' flag on entity draft change, don't emit collection change when draft entity deleted.

[bradenmacdonald]

This PR improves events in openedx-core.

It does these two things that I think we definitely want:

1. Updates the ENTITIES_DRAFT_CHANGED event, to add a restored boolean field. This allows event handlers to distinguish between "newly created" draft entities and "un-deleted" draft entities, which was previously not possible with only the information in the event itself. This is a change to the event data payload, but it's adding a field and the event is brand new and not yet published/used anywhere. I think we do want this, although it does add an extra query to the event generation code.
2. Adds extra validation to prohibit adding entities to a deleted collection. If you're trying to do that, it's likely a mistake/bug.

And it does this thing that I'm a bit on the fence about:

3. Changes the COLLECTIONS_CHANGED event to be less "smart", and not emit a "collection changed" event when an entity in the collection is deleted. This is a change to the event semantics.

---

Background for the third change:

I tried to make the COLLECTIONS_CHANGED event "smart" so that it would reflect any user-visible changes to collections. Specifically, if you soft-deleted an entity that was in a collection, you'd get a COLLECTIONS_CHANGED event with entities_removed=[...] telling you that that entity is no longer in the collection. And if you then un-deleted that entity, you'd get a COLLECTIONS_CHANGED event with entities_added=[...] showing that the entity was "re-added" to the collection when it was un-deleted.

BUT this was fundamentally problematic because collections very deliberately are not part of the draft-publish workflow, and they technically shouldn't care about the "draft" state of an entity in particular. When testing the modulestore migrator in openedx-platform, I noticed that the related code was emitting redundant COLLECTIONS_CHANGED events, because of this mismatch.

The problem was with code like this:

        # Should emit 2 events ("collection created", "entities added to collection") but emits 3 (created, added, added again):
        with api.bulk_draft_changes_for(lp1.id, changed_by=None, changed_at=now_time):
            col1 = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None)
            entity = _create_entity_with_version(lp1.id, "entity1")
            api.add_to_collection(lp1.id, "col1", PublishableEntity.objects.filter(id=entity.id))

When the bulk draft context ends, the emit_collections_changed_for_entity_changes_task would look for "un-deleted" entities that are part of a collection, and emit "added to collection" events for them, but if you created them in this way, there would be duplicate events - one queued by add_to_collection, and one queued when the bulk draft changes context ends.

Now, it was possible to work around this using 7903aec to distinguish between "un-deleted" and "created" entities, and this mostly solved the bug, but there was still at least one major edge case:

        # Delete existing entity:
        api.soft_delete_draft(entity.id, deleted_by=None)

        # Should emit 2 events ("collection created", "entities added to collection") but emits 3 (created, added, added again):
        with api.bulk_draft_changes_for(lp1.id, changed_by=None, changed_at=now_time):
            api.set_draft_version(entity.id, v1.id)  # Un-delete the entity
            col1 = api.create_collection(lp1.id, "col1", title="Collection 1", created_by=None)
            api.add_to_collection(lp1.id, "col1", PublishableEntity.objects.filter(id=entity.id))

In this case, the handler that runs when the bulk context ends ("look for un-deleted entities and emit 'added to collection' events for any collections they're part of") has no way to "know" that the add_to_collection API already emitted events for adding those entities to the collection.

As far as I can tell, there is no clean way to update our code to avoid duplicate events in this case, while still emitting these "smart" events that reflect the draft state of the entities in the collection. Now, this isn't a huge deal as the duplicate events are only in a very particular edge case involving "un-deleting" entities, but the fact that we cannot really avoid this without some ugly hacks tells me that the event design needs to be improved. Which is why I'm suggesting this particular improvement now, because it's technically a breaking change to the semantics of the brand-new COLLECTION_CHANGED event, and I want to "get it right" in our first release if possible, to avoid difficult changes later.

What this PR changes: COLLECTION_CHANGED is no longer emitted when entities are soft-deleted or un-deleted, potentially affecting the contents of a collection. Instead, code that cares about this needs to also listen for ENTITIES_DRAFT_CHANGED events indicating drafts are deleted or un-deleted.

[openedx-webhooks]

Thanks for the pull request, @bradenmacdonald!

This repository is currently maintained by @axim-engineering.

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 approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

---

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:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

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:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[bradenmacdonald]

@ormsbee @kdmccormick Could I get your opinion here?

When we delete a [draft] component or container, do you think openedx-core should:

(A) send both the regular ENTITIES_DRAFT_CHANGED event and a COLLECTION_UPDATED event, indicating that the entity has been removed from the collection? (Because the draft entity has been deleted).

• This makes the platform-side implementation of the events much simpler and generally more correct, but
• This can result in duplicate "added to collection" events in a very specific edge case involving un-deleting entities and assigning them to a collection inside a bulk draft operation. (See above - I don't think this is a big deal though)
• This more or less assumes/formalizes that collections are collections of draft entities, and we care specifically about the draft being deleted. This matches how we want the libraries authoring UI to work. I'm not sure if it was the original intention of "collections" or not - the code explicitly says collections themselves aren't versioned but it's unclear if they are meant to hold entities regardless of published/draft/deleted status, or generally hold draft entities.

or

(B) send only the regular ENTITIES_DRAFT_CHANGED event indicating the component/container has been deleted.

• This requires more work on the platform side, because everything that cares about "draft entities in collections", e.g. library and search code, has to listen for both the ENTITIES_DRAFT_CHANGED and COLLECTION_UPDATED events in almost all cases.
• This avoids the edge case with un-deleting inside a bulk change - no extra duplicate event gets emitted.
• This formalizes that collections are strictly sets of PublishableEntities which may or may not have draft or published versions.

The docs at https://github.com/openedx/openedx-core/blob/main/src/openedx_content/applets/collections/models.py don't say either way. Though collections could have been implemented as foreignkeys to the Draft model instead of PublishableEntity if that's what we had wanted.

[ormsbee]

I would say (B). My concern with (A) is precisely what you lay out:

This more or less assumes/formalizes that collections are collections of draft entities, and we care specifically about the draft being deleted. This matches how we want the libraries authoring UI to work. I'm not sure if it was the original intention of "collections" or not - the code explicitly says collections themselves aren't versioned but it's unclear if they are meant to hold entities regardless of published/draft/deleted status, or generally hold draft entities.

Collections are also used as a way for people authoring Courses to find library content to add to them. When they are doing so, they need to be viewing the published state of those items. Something with a deleted draft (where that deletion has not yet been published) should still show up in that Collection when it's being browsed from the "add to course" UI.

[bradenmacdonald]

Something with a deleted draft (where that deletion has not yet been published) should still show up in that Collection when it's being browsed from the "add to course" UI.

I'm not sure this is working well at the moment. Some of the search index code conflates "draft doesn't exist" with "entity doesn't exist". But that's just a bug we can fix in the future, and I agree in principle, so I'll move forward with (B).