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

Distinguish OrderLine product name and variant name #4702

Merged

Conversation

fowczarek
Copy link
Member

I want to merge this change because resolve #4624

Screenshots

Pull Request Checklist

  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. Database migration files are up to date.
  6. The changes are tested.
  7. GraphQL schema and type definitions are up to date.
  8. Changes are mentioned in the changelog.

@fowczarek fowczarek force-pushed the 4624/Distinguish_product_and_varinat_name_in_OrderLine branch from ab47cc3 to 054cf7f Compare August 30, 2019 08:49
Copy link

django-queries commented Aug 30, 2019

Here is the report for 53b09ef (mirumee/saleor @ 4624/Distinguish_product_and_varinat_name_in_OrderLine)
Base comparison is e28546e.

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

@codecov
Copy link

codecov bot commented Aug 30, 2019

Codecov Report

Merging #4702 into master will decrease coverage by 0.02%.
The diff coverage is 82.75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4702      +/-   ##
=========================================
- Coverage   91.62%   91.6%   -0.03%     
=========================================
  Files         307     308       +1     
  Lines       18412   18438      +26     
  Branches     1825    1827       +2     
=========================================
+ Hits        16870   16890      +20     
- Misses       1036    1040       +4     
- Partials      506     508       +2
Impacted Files Coverage Δ
saleor/core/analytics.py 85.29% <ø> (ø) ⬆️
saleor/product/models.py 93.31% <ø> (-0.7%) ⬇️
saleor/order/templatetags/order_lines.py 100% <100%> (ø)
saleor/order/models.py 90.15% <100%> (+0.07%) ⬆️
saleor/seo/schema/email.py 93.33% <33.33%> (-6.67%) ⬇️
saleor/graphql/order/types.py 90.9% <75%> (-0.48%) ⬇️
saleor/checkout/utils.py 84.02% <85.71%> (-0.04%) ⬇️
saleor/payment/utils.py 91.23% <0%> (+0.79%) ⬆️

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 e28546e...53b09ef. Read the comment docs.

@fowczarek fowczarek force-pushed the 4624/Distinguish_product_and_varinat_name_in_OrderLine branch from 054cf7f to ce4d3b9 Compare August 30, 2019 11:28
saleor/core/analytics.py Outdated Show resolved Hide resolved
@@ -360,7 +360,9 @@ class OrderLine(models.Model):
)
# max_length is as produced by ProductVariant's display_product method
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment obsolete?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think yes. This comment explains why product_name have max_length=386. We can't change max length because we can't get new product_name for order lines with removed products or variants.

templates/dashboard/order/pdf/packing_slip.html Outdated Show resolved Hide resolved
@@ -25,7 +25,7 @@ def test_get_order_payloads(order_with_lines):
for i, line in enumerate(order):
item = data[i + 1]
assert item["ti"] == order.pk
assert item["in"] == line.product_name
assert item["in"] == line.variant.display_product()
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be str(line.variant) here?

Copy link
Member

Choose a reason for hiding this comment

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

And what about rest of calls of .display_product()? As @maarcingebala said, Do we need it at all? Maybe we can remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. No, because str(line.variant) is variant.name or variant.sku and display_product is product.name (variant.name). We need both names in analytics.
  2. I think we shouldn't remove it because google merchant uses it.

saleor/graphql/order/types.py Outdated Show resolved Hide resolved
saleor/order/models.py Outdated Show resolved Hide resolved
saleor/order/templatetags/order_lines.py Outdated Show resolved Hide resolved
saleor/order/templatetags/order_lines.py Outdated Show resolved Hide resolved
@fowczarek fowczarek force-pushed the 4624/Distinguish_product_and_varinat_name_in_OrderLine branch from f559f8a to f50c217 Compare September 2, 2019 08:39
@fowczarek fowczarek force-pushed the 4624/Distinguish_product_and_varinat_name_in_OrderLine branch from f50c217 to 53b09ef Compare September 2, 2019 11:10
@maarcingebala maarcingebala merged commit 03eb427 into master Sep 2, 2019
@maarcingebala maarcingebala deleted the 4624/Distinguish_product_and_varinat_name_in_OrderLine branch September 2, 2019 11:23
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.

Distinguish OrderLine product name and variant name
5 participants