Skip to content
Snippets Groups Projects

Issue #3112812: Add logging for payment failures during checkout

Merged Issue #3112812: Add logging for payment failures during checkout

Merge request reports

Merged results pipeline passed with warnings for 7509e2e1

Code Quality is loading
Test summary results are being parsed

Merged by Jonathan SacksickJonathan Sacksick Dec 6, 2023 (Dec 6, 2023 12:59pm UTC)

Loading

Pipeline #59958 passed with warnings

Pipeline passed with warnings for 9656d5f8 on 8.x-2.x

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • 19 * The event dispatcher.
    20 */
    21 protected EventDispatcherInterface $eventDispatcher;
    22
    23 /**
    24 * The order.
    25 */
    26 protected ?OrderInterface $order = NULL;
    27
    28 /**
    29 * {@inheritdoc}
    30 */
    31 public static function create(ContainerInterface $container) {
    32 $instance = parent::create($container);
    33 $instance->eventDispatcher = $container->get('event_dispatcher');
    34 $instance->order = $container->get('current_route_match')->getParameter('commerce_order');
  • 83 109 $payment_gateway_plugin->createPaymentMethod($payment_method, $values['payment_details']);
    84 110 }
    85 111 catch (DeclineException $e) {
    112 if (
    113 $this->order instanceof OrderInterface &&
    114 $payment_method->getPaymentGateway() instanceof PaymentGatewayInterface
    115 ) {
    116 $event = new FailedPaymentEvent($this->order, $payment_method->getPaymentGateway(), $e);
    117 $this->eventDispatcher->dispatch($event, PaymentEvents::PAYMENT_FAILURE);
  • 180 154 $this->checkoutFlow->redirectToStep($next_step_id);
    181 155 }
    182 156 catch (DeclineException $e) {
    157 $event = new FailedPaymentEvent($this->order, $payment_gateway, $e, $payment);
    158 $this->eventDispatcher->dispatch($event, PaymentEvents::PAYMENT_FAILURE);
  • Dmytrii Kaiun added 3 commits

    added 3 commits

    Compare with previous version

  • Dmytrii Kaiun added 1 commit

    added 1 commit

    Compare with previous version

  • 165 165 public function submitInlineForm(array &$inline_form, FormStateInterface $form_state) {
    166 166 parent::submitInlineForm($inline_form, $form_state);
    167 167
    168 $payment_gateway = $this->entity->getPaymentGateway();
    169 $order = $this->routeMatch->getParameter('commerce_order');
  • 232 212 $this->checkoutFlow->redirectToStep($next_step_id);
    233 213 }
    234 214 catch (PaymentGatewayException $e) {
    215 $event = new FailedPaymentEvent($this->order, $payment_gateway, $e, $payment);
  • 145 147 ])->save();
    146 148 }
    147 149
    150 /**
    151 * Creates a log when payment failed.
    152 *
    153 * @param \Drupal\commerce_payment\Event\FailedPaymentEvent $event
    154 * The failed payment event.
    155 */
    156 public function onPaymentFailure(FailedPaymentEvent $event): void {
    157 $this->logStorage->generate($event->getOrder(), 'payment_failed', [
    158 'error_message' => $event->getGatewayException()->getMessage(),
    159 'gateway' => $event->getPaymentGateway()->label(),
    160 'amount' => $event->getPayment()?->getBalance(),
    161 ])->save();
    • Comment on lines +157 to +161

      Can we also add the payment id to this information. This would allow reports to build a much detailed picture of what is going on here. Maybe we can look at \Drupal\commerce_log\EventSubscriber\PaymentEventSubscriber::onPaymentInsert() as inspiration... so something like:

      Suggested change
      162 $this->logStorage->generate($event->getOrder(), 'payment_failed', [
      163 'error_message' => $event->getGatewayException()->getMessage(),
      164 'gateway' => $event->getPaymentGateway()->label(),
      165 'amount' => $event->getPayment()?->getBalance(),
      166 ])->save();
      162 $this->logStorage->generate($event->getOrder(), 'payment_failed', [
      163 'id' => $payment->id(),
      164 'remote_id' => $payment->getRemoteId(),
      165 'method' => $payment->getPaymentMethod()?->label(),
      166 'error_message' => $event->getGatewayException()->getMessage(),
      167 'gateway' => $event->getPaymentGateway()->label(),
      168 'amount' => $event->getPayment()?->getBalance(),
      169 ])->save();
    • I guess the remote ID will probably not be there but no harm in including it. The reason I think we need more info here is that we'd like to be able to tell which cards are being declined.

      Pushed a fix in my latest commit.

    • Alex Pott changed this line in version 18 of the diff

      changed this line in version 18 of the diff

    • Please register or sign in to reply
  • 19 * Constructs a new FailedPaymentEvent.
    20 *
    21 * @param \Drupal\commerce_order\Entity\OrderInterface $order
    22 * The order entity.
    23 * @param \Drupal\commerce_payment\Entity\PaymentGatewayInterface $paymentGateway
    24 * The payment gateway.
    25 * @param \Drupal\commerce_payment\Exception\PaymentGatewayException $gatewayException
    26 * The payment gateway exception.
    27 * @param \Drupal\commerce_payment\Entity\PaymentInterface|null $payment
    28 * The payment.
    29 */
    30 public function __construct(
    31 protected OrderInterface $order,
    32 protected PaymentGatewayInterface $paymentGateway,
    33 protected PaymentGatewayException $gatewayException,
    34 protected ?PaymentInterface $payment = NULL
  • 72 98 $payment_method->save();
    73 99 }
    74 100 catch (DeclineException $e) {
    101 if (
    102 $this->order instanceof OrderInterface &&
    103 $payment_method->getPaymentGateway() instanceof PaymentGatewayInterface
    104 ) {
    105 $event = new FailedPaymentEvent($this->order, $payment_method->getPaymentGateway(), $e);
    106 $this->eventDispatcher->dispatch($event, PaymentEvents::PAYMENT_FAILURE);
    • Comment on lines +105 to +106

      Ah - here is a place without the payment - maybe we should be providing the payment method in some way here. So we can trace what is responsible for the error.

    • Alex Pott changed this line in version 22 of the diff

      changed this line in version 22 of the diff

    • Please register or sign in to reply
  • Alex Pott added 6 commits

    added 6 commits

    • fceaef13...e7f1f3f3 - 2 commits from branch project:8.x-2.x
    • 250c1fdd - Merge branch '8.x-2.x' into 3112812-add-logging-for
    • 615fae0e - Move to catch as suggested by @jsacksick
    • 33843a9c - refactor to use a single try/catch to make consistent logic easier to follow
    • dd741768 - Add more info to payment_failed log

    Compare with previous version

  • Alex Pott added 1 commit

    added 1 commit

    • 69686ff2 - Make the payment method edit form trigger event for all payment gateway exceptions

    Compare with previous version

  • added 9 commits

    • 69686ff2...f572bec1 - 3 commits from branch project:8.x-2.x
    • cb29f75c - Issue #3112812 by tBKoT: Add logging for failed payment
    • 461f11e7 - Issue #3112812 by tBKoT: Add proposed changes after review
    • 60cadfb5 - Move to catch as suggested by @jsacksick
    • 90ff70c1 - refactor to use a single try/catch to make consistent logic easier to follow
    • 6b6728e4 - Add more info to payment_failed log
    • 6bf7ba0e - Make the payment method edit form trigger event for all payment gateway exceptions

    Compare with previous version

  • Dmytrii Kaiun added 2 commits

    added 2 commits

    Compare with previous version

  • Dmytrii Kaiun added 1 commit

    added 1 commit

    Compare with previous version

  • Alex Pott added 6 commits

    added 6 commits

    Compare with previous version

  • Alex Pott added 1 commit

    added 1 commit

    • b164a8f2 - Allow payment method types to add additional details to the failed payment log

    Compare with previous version

  • Alex Pott added 1 commit

    added 1 commit

    • 2d5b1727 - Test FailedPaymentDetailsInterface

    Compare with previous version

  • Alex Pott added 1 commit
  • Alex Pott added 2 commits

    added 2 commits

    • b467a953 - 1 commit from branch project:8.x-2.x
    • 5d279cdb - Merge branch '8.x-2.x' into 3112812-add-logging-for

    Compare with previous version

  • Alex Pott added 3 commits

    added 3 commits

    Compare with previous version

  • Alex Pott added 1 commit

    added 1 commit

    Compare with previous version

  • Alex Pott added 1 commit

    added 1 commit

    • 603c49cd - This happens in the event so no need to do it here

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 4 commits

    • 5854e3e7...542b8be4 - 3 commits from branch project:8.x-2.x
    • 493c2f50 - Merge remote-tracking branch 'origin/8.x-2.x' into 3112812-add-logging-for

    Compare with previous version

  • Please register or sign in to reply
    Loading