-
-
Notifications
You must be signed in to change notification settings - Fork 26
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 10 commits
81c6163
2621125
dbc80b3
1d6b82e
6cd32cd
cafec7e
67a476b
ed11b97
4077289
fd61329
1f7fdb9
591eb67
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 |
|---|---|---|
|
|
@@ -227,6 +227,38 @@ The default processor is `pictures.tasks.process_picture`. It takes a single | |
| argument, the `PictureFileFile` instance. You can use this to override the | ||
| processor, should you need to do some custom processing. | ||
|
|
||
| ### Signals | ||
|
|
||
| The async image processing emits a signal when the task is complete. | ||
| You can use that to store when the pictures have been processed, | ||
| so that a placeholder could be rendered in the meantime. | ||
|
|
||
| ```python | ||
| # models.py | ||
| from django.db import models | ||
| from pictures.models import PictureField | ||
|
|
||
|
|
||
| class Profile(models.Model): | ||
| title = models.CharField(max_length=255) | ||
| picture = PictureField(upload_to="avatars") | ||
| picture_processed = models.BooleanField(editable=False, null=True) | ||
|
codingjoe marked this conversation as resolved.
Outdated
|
||
|
|
||
|
|
||
| # signals.py | ||
| from django.dispatch import receiver | ||
| from pictures import signals | ||
|
|
||
| from .models import Profile | ||
|
codingjoe marked this conversation as resolved.
Outdated
|
||
|
|
||
|
|
||
| @receiver(signals.picture_processed, sender=Profile._meta.get_field("picture")) | ||
|
codingjoe marked this conversation as resolved.
Outdated
|
||
| def picture_processed_handler(*, sender, file_name, **__): | ||
|
codingjoe marked this conversation as resolved.
Outdated
|
||
| sender.model.objects.filter(**{sender.name: file_name}).update( | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm… we should probably pass the model Your filename doesn't need to be unique. It should, but it really doesn't have to. However, this would mean fetching it from the DB in the processing task. I'd love to avoid DB IO in the processing task by default. If you add a unique constraint and index, including a comment on why they matter, that might be the best solution. What do you think?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An index makes sense for the lookup here, but I'm not sure if a unique constraint is strictly necessary. To send along the instance we would need acceess to it to serialize it. Accessing the instance in https://github.com/jmsmkn/django-pictures/blob/4077289ae6a75c8f8a810d04487af4a95b4f1a45/pictures/models.py#L179-L185 doesn't work as Django cleanup assigns a fake instance to
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm… good point. Let's add the index. People will copy past this. The extra mile counts. I agree; injecting the instance will cause default DB IO. This should be up to the individual implementation. |
||
| picture_processed=True | ||
| ) | ||
| ``` | ||
|
|
||
| ### Validators | ||
|
|
||
| The library ships with validators to restrain image dimensions: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import django.dispatch | ||
|
|
||
| picture_processed = django.dispatch.Signal() | ||
|
codingjoe marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| from unittest.mock import Mock | ||
|
|
||
| import pytest | ||
| from django.dispatch import receiver | ||
|
|
||
| from pictures import signals, tasks | ||
| from tests.test_migrations import skip_dramatiq | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably be moved somewhere else. Otherwise, module-level fixtures will be unintentionally loaded too and introduce side effects.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, but I do not see how this is an issue. The module, and its potential fixtures, are already loaded when you run pytest. I do not see how to move this method either without duplicating the multiple if statements for the imports. |
||
| from tests.testapp.models import SimpleModel | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| @skip_dramatiq | ||
| def test_process_picture_sends_picture_processed(image_upload_file): | ||
| obj = SimpleModel.objects.create(picture=image_upload_file) | ||
|
|
||
| handler = Mock() | ||
| signals.picture_processed.connect(handler) | ||
|
|
||
| try: | ||
| tasks._process_picture( | ||
| obj.picture.storage.deconstruct(), | ||
| obj.picture.name, | ||
| obj.picture.sender, | ||
| new=[i.deconstruct() for i in obj.picture.get_picture_files_list()], | ||
| ) | ||
| finally: | ||
| signals.picture_processed.disconnect(handler) | ||
|
|
||
| handler.assert_called_once_with( | ||
| signal=signals.picture_processed, | ||
| sender=SimpleModel._meta.get_field("picture"), | ||
| file_name=obj.picture.name, | ||
| new=[i.deconstruct() for i in obj.picture.get_picture_files_list()], | ||
| old=[], | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| @skip_dramatiq | ||
| def test_process_picture_sends_picture_processed_on_create(image_upload_file): | ||
| handler = Mock() | ||
| signals.picture_processed.connect(handler) | ||
|
|
||
| try: | ||
| obj = SimpleModel.objects.create(picture=image_upload_file) | ||
| finally: | ||
| signals.picture_processed.disconnect(handler) | ||
|
|
||
| handler.assert_called_once_with( | ||
| signal=signals.picture_processed, | ||
| sender=SimpleModel._meta.get_field("picture"), | ||
| file_name=obj.picture.name, | ||
| new=[i.deconstruct() for i in obj.picture.get_picture_files_list()], | ||
| old=[], | ||
| ) | ||
|
|
||
|
|
||
|
jmsmkn marked this conversation as resolved.
jmsmkn marked this conversation as resolved.
|
||
| @pytest.mark.django_db | ||
| @skip_dramatiq | ||
| def test_processed_object_found(image_upload_file): | ||
| obj = SimpleModel.objects.create() | ||
|
|
||
| found_object = None | ||
|
|
||
| @receiver(signals.picture_processed, sender=SimpleModel._meta.get_field("picture")) | ||
| def handler(*, sender, file_name, **__): | ||
| nonlocal found_object | ||
|
|
||
| # Users can now modify the object that picture_processed corresponds to | ||
| found_object = sender.model.objects.get(**{sender.name: file_name}) | ||
|
|
||
| try: | ||
| obj.picture.save("image.png", image_upload_file) | ||
| finally: | ||
| signals.picture_processed.disconnect(handler) | ||
|
|
||
| assert obj == found_object | ||
Uh oh!
There was an error while loading. Please reload this page.