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

Add error codes to API #4676

Merged
merged 49 commits into from
Sep 19, 2019
Merged

Add error codes to API #4676

merged 49 commits into from
Sep 19, 2019

Conversation

Kwaidan00
Copy link
Contributor

@Kwaidan00 Kwaidan00 commented Aug 22, 2019

I want to merge this change because it adds error codes for mutation errors.

It closes #4119.

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.

saleor/checkout/utils.py Outdated Show resolved Hide resolved
saleor/core/utils/promo_code.py Outdated Show resolved Hide resolved
saleor/graphql/account/mutations/account.py Outdated Show resolved Hide resolved
saleor/graphql/core/utils/error_codes.py Outdated Show resolved Hide resolved
saleor/graphql/menu/mutations.py Outdated Show resolved Hide resolved
saleor/graphql/core/utils/error_codes.py Outdated Show resolved Hide resolved
saleor/graphql/order/mutations/draft_orders.py Outdated Show resolved Hide resolved
saleor/graphql/product/mutations/attributes.py Outdated Show resolved Hide resolved
@maarcingebala
Copy link
Member

I did the first review and I think we have to look through the codes and think whether we can unify/generalize some of them. Another question is whether we need to start exposing them all at once. Maybe a better idea would be to have dedicated enums for each group e.g. CheckoutErrorCodes.

Copy link

django-queries commented Aug 26, 2019

Here is the report for 702e735 (mirumee/saleor @ add-error-codes-in-api)
Base comparison is 9e68873.

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                     	         15	         15	              3

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

@codecov
Copy link

codecov bot commented Aug 26, 2019

Codecov Report

Merging #4676 into master will increase coverage by 0.07%.
The diff coverage is 89.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4676      +/-   ##
==========================================
+ Coverage   91.41%   91.49%   +0.07%     
==========================================
  Files         307      308       +1     
  Lines       18445    18572     +127     
  Branches     1844     1848       +4     
==========================================
+ Hits        16862    16992     +130     
+ Misses       1065     1063       -2     
+ Partials      518      517       -1
Impacted Files Coverage Δ
saleor/graphql/order/utils.py 100% <100%> (ø) ⬆️
saleor/checkout/forms.py 94.47% <100%> (ø) ⬆️
saleor/graphql/shipping/mutations.py 98.21% <100%> (+0.01%) ⬆️
saleor/graphql/giftcard/mutations.py 98.64% <100%> (+0.01%) ⬆️
saleor/graphql/core/mutations.py 93.29% <100%> (+0.01%) ⬆️
saleor/account/validators.py 100% <100%> (ø) ⬆️
...aleor/graphql/order/bulk_mutations/draft_orders.py 100% <100%> (ø) ⬆️
saleor/graphql/account/utils.py 97.61% <100%> (+0.05%) ⬆️
saleor/checkout/utils.py 84.22% <100%> (+0.02%) ⬆️
saleor/graphql/account/mutations/staff.py 96.41% <100%> (+0.01%) ⬆️
... 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 1ac9086...43625e6. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 26, 2019

Codecov Report

Merging #4676 into master will increase coverage by 0.28%.
The diff coverage is 96.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4676      +/-   ##
==========================================
+ Coverage   91.67%   91.96%   +0.28%     
==========================================
  Files         313      324      +11     
  Lines       18855    19491     +636     
  Branches     1851     1863      +12     
==========================================
+ Hits        17286    17925     +639     
+ Misses       1049     1047       -2     
+ Partials      520      519       -1
Impacted Files Coverage Δ
saleor/core/weight.py 87.69% <ø> (ø) ⬆️
saleor/graphql/extensions/mutations.py 100% <100%> (ø) ⬆️
saleor/graphql/order/utils.py 100% <100%> (ø) ⬆️
saleor/checkout/forms.py 94.47% <100%> (ø) ⬆️
...aleor/graphql/account/mutations/service_account.py 97.53% <100%> (+0.38%) ⬆️
saleor/extensions/plugins/vatlayer/plugin.py 72.13% <100%> (+0.15%) ⬆️
saleor/graphql/menu/bulk_mutations.py 100% <100%> (ø) ⬆️
saleor/extensions/plugins/avatax/plugin.py 69.87% <100%> (+0.12%) ⬆️
saleor/graphql/giftcard/mutations.py 98.79% <100%> (+0.16%) ⬆️
saleor/account/error_codes.py 100% <100%> (ø)
... and 57 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 9e68873...702e735. Read the comment docs.

@patrys
Copy link
Member

patrys commented Aug 28, 2019

Can we plan a meeting where we go through all of these codes and determine whether they are correct or even needed?

@Kwaidan00 Kwaidan00 force-pushed the add-error-codes-in-api branch 2 times, most recently from 679b223 to 57e93a8 Compare September 3, 2019 15:25
@Kwaidan00
Copy link
Contributor Author

@patrys @maarcingebala I've created typed versions of Error type and separated enums. Please take a look.

saleor/graphql/schema.graphql Outdated Show resolved Hide resolved
saleor/graphql/schema.graphql Outdated Show resolved Hide resolved
saleor/graphql/schema.graphql Outdated Show resolved Hide resolved
saleor/graphql/schema.graphql Outdated Show resolved Hide resolved
saleor/payment/gateways/braintree/forms.py Outdated Show resolved Hide resolved
saleor/graphql/product/mutations/digital_contents.py Outdated Show resolved Hide resolved
saleor/graphql/schema.graphql Outdated Show resolved Hide resolved
saleor/graphql/account/mutations/base.py Outdated Show resolved Hide resolved
@Kwaidan00
Copy link
Contributor Author

Kwaidan00 commented Sep 18, 2019

@maarcingebala @korycins @NyanKiyoshi @patrys I've changed error code mechanism to use Meta class instead of inheritance, for some of your comments I've created separated issues, I've rebased branch and added error codes for new parts of the codebase.

Copy link
Member

@NyanKiyoshi NyanKiyoshi left a comment

Choose a reason for hiding this comment

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

❤️

@korycins korycins merged commit d369a4b into master Sep 19, 2019
@korycins korycins deleted the add-error-codes-in-api branch September 19, 2019 09:58
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.

Return error codes in API
6 participants