Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add versatile image field prewarmer #1947

Merged
merged 13 commits into from
Mar 27, 2018
Merged

Add versatile image field prewarmer #1947

merged 13 commits into from
Mar 27, 2018

Conversation

Pacu2
Copy link
Contributor

@Pacu2 Pacu2 commented Mar 20, 2018 •
edited by maarcingebala

This pull requests add versatile image field prewarmer for product images.
close #1436

Pull Request Checklist

(Please keep this section. It will make maintainer's life easier.)

  1. Privileged views and APIs are guarded by proper permission checks.
  2. All visible strings are translated with proper context.
  3. All data-formatting is locale-aware (dates, numbers, and so on).
  4. Database queries are optimized and the number of queries is constant.
  5. The changes are tested.
  6. The code is documented (docstrings, project documentation).
  7. Python code quality checks pass: pycodestyle, pydocstyle, pylint.
  8. JavaScript code quality checks pass: eslint.

@Pacu2 Pacu2 self-assigned this Mar 20, 2018
@codecov
Copy link

codecov bot commented Mar 20, 2018 •
edited

Codecov Report

Merging #1947 into master will decrease coverage by 0.01%.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1947      +/-   ##
==========================================
- Coverage   84.74%   84.73%   -0.02%     
==========================================
  Files         161      162       +1     
  Lines        6911     6942      +31     
  Branches      701      705       +4     
==========================================
+ Hits         5857     5882      +25     
- Misses        863      866       +3     
- Partials      191      194       +3
Impacted Files Coverage Δ
saleor/product/thumbnails.py 100% <100%> (ø)
saleor/product/models.py 94.5% <100%> (+0.02%) ⬆️
saleor/dashboard/product/forms.py 82.69% <66.66%> (-0.58%) ⬇️
saleor/core/utils/__init__.py 74.56% <66.66%> (-1.73%) ⬇️
saleor/dashboard/graphql/product/mutations.py 100% <0%> (ø) ⬆️
saleor/product/templatetags/product_images.py 100% <0%> (+6.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19a5e9a...1e741a0. Read the comment docs.

@Pacu2 Pacu2 force-pushed the feature/versatile-image-field-prewarmer branch from 530ad00 to 219c00c Compare March 20, 2018 10:39
@NyanKiyoshi
Copy link
Member

It would probably be useful to also create a command for manage.py to warm up any (pre)existing image.

@Pacu2
Copy link
Contributor Author

Pacu2 commented Mar 20, 2018

@NyanKiyoshi Thought of the same thing, it will be definitely added.

@Pacu2 Pacu2 force-pushed the feature/versatile-image-field-prewarmer branch from b9a3b51 to 4604649 Compare March 20, 2018 15:51
@Pacu2 Pacu2 removed the in progress label Mar 20, 2018
maarcingebala
maarcingebala approved these changes Mar 21, 2018
View reviewed changes
Copy link
Member

@maarcingebala maarcingebala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very good. We also have to add a section in the documentation explaining how this works and how to use it.

@Pacu2 Pacu2 force-pushed the feature/versatile-image-field-prewarmer branch from 6e7b4f0 to 283a720 Compare March 21, 2018 11:25
patrys
patrys reviewed Mar 21, 2018
View reviewed changes
saleor/product/models.py Outdated
@@ -392,6 +393,15 @@ def delete(self, *args, **kwargs):
super().delete(*args, **kwargs)


@receiver(models.signals.post_delete, sender=ProductImage)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way not to use signals to implement this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should cover all places where ProductImage or its parent can be deleted.
Eg. Category/Product/ProductImage delete actions
Plus something can be always deleted from the shell (edge case,but still, most likely during development phase)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we currently delete the main image? If so, does it also rely on signals? If not, do we want to remove thumbnails?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use signals at all, except for one that comes from django-payments if I'm correct. Also, we should care about cases when someone does changes using shell.

@Pacu2 Pacu2 force-pushed the feature/versatile-image-field-prewarmer branch from 1466233 to a8115c4 Compare March 24, 2018 10:33
@maarcingebala maarcingebala merged commit 232b24c into master Mar 27, 2018
@maarcingebala maarcingebala deleted the feature/versatile-image-field-prewarmer branch March 27, 2018 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrys patrys patrys left review comments

@maarcingebala maarcingebala maarcingebala approved these changes

@akjanik akjanik akjanik approved these changes

@mad-anne mad-anne Awaiting requested review from mad-anne

Assignees

@Pacu2 Pacu2

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Images generation performance issue
5 participants
@Pacu2 @NyanKiyoshi @patrys @maarcingebala @akjanik