Issue #2946909 by Sophie.SK, bojanz: When a payment gateway error occurs, the order does not become unocked

bojanz · bojanz · commit 4250f83fb616 · 2018-04-04T11:53:39.000+02:00
diff --git a/modules/checkout/commerce_checkout.module b/modules/checkout/commerce_checkout.module
@@ -6,7 +6,6 @@
  */
 
 use Drupal\commerce\EntityHelper;
-use Drupal\commerce_order\Entity\OrderInterface;
 use Drupal\Core\Entity\EntityTypeInterface;
 use Drupal\Core\Field\BaseFieldDefinition;
 use Drupal\Core\Form\FormStateInterface;
@@ -118,26 +117,6 @@ function commerce_checkout_entity_base_field_info(EntityTypeInterface $entity_ty
   }
 }
 
-/**
- * Implements hook_ENTITY_TYPE_presave().
- */
-function commerce_checkout_commerce_order_presave(OrderInterface $order) {
-  if (!$order->hasField('checkout_step')) {
-    return;
-  }
-
-  // Lock the order while on the 'payment' checkout step.
-  $checkout_step = $order->get('checkout_step')->value;
-  $original_order = $order->original;
-  $previous_checkout_step = $original_order ? $original_order->get('checkout_step')->value : '';
-  if ($checkout_step == 'payment' && $previous_checkout_step != 'payment') {
-    $order->lock();
-  }
-  elseif ($checkout_step != 'payment' && $previous_checkout_step == 'payment') {
-    $order->unlock();
-  }
-}
-
 /**
  * Implements hook_form_FORM_ID_alter() for 'commerce_order_type_form'.
  */
