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.

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

Feature/minimal variant price sorting #4416

Merged
merged 31 commits into from
Aug 21, 2019

Conversation

derenio
Copy link
Contributor

@derenio derenio commented Jul 8, 2019

This PR:

  • adds minimal_variant_price to the Product model,
    • a field that stores the minimal price among all the variants of the product,
  • updates the minimal_variant_price on:
    • product's creation/update,
    • product variant's creation/update/removal,
    • sale's creation/update.

@NyanKiyoshi
Copy link
Member

NyanKiyoshi commented Jul 9, 2019

Did you notice that pytest is printing 100 time:

.tox/py36-django22/lib/python3.6/site-packages/django/db/models/fields/init.py:1363

I believe it's a warning, but I don't see any message. I checked, the travis build on master doesn't do that. Could you take a look?

saleor/discount/models.py Outdated Show resolved Hide resolved
saleor/discount/models.py Outdated Show resolved Hide resolved
saleor/discount/utils.py Outdated Show resolved Hide resolved
saleor/product/tasks.py Show resolved Hide resolved
saleor/product/utils/variant_prices.py Show resolved Hide resolved
saleor/product/utils/variant_prices.py Outdated Show resolved Hide resolved
tests/api/test_product.py Show resolved Hide resolved
tests/test_product_minimal_variant_price.py Outdated Show resolved Hide resolved
Copy link

django-queries commented Jul 16, 2019

Here is the report for d7eded6 (mirumee/saleor @ feature/minimal_variant_price_sorting)
Base comparison is 246c788.

No differences were found. (click me)

# api.benchmark checkout
  test name                           	left count 	right count	duplicate count
  ------------------------------------	-----------	-----------	---------------
  add billing address to checkout     	         34	         34	             20
  add shipping to checkout            	          7	          7	              0
  checkout payment charge             	         14	         14	              0
  complete checkout                   	          6	          6	              0
  create checkout                     	         48	         48	             24

# api.benchmark homepage
  test name                           	left count 	right count	duplicate count
  ------------------------------------	-----------	-----------	---------------
  retrieve main menu                  	          5	          5	              0
  retrieve product list               	          4	          4	              0
  retrieve secondary menu             	          5	          5	              0
  retrieve shop                       	          2	          2	              0

# api.benchmark product
  test name                           	left count 	right count	duplicate count
  ------------------------------------	-----------	-----------	---------------
  product details                     	         13	         13	              3

# api.benchmark variant
  test name                           	left count 	right count	duplicate count
  ------------------------------------	-----------	-----------	---------------
  retrieve variant list               	         15	         15	              8

@derenio derenio force-pushed the feature/minimal_variant_price_sorting branch from f2d0ec2 to e694777 Compare July 16, 2019 14:33
@codecov
Copy link

codecov bot commented Jul 16, 2019

Codecov Report

Merging #4416 into master will increase coverage by <.01%.
The diff coverage is 96.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4416      +/-   ##
=========================================
+ Coverage   91.39%   91.4%   +<.01%     
=========================================
  Files         308     308              
  Lines       18261   18326      +65     
  Branches     1814    1835      +21     
=========================================
+ Hits        16690   16751      +61     
  Misses       1058    1058              
- Partials      513     517       +4
Impacted Files Coverage Δ
saleor/graphql/product/schema.py 100% <ø> (ø) ⬆️
saleor/graphql/product/resolvers.py 85% <100%> (-0.15%) ⬇️
saleor/discount/utils.py 100% <100%> (+3.15%) ⬆️
saleor/dashboard/product/forms.py 91.8% <100%> (+0.08%) ⬆️
saleor/graphql/product/mutations/products.py 96.64% <100%> (+0.05%) ⬆️
saleor/graphql/product/enums.py 92.85% <100%> (+0.26%) ⬆️
saleor/graphql/product/filters.py 91.77% <100%> (-0.01%) ⬇️
saleor/graphql/discount/mutations.py 98.3% <100%> (+0.09%) ⬆️
saleor/graphql/product/types/products.py 91.9% <100%> (+0.01%) ⬆️
saleor/dashboard/product/views.py 93.4% <100%> (+0.03%) ⬆️
... and 27 more

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 246c788...d7eded6. Read the comment docs.

saleor/product/tasks.py Show resolved Hide resolved
saleor/product/tasks.py Outdated Show resolved Hide resolved
@koradon
Copy link
Contributor

koradon commented Aug 2, 2019

@derenio please rebase this PR to master

@derenio derenio force-pushed the feature/minimal_variant_price_sorting branch from 6c5c018 to 03171cb Compare August 2, 2019 16:07
@koradon
Copy link
Contributor

koradon commented Aug 5, 2019

@derenio please fix CI

saleor/graphql/discount/mutations.py Outdated Show resolved Hide resolved
saleor/product/utils/variant_prices.py Outdated Show resolved Hide resolved
saleor/product/migrations/0102_merge_20190802_1110.py Outdated Show resolved Hide resolved
saleor/product/tasks.py Show resolved Hide resolved
tests/api/test_product_minimal_variant_price.py Outdated Show resolved Hide resolved
@derenio derenio force-pushed the feature/minimal_variant_price_sorting branch from 343de14 to 273734a Compare August 8, 2019 13:02
@maarcingebala
Copy link
Member

@derenio Please rebase this branch with master as we've merged a large PR yesterday. Then I'll be able to re-review it.

@derenio derenio force-pushed the feature/minimal_variant_price_sorting branch from 8a8fdf1 to 566c0dc Compare August 8, 2019 13:55
@derenio
Copy link
Contributor Author

derenio commented Aug 8, 2019

@maarcingebala just rebased the PR.

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.

In API we also have bulk mutations that would affect minimal prices, such as:

  • saleBulkDelete
  • categoryBulkDelete
  • collectionBulkDelete
  • collectionBulkPublish
  • productVariantBulkDelete

But I guess we could add support for calculating minimal prices in bulk actions in a separate PR as well.

saleor/graphql/product/schema.py Outdated Show resolved Hide resolved
@maarcingebala
Copy link
Member

How do we handle published vs. unpublished collections in terms of minimal prices? E.g., if there is a sale for a particular collection, but the collection isn't published yet. Can we even create a sale for unpublished collections? I'm not sure how it works now but I'm writing that down for us not to forget to check it.

@koradon
Copy link
Contributor

koradon commented Aug 13, 2019

Publish/unpublish and calculating of minimal price are for me two separate things.
We should be able to publish/unpublish every variant we want and calculate the minimal price if there is any price to calculate.

@maarcingebala
Copy link
Member

@koradon I think it is related, but the problem is more general. Consider this case:

  1. I have a collection with a bunch of products. The collection is not published.
  2. I'm creating a sale and associating it with the above collection.
    Now - is the discount active, if the collection is not published yet? Currently, it seems that it is which I consider a bug. But in general - both discounts and collections can be activated/published at some point in the future, and minimal variant price should become valid from that point of time (or should be recalculated at that point).

Maybe the simplest solution is to have a periodic task that recalculates all prices based on what is currently published in the store.

@koradon
Copy link
Contributor

koradon commented Aug 14, 2019

@maarcingebala yes, we ware talking about designing periodic task for recalculating prices. That was the main save behavior that will fix any mistakes in our logic.
According to your example:
We should calculate min_price every time when Sale or Collection changes state.

@derenio derenio force-pushed the feature/minimal_variant_price_sorting branch from 25f51a9 to 9259837 Compare August 21, 2019 10:45
of:
- Product,
- ProductVariant,
- Sale, and
- Collection,

Categories don't require updating as they cascade delete products
on their removal. Also, products are already updated after they are
assigned a category on their creation.
categorie(s)_ids -> category_ids
it probably worked till now because:
	```sale.pk == collection.pk```
in the fixtures.
as its content was copy-pasted from the ProductCreate test
without the necessary changes
They are handled in the "ProductFilterInput"
@derenio derenio force-pushed the feature/minimal_variant_price_sorting branch from 6acdd04 to 521934c Compare August 21, 2019 12:33
@derenio
Copy link
Contributor Author

derenio commented Aug 21, 2019

@maarcingebala I've rebased (twice x|) onto the latest master to resolve conflicts on the Pipfile.lock and the migration files.

saleor/product/models.py Show resolved Hide resolved
@maarcingebala maarcingebala merged commit 816e589 into master Aug 21, 2019
@maarcingebala maarcingebala deleted the feature/minimal_variant_price_sorting branch August 21, 2019 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants