-
-
Notifications
You must be signed in to change notification settings - Fork 24
Resolve #160 -- Add picture_processed signal
#231
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
81c6163
2621125
dbc80b3
1d6b82e
6cd32cd
cafec7e
67a476b
ed11b97
4077289
fd61329
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import django.dispatch | ||
|
|
||
| process_picture_done = django.dispatch.Signal() | ||
jmsmkn marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,10 +2,11 @@ | |||||
|
|
||||||
| from typing import Protocol | ||||||
|
|
||||||
| from django.apps import apps | ||||||
| from django.db import transaction | ||||||
| from PIL import Image | ||||||
|
|
||||||
| from pictures import conf, utils | ||||||
| from pictures import conf, signals, utils | ||||||
|
|
||||||
|
|
||||||
| def noop(*args, **kwargs) -> None: | ||||||
|
|
@@ -19,6 +20,7 @@ def __call__( | |||||
| file_name: str, | ||||||
| new: list[tuple[str, list, dict]] | None = None, | ||||||
| old: list[tuple[str, list, dict]] | None = None, | ||||||
| field: str = "", | ||||||
|
||||||
| field: str = "", | |
| sender: tuple[str, str, str], |
You can drop the default, since this will be required. And I'd prefer to keep the naming somewhat consistent. Thus, this would be the sender (sending the task).
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.
Making it required means that it needs to be placed before new and old, which is more of a breaking change than making it optional. It does make upgrading more difficult as existing celery tasks without the kwarg being set could be on users queues, so they would end up being rejected.
Now that we're sending along the field as the sender the storage could also be dropped as that could be found in the task from the field, but that would cause issues on upgrading again due to it being set as a kwarg to the celery tasks.
Having sender as optional would allow a more graceful upgrade path, they could then be made required in a further major release. Same with making storage optional for now before removal in a further major release.
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.
Hm… but we'd always include a sender in the function call. Thus, custom processors would immediately break, right?
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.
In the codebase process_picture is called with args, not kwargs, so the calls would break with TypeError: process_picture() takes 4 positional arguments but 5 were given and the tasks would not be scheduled. That would be caught in any CI test that assigns an image to a picture field.
Our problem would be on a higher level and only occur when deploying a new version. The tasks from the older version of Django pictures would have already been created with 4 kwargs (storage, filename, new (optional), old (optional)) and are sat on the message broker. With an active site it is not feasible to ensure that none of the old tasks exist on the queue when upgrading. You then deploy a new version and the workers now only accept 5 kwargs (storage, filename, sender, new (optional), old (optional)). There, Celery will reject the message as sender is not set, raise an exception and put it back on the queue as a new message.
Maybe we have Celery misconfigured in some way but the worker will then pick the message up again, and repeat. We end up flooding Sentry, and the only way around it from that point is to create our own celery task that can handle both types of message on the queue.
jmsmkn marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
jmsmkn marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| from unittest.mock import Mock | ||
|
|
||
| import pytest | ||
| from django.apps import apps | ||
| from django.dispatch import receiver | ||
|
|
||
| from pictures import signals, tasks | ||
| from tests.testapp.models import SimpleModel | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_process_picture_sends_process_picture_done(image_upload_file): | ||
jmsmkn marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| obj = SimpleModel.objects.create(picture=image_upload_file) | ||
|
|
||
| handler = Mock() | ||
| signals.process_picture_done.connect(handler) | ||
|
|
||
| tasks._process_picture( | ||
| obj.picture.storage.deconstruct(), | ||
| obj.picture.name, | ||
| new=[i.deconstruct() for i in obj.picture.get_picture_files_list()], | ||
| ) | ||
|
|
||
| handler.assert_called_once_with( | ||
| signal=signals.process_picture_done, | ||
| sender=tasks._process_picture, | ||
| storage=obj.picture.storage.deconstruct(), | ||
| file_name=obj.picture.name, | ||
| new=[i.deconstruct() for i in obj.picture.get_picture_files_list()], | ||
| old=[], | ||
| field="", | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_process_picture_sends_process_picture_done_on_create(image_upload_file): | ||
jmsmkn marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| handler = Mock() | ||
| signals.process_picture_done.connect(handler) | ||
|
|
||
| obj = SimpleModel.objects.create(picture=image_upload_file) | ||
|
|
||
| handler.assert_called_once_with( | ||
| signal=signals.process_picture_done, | ||
| sender=SimpleModel, | ||
| storage=obj.picture.storage.deconstruct(), | ||
| file_name=obj.picture.name, | ||
| new=[i.deconstruct() for i in obj.picture.get_picture_files_list()], | ||
| old=[], | ||
| field="testapp.simplemodel.picture", | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_processed_object_found(image_upload_file): | ||
| obj = SimpleModel.objects.create() | ||
|
|
||
| found_object = None | ||
|
|
||
| @receiver(signals.process_picture_done, sender=SimpleModel) | ||
| def handler(*, file_name, field, **__): | ||
| nonlocal found_object | ||
| app_label, model_name, field_name = field.split(".") | ||
| model = apps.get_model(app_label=app_label, model_name=model_name) | ||
|
|
||
| # Users can now modify the object that process_picture_done | ||
| # corresponds to | ||
| found_object = model.objects.get(**{field_name: file_name}) | ||
|
|
||
| obj.picture.save("image.png", image_upload_file) | ||
|
|
||
| assert obj == found_object | ||
Uh oh!
There was an error while loading. Please reload this page.