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

Migrate to django-prices 2.1 #4639

Merged
merged 4 commits into from
Aug 26, 2019

Conversation

NyanKiyoshi
Copy link
Member

@NyanKiyoshi NyanKiyoshi commented Aug 12, 2019

Closes #4337, fixes #4636, fixes #4378.

TBD

Dashboard 1.0

  • Update product form
  • Update product variant form

GraphQL

  • Fix accidental refactor of inputs
  • Deprecate invalid naming in the graphql types

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.

@NyanKiyoshi NyanKiyoshi self-assigned this Aug 12, 2019
@NyanKiyoshi NyanKiyoshi changed the title Updated django-prices to 2.1 Migrate to django-prices to 2.1 Aug 12, 2019
@NyanKiyoshi NyanKiyoshi changed the title Migrate to django-prices to 2.1 Migrate to django-prices 2.1 Aug 12, 2019
Pipfile Outdated Show resolved Hide resolved
saleor/dashboard/widgets.py Outdated Show resolved Hide resolved
saleor/giftcard/migrations/0002_auto_20190814_0413.py Outdated Show resolved Hide resolved
saleor/order/models.py Outdated Show resolved Hide resolved
saleor/shipping/migrations/0017_auto_20190814_0413.py Outdated Show resolved Hide resolved
saleor/shipping/migrations/0017_auto_20190814_0413.py Outdated Show resolved Hide resolved
@NyanKiyoshi NyanKiyoshi force-pushed the django-prices/migrate/2.0 branch 2 times, most recently from da7c237 to bad3f91 Compare August 14, 2019 11:32
@NyanKiyoshi NyanKiyoshi marked this pull request as ready for review August 14, 2019 12:00
@NyanKiyoshi NyanKiyoshi force-pushed the django-prices/migrate/2.0 branch 2 times, most recently from e60e52e to bb87c0c Compare August 14, 2019 13:08
Copy link

django-queries commented Aug 14, 2019

Here is the report for d28c53c (NyanKiyoshi/saleor @ django-prices/migrate/2.0)
Base comparison is 1ac9086.

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

saleor/dashboard/product/forms.py Outdated Show resolved Hide resolved
saleor/discount/migrations/0017_auto_20190814_0413.py Outdated Show resolved Hide resolved
saleor/extensions/plugins/avatax/__init__.py Outdated Show resolved Hide resolved
saleor/extensions/plugins/avatax/plugin.py Outdated Show resolved Hide resolved
saleor/checkout/models.py Show resolved Hide resolved
saleor/graphql/discount/types.py Outdated Show resolved Hide resolved
saleor/graphql/schema.graphql Outdated Show resolved Hide resolved
saleor/order/utils.py Outdated Show resolved Hide resolved
saleor/product/migrations/0105_auto_20190814_0413.py Outdated Show resolved Hide resolved
saleor/shipping/migrations/0017_auto_20190814_0413.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 19, 2019

Codecov Report

Merging #4639 into master will decrease coverage by 0.04%.
The diff coverage is 91.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4639      +/-   ##
==========================================
- Coverage   91.37%   91.33%   -0.05%     
==========================================
  Files         308      308              
  Lines       18235    18339     +104     
  Branches     1814     1830      +16     
==========================================
+ Hits        16663    16750      +87     
- Misses       1058     1068      +10     
- Partials      514      521       +7
Impacted Files Coverage Δ
saleor/graphql/product/types/products.py 91.88% <ø> (ø) ⬆️
saleor/dashboard/product/filters.py 100% <ø> (ø) ⬆️
saleor/extensions/plugins/vatlayer/plugin.py 71.59% <ø> (ø) ⬆️
saleor/dashboard/order/filters.py 91.17% <ø> (ø) ⬆️
saleor/graphql/order/mutations/orders.py 95.02% <ø> (ø) ⬆️
saleor/extensions/plugins/avatax/__init__.py 77.88% <0%> (ø) ⬆️
saleor/dashboard/order/utils.py 76.47% <0%> (ø) ⬆️
saleor/extensions/plugins/avatax/plugin.py 67.72% <100%> (ø) ⬆️
saleor/dashboard/widgets.py 91.3% <100%> (-0.37%) ⬇️
saleor/graphql/giftcard/mutations.py 98.63% <100%> (ø) ⬆️
... and 33 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 b76eaee...65e40f8. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 19, 2019

