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 redirectUrl to staff and user create mutations #4717

Merged
merged 6 commits into from
Sep 9, 2019

Conversation

fowczarek
Copy link
Member

@fowczarek fowczarek commented Sep 4, 2019

I want to merge this change because resolve #4716

TODO:

  • The staff create mutations should have a URL for a set password view as an additional field.
  • The customer create mutations should have a URL for a set password view as an additional field.
  • Remove port from allowed storefront validation
  • Fix send an email on request account delete mutation

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 added in progress graphql Issues related to the GraphQL API labels Sep 4, 2019
@fowczarek fowczarek self-assigned this Sep 4, 2019
Copy link

django-queries commented Sep 4, 2019

Here is the report for 5ccd99a (mirumee/saleor @ 4716/redirect_url_in_staff_create_mutation)
Base comparison is 0ea50c9.

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 Sep 4, 2019

Codecov Report

Merging #4717 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4717      +/-   ##
==========================================
+ Coverage   91.63%   91.66%   +0.02%     
==========================================
  Files         310      310              
  Lines       18633    18659      +26     
  Branches     1843     1847       +4     
==========================================
+ Hits        17075    17104      +29     
+ Misses       1045     1042       -3     
  Partials      513      513
Impacted Files Coverage Δ
saleor/graphql/account/mutations/staff.py 96.49% <100%> (+0.09%) ⬆️
saleor/dashboard/staff/views.py 100% <100%> (ø) ⬆️
saleor/dashboard/emails.py 100% <100%> (ø) ⬆️
saleor/graphql/account/mutations/base.py 98.4% <100%> (+0.04%) ⬆️
saleor/account/emails.py 84.09% <100%> (+7.34%) ⬆️
saleor/core/utils/url.py 84.61% <100%> (+2.79%) ⬆️
saleor/dashboard/customer/views.py 94.73% <100%> (ø) ⬆️

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 0ea50c9...5ccd99a. Read the comment docs.

@fowczarek fowczarek force-pushed the 4716/redirect_url_in_staff_create_mutation branch from cee764d to d509906 Compare September 5, 2019 12:48
saleor/core/utils/url.py Outdated Show resolved Hide resolved
saleor/core/utils/url.py Outdated Show resolved Hide resolved
saleor/dashboard/emails.py Outdated Show resolved Hide resolved
)
content = get_graphql_content(response)
data = content["data"]["customerCreate"]
assert data["errors"][0]["field"] == "redirectUrl"
Copy link
Member

Choose a reason for hiding this comment

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

Comparing the message as well would be better to ensure that's actually the error we were expecting, and to allow developers to search for the associated test

Copy link
Member

Choose a reason for hiding this comment

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

I hope that today we'll merge #4676 which would allow us to check the error codes in cases like this. Relying on error messages comparisons in tests always feels a little bit clunky to me ;)

)
content = get_graphql_content(response)
data = content["data"]["customerCreate"]
assert data["errors"][0]["field"] == "redirectUrl"
Copy link
Member

Choose a reason for hiding this comment

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

Same

)
content = get_graphql_content(response)
data = content["data"]["customerCreate"]
assert data["errors"][0]["field"] == "redirectUrl"
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

tests/api/test_account.py Outdated Show resolved Hide resolved
tests/api/test_account.py Outdated Show resolved Hide resolved
saleor/dashboard/emails.py Outdated Show resolved Hide resolved
tests/api/test_account.py Show resolved Hide resolved
tests/api/test_account.py Outdated Show resolved Hide resolved
tests/api/test_account.py Outdated Show resolved Hide resolved
@fowczarek fowczarek force-pushed the 4716/redirect_url_in_staff_create_mutation branch from c112349 to 5ccd99a Compare September 6, 2019 09:21
@maarcingebala maarcingebala merged commit e27a609 into master Sep 9, 2019
@maarcingebala maarcingebala deleted the 4716/redirect_url_in_staff_create_mutation branch September 9, 2019 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
graphql Issues related to the GraphQL API in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add redirectUrl to staff and user create mutations
5 participants