search

openedx / platform-plugin-aspects

Aspects plugins for the Open edX LMS and Studio
Apache License 2.0
2 stars 8 forks source link

fix: wait for transaction commit before trying to sink models #90

Closed Ian2012 closed 1 month ago

Ian2012 commented 2 months ago

Closes: https://github.com/openedx/platform-plugin-aspects/pull/90 Merge checklist: Check off if complete or not applicable:

openedx-webhooks commented 2 months ago

Thanks for the pull request, @Ian2012!

What's next?

Please work through the following steps to get your changes ready for engineering review:

:radio_button: Get product approval

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

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

:radio_button: 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.

:radio_button: Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @bmtcril. Tag them in a comment and let them know that your changes are ready for review.

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:

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

github-actions[bot] commented 2 months ago

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)		Lines missing
  platform_plugin_aspects
  __init__.py
  signals.py 77-93, 191, 229, 267
Project Total  

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

bmtcril commented 1 month ago

I've been trying to test this but have only been able to trigger the course related sinks, not the user profile / external_id sinks or the taxonomy / tag / object_tag. Any ideas what I may be missing? Config looks like:

EVENT_SINK_CLICKHOUSE_PII_MODELS = ['user_profile', 'external_id', 'auth_user']
...
EVENT_SINK_CLICKHOUSE_COURSE_OVERVIEWS_ENABLED = True
EVENT_SINK_CLICKHOUSE_TAG_ENABLED = True
EVENT_SINK_CLICKHOUSE_TAXONOMY_ENABLED = True
EVENT_SINK_CLICKHOUSE_OBJECT_TAG_ENABLED = True
EVENT_SINK_CLICKHOUSE_COURSE_ENROLLMENT_ENABLED = True
# User PII models
EVENT_SINK_CLICKHOUSE_USER_PROFILE_ENABLED = True
EVENT_SINK_CLICKHOUSE_EXTERNAL_ID_ENABLED = True
EVENT_SINK_CLICKHOUSE_AUTH_USER_ENABLED = True

Nothing shows in the worker logs when these get triggered. The known Celery tasks for platform-plugin-aspects only show:

  . platform_plugin_aspects.tasks.dump_course_to_clickhouse
  . platform_plugin_aspects.tasks.dump_data_to_clickhouse
Ian2012 commented 1 month ago

Yeah, it's assuming that the setting: EVENT_SINK_CLICKHOUSE_PII_MODELS contains a list of tables to delete records where the column user_id is present. We need to create a new setting that keeps track of those tables (external_id, user_profile) and any new one. Not related

bmtcril commented 1 month ago

This one should be ready to go if you want to bump the version