Skip to content

RPP: Added error states #7739

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

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions changelog/rpp-7425-handle-error-states
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: add

Added error states in new payment processing flow.
5 changes: 5 additions & 0 deletions includes/class-wc-payment-gateway-wcpay.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
use WCPay\Duplicate_Payment_Prevention_Service;
use WCPay\Fraud_Prevention\Fraud_Prevention_Service;
use WCPay\Fraud_Prevention\Fraud_Risk_Tools;
use WCPay\Internal\Payment\State\AbstractPaymentErrorState;
use WCPay\Internal\Payment\State\AuthenticationRequiredState;
use WCPay\Internal\Payment\State\DuplicateOrderDetectedState;
use WCPay\Internal\Service\DuplicatePaymentPreventionService;
Expand Down Expand Up @@ -886,6 +887,10 @@ public function new_process_payment( WC_Order $order ) {
];
}

if ( $state instanceof AbstractPaymentErrorState ) {
return $state->handle_error_state();
}

throw new Exception( __( 'The payment process could not be completed.', 'woocommerce-payments' ) );
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@
use WCPay\Internal\Payment\State\CompletedState;
use WCPay\Internal\Payment\State\InitialState;
use WCPay\Internal\Payment\State\PaymentErrorState;
use WCPay\Internal\Payment\State\PaymentRequestErrorState;
use WCPay\Internal\Payment\State\ProcessedState;
use WCPay\Internal\Payment\State\DuplicateOrderDetectedState;
use WCPay\Internal\Payment\State\StateFactory;
use WCPay\Internal\Payment\State\SystemErrorState;
use WCPay\Internal\Payment\State\WooPaymentsApiServerErrorState;
use WCPay\Internal\Proxy\HooksProxy;
use WCPay\Internal\Proxy\LegacyProxy;
use WCPay\Internal\Service\MinimumAmountService;
Expand Down Expand Up @@ -113,10 +115,24 @@ public function register(): void {
->addArgument( StateFactory::class );

$container->add( SystemErrorState::class )
->addArgument( StateFactory::class );
->addArgument( StateFactory::class )
->addArgument( Logger::class )
->addArgument( OrderService::class );

$container->add( PaymentRequestErrorState::class )
->addArgument( StateFactory::class )
->addArgument( Logger::class )
->addArgument( OrderService::class );

$container->add( WooPaymentsApiServerErrorState::class )
->addArgument( StateFactory::class )
->addArgument( Logger::class )
->addArgument( OrderService::class );

$container->add( PaymentErrorState::class )
->addArgument( StateFactory::class );
->addArgument( StateFactory::class )
->addArgument( Logger::class )
->addArgument( OrderService::class );

$container->add( DuplicateOrderDetectedState::class )
->addArgument( StateFactory::class );
Expand Down
25 changes: 25 additions & 0 deletions src/Internal/Payment/PaymentContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,13 @@ class PaymentContext {
*/
private $transitions = [];

/**
* Stores exception that is triggered before error state is created.
*
* @var \Exception
*/
private $exception;
Comment on lines +40 to +45
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like request classes, there is no need to define a property of the object: It's all stored within data. With that in mind, this one is not used, and can be removed.


/**
* Constructs the class, receiving an order ID.
*
Expand Down Expand Up @@ -300,6 +307,24 @@ public function get_transitions(): array {
return $this->transitions;
}

/**
* Stores state exception.
*
* @param \Exception $exception Exception to store.
*/
public function set_exception( $exception ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not insisting on this, but I'd prefer to call those set_last_exception and get_last_exception, similarly to how there's a $wpdb->last_error.

Also, please add a proper type hint to the method:

Suggested change
public function set_exception( $exception ) {
public function set_exception( Exception $exception ) {

Similar,y get_(last_)exception() should return : ?Exception

$this->set( 'exception', $exception );
}

/**
* Get state exception
*
* @return \Exception|null
*/
public function get_exception() {
return $this->get( 'exception' );
}

/**
* Sets the mode (test or prod).
*
Expand Down
91 changes: 91 additions & 0 deletions src/Internal/Payment/State/AbstractPaymentErrorState.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
<?php
/**
* Class AbstractPaymentErrorState
*
* @package WooCommerce\Payments
*/

namespace WCPay\Internal\Payment\State;

use Exception;
use WCPay\Internal\Logger;
use WCPay\Internal\Service\OrderService;

/**
* Base abstract class for all error states that will share common error state logic.
*/
abstract class AbstractPaymentErrorState extends AbstractPaymentState {

/**
* Logger instance.
*
* @var Logger
*/
private $logger;

/**
* Order service.
*
* @var OrderService
*/
private $order_service;

/**
* Class constructor.
*
* @param StateFactory $state_factory State factory.
* @param Logger $logger Logger service.
* @param OrderService $order_service Order service.
*/
public function __construct( StateFactory $state_factory, Logger $logger, OrderService $order_service ) {
parent::__construct( $state_factory );
$this->logger = $logger;
$this->order_service = $order_service;
}

/**
* Handle error state.
*
* @throws Exception
*/
public function handle_error_state() {
$context = $this->get_context();
$order_id = $context->get_order_id();
$exception = $context->get_exception();
if ( $this->should_log_error() ) {
$this->logger->error( "Failed to process order with ID: $order_id . Reason: " . $exception );
}

if ( $this->should_mark_order_as_failed() ) {
$reason = '';
if ( $exception ) {
$reason = $exception->getMessage();
}
$this->order_service->mark_order_as_failed( $order_id, $reason );
}

// After everything is done, just throw the same exception and that's it. Gateway will pick it up and do its own thing.
if ( null !== $exception ) {
throw $exception;
}
throw new Exception( __( 'The payment process could not be completed.', 'woocommerce-payments' ) );
}

/**
* Determines whether an error should be logged.
*
* @return bool True if the error should be logged, otherwise false.
*/
protected function should_log_error(): bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't all errors be logged by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they need to, but I wasn't 100% certain, so I made it to false. We can easily set that all error states needs to be logged, by the default.

return false;
}
Comment on lines +74 to +81
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logging of the whole context is already built, and specific errors (ex. API errors) are logged already. I'd remove the logging functionality from erroneous states altogether.


/**
* Determines whether the order should be marked as failed.
*
* @return bool True if the order should be marked as failed, otherwise false.
*/
protected function should_mark_order_as_failed(): bool {
return false;
}
}
23 changes: 18 additions & 5 deletions src/Internal/Payment/State/AbstractPaymentState.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@

namespace WCPay\Internal\Payment\State;

use Exception;
use WCPay\Vendor\League\Container\Exception\ContainerException;
use WCPay\Internal\Payment\Exception\StateTransitionException;
use WCPay\Internal\Payment\PaymentContext;
use WCPay\Internal\Payment\PaymentRequest;
use WCPay\Internal\Payment\PaymentRequestException;
use WCPay\Exceptions\Order_Not_Found_Exception;

/**
* Base class for payment states.
Expand Down Expand Up @@ -75,8 +74,6 @@ public function get_context(): PaymentContext {
*
* @throws StateTransitionException In case the completed state could not be initialized.
* @throws ContainerException When the dependency container cannot instantiate the state.
* @throws Order_Not_Found_Exception Order could not be found.
* @throws PaymentRequestException When data is not available or invalid.
*/
public function start_processing( PaymentRequest $request ) {
$this->throw_unavailable_method_exception( __METHOD__ );
Expand All @@ -88,7 +85,6 @@ public function start_processing( PaymentRequest $request ) {
* @psalm-suppress InvalidReturnType
*
* @return AbstractPaymentState
* @throws Order_Not_Found_Exception
* @throws StateTransitionException
*/
public function complete_processing() {
Expand Down Expand Up @@ -119,6 +115,23 @@ protected function create_state( string $state_class ) {
return $state;
}

/**
* Create error state function. Almost same as original create state, but it also stores occurred exception.
*
* @param string $state_class State class.
* @param Exception $exception Occurred exception that triggered error state change.
*
* @return AbstractPaymentState
*
* @throws StateTransitionException
* @throws ContainerException
*/
protected function create_error_state( string $state_class, $exception ) {
$this->context->set_exception( $exception );
return $this->create_state( $state_class );

}

/**
* Throws an exception, indicating that a given method is not available.
*
Expand Down
112 changes: 59 additions & 53 deletions src/Internal/Payment/State/InitialState.php
Original file line number Diff line number Diff line change
Expand Up @@ -106,49 +106,44 @@ public function __construct(
*
* @param PaymentRequest $request The incoming payment processing request.
*
* @return AbstractPaymentState The next state.
* @throws StateTransitionException In case the completed state could not be initialized.
* @throws ContainerException When the dependency container cannot instantiate the state.
* @throws Order_Not_Found_Exception Order could not be found.
* @throws PaymentRequestException When data is not available or invalid.
* @throws API_Exception When server request fails.
* @throws Amount_Too_Small_Exception When the order amount is too small.
* @return AbstractPaymentState The next state.
* @throws StateTransitionException In case the completed state could not be initialized.
* @throws ContainerException In case DI has exception.
*/
public function start_processing( PaymentRequest $request ) {
// Populate basic details from the request.
$this->populate_context_from_request( $request );

// Populate further details from the order.
$this->populate_context_from_order();

// Start multiple verification checks.
$this->process_order_phone_number();

$duplicate_order_result = $this->process_duplicate_order();
if ( null !== $duplicate_order_result ) {
return $duplicate_order_result;
}

$duplicate_payment_result = $this->process_duplicate_payment();
if ( null !== $duplicate_payment_result ) {
return $duplicate_payment_result;
}

$context = $this->get_context();
$this->minimum_amount_service->verify_amount(
$context->get_currency(),
$context->get_amount()
);
// End multiple verification checks.

/**
* Payments are based on intents, and intents use customer objects for billing details.
*
* The customer is created/updated right before requesting the creation of
* a payment intent, and the two actions must be adjacent to each-other.
*/
try {
$context = $this->get_context();
$this->populate_context_from_request( $request );
// Populate further details from the order.
$this->populate_context_from_order();

// Start multiple verification checks.
$this->process_order_phone_number();

$duplicate_order_result = $this->process_duplicate_order();
if ( null !== $duplicate_order_result ) {
return $duplicate_order_result;
}

$duplicate_payment_result = $this->process_duplicate_payment();
if ( null !== $duplicate_payment_result ) {
return $duplicate_payment_result;
}

/**
* Payments are based on intents, and intents use customer objects for billing details.
*
* The customer is created/updated right before requesting the creation of
* a payment intent, and the two actions must be adjacent to each-other.
*/
Comment on lines +133 to +138
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docblock is completely out of place now.


$context = $this->get_context();

$this->minimum_amount_service->verify_amount(
$context->get_currency(),
$context->get_amount()
);

$order_id = $context->get_order_id();

// Create or update customer and customer details.
Expand All @@ -161,23 +156,34 @@ public function start_processing( PaymentRequest $request ) {
// After customer is updated or created, make sure that intent is created.
$intent = $this->payment_request_service->create_intent( $context );
$context->set_intent( $intent );
} catch ( Amount_Too_Small_Exception $e ) {
$this->minimum_amount_service->store_amount_from_exception( $e );
throw $e;
} catch ( Invalid_Request_Parameter_Exception | Extend_Request_Exception | Immutable_Parameter_Exception $e ) {
return $this->create_state( SystemErrorState::class );
}

// Intent requires authorization (3DS check).
if ( Intent_Status::REQUIRES_ACTION === $intent->get_status() ) {
$this->order_service->update_order_from_intent_that_requires_action( $order_id, $intent, $context );
return $this->create_state( AuthenticationRequiredState::class );
}
// Intent requires authorization (3DS check).
if ( Intent_Status::REQUIRES_ACTION === $intent->get_status() ) {
$this->order_service->update_order_from_intent_that_requires_action( $order_id, $intent, $context );
return $this->create_state( AuthenticationRequiredState::class );
}

// All good. Proceed to processed state.
$next_state = $this->create_state( ProcessedState::class );
// All good. Proceed to processed state.
$next_state = $this->create_state( ProcessedState::class );

return $next_state->complete_processing();
return $next_state->complete_processing();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By positioning this line within the catch block of this state, we're essentially trying to handle all exceptions from the ProcessedState here as well. This goes against the idea of granularly handling exceptions.

} catch ( Amount_Too_Small_Exception $e ) {
$this->minimum_amount_service->store_amount_from_exception( $e );
return $this->create_error_state( SystemErrorState::class, $e );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The amount being too small is not a system exception, it happens whenever the amount to pay is less than the required minimum. This is not an indication that there's something wrong with the system, just a payment error.

} catch (
Invalid_Request_Parameter_Exception |
Extend_Request_Exception |
Immutable_Parameter_Exception |
Order_Not_Found_Exception |
StateTransitionException |
ContainerException $e
) {
return $this->create_error_state( SystemErrorState::class, $e );
} catch ( API_Exception $e ) {
return $this->create_error_state( WooPaymentsApiServerErrorState::class, $e );
} catch ( PaymentRequestException $e ) {
return $this->create_error_state( PaymentRequestErrorState::class, $e );
}
}

/**
Expand Down
10 changes: 9 additions & 1 deletion src/Internal/Payment/State/PaymentErrorState.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,14 @@
* Though this is an erroneous state, it is not unexpected, and
* represents an error on the buyer's side, which cannot be fixed through code.
*/
class PaymentErrorState extends AbstractPaymentState {
class PaymentErrorState extends AbstractPaymentErrorState {

/**
* Determines whether an error should be logged.
*
* @return bool True if the error should be logged, otherwise false.
*/
public function should_log_error() : bool {
return true;
}
}
Loading