diff --git a/modules/checkout/src/Plugin/Commerce/CheckoutFlow/CheckoutFlowBase.php b/modules/checkout/src/Plugin/Commerce/CheckoutFlow/CheckoutFlowBase.php
@@ -126,11 +126,9 @@ public function getNextStepId($step_id) {
    */
   public function redirectToStep($step_id) {
     $this->order->set('checkout_step', $step_id);
-    if ($step_id == 'complete') {
-      $transition = $this->order->getState()->getWorkflow()->getTransition('place');
-      $this->order->getState()->applyTransition($transition);
-    }
+    $this->onStepChange($step_id);
     $this->order->save();
+
     throw new NeedsRedirectException(Url::fromRoute('commerce_checkout.form', [
       'commerce_order' => $this->order->id(),
       'step' => $step_id,
@@ -291,21 +289,45 @@ public function validateForm(array &$form, FormStateInterface $form_state) {}
   public function submitForm(array &$form, FormStateInterface $form_state) {
     if ($next_step_id = $this->getNextStepId($form['#step_id'])) {
       $this->order->set('checkout_step', $next_step_id);
+      $this->onStepChange($next_step_id);
+
       $form_state->setRedirect('commerce_checkout.form', [
         'commerce_order' => $this->order->id(),
         'step' => $next_step_id,
       ]);
-
-      if ($next_step_id == 'complete') {
-        // Place the order.
-        $transition = $this->order->getState()->getWorkflow()->getTransition('place');
-        $this->order->getState()->applyTransition($transition);
-      }
     }
 
     $this->order->save();
   }
 
+  /**
+   * Reacts to the current step changing.
+   *
+   * Called before saving the order and redirecting.
+   *
+   * Handles the following logic
+   * 1) Locks the order before the payment page,
+   * 2) Unlocks the order when leaving the payment page
+   * 3) Places the order before the complete page.
+   *
+   * @param string $step_id
+   *   The new step ID.
+   */
+  protected function onStepChange($step_id) {
+    // Lock the order while on the 'payment' checkout step. Unlock elsewhere.
+    if ($step_id == 'payment') {
+      $this->order->lock();
+    }
+    elseif ($step_id != 'payment') {
+      $this->order->unlock();
+    }
+    // Place the order.
+    if ($step_id == 'complete') {
+      $transition = $this->order->getState()->getWorkflow()->getTransition('place');
+      $this->order->getState()->applyTransition($transition);
+    }
+  }
+
   /**
    * Gets whether the given step has a sidebar.
    *
diff --git a/modules/payment/src/Plugin/Commerce/CheckoutPane/PaymentProcess.php b/modules/payment/src/Plugin/Commerce/CheckoutPane/PaymentProcess.php
@@ -2,7 +2,6 @@
 
 namespace Drupal\commerce_payment\Plugin\Commerce\CheckoutPane;
 
-use Drupal\commerce\Response\NeedsRedirectException;
 use Drupal\commerce_checkout\Plugin\Commerce\CheckoutFlow\CheckoutFlowInterface;
 use Drupal\commerce_checkout\Plugin\Commerce\CheckoutPane\CheckoutPaneBase;
 use Drupal\commerce_payment\Exception\DeclineException;
@@ -200,8 +199,8 @@ public function buildPaneForm(array $pane_form, FormStateInterface $form_state,
       $complete_form['actions']['next']['#value'] = $this->t('Proceed to @gateway', [
         '@gateway' => $payment_gateway_plugin->getDisplayLabel(),
       ]);
-      // The 'Go back' link needs to use the cancel URL to ensure that the
-      // order is unlocked when the customer is sent to the previous page.
+      // Make sure that the payment gateway's onCancel() method is invoked,
+      // by pointing the "Go back" link to the cancel URL.
       $complete_form['actions']['next']['#suffix'] = Link::fromTextAndUrl($this->t('Go back'), $this->buildCancelUrl())->toString();
       // Hide the actions by default, they are not needed by gateways that
       // embed iframes or redirect via GET. The offsite-payment form can
@@ -272,7 +271,8 @@ protected function buildPaymentInformationStepUrl() {
    * @throws \Drupal\commerce\Response\NeedsRedirectException
    */
   protected function redirectToPreviousStep() {
-    throw new NeedsRedirectException($this->buildPaymentInformationStepUrl()->toString());
+    $step_id = $this->checkoutFlow->getPane('payment_information')->getStepId();
+    return $this->checkoutFlow->redirectToStep($step_id);
   }
 
 }
diff --git a/modules/payment/tests/src/FunctionalJavascript/PaymentCheckoutTest.php b/modules/payment/tests/src/FunctionalJavascript/PaymentCheckoutTest.php
@@ -261,7 +261,7 @@ public function testCheckoutWithExistingPaymentMethod() {
     $order = Order::load(1);
     $this->assertEquals('onsite', $order->get('payment_gateway')->target_id);
     $this->assertEquals('1', $order->get('payment_method')->target_id);
-
+    $this->assertFalse($order->isLocked());
     // Verify that a payment was created.
     $payment = Payment::load(1);
     $this->assertNotNull($payment);
@@ -315,7 +315,7 @@ public function testCheckoutWithNewPaymentMethod() {
     $payment_method = $order->get('payment_method')->entity;
     $this->assertEquals('1881', $payment_method->get('card_number')->value);
     $this->assertEquals('123 New York Drive', $payment_method->getBillingProfile()->get('address')->address_line1);
-
+    $this->assertFalse($order->isLocked());
     // Verify that a payment was created.
     $payment = Payment::load(1);
     $this->assertNotNull($payment);
@@ -354,6 +354,8 @@ public function testCheckoutWithDeclinedPaymentMethod() {
     $this->assertSession()->pageTextContains('We encountered an error processing your payment method. Please verify your details and try again.');
     $this->assertSession()->addressEquals('checkout/1/order_information');
 
+    $order = Order::load(1);
+    $this->assertFalse($order->isLocked());
     // Verify a payment was not created.
     $payment = Payment::load(1);
     $this->assertNull($payment);
@@ -384,9 +386,63 @@ public function testCheckoutWithOffsiteRedirectPost() {
     $this->assertSession()->pageTextContains('123 New York Drive');
     $this->submitForm([], 'Pay and complete purchase');
     $this->assertSession()->pageTextContains('Your order number is 1. You can view your order on your account page when logged in.');
+
     $order = Order::load(1);
     $this->assertEquals('offsite', $order->get('payment_gateway')->target_id);
+    $this->assertFalse($order->isLocked());
+    // Verify that a payment was created.
+    $payment = Payment::load(1);
+    $this->assertNotNull($payment);
+    $this->assertEquals($payment->getAmount(), $order->getTotalPrice());
+  }
+
+  /**
+   * Tests checkout with an off-site gateway (POST redirect method, manual).
+   *
+   * In this scenario the customer must click the submit button on the payment
+   * page in order to proceed to the gateway.
+   */
+  public function testCheckoutWithOffsiteRedirectPostManual() {
+    $payment_gateway = PaymentGateway::load('offsite');
+    $payment_gateway->getPlugin()->setConfiguration([
+      'redirect_method' => 'post_manual',
+      'payment_method_types' => ['credit_card'],
+    ]);
+    $payment_gateway->save();
 
+    $this->drupalGet($this->product->toUrl()->toString());
+    $this->submitForm([], 'Add to cart');
+    $this->drupalGet('checkout/1');
+    $radio_button = $this->getSession()->getPage()->findField('Example');
+    $radio_button->click();
+    $this->waitForAjaxToFinish();
+
+    $this->submitForm([
+      'payment_information[billing_information][address][0][address][given_name]' => 'Johnny',
+      'payment_information[billing_information][address][0][address][family_name]' => 'Appleseed',
+      'payment_information[billing_information][address][0][address][address_line1]' => '123 New York Drive',
+      'payment_information[billing_information][address][0][address][locality]' => 'New York City',
+      'payment_information[billing_information][address][0][address][administrative_area]' => 'NY',
+      'payment_information[billing_information][address][0][address][postal_code]' => '10001',
+    ], 'Continue to review');
+    $this->assertSession()->pageTextContains('Payment information');
+    $this->assertSession()->pageTextContains('Example');
+    $this->assertSession()->pageTextContains('Johnny Appleseed');
+    $this->assertSession()->pageTextContains('123 New York Drive');
+    $this->submitForm([], 'Pay and complete purchase');
+
+    $this->assertSession()->addressEquals('checkout/1/payment');
+    $order = Order::load(1);
+    $this->assertEquals('offsite', $order->get('payment_gateway')->target_id);
+    $this->assertTrue($order->isLocked());
+
+    $this->submitForm([], 'Proceed to Example');
+    $this->assertSession()->pageTextContains('Your order number is 1. You can view your order on your account page when logged in.');
+
+    \Drupal::entityTypeManager()->getStorage('commerce_order')->resetCache(['1']);
+    $order = Order::load(1);
+    $this->assertEquals('offsite', $order->get('payment_gateway')->target_id);
+    $this->assertFalse($order->isLocked());
     // Verify that a payment was created.
     $payment = Payment::load(1);
     $this->assertNotNull($payment);
@@ -431,9 +487,10 @@ public function testCheckoutWithOffsiteRedirectGet() {
     $this->assertSession()->pageTextContains('123 New York Drive');
     $this->submitForm([], 'Pay and complete purchase');
     $this->assertSession()->pageTextContains('Your order number is 1. You can view your order on your account page when logged in.');
+
     $order = Order::load(1);
     $this->assertEquals('offsite', $order->get('payment_gateway')->target_id);
-
+    $this->assertFalse($order->isLocked());
     // Verify that a payment was created.
     $payment = Payment::load(1);
     $this->assertNotNull($payment);
@@ -477,6 +534,9 @@ public function testFailedCheckoutWithOffsiteRedirectGet() {
     $this->assertSession()->pageTextContains('We encountered an unexpected error processing your payment. Please try again later.');
     $this->assertSession()->addressEquals('checkout/1/order_information');
 
+    $order = Order::load(1);
+    // @todo Fix order unlocking in this situation.
+    // $this->assertFalse($order->isLocked());
     // Verify a payment was not created.
     $payment = Payment::load(1);
     $this->assertNull($payment);
@@ -508,9 +568,10 @@ public function testCheckoutWithManual() {
     $this->submitForm([], 'Pay and complete purchase');
     $this->assertSession()->pageTextContains('Your order number is 1. You can view your order on your account page when logged in.');
     $this->assertSession()->pageTextContains('Sample payment instructions.');
+
     $order = Order::load(1);
     $this->assertEquals('manual', $order->get('payment_gateway')->target_id);
-
+    $this->assertFalse($order->isLocked());
     // Verify that a payment was created.
     $payment = Payment::load(1);
     $this->assertNotNull($payment);
diff --git a/modules/payment_example/src/Plugin/Commerce/PaymentGateway/OffsiteRedirect.php b/modules/payment_example/src/Plugin/Commerce/PaymentGateway/OffsiteRedirect.php
@@ -47,7 +47,8 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
       '#title' => $this->t('Redirect method'),
       '#options' => [
         'get' => $this->t('Redirect via GET (302 header)'),
-        'post' => $this->t('Redirect via POST'),
+        'post' => $this->t('Redirect via POST (automatic)'),
+        'post_manual' => $this->t('Redirect via POST (manual)'),
       ],
       '#default_value' => $this->configuration['redirect_method'],
     ];
diff --git a/modules/payment_example/src/PluginForm/OffsiteRedirect/PaymentOffsiteForm.php b/modules/payment_example/src/PluginForm/OffsiteRedirect/PaymentOffsiteForm.php
@@ -20,8 +20,10 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
     /** @var \Drupal\commerce_payment\Plugin\Commerce\PaymentGateway\OffsitePaymentGatewayInterface $payment_gateway_plugin */
     $payment_gateway_plugin = $payment->getPaymentGateway()->getPlugin();
     $redirect_method = $payment_gateway_plugin->getConfiguration()['redirect_method'];
-    if ($redirect_method == 'post') {
+    $remove_js = ($redirect_method == 'post_manual');
+    if (in_array($redirect_method, ['post', 'post_manual'])) {
       $redirect_url = Url::fromRoute('commerce_payment_example.dummy_redirect_post')->toString();
+      $redirect_method = 'post';
     }
     else {
       // Gateways that use the GET redirect method usually perform an API call
@@ -44,7 +46,13 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
       'total' => $payment->getAmount()->getNumber(),
     ];
 
-    return $this->buildRedirectForm($form, $form_state, $redirect_url, $data, $redirect_method);
+    $form = $this->buildRedirectForm($form, $form_state, $redirect_url, $data, $redirect_method);
+    if ($remove_js) {
+      // Disable the javascript that auto-clicks the Submit button.
+      unset($form['#attached']['library']);
+    }
+
+    return $form;
   }
 
 }
