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

Fixed the handling of user errors #1944

Merged
merged 4 commits into from Mar 12, 2019

Conversation

4 participants
@juliangruber
Copy link
Collaborator

juliangruber commented Mar 11, 2019

No description provided.

@juliangruber juliangruber requested review from javivelasco and leo Mar 11, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 11, 2019

Codecov Report

Merging #1944 into canary will decrease coverage by 0.26%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           canary   #1944      +/-   ##
=========================================
- Coverage    6.61%   6.34%   -0.27%     
=========================================
  Files          91      91              
  Lines        4069    4066       -3     
=========================================
- Hits          269     258      -11     
- Misses       3800    3808       +8
Impacted Files Coverage Δ
src/commands/list.js 0% <0%> (ø) ⬆️
src/commands/certs/add.js 0% <0%> (ø) ⬆️
src/util/deploy/create-deploy.js 0% <0%> (ø) ⬆️
src/util/index.js 0% <0%> (ø) ⬆️
src/commands/alias/rm.js 0% <0%> (ø) ⬆️
src/commands/scale.js 0% <0%> (ø) ⬆️
src/commands/logs.js 0% <0%> (ø) ⬆️
src/commands/remove.js 0% <0%> (ø) ⬆️
src/commands/billing/index.js 0% <0%> (ø) ⬆️
src/commands/alias/ls.js 0% <0%> (ø) ⬆️
... and 13 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 6bcb865...3177ec7. Read the comment docs.

@@ -62,7 +62,8 @@ export default async function purchaseDomainIfAvailable(
result instanceof ERRORS.DomainNotAvailable ||

This comment has been minimized.

@javivelasco

javivelasco Mar 12, 2019

Member

@juliangruber I think this pattern matching is for when we used to have Flow and it wasn't smart enough to infer the types when checking against Error. Since all of these are inheriting from the Error prototype I'm quite sure we can just remove this lines into a simpler:

if (result instanceof Error) {
  return result
}

Please try it out as it makes the code easier. Same for other places where you needed to add the check.

This comment has been minimized.

@juliangruber

juliangruber Mar 12, 2019

Author Collaborator

Oh wow, I was wondering about all those manual checks. Will refactor!

This comment has been minimized.

@juliangruber

juliangruber Mar 12, 2019

Author Collaborator

This comment has been minimized.

@juliangruber

juliangruber Mar 12, 2019

Author Collaborator

Let's keep refactoring this in that PR and merge this following the current structure

super({
code: 'DOMAIN_PAYMENT_ERROR',
meta: {},
message: `Your card was declined.`

This comment has been minimized.

@javivelasco

javivelasco Mar 12, 2019

Member

Is the error always Card Declined?

This comment has been minimized.

@juliangruber

juliangruber Mar 12, 2019

Author Collaborator

I don't have insight into our API so I can't tell. I assumed that a payment error always means charging the card fails which people usually refer to as the card being declined?

@juliangruber juliangruber force-pushed the fix/unexpected-payment-error branch from 73065d9 to c6cda40 Mar 12, 2019

@juliangruber juliangruber changed the title fix unexpected `payment_error` fix handling of some user errors Mar 12, 2019

@juliangruber juliangruber force-pushed the fix/unexpected-payment-error branch from 564b1a5 to 3177ec7 Mar 12, 2019

@leo

leo approved these changes Mar 12, 2019

Copy link
Member

leo left a comment

Finally!

@leo leo changed the title fix handling of some user errors Fixed the handling of user errors Mar 12, 2019

@juliangruber juliangruber merged commit 12653f9 into canary Mar 12, 2019

5 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: test-integration Your tests passed on CircleCI!
Details
ci/circleci: test-lint Your tests passed on CircleCI!
Details
ci/circleci: test-unit Your tests passed on CircleCI!
Details

@juliangruber juliangruber deleted the fix/unexpected-payment-error branch Mar 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.