fix: respect whether a sink is enabled when dumping data to ClickHouse

[ccantillo]

dump_data_to_clickhouse should respect whether a sink is enabled

Summary

The dump_data_to_clickhouse management command now checks Sink.is_enabled()
before dumping. If the sink for the requested --object is disabled it logs at
info level and dumps nothing, which is what the Celery task already does.

Why

The async path already gates on this, in platform_plugin_aspects/tasks.py:

Sink = getattr(import_module(sink_module), sink_name)

if Sink.is_enabled():
    sink = Sink(connection_overrides=connection_overrides, log=celery_log)
    sink.dump(object_id)
    return "Dumped"

return "Disabled"

The management command did not. handle() went straight from --object to
ModelBaseSink.get_sink_by_model_name(...) and dumped unconditionally, so
EVENT_SINK_CLICKHOUSE_<MODEL>_ENABLED was honoured for signal-driven syncs but
silently ignored for manual ones.

The practical consequence is on the PII models. tutor-contrib-aspects only
sets EVENT_SINK_CLICKHOUSE_USER_PROFILE_ENABLED / ..._EXTERNAL_ID_ENABLED
when ASPECTS_ENABLE_PII is true, so an operator who has deliberately opted out
of PII collection could still populate the user_profile and external_id
tables in ClickHouse by running the command by hand. Verified on a live
instance: with ASPECTS_ENABLE_PII=false, running
./manage.py lms dump_data_to_clickhouse --object user_profile logged
Dumped 4 objects to ClickHouse and added those rows to
event_sink.user_profile.

Where the enabled flag comes from

tutor-contrib-aspects renders two different settings into the LMS, and only
one of them carries the on/off state:

• EVENT_SINK_CLICKHOUSE_PII_MODELS is rendered unconditionally. Its only
  runtime use is UserRetirementSink (sinks/user_retire_sink.py:37), which
  reads it to delete PII rows when a user retires — it describes where PII
  lives, not whether it is being collected.
• EVENT_SINK_CLICKHOUSE_<MODEL>_ENABLED is rendered only when
  ASPECTS_ENABLE_PII is true. This is the one is_enabled() reads
  (sinks/base_sink.py:346), building the name at runtime from
  WAFFLE_FLAG_NAMESPACE and the sink's model.

So with ASPECTS_ENABLE_PII=false the per-model flag is simply absent and
getattr(..., False) reports the sink as disabled. This is also the mechanism
the README documents for toggling sinks, via that setting or the
event_sink_clickhouse.<model>.enabled waffle flag.

Note on the tests

The existing tests in this file drive a DummySink, which is_enabled() would
report as disabled — it has neither a model config entry nor an enabled setting.
Rather than weaken the new check to keep them green, the two existing test
functions are now decorated with an override_settings that registers dummy
in EVENT_SINK_CLICKHOUSE_MODEL_CONFIG and sets
EVENT_SINK_CLICKHOUSE_DUMMY_ENABLED = True, which is what those tests were
already assuming implicitly. A new test covers the disabled case, patching
WaffleFlag.is_enabled the same way sinks/tests/test_base_sink.py does.

Test plan

• pytest platform_plugin_aspects — 110 passed, including the new test.
• black --check and isort --check-only clean on both files.
• pylint produces no new warnings: the message set for the test file is
  byte-identical before and after this change (the remaining ones are
  pre-existing).

AI disclosure

Claude (Anthropic) was used to trace the difference between the task and command
code paths and to draft this change and its tests. I reviewed and understand it:
it is a single is_enabled() guard mirroring an existing pattern in this repo,
plus test setup that makes the dummy sink's enabled state explicit. I ran the
test suite and the quality checks myself, and confirmed the underlying problem on
a live instance before writing the fix.

[openedx-webhooks]

Thanks for the pull request, @ccantillo!

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.

[felipemontoya]

This approach is more elegant than the tutor-contrib-aspects pr. I like it. However I am unsure the message is correct.

From your description, the tutor command is tutor local do dump-data-to-clickhouse --options "--object user_profile"

Which would turn EVENT_SINK_CLICKHOUSE_{options['object'].upper()}_ENABLED into EVENT_SINK_CLICKHOUSE_USER_PROFILE_ENABLED. But the actual config value is passed as: EVENT_SINK_CLICKHOUSE_PII_MODELS.

Could you please double check and explain why your approach works.

[ccantillo]

This approach is more elegant than the tutor-contrib-aspects pr. I like it. However I am unsure the message is correct.

From your description, the tutor command is tutor local do dump-data-to-clickhouse --options "--object user_profile"

Which would turn EVENT_SINK_CLICKHOUSE_{options['object'].upper()}_ENABLED into EVENT_SINK_CLICKHOUSE_USER_PROFILE_ENABLED. But the actual config value is passed as: EVENT_SINK_CLICKHOUSE_PII_MODELS.

Could you please double check and explain why your approach works.

Thanks for the review @felipemontoya.
turns out both settings are in play

Tutor renders two different things into the LMS settings (openedx-common-settings):

# line 20 — always, no condition
EVENT_SINK_CLICKHOUSE_PII_MODELS = {{ EVENT_SINK_PII_MODELS }}

# lines 76-78 — only when ASPECTS_ENABLE_PII is true
{% if ASPECTS_ENABLE_PII %}# User PII models
{% for model in EVENT_SINK_PII_MODELS %}EVENT_SINK_CLICKHOUSE_{{model.upper()}}_ENABLED = True
{% endfor %}{% endif %}

The one you spotted (PII_MODELS) gets sent either way, so it isn't what carries
the on/off state. Its only runtime use is UserRetirementSink
(sinks/user_retire_sink.py:37), which reads it to DELETE the PII rows when a
user retires — it says where PII lives, not whether we're collecting it.

The per-model flag is the one with the state, and that's what is_enabled()
reads (sinks/base_sink.py:346). The name is built at runtime:

getattr(settings, f"{WAFFLE_FLAG_NAMESPACE.upper()}_{cls.model.upper()}_ENABLED", False)
#                  "EVENT_SINK_CLICKHOUSE"  +  "USER_PROFILE"  +  "_ENABLED"

So with ASPECTS_ENABLE_PII=false that flag just isn't rendered → getattr
falls back to False → sink disabled.

I checked on a running instance rather than just reading the code. Before:

$ ./manage.py lms dump_data_to_clickhouse --object user_profile
INFO ... Dumped 4 objects to ClickHouse     # PII written with the flag off

After:

$ ./manage.py lms dump_data_to_clickhouse --object user_profile
INFO ... Sink for user_profile is disabled, no data will be dumped.

[felipemontoya]

Thanks for the updates to the PR @ccantillo. I think this is ready for your review @saraburns1

[felipemontoya]

I meant to rebase, but clicked on merge main instead. I had to actually pull/rebase/force to get it in line.