[felipemontoya]

What the PR adds

Two model sinks for the openedx-core Pathways models (openedx_catalog, openedx-core ≥ 1.14), built like the existing tag and external-id sinks: a post_save handler queues dump_data_to_clickhouse on commit, and the existing backfill command works for both (--object catalog_pathway, --object pathway_enrollment).

Requirements:

• Send post_save when unenrolling from a Catalog Pathway openedx-core#918
• After this is merged, we also need to merge Add catalog_pathway and pathway_enrollment event sink tables tutor-contrib-aspects#1384

Tests

• Unit tests get_queryset of both sinks, both signal handlers deferring to on_commit, the serializer's actor_id, and get_xapi_actor_id (happy path and missing xapi type).
• pylint, pycodestyle, pydocstyle, isort and black are clean on the changed files.

Checked by hand on a Tutor main dev stack (edx-platform master, openedx-core main, ERB 10.1.0, PII off), with the ompanion tables applied.

• Creating and editing a Catalog Pathway each wrote a row; --force backfill wrote the pre-existing ones.
• Enrolling and re-enrolling wrote rows. The learner had no xapi ExternalId beforehand; the dump created it, and the row's actor_id matches it. For an existing learner the id equals ERB's get_anonymous_user_id(user_id, "XAPI").
• Backfill of enrollments worked, including a learner without an ExternalId.

Pending:

• decide the fate of the on_pathway_model_deleted handler

---

Merge checklist:
Check off if complete or not applicable:

• [x ] Commit messages (and PR title, if squash merging) use the correct
  Conventional Commits type — they determine the
  release: fix: → patch, feat: → minor, ! / BREAKING CHANGE: → major
• Documentation updated (not only docstrings)
• Fixup commits are squashed away
• Unit tests added/updated
• Manual testing instructions provided
• Noted any: Concerns, dependencies, migration issues, deadlines, tickets

[openedx-webhooks]

Thanks for the pull request, @felipemontoya!

This repository is currently maintained by @openedx/committers-analytics.

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.

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
src/platform_plugin_aspects
signals.py
utils.py
src/platform_plugin_aspects/sinks
__init__.py
catalog_pathway_sink.py
pathway_enrollment_sink.py
serializers.py					214
Project Total

This report was generated by python-coverage-comment-action

[felipemontoya]

@bmtcril I believe this is the major open point I have at this moment. We don't have things that delete their clickhouse counterpart yet in the codebase. The only Sink that issues a delete command is the UserRetirement, but that is for PII reasons.

I see three options:

1. Drop the handler. Consistent with the existing sinks. A deleted pathway stays in the ClickHouse catalog_pathway table forever and keeps showing up in reports. I think this is not ideal for data consistency.
2. Delete the row in ClickHouse (lightweight DELETE FROM … WHERE id = …, queued on commit). Needs only the id and mirrors what UserRetirementSink already does. ClickHouse deletes are relatively expensive, which is acceptable given how rarely pathways are deleted. There is produect decision on wheter we want the reports to show that a catalog pathway existed and then was deleted because we will still keep the item fulfillment in clickhouse.
3. Tombstone column (is_deleted) set by the delete handler. Keeps a "deleted at" history. The row is gone from MySQL, so the handler must serialize the in-memory instance rather than queue a dump by id, which breaks the current sink pattern. It also needs a new column in the tutor-contrib-aspects schema and ReplacingMergeTree(ver, is_deleted).

[bmtcril]

I think we want option 3 for this. It could have a separate sink for the tombstone path than the model save.

[felipemontoya]

So the pattern would be a separate tombstone sink class that writes to the same catalog_pathway table, and is_deleted has to be added to the table in the tutor-contrib-aspects alembic migration. Correct?

[felipemontoya]

@bmtcril I added the tombstone field for CatalogPathways sink here and the fields in the alembic PR

[bmtcril]

As mentioned on the other PR, I have some unvalidated AI feedback. I can dig into it more Monday or if you get to it sooner that's fine too. Here's what I've got:

Should fix:

1. Pathway enrollments aren't marked deleted. Only CatalogPathway has a post_delete handler. When a pathway is deleted, its PathwayEnrollment rows cascade-delete in MySQL but stay active in ClickHouse. The PR's own "Pending: fate of the delete handler" item is unresolved, so decide this before merge.
2. Backups write to MySQL. PathwayEnrollmentSerializer.get_actor_id creates an ExternalId for each learner who lacks one. Each creation fires post_save, which queues an ExternalIdSink Celery task. A --force backup of 50k enrollments could queue up to 50k extra tasks. Either note this in the docs or skip the signal during bulk backups.
3. The two new tables are built differently.
   • catalog_pathway: DateTime64 version column, ReplacingMergeTree(time_last_dumped, is_deleted), ORDER BY id
   • pathway_enrollment: String time_last_dumped, ordered by (id, time_last_dumped). Because the dump time is in the sort key, every dump adds a new row and older versions are never collapsed. This matches the legacy sink tables, but it's a different choice within one PR, and it changes how downstream queries must dedupe.

Fine as-is:

• The on_commit signal handlers follow the existing tag/external-id pattern.
• The tombstone uses int(item_id), so there's no injection risk, and it reads the latest version through a subquery, which is correct.
• get_model returns None without openedx-core ≥ 1.14, which degrades the same way the tagging sinks do.

How the fake data compares to the real data (xapi-db-load #286)

• ✅ Column order and types match. org uses OrgN, the same value as the fake courses, so joins work.
• ✅ ClickHouse accepts the string time_last_dumped (clickhouse-connect runs fromisoformat on it), and the chdb/S3 load runs with best_effort. I checked both, so they're no longer a concern.
• ⚠️ created/modified formats differ. The fake rows write str(naive_datetime), e.g. 2026-01-01 12:34:56. The real sink writes DRF ISO 8601, likely 2026-01-01T12:34:56.123456Z (depends on LMS settings). Both are String columns, so any dbt model that parses them is tested against the wrong format. Fix: created.replace(tzinfo=UTC).isoformat().
• ❌ No fake pathway_enrollment rows. This is the table with the most reporting value: it joins pathways to xAPI learners through actor_id. xapi-db-load already has self.actors with xAPI ids, so generating enrollments is about 30–60 minutes of work and gives dashboards real data to test against.
• Still open from my last review: the default num_catalog_pathways: 6 breaks older Aspects installs.

[felipemontoya]

1. Pathway enrollments aren't marked deleted. Only CatalogPathway has a post_delete handler. When a pathway is deleted, its PathwayEnrollment rows cascade-delete in MySQL but stay active in ClickHouse. The PR's own "Pending: fate of the delete handler" item is unresolved, so decide this before merge.

Deleting a pathway doesn't cascade. In openedx-core, PathwayEnrollment.catalog_pathway is on_delete=PROTECT, put there on purpose so deleting the wrong pathway can't wipe out enrollment history. delete_catalog_pathway() documents that it raises ProtectedError while anyone is or ever was enrolled.

To check it, I ran this in the LMS shell on my env: pick an existing enrollment, then call .delete() on its pathway inside a transaction.atomic() that rolls back. It raised ProtectedError: Cannot delete some instances of model 'CatalogPathway' because they are referenced through protected foreign keys: 'PathwayEnrollment.catalog_pathway'. Unenrolling doesn't delete the row either. It sets is_active=False and sends post_save (that's the openedx-core fix), so the sink already picks it up. The only hard deletes left are a user deletion (the user FK is CASCADE) and someone deleting an enrollment by hand in the admin. Both are rare, since retirement doesn't delete auth_user.
The "Pending" item is stale. We settled it in the thread on signals.py (option 3, tombstone). I ticked the box in the description which was missing.

2. Backups write to MySQL. PathwayEnrollmentSerializer.get_actor_id creates an ExternalId for each learner who lacks one. Each creation fires post_save, which queues an ExternalIdSink Celery task. A --force backup of 50k enrollments could queue up to 50k extra tasks. Either note this in the docs or skip the signal during bulk backups.

This one is true. For each row, get_actor_id goes through ExternalId.add_new_user_id, which does a get_or_create. Each new ExternalId fires post_save, and the existing handler queues an ExternalIdSink task. I followed this path in the code. I didn't reproduce it at 50k scale.

Some context on the size of it: tasks are only queued for learners who don't have an xapi ExternalId yet. event-routing-backends makes the same add_new_user_id call the first time a learner emits an xAPI event, so anyone with any activity already has one. With PII off, external_id isn't enabled, so each queued task returns "Disabled" right away. The cost is the queue churn. One more thing the report missed: even when nothing is created, it's 2 extra MySQL queries per enrollment.

I didn't document any of this because I thought it is a detail of implementation that it's very rare to hit real actual users and I didn't want to overdocument stuff. I have this issue with my coding assistant that it wants to write comments for every line and I tent towards only writing really important one liners. If you think it's worth the annotation, I can update the readme.

3. The two new tables are built differently.
   
   * catalog_pathway: DateTime64 version column, ReplacingMergeTree(time_last_dumped, is_deleted), ORDER BY id
   * pathway_enrollment: String time_last_dumped, ordered by (id, time_last_dumped). Because the dump time is in the sort key, every dump adds a new row and older versions are never collapsed. This matches the legacy sink tables, but it's a different choice within one PR, and it changes how downstream queries must dedupe.

True, and it's on purpose, but I didn't document why (similar to the prev comment). catalog_pathway needs the tombstone, and ReplacingMergeTree(ver, is_deleted) only collapses versions when the sort key is just id and there's a real version column, hence the DateTime64. pathway_enrollment has no delete path (unenroll is a save with is_active=False), so I kept the same layout as every other sink table (String time_last_dumped, ORDER BY (id, time_last_dumped)). I checked migrations 0007, 0016/0027, 0026, 0036 and 0039, and they all use that layout.

Downstream it gets deduped like the others, with ROW_NUMBER() OVER (PARTITION BY id ORDER BY time_last_dumped DESC). If you'd rather have both on the replacing layout for consistency, I can switch pathway_enrollment too. As with the last one if you think this is really key to document I can update the docstrings in the migration.

---

About the xapi-db-load I fixed the datetime format to match what comes out of a real pathway sink. Also added the code to load pathway enrollments. The update is in the same PR.

[bmtcril]

This is released as v2.2.0