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

Revise order statuses (Part 1, failing payment should result as order failed) #171

Merged
merged 3 commits into from
Jun 25, 2020

Conversation

guzzilar
Copy link
Contributor

@guzzilar guzzilar commented Jun 23, 2020

1. Objective

There is a query has been raised of the confusion regarding on how the Omise payment status maps with WooCommerce order status for the failure payment cases.

This pull request is aiming to revise the order status for the failure payment cases.

2. Description of change

  • Renaming 'Omise_Payment::payment_failed_no_order' method to 'invalid_order'
    As for the conclusion of the internal team about the confusion of order status when payment is failed. The first step first this commit is aiming is to differentiate between what method should count as 'payment failed' and whats not.

    Therefore the current 'Omise_Payment::payment_failed_no_order' giving an impression that the payment result is failed while it actually hasn't been started at this point.

Note, there are 2 places that using payment_failed_no_order.
Renaming here should be safe to go.
Screen Shot 2563-06-23 at 14 23 20

  • Updating order status to 'failed' when the payment result is failed in any circumstances)
    All the pending-payment order status will now be updated to failed if a particular payment transaction is resulted in failed status for all the payment methods.

Note, beside class-omise-callback, there are 4 files that are executing payment_failed method. The execution can be separated into 2 main use-cases.

  • When a particular Omise charge transaction gets failed status, the plugin triggers payment_failed(). 👈 This pull request is making this case to also update WC Order status to failed.
  • A fallback case when the plugin cannot validate a particular charge status. (see [6. Additional Notes])

Screen Shot 2563-06-23 at 14 24 18

3. Quality assurance

🔧 Environments:

  • WooCommerce: v3.9.3.
  • WordPress: v5.4.2.
  • PHP version: 7.3.3.

✏️ Details:

3.1 Making sure that the Omise_Payment::invalid_order() method is working properly.
To test this case, you will need to update the plugin code to make the plugin not be able to retrieve an order object properly:
At file: includes/gateway/class-omise-payment.php, line 185. Replacing $order_id with any string to make a false-id.
Screen Shot 2563-06-23 at 15 17 35

Then, try to place an order as normal.
The plugin will display a warning message on the checkout page screen saying that the payment cannot be processed.
Screen Shot 2563-06-23 at 15 16 12

As the plugin cannot retrieve a proper Order object, so the plugin will not be able to alter any of order status, however, there will be new order transaction created at Admin, WooCommerce Order page.
Screen Shot 2563-06-23 at 15 16 49

This is an edge case that normally won't happen unless there is an unexpected bug from WooCommerce itself or database went down right at this moment and the plugin cannot retrieve WooCommerce Order object properly. (as the $order_id variable is coming from WooCommerce itself).

3.2 Testing placing an order using Credit Card payment method. The result of failed-charge should update that particular Order status to failed.
To test the failure payment case, you must use any of those testing cards that make the charge failed. https://www.omise.co/api-testing#creating-failed-charges
Screen Shot 2563-06-23 at 15 31 33

Then, go through the checkout step as normal, placing an order using Credit Card payment method. There should be an error message displayed on your checkout page saying that the payment is failed.
Screen Shot 2563-06-23 at 15 32 30

A particular order status should be updated to failed accordingly.
Screen Shot 2563-06-23 at 15 38 47

3.3 Testing placing the same order using a proper test card to make the charge successful should result in order status changes from failed to processing.
Continuing from [3.2.], you may try to place an order again with a proper successful test card.
The order will be placed and redirect buyer to the order confirmation (thank-you page) properly.
Screen Shot 2563-06-23 at 15 43 18

Order status will be updated from failed to processing.
Screen Shot 2563-06-23 at 15 45 17

3.4 We may as well test for a manual capture case. Failing to capture an authorized charge should result in order status = failed.

To test this case, you must set the Credit Card payment Payment Action to Manual Capture.
Screen Shot 2563-06-23 at 16 11 28

As well, you must update the plugin code to simulate the capture-failure case.
At the file: includes/gateway/class-omise-payment-creditcard.php, line: 365. Commenting out line 364 and 366 to make sure that the program will go through the throw new Exception case.
Screen Shot 2563-06-24 at 00 08 51

You now then, may place a new order using Credit Card payment method.
Once done ordering, you may check your order at the Admiin order detail page. Then make a manual capture action.
Screen Shot 2563-06-24 at 00 15 45

The order status will be updated to Failed as the capture will be failed at this point.
Screen Shot 2563-06-24 at 00 16 14

4. Impact of the change

Order status flow changed, this may result in a confusion of the current merchants that are currently using, or adapting their way of order-management to the previous flow (mostly for credit card payment method).

5. Priority of change

Normal

6. Additional Notes

At the moment, the plugin is handling the "unknown" case as payment failed.
In fact, I think it would be better to update order status to on-hold for those unknown payment result cases (those fallback cases).

public function result( $order_id, $order, $charge ) {
	if ( self::STATUS_FAILED === $charge['status'] ) {
		return $this->payment_failed( $charge['failure_message'] . ' (code: ' . $charge['failure_code'] . ')' );
	}

	if ( self::STATUS_PENDING === $charge['status'] ) {
		$order->add_order_note( sprintf( __( 'Omise: Redirecting buyer to %s', 'omise' ), esc_url( $charge['authorize_uri'] ) ) );

		return array (
			'result'   => 'success',
			'redirect' => $charge['authorize_uri'],
		);
	}

	// This fallback case should be treat as "cannot validate the payment result"
	// The order status should be set as `on-hold` instead of treating it as `payment-failed`.
	return $this->payment_failed(
		sprintf(
			__( 'Please feel free to try submitting your order again, or contact our support team if you have any questions (Your temporary order id is \'%s\')', 'omise' ),
			$order_id
		)
	);
}

guzzilar added 2 commits June 23, 2020 14:40
…order'.

As for the conclusion of the internal team about the confusion of order status when payment is failed.
The first step first this commit is aiming is to differentiate between what method should count as 'payment failed' and whats not.
Therefore the current 'Omise_Payment::payment_failed_no_order' giving an impression that the payment result is failed while it actually hasn't been started at this point.
@guzzilar guzzilar force-pushed the revise-order-status-part1 branch from f1d847a to f50eb4f Compare June 23, 2020 08:36
__( 'Omise: Payment failed (manual capture).<br/>%s', 'omise' ),
array( 'br' => array() )
),
wp_kses( __( 'Omise: Payment failed (manual capture).<br/>%s', 'omise' ), array( 'br' => array() ) ),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, this line is just to remove unnecessary breakline to make it a little more easy to read. Nothing change in term of function.

$this->order()->add_order_note(
sprintf( __( 'Omise: Payment failed, %s', 'omise' ), $message )
);
$this->order()->add_order_note( sprintf( __( 'Omise: Payment failed, %s', 'omise' ), $message ) );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, this line is just to remove unnecessary breakline to make it a little more easy to read. Nothing change in term of function.

@guzzilar guzzilar marked this pull request as ready for review June 23, 2020 17:19
@guzzilar guzzilar changed the title [WIP] Revise order statuses (Part 1, failing payment should result as order failed) Revise order statuses (Part 1, failing payment should result as order failed) Jun 23, 2020
@guzzilar guzzilar requested a review from keeratita June 23, 2020 17:20
…ansactions (after redirecting back to the callback).
@guzzilar
Copy link
Contributor Author

Just pushed one new commit for a missing case of 3-D Secure enabled payment.
cbc48ba

This change removes the condition that marks order as failed only for Offsite payment, instead, the plugin will mark order status as failed for all the failure charge transactions that is redirected back to Callback (technically, Offsite payment, and 3-D Secure payment)

@danfowler danfowler self-requested a review June 25, 2020 05:53
@guzzilar guzzilar merged commit 250e2c0 into master Jun 25, 2020
@guzzilar guzzilar deleted the revise-order-status-part1 branch June 25, 2020 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants