Skip to content
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

Store balance transaction ID in order metadata #7945

Merged
merged 21 commits into from
Jan 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
f33af70
Streamline the params in attach_intent_info_to_order
anu-rock Dec 27, 2023
fbc8664
Save balance transaction id to order meta while processing a payment
anu-rock Dec 27, 2023
7f9fc28
Add changelog
anu-rock Dec 27, 2023
1bf3da7
Merge branch 'develop' into update/7856-add-charge-txn-id-to-order-meta
anu-rock Dec 27, 2023
83d5af8
Replace the value of balance transaction in create charge helper with…
anu-rock Dec 27, 2023
d73ab09
Fix psalm error UndefinedMethod
anu-rock Jan 2, 2024
5f1a297
Merge branch 'develop' into update/7856-add-charge-txn-id-to-order-meta
anu-rock Jan 2, 2024
8c2f80f
Update method call in the main gateway class
anu-rock Jan 2, 2024
1e58fcc
Merge branch 'develop' into update/7856-add-charge-txn-id-to-order-meta
mordeth Jan 8, 2024
a7bef87
Remove code duplication between the two attach intent methods
anu-rock Jan 11, 2024
adc9a67
Prepend wcpay to the meta key
anu-rock Jan 11, 2024
1f4681a
Remove the unnecessary variable in test
anu-rock Jan 11, 2024
69c82bd
Use null coalescing instead of ternary operator
anu-rock Jan 11, 2024
bb87366
Revert "Remove the unnecessary variable in test"
anu-rock Jan 11, 2024
4d9d729
Merge branch 'develop' into update/7856-add-charge-txn-id-to-order-meta
dpaun1985 Jan 15, 2024
e745e71
Prevent overwriting order meta after successful payment
anu-rock Jan 16, 2024
2af5d52
Add test for the new setter method
anu-rock Jan 16, 2024
8022a4a
Save payment transaction id in the new order service
anu-rock Jan 16, 2024
edd6872
Fix the undefined variable error
anu-rock Jan 16, 2024
16f83e8
Merge branch 'develop' into update/7856-add-charge-txn-id-to-order-meta
anu-rock Jan 18, 2024
d3d2655
Merge branch 'develop' into update/7856-add-charge-txn-id-to-order-meta
anu-rock Jan 18, 2024
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/update-7856-add-charge-txn-id-to-order-meta
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: update

Store balance transaction ID in order metadata.
14 changes: 1 addition & 13 deletions includes/admin/class-wc-rest-payments-orders-controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -189,19 +189,7 @@ public function capture_terminal_payment( WP_REST_Request $request ) {
// Update the order: set the payment method and attach intent attributes.
$order->set_payment_method( WC_Payment_Gateway_WCPay::GATEWAY_ID );
$order->set_payment_method_title( __( 'WooCommerce In-Person Payments', 'woocommerce-payments' ) );
$intent_id = $intent->get_id();
$intent_status = $intent->get_status();
$charge = $intent->get_charge();
$charge_id = $charge ? $charge->get_id() : null;
$this->order_service->attach_intent_info_to_order(
$order,
$intent_id,
$intent_status,
$intent->get_payment_method_id(),
$intent->get_customer_id(),
$charge_id,
$intent->get_currency()
);
$this->order_service->attach_intent_info_to_order( $order, $intent );
$this->order_service->update_order_status_from_intent( $order, $intent );

// Certain payments (eg. Interac) are captured on the client-side (mobile app).
Expand Down
6 changes: 3 additions & 3 deletions includes/class-wc-payment-gateway-wcpay.php
Original file line number Diff line number Diff line change
Expand Up @@ -1689,7 +1689,7 @@ public function process_payment_for_order( $cart, $payment_information, $schedul
}
}

$this->order_service->attach_intent_info_to_order( $order, $intent_id, $status, $payment_method, $customer_id, $charge_id, $currency );
$this->order_service->attach_intent_info_to_order( $order, $intent );
$this->attach_exchange_info_to_order( $order, $charge_id );
if ( Intent_Status::SUCCEEDED === $status ) {
$this->duplicate_payment_prevention_service->remove_session_processing_order( $order->get_id() );
Expand Down Expand Up @@ -1884,7 +1884,7 @@ public function process_redirect_payment( $order, $intent_id, $save_payment_meth
}
}

$this->order_service->attach_intent_info_to_order( $order, $intent_id, $status, $payment_method_id, $customer_id, $charge_id, $currency );
$this->order_service->attach_intent_info_to_order( $order, $intent );
$this->attach_exchange_info_to_order( $order, $charge_id );
if ( Intent_Status::SUCCEEDED === $status ) {
$this->duplicate_payment_prevention_service->remove_session_processing_order( $order->get_id() );
Expand Down Expand Up @@ -3425,7 +3425,7 @@ public function update_order_status() {
$charge_id = ! empty( $charge ) ? $charge->get_id() : null;

$this->attach_exchange_info_to_order( $order, $charge_id );
$this->order_service->attach_intent_info_to_order( $order, $intent_id, $status, $intent->get_payment_method_id(), $intent->get_customer_id(), $charge_id, $intent->get_currency() );
$this->order_service->attach_intent_info_to_order( $order, $intent );
$this->order_service->attach_transaction_fee_to_order( $order, $charge );
} else {
// For $0 orders, fetch the Setup Intent instead.
Expand Down
57 changes: 56 additions & 1 deletion includes/class-wc-payments-order-service.php
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,13 @@ class WC_Payments_Order_Service {
*/
const WCPAY_MODE_META_KEY = '_wcpay_mode';

/**
* Meta key used to store payment transaction Id.
*
* @const string
*/
const WCPAY_PAYMENT_TRANSACTION_ID_META_KEY = '_wcpay_payment_transaction_id';

/**
* Client for making requests to the WooCommerce Payments API
*
Expand Down Expand Up @@ -531,6 +538,23 @@ public function set_charge_id_for_order( $order, $charge_id ) {
$order->save_meta_data();
}

/**
* Set the payment metadata for payment transaction id.
*
* @param mixed $order The order.
* @param string $payment_transaction_id The value to be set.
*
* @throws Order_Not_Found_Exception
*/
public function set_payment_transaction_id_for_order( $order, $payment_transaction_id ) {
if ( ! isset( $payment_transaction_id ) || null === $payment_transaction_id ) {
return;
}
$order = $this->get_order( $order );
$order->update_meta_data( self::WCPAY_PAYMENT_TRANSACTION_ID_META_KEY, $payment_transaction_id );
$order->save_meta_data();
}

/**
* Get the payment metadata for charge id.
*
Expand Down Expand Up @@ -773,17 +797,47 @@ public function get_fraud_meta_box_type_for_order( $order ) : string {
/**
* Given the payment intent data, adds it to the given order as metadata and parses any notes that need to be added
*
* @param WC_Order $order The order.
* @param WC_Payments_API_Payment_Intention|WC_Payments_API_Setup_Intention $intent The payment or setup intention object.
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 was getting Psalm UndefinedMethod linting error, so replaced the abstract class type hint with a list of possible concrete types. I wonder if there's a proper way to use abstract types as type hints without Psalm complaining.

*
* @throws Order_Not_Found_Exception
*/
public function attach_intent_info_to_order( WC_Order $order, $intent ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

part of this method ( calling the setters and calling the save method) is identical to attach_intent_info_to_order__legacy .
This method extracts the data that needs to be added to the order, and saves it on the order. attach_intent_info_to_order__legacy only saves the information to the order.
We can keep this method to extract the data and call the attach_intent_info_to_order__legacy in this method.
So, we could:

  1. change this method name to add_intent_info_to_order
  2. keep the old name for attach_intent_info_to_order__legacy ( attach_intent_info_to_order )
  3. call attach_intent_info_to_order in this method

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this, common code can be extracted to a separate function as part of this refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dpaun1985 @naman03malhotra Thanks for the feedback. I fixed the code duplication in a7bef87.

In the long term, I think it's desirable to remove the legacy method and keep only the refactored one. This will allow us to care about only one method when adding new intent properties to order meta.

change this method name to add_intent_info_to_order

Unfortunately, I cannot change the name as I'd like my changes to apply to all uses of this method.

// We don't want to allow metadata for a successful payment to be disrupted.
if ( Intent_Status::SUCCEEDED === $this->get_intention_status_for_order( $order ) ) {
return;
}
// first, let's prepare all the metadata needed for refunds, required for status change etc.
$intent_id = $intent->get_id();
$intent_status = $intent->get_status();
$payment_method = $intent->get_payment_method_id();
$customer_id = $intent->get_customer_id();
$currency = $intent instanceof WC_Payments_API_Payment_Intention ? $intent->get_currency() : $order->get_currency();
$charge = $intent instanceof WC_Payments_API_Payment_Intention ? $intent->get_charge() : null;
$charge_id = $charge ? $charge->get_id() : null;
$payment_transaction = $charge ? $charge->get_balance_transaction() : null;
$payment_transaction_id = $payment_transaction['id'] ?? '';
// next, save it in order meta.
$this->attach_intent_info_to_order__legacy( $order, $intent_id, $intent_status, $payment_method, $customer_id, $charge_id, $currency, $payment_transaction_id );
}

/**
* Legacy version of the attach_intent_info_to_order method.
*
* TODO: This method should ultimately be merged with `attach_intent_info_to_order` and then removed.
*
* @param WC_Order $order The order.
* @param string $intent_id The intent ID.
* @param string $intent_status Intent status.
* @param string $payment_method Payment method ID.
* @param string $customer_id Customer ID.
* @param string $charge_id Charge ID.
* @param string $currency Currency code.
* @param string $payment_transaction_id The transaction ID of the linked charge.
*
* @throws Order_Not_Found_Exception
*/
public function attach_intent_info_to_order( $order, $intent_id, $intent_status, $payment_method, $customer_id, $charge_id, $currency ) {
public function attach_intent_info_to_order__legacy( $order, $intent_id, $intent_status, $payment_method, $customer_id, $charge_id, $currency, $payment_transaction_id = null ) {
// first, let's save all the metadata that needed for refunds, required for status change etc.
$order->set_transaction_id( $intent_id );
$this->set_intent_id_for_order( $order, $intent_id );
Expand All @@ -792,6 +846,7 @@ public function attach_intent_info_to_order( $order, $intent_id, $intent_status,
$this->set_intention_status_for_order( $order, $intent_status );
$this->set_customer_id_for_order( $order, $customer_id );
$this->set_wcpay_intent_currency_for_order( $order, $currency );
$this->set_payment_transaction_id_for_order( $order, $payment_transaction_id );
$order->save();
}

Expand Down
10 changes: 1 addition & 9 deletions includes/subscriptions/class-wc-payments-invoice-service.php
Original file line number Diff line number Diff line change
Expand Up @@ -299,15 +299,7 @@ public function get_and_attach_intent_info_to_order( $order, $intent_id ) {

$charge = $intent_object->get_charge();

$this->order_service->attach_intent_info_to_order(
$order,
$intent_id,
$intent_object->get_status(),
$intent_object->get_payment_method_id(),
$intent_object->get_customer_id(),
$charge ? $charge->get_id() : null,
$intent_object->get_currency()
);
$this->order_service->attach_intent_info_to_order( $order, $intent_object );
}

/**
Expand Down
18 changes: 11 additions & 7 deletions src/Internal/Service/OrderService.php
Original file line number Diff line number Diff line change
Expand Up @@ -186,21 +186,25 @@ public function update_order_from_successful_intent(
) {
$order = $this->get_order( $order_id );

$charge = null;
$charge_id = null;
$charge = null;
$charge_id = null;
$payment_transaction_id = null;
if ( $intent instanceof WC_Payments_API_Payment_Intention ) {
$charge = $intent->get_charge();
$charge_id = $intent->get_charge()->get_id();
$charge = $intent->get_charge();
$charge_id = $intent->get_charge()->get_id();
$payment_transaction = $charge ? $charge->get_balance_transaction() : null;
$payment_transaction_id = $payment_transaction['id'] ?? '';
}

$this->legacy_service->attach_intent_info_to_order(
$this->legacy_service->attach_intent_info_to_order__legacy(
$order,
$intent->get_id(),
$intent->get_status(),
$context->get_payment_method()->get_id(),
$context->get_customer_id(),
$charge_id,
$context->get_currency()
$context->get_currency(),
$payment_transaction_id,
);

$this->legacy_service->attach_transaction_fee_to_order( $order, $charge );
Expand Down Expand Up @@ -253,7 +257,7 @@ public function update_order_from_intent_that_requires_action(
) {
$order = $this->get_order( $order_id );

$this->legacy_service->attach_intent_info_to_order(
$this->legacy_service->attach_intent_info_to_order__legacy(
$order,
$intent->get_id(),
$intent->get_status(),
Expand Down
21 changes: 3 additions & 18 deletions tests/unit/admin/test-class-wc-rest-payments-orders-controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,7 @@ public function test_capture_terminal_payment_success() {
->method( 'attach_intent_info_to_order' )
->with(
$this->isInstanceOf( WC_Order::class ),
$this->mock_intent_id,
Intent_Status::REQUIRES_CAPTURE,
'pm_mock',
'cus_mock',
$this->mock_charge_id,
'USD'
$mock_intent,
);

$request = new WP_REST_Request( 'POST' );
Expand Down Expand Up @@ -171,12 +166,7 @@ public function test_capture_terminal_payment_succeeded_intent() {
->method( 'attach_intent_info_to_order' )
->with(
$this->isInstanceOf( WC_Order::class ),
$this->mock_intent_id,
Intent_Status::SUCCEEDED,
'pm_mock',
'cus_mock',
$this->mock_charge_id,
'USD'
$mock_intent,
);

$this->mock_gateway
Expand Down Expand Up @@ -236,12 +226,7 @@ public function test_capture_terminal_payment_completed_order() {
->method( 'attach_intent_info_to_order' )
->with(
$this->isInstanceOf( WC_Order::class ),
$this->mock_intent_id,
Intent_Status::SUCCEEDED,
'pm_mock',
'cus_mock',
$this->mock_charge_id,
'USD'
$mock_intent,
);

$this->mock_gateway
Expand Down
10 changes: 9 additions & 1 deletion tests/unit/helpers/class-wc-helper-intention.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,15 @@ public static function create_charge( $data = [] ) {
'amount_captured' => 5000,
'amount_refunded' => 0,
'application_fee_amount' => 0,
'balance_transaction' => 'txn_mock',
'balance_transaction' => [
'id' => 'txn_mock',
'amount' => 5000,
'available_on' => 1703808000,
'created' => new DateTime( '2022-05-20 19:05:38' ),
'currency' => 'usd',
'exchange_rate' => null,
'fee' => 82,
],
'billing_details' => [],
'currency' => 'usd',
'dispute' => [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -621,7 +621,7 @@ public function test_process_redirect_setup_intent_succeded() {

$setup_intent = WC_Helper_Intention::create_setup_intention(
[
'id' => 'pi_mock',
'id' => $intent_id,
'client_secret' => $client_secret,
'status' => $intent_status,
'payment_method' => $payment_method_id,
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/src/Internal/Service/OrderServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ public function test_update_order_from_successful_intent( $intent ) {
->willReturn( $mock_charge );
}

// Prepare all parameters for `attach_intent_info_to_order`.
// Prepare all parameters for `attach_intent_info_to_order__legacy`.
$intent->expects( $this->once() )
->method( 'get_id' )
->willReturn( $intent_id );
Expand All @@ -355,7 +355,7 @@ public function test_update_order_from_successful_intent( $intent ) {
->willReturn( 'prod' );

$this->mock_legacy_service->expects( $this->once() )
->method( 'attach_intent_info_to_order' )
->method( 'attach_intent_info_to_order__legacy' )
->with(
$mock_order,
$intent_id,
Expand Down Expand Up @@ -412,7 +412,7 @@ public function test_update_order_from_intent_that_requires_action() {
$mock_intent->expects( $this->once() )->method( 'get_status' )->willReturn( $intent_status );

$this->mock_legacy_service->expects( $this->once() )
->method( 'attach_intent_info_to_order' )
->method( 'attach_intent_info_to_order__legacy' )
->with(
$mock_order,
$intent_id,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ public function test_intent_status_success() {
$this->mock_order_service
->expects( $this->once() )
->method( 'attach_intent_info_to_order' )
->with( $mock_order, $intent_id, $status, 'pm_mock', $customer_id, $charge_id, 'USD' );
->with( $mock_order, $intent );

$this->mock_order_service
->expects( $this->once() )
Expand Down Expand Up @@ -471,7 +471,7 @@ public function test_intent_status_requires_capture() {
$this->mock_order_service
->expects( $this->once() )
->method( 'attach_intent_info_to_order' )
->with( $mock_order, $intent_id, $status, 'pm_mock', $customer_id, $charge_id, 'USD' );
->with( $mock_order, $intent );

// Assert: The Order_Service is called correctly.
$this->mock_order_service
Expand Down Expand Up @@ -919,7 +919,7 @@ public function test_intent_status_requires_action() {
$this->mock_order_service
->expects( $this->once() )
->method( 'attach_intent_info_to_order' )
->with( $mock_order, $intent_id, $status, 'pm_mock', $customer_id, $charge_id, 'USD' );
->with( $mock_order, $intent );

$this->mock_order_service
->expects( $this->once() )
Expand Down Expand Up @@ -1035,7 +1035,7 @@ public function test_setup_intent_status_requires_action() {
$this->mock_order_service
->expects( $this->once() )
->method( 'attach_intent_info_to_order' )
->with( $mock_order, $intent_id, $status, 'pm_mock', $customer_id, '', 'USD' );
->with( $mock_order, $intent );

// Assert: Order status was not updated.
$mock_order
Expand Down
36 changes: 29 additions & 7 deletions tests/unit/test-class-wc-payments-order-service.php
Original file line number Diff line number Diff line change
Expand Up @@ -1211,18 +1211,40 @@ public function test_get_fraud_meta_box_type() {
$this->assertEquals( $fraud_meta_box_type_from_service, $fraud_meta_box_type );
}

public function test_set_payment_transaction_id_for_order() {
$transaction_id = 'txn_mock';
$this->order_service->set_payment_transaction_id_for_order( $this->order, $transaction_id );
$this->assertSame( $this->order->get_meta( '_wcpay_payment_transaction_id', true ), $transaction_id );
}

public function test_attach_intent_info_to_order() {
$intent_id = 'pi_mock';
$intent_status = 'succeeded';
$payment_method = 'woocommerce_payments';
$customer_id = 'cus_12345';
$charge_id = 'ch_mock';
$currency = 'USD';
$this->order_service->attach_intent_info_to_order( $this->order, $intent_id, $intent_status, $payment_method, $customer_id, $charge_id, $currency );
$intent_id = 'pi_mock';
$intent = WC_Helper_Intention::create_intention( [ 'id' => $intent_id ] );
$this->order_service->attach_intent_info_to_order( $this->order, $intent );

$this->assertEquals( $intent_id, $this->order->get_meta( '_intent_id', true ) );
}

public function test_attach_intent_info_to_order_after_successful_payment() {
$intent = WC_Helper_Intention::create_intention(
[
'id' => 'pi_mock',
'status' => Intent_Status::SUCCEEDED,
]
);
$this->order_service->attach_intent_info_to_order( $this->order, $intent );

$another_intent = WC_Helper_Intention::create_intention(
[
'id' => 'pi_mock_2',
'status' => Intent_Status::CANCELED,
]
);
$this->order_service->attach_intent_info_to_order( $this->order, $another_intent );

$this->assertEquals( Intent_Status::SUCCEEDED, $this->order->get_meta( '_intention_status', true ) );
}

/**
* Several methods use the private method get_order to get the order being worked on. If an order is not found
* then an exception is thrown. This test attempt to confirm that exception gets thrown.
Expand Down
Loading