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

Cancel on expiry and duplication of payments #121

Merged
merged 2 commits into from
Oct 10, 2024
Merged

Conversation

makotom
Copy link
Contributor

@makotom makotom commented Jul 31, 2024

Konbini payments are unique: There's a considerable time gap between the timing that a payment is ready for capture (payment codes issued, or "authorized" in KOMOJU) and the timing of actual capture. For this purpose, the KOMOJU Payment plugin puts a WooCommerce order pending or on hold when a Konbini payment is "authorized".

During the period that orders on WooCommerce are pending or on-hold, however, WooCommerce allows shoppers to make another payment for the same order, through the page called order-pay (/checkout/order-pay - cf https://github.com/woocommerce/woocommerce/blob/4318dac7ecd6547a983148273fd2e278cbb18547/docs/getting-started/woocommerce-endpoints.md#L18). Due to this nature, there are multiple problematic scenarios that can happen, including:

  • duplicated payments, needing refunds,
  • payments can be recreated even after the Konbini payment code has expired, and time-limited offers can be "life-extended" improperly

This PR adds countermeasures against these scenarios. Particularly, it amends the codes to:

  • call set_transaction_id much earlier, so that expired webhook events are handled correctly,
  • it attempts to save komoju_payment_id (through save_komoju_meta_data) much earlier, for easier detection of duplicated payments, and
  • it attempts to cancel a preceding payment if another payment is being created for the same order.

@Dinwy
Copy link
Collaborator

Dinwy commented Aug 2, 2024

This is really a good catch! User can pay with Konbini and later they can change the payment method....

Screenshot 2024-08-02 at 13 45 41

I believe we should prevent this and cancel the previous order and placing a new order when payment
And I found a failing scenario when testing it,

  • When payment amount is the same as before, it is not cancel -> create new order, but cancel -> overring the old order.

I will explain why this can be a problem,

  1. Pay with Konbini

image

  1. You go to shop and buy same amount(=price) of the item, but pay with different payment method.

image

  1. Previous order has been cancelled, (In this case Add wp store assets #26). But when you click Pay then
Screenshot 2024-08-02 at 13 44 22

the preivous order become processing.


Q. Why this is problem?

  1. If the user wanted to pay with Konbini with ¥100.
  2. After that, user decide to pay $100 with Pay, but the user want to order ¥100 with Konbini and ¥100 with Paypay.
  3. We override the preveious, the old oder is now gone.

So, this should only applied who went to through /checkout/order-pay address. Whoever really want to override their previous payment not who want to pay with 2 different method.

I also check with different total amount,

Screenshot 2024-08-02 at 13 59 53

It is not cancelling the previous order. So it only happens when user try to order with the same amount based on the test.
Could you check this?

@makotom
Copy link
Contributor Author

makotom commented Aug 2, 2024

@Dinwy Thank you for your review, and that's a great catch!

HOWEVER - I have a bad news... I can reproduce this behaviour with the stable release of the plugin version 3.1.4! 😹
Let's put this PR on hold, and focus on fixing this new issue! I'd describe it as "if the order content is same except for payment methods, WC tries to reuse an pre-existing pending order".

@makotom
Copy link
Contributor Author

makotom commented Aug 5, 2024

WRT the point that a payment for another new checkout will pay for an existing payment, it turned out that this was caused by this sequence of stack:

Specifically these portions:

// https://github.com/woocommerce/woocommerce/blob/6c22840dcf313e053f9e20a50a75ef728b2b9d21/plugins/woocommerce/src/StoreApi/Utilities/DraftOrderTrait.php#L63-L66
		// Pending and failed orders can be retried if the cart hasn't changed.
		if ( $order_object->needs_payment() && $order_object->has_cart_hash( wc()->cart->get_cart_hash() ) ) {
			return true;
		}
// https://github.com/woocommerce/woocommerce/blob/6c22840dcf313e053f9e20a50a75ef728b2b9d21/plugins/woocommerce/src/StoreApi/Routes/V1/Checkout.php#L425-L431
		$this->order = $this->get_draft_order();

		if ( ! $this->order ) {
			$this->order = $this->order_controller->create_order_from_cart();
		} else {
			$this->order_controller->update_order_from_cart( $this->order, true );
		}

With these codes, WooCommerce avoids to create a new order and instead tries to reuse a pre-existing one, if the contents of the shopping carts are identical ($order_object->has_cart_hash( wc()->cart->get_cart_hash() )).
I researched if there's a straightforward way to get different card hash e.g. based on the timing, but apparently the cart hash considers the item ID and amount only, and it will get the same hash unless we add a bogus item.

As such, my conclusion is that:

  • It's a by-design behaviour of WooCommerce. The changes in this PR aren't contributing to production of this issue.
  • There is no feasible way to hack the behaviour unfortunately.

@Dinwy I appreciate your another review, with this in your mind 🙏 There can be another corner case, and I truly appreciate your scrutiny!

@Dinwy Dinwy self-assigned this Oct 7, 2024
@Dinwy
Copy link
Collaborator

Dinwy commented Oct 10, 2024

LGTM

Thank you for the work! I checked again and looks good to me.

@Dinwy Dinwy merged commit a58985b into master Oct 10, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants