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
Merged
Diff settings

Always

Just for now

@@ -348,7 +348,15 @@ function handleSetupDomainError<T>(

if (error instanceof ERRORS.SourceNotFound) {
output.error(
`You can't purchase the domain your aliasing to since you have no valid payment method.`
`You can't purchase the domain you're aliasing to since you have no valid payment method.`
);
output.print(` Please add a valid payment method and retry.\n`);
return 1;
}

if (error instanceof ERRORS.DomainPaymentError) {
output.error(
`You can't purchase the domain you're aliasing to since your card was declined.`
);
output.print(` Please add a valid payment method and retry.\n`);
return 1;
@@ -135,6 +135,11 @@ export default async function buy(
return 1;
}

if (buyResult instanceof ERRORS.DomainPaymentError) {
output.error(`Your card was declined.`);
return 1;
}

if (buyResult.pending) {
console.log(
`${chalk.cyan('> Success!')} Domain ${param(
@@ -106,6 +106,7 @@ export default async function assignAlias(
result instanceof ERRORS.InvalidDomain ||
result instanceof ERRORS.SourceNotFound ||
result instanceof ERRORS.UnexpectedDomainPurchaseError ||
result instanceof ERRORS.DomainPaymentError ||
result instanceof ERRORS.UnsupportedTLD ||
result instanceof ERRORS.UserAborted
) {
@@ -37,6 +37,7 @@ export default async function upsertPathAlias(
domainInfo instanceof ERRORS.InvalidDomain ||
domainInfo instanceof ERRORS.SourceNotFound ||
domainInfo instanceof ERRORS.UnexpectedDomainPurchaseError ||
domainInfo instanceof ERRORS.DomainPaymentError ||
domainInfo instanceof ERRORS.UnsupportedTLD ||
domainInfo instanceof ERRORS.UserAborted
) {
@@ -61,6 +61,11 @@ export default async function createDeploy(
return createDeploy(output, now, contextName, paths, createArgs);
}

if (error.code === 'not_found') {
// The first argument is the deployment id, which is not available here
return new ERRORS_TS.DeploymentNotFound('', contextName)
This conversation was marked as resolved by leo

This comment has been minimized.

Copy link
@juliangruber

juliangruber Mar 12, 2019

Author Contributor

@leo what do you think about this? Should the DeploymentNotFound constructor be updated so the deployment id is optional, or can we get the id of the deployment here?

This comment has been minimized.

Copy link
@leo

leo Mar 12, 2019

Member

@juliangruber I would update it to make it optional, yep.

}

// If the error is unknown, we just throw
throw error;
}
@@ -62,7 +62,8 @@ export default async function purchaseDomainIfAvailable(
result instanceof ERRORS.DomainNotAvailable ||

This comment has been minimized.

Copy link
@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.

Copy link
@juliangruber

juliangruber Mar 12, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@juliangruber

juliangruber Mar 12, 2019

Author Contributor

This comment has been minimized.

Copy link
@juliangruber

juliangruber Mar 12, 2019

Author Contributor

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

result instanceof ERRORS.DomainServiceNotAvailable ||
result instanceof ERRORS.InvalidDomain ||
result instanceof ERRORS.UnexpectedDomainPurchaseError
result instanceof ERRORS.UnexpectedDomainPurchaseError ||
result instanceof ERRORS.DomainPaymentError
) {
return result;
}
@@ -35,6 +35,9 @@ export default async function purchaseDomain(
if (error.code === 'source_not_found') {
return new ERRORS.SourceNotFound();
}
if (error.code === 'payment_error') {
return new ERRORS.DomainPaymentError();
}
throw error;
}
}
@@ -338,6 +338,19 @@ export class UnexpectedDomainPurchaseError extends NowError<
}
}

/**
* Returned when there is an expected error charging the card.
*/
export class DomainPaymentError extends NowError<'DOMAIN_PAYMENT_ERROR', {}> {
constructor() {
super({
code: 'DOMAIN_PAYMENT_ERROR',
meta: {},
message: `Your card was declined.`

This comment has been minimized.

Copy link
@javivelasco

javivelasco Mar 12, 2019

Member

Is the error always Card Declined?

This comment has been minimized.

Copy link
@juliangruber

juliangruber Mar 12, 2019

Author Contributor

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?

});
}
}

/**
* Returned during purchase in alias when the domain was purchased but the
* order is pending so the alias can't be completed yet
@@ -348,6 +348,14 @@ export default class Now extends EventEmitter {
return body;
}

if (
res.status === 404 &&
body.error &&
body.error.code === 'not_found'
) {
return body;
}

if (res.status >= 400 && res.status < 500) {
const err = new Error();

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.