From 4ef547f8d6db03916369f537321767a16002f682 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 6 Oct 2014 14:20:40 -0700 Subject: [PATCH] Give WePay complete payment logic in Phortune Summary: Ref T2787. This doesn't get all the edge cases quite correct, but is generally a safe, complete payment workflow: - Shares the actual charging state logic. - Makes it appropriately stateful with locking and transactions. - Gets the main flow correct. - Detects failure cases, just tends to blow up rather than help the user resolve them. Test Plan: - Charged with WePay. - Charged with Infinite Free Money. - Resumed an abandoned cart. - Hit all failure states where we just dead-end the cart. Not ideal, but (seemingly) complete/safe/correct. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T2787 Differential Revision: https://secure.phabricator.com/D10639 --- .../PhortuneCartCheckoutController.php | 37 +++++----- .../provider/PhortunePaymentProvider.php | 13 +++- .../provider/PhortuneWePayPaymentProvider.php | 64 +++++++++-------- .../phortune/query/PhortuneChargeQuery.php | 13 ++++ .../phortune/storage/PhortuneCart.php | 68 ++++++++++++++++--- .../phortune/storage/PhortuneCharge.php | 7 +- 6 files changed, 143 insertions(+), 59 deletions(-) diff --git a/src/applications/phortune/controller/PhortuneCartCheckoutController.php b/src/applications/phortune/controller/PhortuneCartCheckoutController.php index 8f4cc73ea1..81b1d70b1a 100644 --- a/src/applications/phortune/controller/PhortuneCartCheckoutController.php +++ b/src/applications/phortune/controller/PhortuneCartCheckoutController.php @@ -38,6 +38,25 @@ final class PhortuneCartCheckoutController // This is the expected, normal state for a cart that's ready for // checkout. break; + case PhortuneCart::STATUS_PURCHASING: + // We've started the purchase workflow for this cart, but were not able + // to complete it. If the workflow is on an external site, this could + // happen because the user abandoned the workflow. Just return them to + // the right place so they can resume where they left off. + $uri = $cart->getMetadataValue('provider.checkoutURI'); + if ($uri !== null) { + return id(new AphrontRedirectResponse()) + ->setIsExternal(true) + ->setURI($uri); + } + + return $this->newDialog() + ->setTitle(pht('Charge Failed')) + ->appendParagraph( + pht( + 'Failed to charge this cart.')) + ->addCancelButton($cancel_uri); + break; case PhortuneCart::STATUS_CHARGED: // TODO: This is really bad (we took your money and at least partially // failed to fulfill your order) and should have better steps forward. @@ -89,24 +108,8 @@ final class PhortuneCartCheckoutController if (!$errors) { $provider = $method->buildPaymentProvider(); - $charge = id(new PhortuneCharge()) - ->setAccountPHID($account->getPHID()) - ->setCartPHID($cart->getPHID()) - ->setAuthorPHID($viewer->getPHID()) - ->setPaymentProviderKey($provider->getProviderKey()) - ->setPaymentMethodPHID($method->getPHID()) - ->setAmountAsCurrency($cart->getTotalPriceAsCurrency()) - ->setStatus(PhortuneCharge::STATUS_PENDING); - - $charge->openTransaction(); - $charge->save(); - - $cart->setStatus(PhortuneCart::STATUS_PURCHASING); - $cart->save(); - $charge->saveTransaction(); - + $charge = $cart->willApplyCharge($viewer, $provider, $method); $provider->applyCharge($method, $charge); - $cart->didApplyCharge($charge); $done_uri = $cart->getDoneURI(); diff --git a/src/applications/phortune/provider/PhortunePaymentProvider.php b/src/applications/phortune/provider/PhortunePaymentProvider.php index 85cbdced3a..a0fa333b93 100644 --- a/src/applications/phortune/provider/PhortunePaymentProvider.php +++ b/src/applications/phortune/provider/PhortunePaymentProvider.php @@ -204,11 +204,13 @@ abstract class PhortunePaymentProvider { ->setText($description) ->setSubtext($details); + // NOTE: We generate a local URI to make sure the form picks up CSRF tokens. $uri = $this->getControllerURI( 'checkout', array( 'cartID' => $cart->getID(), - )); + ), + $local = true); return phabricator_form( $user, @@ -225,7 +227,8 @@ abstract class PhortunePaymentProvider { final public function getControllerURI( $action, - array $params = array()) { + array $params = array(), + $local = false) { $digest = PhabricatorHash::digestForIndex($this->getProviderKey()); @@ -235,7 +238,11 @@ abstract class PhortunePaymentProvider { $uri = new PhutilURI($path); $uri->setQueryParams($params); - return PhabricatorEnv::getURI((string)$uri); + if ($local) { + return $uri; + } else { + return PhabricatorEnv::getURI((string)$uri); + } } public function canRespondToControllerAction($action) { diff --git a/src/applications/phortune/provider/PhortuneWePayPaymentProvider.php b/src/applications/phortune/provider/PhortuneWePayPaymentProvider.php index b52dc87e5d..0c11e4186b 100644 --- a/src/applications/phortune/provider/PhortuneWePayPaymentProvider.php +++ b/src/applications/phortune/provider/PhortuneWePayPaymentProvider.php @@ -91,8 +91,6 @@ final class PhortuneWePayPaymentProvider extends PhortunePaymentProvider { return new Aphront404Response(); } - $cart_uri = '/phortune/cart/'.$cart->getID().'/'; - $root = dirname(phutil_get_library_root('phabricator')); require_once $root.'/externals/wepay/wepay.php'; @@ -102,6 +100,29 @@ final class PhortuneWePayPaymentProvider extends PhortunePaymentProvider { $wepay = new WePay($this->getWePayAccessToken()); + $charge = id(new PhortuneChargeQuery()) + ->setViewer($viewer) + ->withCartPHIDs(array($cart->getPHID())) + ->withStatuses( + array( + PhortuneCharge::STATUS_CHARGING, + )) + ->executeOne(); + + switch ($controller->getAction()) { + case 'checkout': + if ($charge) { + throw new Exception(pht('Cart is already charging!')); + } + break; + case 'charge': + case 'cancel': + if (!$charge) { + throw new Exception(pht('Cart is not charging yet!')); + } + break; + } + switch ($controller->getAction()) { case 'checkout': $return_uri = $this->getControllerURI( @@ -142,10 +163,13 @@ final class PhortuneWePayPaymentProvider extends PhortunePaymentProvider { 'funding_sources' => 'bank,cc' ); + $cart->willApplyCharge($viewer, $this); + $result = $wepay->request('checkout/create', $params); - // TODO: We must store "$result->checkout_id" on the Cart since the - // user might not end up back here. Really this needs a bunch of junk. + $cart->setMetadataValue('provider.checkoutURI', $result->checkout_uri); + $cart->setMetadataValue('wepay.checkoutID', $result->checkout_id); + $cart->save(); $uri = new PhutilURI($result->checkout_uri); return id(new AphrontRedirectResponse()) @@ -175,38 +199,22 @@ final class PhortuneWePayPaymentProvider extends PhortunePaymentProvider { $result->state)); } - $currency = PhortuneCurrency::newFromString($checkout->gross, 'USD'); - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - - $charge = id(new PhortuneCharge()) - ->setAmountAsCurrency($currency) - ->setAccountPHID($cart->getAccount()->getPHID()) - ->setAuthorPHID($viewer->getPHID()) - ->setPaymentProviderKey($this->getProviderKey()) - ->setCartPHID($cart->getPHID()) - ->setStatus(PhortuneCharge::STATUS_CHARGING) - ->save(); - - $cart->openTransaction(); - $charge->setStatus(PhortuneCharge::STATUS_CHARGED); - $charge->save(); - - $cart->setStatus(PhortuneCart::STATUS_PURCHASED); - $cart->save(); - $cart->saveTransaction(); - + $cart->didApplyCharge($charge); unset($unguarded); return id(new AphrontRedirectResponse()) - ->setIsExternal(true) - ->setURI($cart_uri); + ->setURI($cart->getDoneURI()); case 'cancel': - var_dump($_REQUEST); + // TODO: I don't know how it's possible to cancel out of a WePay + // charge workflow. + throw new Exception( + pht('How did you get here? WePay has no cancel flow in its UI...?')); break; } - throw new Exception("The rest of this isn't implemented yet."); + throw new Exception( + pht('Unsupported action "%s".', $controller->getAction())); } diff --git a/src/applications/phortune/query/PhortuneChargeQuery.php b/src/applications/phortune/query/PhortuneChargeQuery.php index a706f7e7ed..aba655b4e4 100644 --- a/src/applications/phortune/query/PhortuneChargeQuery.php +++ b/src/applications/phortune/query/PhortuneChargeQuery.php @@ -7,6 +7,7 @@ final class PhortuneChargeQuery private $phids; private $accountPHIDs; private $cartPHIDs; + private $statuses; private $needCarts; @@ -30,6 +31,11 @@ final class PhortuneChargeQuery return $this; } + public function withStatuses(array $statuses) { + $this->statuses = $statuses; + return $this; + } + public function needCarts($need_carts) { $this->needCarts = $need_carts; return $this; @@ -121,6 +127,13 @@ final class PhortuneChargeQuery $this->cartPHIDs); } + if ($this->statuses !== null) { + $where[] = qsprintf( + $conn, + 'charge.status IN (%Ls)', + $this->statuses); + } + return $this->formatWhereClause($where); } diff --git a/src/applications/phortune/storage/PhortuneCart.php b/src/applications/phortune/storage/PhortuneCart.php index 6da806f949..36e1f8c2d7 100644 --- a/src/applications/phortune/storage/PhortuneCart.php +++ b/src/applications/phortune/storage/PhortuneCart.php @@ -52,17 +52,67 @@ final class PhortuneCart extends PhortuneDAO return $this; } - public function didApplyCharge(PhortuneCharge $charge) { - if ($this->getStatus() !== self::STATUS_PURCHASING) { - throw new Exception( - pht( - 'Cart has wrong status ("%s") to call didApplyCharge(), expected '. - '"%s".', - $this->getStatus(), - self::STATUS_PURCHASING)); + public function willApplyCharge( + PhabricatorUser $actor, + PhortunePaymentProvider $provider, + PhortunePaymentMethod $method = null) { + + $account = $this->getAccount(); + + $charge = PhortuneCharge::initializeNewCharge() + ->setAccountPHID($account->getPHID()) + ->setCartPHID($this->getPHID()) + ->setAuthorPHID($actor->getPHID()) + ->setPaymentProviderKey($provider->getProviderKey()) + ->setAmountAsCurrency($this->getTotalPriceAsCurrency()); + + if ($method) { + $charge->setPaymentMethodPHID($method->getPHID()); } - $this->setStatus(self::STATUS_CHARGED)->save(); + $this->openTransaction(); + $this->beginReadLocking(); + + $copy = clone $this; + $copy->reload(); + + if ($copy->getStatus() !== self::STATUS_READY) { + throw new Exception( + pht( + 'Cart has wrong status ("%s") to call willApplyCharge(), expected '. + '"%s".', + $copy->getStatus(), + self::STATUS_READY)); + } + + $charge->save(); + $this->setStatus(PhortuneCart::STATUS_PURCHASING)->save(); + $this->saveTransaction(); + + return $charge; + } + + public function didApplyCharge(PhortuneCharge $charge) { + $charge->setStatus(PhortuneCharge::STATUS_CHARGED); + + $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 didApplyCharge(), expected '. + '"%s".', + $copy->getStatus(), + self::STATUS_PURCHASING)); + } + + $charge->save(); + $this->setStatus(self::STATUS_CHARGED)->save(); + $this->saveTransaction(); foreach ($this->purchases as $purchase) { $purchase->getProduct()->didPurchaseProduct($purchase); diff --git a/src/applications/phortune/storage/PhortuneCharge.php b/src/applications/phortune/storage/PhortuneCharge.php index c7d04e647b..1a33c36dd2 100644 --- a/src/applications/phortune/storage/PhortuneCharge.php +++ b/src/applications/phortune/storage/PhortuneCharge.php @@ -9,8 +9,6 @@ final class PhortuneCharge extends PhortuneDAO implements PhabricatorPolicyInterface { - const STATUS_PENDING = 'charge:pending'; - const STATUS_AUTHORIZED = 'charge:authorized'; const STATUS_CHARGING = 'charge:charging'; const STATUS_CHARGED = 'charge:charged'; const STATUS_FAILED = 'charge:failed'; @@ -27,6 +25,11 @@ final class PhortuneCharge extends PhortuneDAO private $account = self::ATTACHABLE; private $cart = self::ATTACHABLE; + public static function initializeNewCharge() { + return id(new PhortuneCharge()) + ->setStatus(self::STATUS_CHARGING); + } + public function getConfiguration() { return array( self::CONFIG_AUX_PHID => true,