Codecov Report

Merging #4639 into master will increase coverage by <.01%.
The diff coverage is 95.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4639      +/-   ##
==========================================
+ Coverage   91.41%   91.41%   +<.01%     
==========================================
  Files         307      307              
  Lines       18445    18542      +97     
  Branches     1844     1856      +12     
==========================================
+ Hits        16862    16951      +89     
- Misses       1065     1070       +5     
- Partials      518      521       +3
Impacted Files Coverage Δ
saleor/graphql/product/types/products.py 91.9% <ø> (ø) ⬆️
saleor/dashboard/product/filters.py 100% <ø> (ø) ⬆️
saleor/extensions/plugins/vatlayer/plugin.py 71.59% <ø> (ø) ⬆️
saleor/dashboard/order/filters.py 91.17% <ø> (ø) ⬆️
saleor/graphql/order/mutations/orders.py 95.02% <ø> (ø) ⬆️
saleor/extensions/plugins/avatax/__init__.py 77.88% <0%> (ø) ⬆️
saleor/extensions/plugins/avatax/plugin.py 67.72% <100%> (ø) ⬆️
saleor/graphql/giftcard/mutations.py 98.63% <100%> (ø) ⬆️
saleor/product/utils/variants_picker.py 94.91% <100%> (ø) ⬆️
saleor/shipping/models.py 80% <100%> (+0.66%) ⬆️
... and 38 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 1ac9086...d28c53c. Read the comment docs.

@NyanKiyoshi
Copy link
Member Author

NyanKiyoshi commented Aug 19, 2019

A lot of the money logic really needs refactoring as it was never meant to be able to handle multiple currencies. Overall, it should be able to handle having different currencies, e.g. the storefront owner changed the default currency. But it is not meant to be achievable.

For now we only want to upgrade to the latest django-prices to fix a blocking bug (#4378) and once we get the time for it, we will implement proper multi-currency support, which shouldn't have any dependency on the settings' default currency.

@NyanKiyoshi NyanKiyoshi force-pushed the django-prices/migrate/2.0 branch 2 times, most recently from 7dd257f to d5d0807 Compare August 19, 2019 11:19
@Kwaidan00
Copy link
Contributor

@NyanKiyoshi you're right, a lot of code will have to be refactored for multi-currency support. I think the hardest part will be sorting and filtering stuff. And of course checking if values in sum has the same currency - or catching exception.

saleor/discount/models.py Outdated Show resolved Hide resolved
templates/checkout/shipping_method.html Outdated Show resolved Hide resolved
saleor/product/utils/variants_picker.py Show resolved Hide resolved
saleor/dashboard/product/forms.py Outdated Show resolved Hide resolved
saleor/graphql/order/types.py Outdated Show resolved Hide resolved
saleor/graphql/shipping/mutations.py Outdated Show resolved Hide resolved
saleor/dashboard/shipping/forms.py Outdated Show resolved Hide resolved
@NyanKiyoshi NyanKiyoshi force-pushed the django-prices/migrate/2.0 branch 2 times, most recently from 0cd6592 to a9c58ee Compare August 26, 2019 12:25
NyanKiyoshi added 3 commits August 26, 2019 14:26
- zero_money takes the instances' currency (can be null, defaults to
  the settings' default currency)
- the models having a currency field now have the same length everywhere
  in the code: 3 instead of 10, the value can be changed in settings
  if needed.
- deprecated Order.discountAmount which was missing
@NyanKiyoshi NyanKiyoshi force-pushed the django-prices/migrate/2.0 branch 2 times, most recently from 1f81db2 to 39f0131 Compare August 26, 2019 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants