From ad991b019709e83ee14ec1be914828eb4d3af83d Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 8 Oct 2014 17:23:02 -0700 Subject: [PATCH] Handle Phortune charge failures cleanly Summary: Ref T2787. Currently, we kill a cart and dead-end the workflow on a charge failure. Instead, fail the charge and reset the cart so the user can try using a valid payment instrument like a normal checkout workflow would. Some shakiness/smoothing on WePay for the moment; PayPal is still made up since we don't have a "Hold" state yet. Test Plan: {F215214} Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T2787 Differential Revision: https://secure.phabricator.com/D10666 --- .../base/controller/PhabricatorController.php | 2 +- .../PhortuneCartCheckoutController.php | 15 ++++- .../PhortunePayPalPaymentProvider.php | 22 ++++++- .../provider/PhortuneWePayPaymentProvider.php | 63 +++++++++++++------ .../phortune/storage/PhortuneCart.php | 31 +++++++++ 5 files changed, 110 insertions(+), 23 deletions(-) diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php index df39349240..ca1aa1e6e8 100644 --- a/src/applications/base/controller/PhabricatorController.php +++ b/src/applications/base/controller/PhabricatorController.php @@ -518,7 +518,7 @@ abstract class PhabricatorController extends AphrontController { * * @return AphrontDialogView New dialog. */ - protected function newDialog() { + public function newDialog() { $submit_uri = new PhutilURI($this->getRequest()->getRequestURI()); $submit_uri = $submit_uri->getPath(); diff --git a/src/applications/phortune/controller/PhortuneCartCheckoutController.php b/src/applications/phortune/controller/PhortuneCartCheckoutController.php index 8e1d240a56..4ec47de752 100644 --- a/src/applications/phortune/controller/PhortuneCartCheckoutController.php +++ b/src/applications/phortune/controller/PhortuneCartCheckoutController.php @@ -111,7 +111,20 @@ final class PhortuneCartCheckoutController $provider = $method->buildPaymentProvider(); $charge = $cart->willApplyCharge($viewer, $provider, $method); - $provider->applyCharge($method, $charge); + + try { + $provider->applyCharge($method, $charge); + } catch (Exception $ex) { + $cart->didFailCharge($charge); + return $this->newDialog() + ->setTitle(pht('Charge Failed')) + ->appendParagraph( + pht( + 'Unable to make payment: %s', + $ex->getMessage())) + ->addCancelButton($cart->getCheckoutURI(), pht('Continue')); + } + $cart->didApplyCharge($charge); $done_uri = $cart->getDoneURI(); diff --git a/src/applications/phortune/provider/PhortunePayPalPaymentProvider.php b/src/applications/phortune/provider/PhortunePayPalPaymentProvider.php index d4755d2ffc..57601616fc 100644 --- a/src/applications/phortune/provider/PhortunePayPalPaymentProvider.php +++ b/src/applications/phortune/provider/PhortunePayPalPaymentProvider.php @@ -339,9 +339,27 @@ final class PhortunePayPalPaymentProvider extends PhortunePaymentProvider { // difficult for now and we can't reasonably just fail these charges. var_dump($result); - die(); - break; + + $success = false; // TODO: <---- + + // TODO: Clean this up once that mess up there ^^^^^ gets cleaned up. + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + if ($success) { + $cart->didApplyCharge($charge); + $response = id(new AphrontRedirectResponse())->setURI( + $cart->getDoneURI()); + } else { + $cart->didFailCharge($charge); + + $response = $controller + ->newDialog() + ->setTitle(pht('Charge Failed')) + ->addCancelButton($cart->getCheckoutURI(), pht('Continue')); + } + unset($unguarded); + + return $response; case 'cancel': var_dump($_REQUEST); break; diff --git a/src/applications/phortune/provider/PhortuneWePayPaymentProvider.php b/src/applications/phortune/provider/PhortuneWePayPaymentProvider.php index 705c6c08aa..c4d927f8da 100644 --- a/src/applications/phortune/provider/PhortuneWePayPaymentProvider.php +++ b/src/applications/phortune/provider/PhortuneWePayPaymentProvider.php @@ -145,7 +145,7 @@ final class PhortuneWePayPaymentProvider extends PhortunePaymentProvider { } public function getPaymentMethodDescription() { - return pht('Credit Card or Bank Account'); + return pht('Credit or Debit Card'); } public function getPaymentMethodIcon() { @@ -286,10 +286,10 @@ final class PhortuneWePayPaymentProvider extends PhortunePaymentProvider { $params = array( 'account_id' => $this->getWePayAccountID(), - 'short_description' => 'Services', // TODO + 'short_description' => $cart->getName(), 'type' => 'SERVICE', 'amount' => $price->formatBareValue(), - 'long_description' => 'Services', // TODO + 'long_description' => $cart->getName(), 'reference_id' => $cart->getPHID(), 'app_fee' => 0, 'fee_payer' => 'Payee', @@ -305,7 +305,10 @@ final class PhortuneWePayPaymentProvider extends PhortunePaymentProvider { 'shipping_fee' => 0, 'charge_tax' => 0, 'mode' => 'regular', - 'funding_sources' => 'bank,cc', + + // TODO: We could accept bank accounts but the hold/capture rules + // are not quite clear. Just accept credit cards for now. + 'funding_sources' => 'cc', ); $charge = $cart->willApplyCharge($viewer, $this); @@ -322,6 +325,11 @@ final class PhortuneWePayPaymentProvider extends PhortunePaymentProvider { ->setIsExternal(true) ->setURI($uri); case 'charge': + if ($cart->getStatus() !== PhortuneCart::STATUS_PURCHASING) { + return id(new AphrontRedirectResponse()) + ->setURI($cart->getCheckoutURI()); + } + $checkout_id = $request->getInt('checkout_id'); $params = array( 'checkout_id' => $checkout_id, @@ -333,24 +341,41 @@ final class PhortuneWePayPaymentProvider extends PhortunePaymentProvider { pht('Checkout reference ID does not match cart PHID!')); } - switch ($checkout->state) { - case 'authorized': - case 'reserved': - case 'captured': - break; - default: - throw new Exception( - pht( - 'Checkout is in bad state "%s"!', - $result->state)); - } - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $cart->didApplyCharge($charge); + switch ($checkout->state) { + case 'authorized': + case 'reserved': + case 'captured': + // TODO: Are these all really "done" states, and not "hold" + // states? Cards and bank accounts both come back as "authorized" + // on the staging environment. Figure out what happens in + // production? + + $cart->didApplyCharge($charge); + + $response = id(new AphrontRedirectResponse())->setURI( + $cart->getDoneURI()); + break; + default: + // It's not clear if we can ever get here on the web workflow, + // WePay doesn't seem to return back to us after a failure (the + // workflow dead-ends instead). + + $cart->didFailCharge($charge); + + $response = $controller + ->newDialog() + ->setTitle(pht('Charge Failed')) + ->appendParagraph( + pht( + 'Unable to make payment (checkout state is "%s").', + $checkout->state)) + ->addCancelButton($cart->getCheckoutURI(), pht('Continue')); + break; + } unset($unguarded); - return id(new AphrontRedirectResponse()) - ->setURI($cart->getDoneURI()); + return $response; case 'cancel': // TODO: I don't know how it's possible to cancel out of a WePay // charge workflow. diff --git a/src/applications/phortune/storage/PhortuneCart.php b/src/applications/phortune/storage/PhortuneCart.php index c78d332c43..23e8807c92 100644 --- a/src/applications/phortune/storage/PhortuneCart.php +++ b/src/applications/phortune/storage/PhortuneCart.php @@ -146,6 +146,37 @@ final class PhortuneCart extends PhortuneDAO return $this; } + public function didFailCharge(PhortuneCharge $charge) { + $charge->setStatus(PhortuneCharge::STATUS_FAILED); + + $this->openTransaction(); + $this->beginReadLocking(); + + $copy = clone $this; + $copy->reload(); + + if ($copy->getStatus() !== self::STATUS_PURCHASING) { + throw new Exception( + pht( + 'Cart has wrong status ("%s") to call didFailCharge(), '. + 'expected "%s".', + $copy->getStatus(), + self::STATUS_PURCHASING)); + } + + $charge->save(); + + // Move the cart back into STATUS_READY so the user can try + // making the purchase again. + $this->setStatus(self::STATUS_READY)->save(); + + $this->endReadLocking(); + $this->saveTransaction(); + + return $this; + } + + public function willRefundCharge( PhabricatorUser $actor, PhortunePaymentProvider $provider,