-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: wait for transaction commit before trying to sink models #90
Conversation
Thanks for the pull request, @Ian2012! What's next?Please work through the following steps to get your changes 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. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by 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. |
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
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:
Nothing shows in the worker logs when these get triggered. The known Celery tasks for platform-plugin-aspects only show:
|
359a214
to
04f57c2
Compare
a44c39c
to
f5c185c
Compare
64736f9
to
6ee1813
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from user profile being broken, I've tested the following successfully:
- external id
- course enrollment
- course publish
- add / delete tags to objects
Things I haven't been able to test:
- user retirement
- taxonomy saved
platform_plugin_aspects/signals.py
Outdated
lambda: on_user_profile_updated(kwargs["instance"]) | ||
) # pragma: no cover | ||
|
||
def on_user_profile_updated(instance): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there is an issue here:
2024-10-01 15:11:04,575 ERROR 1 [django.request] [user 6] [ip 192.168.247.1] log.py:241 - Internal Server Error: /api/user/v1/accounts/bmtcril
Traceback (most recent call last):
File "/openedx/venv/lib/python3.11/site-packages/django/core/handlers/exception.py", line 55, in inner
response = get_response(request)
^^^^^^^^^^^^^^^^^^^^^
File "/openedx/venv/lib/python3.11/site-packages/django/core/handlers/base.py", line 197, in _get_response
response = wrapped_callback(request, *callback_args, **callback_kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/pyenv/versions/3.11.8/lib/python3.11/contextlib.py", line 80, in inner
with self._recreate_cm():
File "/openedx/venv/lib/python3.11/site-packages/django/db/transaction.py", line 307, in __exit__
connection.set_autocommit(True)
File "/openedx/venv/lib/python3.11/site-packages/django/db/backends/base/base.py", line 501, in set_autocommit
self.run_and_clear_commit_hooks()
File "/openedx/venv/lib/python3.11/site-packages/django/db/backends/base/base.py", line 779, in run_and_clear_commit_hooks
func()
File "/mnt/platform-plugin-aspects/platform_plugin_aspects/signals.py", line 93, in <lambda>
transaction.on_commit(lambda: on_user_profile_updated(**kwargs))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: on_user_profile_updated_txn.<locals>.on_user_profile_updated() got an unexpected keyword argument 'signal'
Changing it to def on_user_profile_updated(sender, instance, **kwargs):
seemed to fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. The taxonomy saved workflow can be tested from the django admin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not related to your changes, but the retirement sink fails now trying to delete from event_sink.auth_user
, which we never create. I think this is a bug in tutor-contrib-aspects, but just in case you can think of a reason why it might be related here I'm mentioning it.
Removing auth_user from the EVENT_SINK_PII_MODELS in tutor-contrib-aspects seemed to fix it for me, but I'm still not 100% sure it did the right thing.
Approving this on the assumption it's unrelated to this PR.
Yeah, it's assuming that the setting: |
This one should be ready to go if you want to bump the version |
Closes: #90
Merge checklist:
Check off if complete or not applicable: