Skip to content

EUR currency was getting saved in all refunds for non EUR currencies.#255

Open
knit-pay wants to merge 4 commits into
pronamic:mainfrom
knit-pay:patch-1
Open

EUR currency was getting saved in all refunds for non EUR currencies.#255
knit-pay wants to merge 4 commits into
pronamic:mainfrom
knit-pay:patch-1

Conversation

@knit-pay

@knit-pay knit-pay commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Prevent the default Money currency (EUR) from leaking into the refunded amount for non-EUR payments.

Prevent the default Money currency (EUR) from leaking into the refunded amount for non-EUR payments.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request updates src/Plugin.php to prevent the default EUR currency from leaking into refunded amounts for non-EUR payments by explicitly setting the refunded amount's currency to match the payment's total amount currency. The reviewer pointed out a broader issue where unrefunded non-EUR payments will still default to 0 EUR due to how the refunded amount is initialized in the Payment class. They recommend refactoring Payment::get_refunded_amount() in a follow-up to dynamically set the currency and resolve this display bug globally.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/Plugin.php Outdated
Comment on lines +1151 to +1153
// Prevent the default Money currency (EUR) from leaking into the refunded amount for non-EUR payments.
$refunded_amount->set_currency( $payment->get_total_amount()->get_currency() );

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

While this fix successfully prevents the default EUR currency from leaking into the refunded amount when a refund is created, there is a broader issue: any non-EUR payment (e.g., USD) that has not been refunded yet will still return 0 EUR when $payment->get_refunded_amount() is called. This is because refunded_amount is initialized as new Money() (defaulting to EUR) in Payment::__construct and is never updated unless a refund is processed.

This can lead to UI display bugs where unrefunded USD payments show a refunded amount of €0,00 instead of $0.00 in dashboards or reports.

Recommendation

A more robust and centralized solution would be to ensure that the refunded amount's currency always matches the payment's total amount currency directly within the Payment class. For example, we could update Payment::get_refunded_amount() to dynamically set its currency if it doesn't match the total amount's currency:

public function get_refunded_amount() {
	if ( $this->refunded_amount->get_currency() !== $this->get_total_amount()->get_currency() ) {
		$this->refunded_amount->set_currency( $this->get_total_amount()->get_currency() );
	}

	return $this->refunded_amount;
}

Since Payment.php is not part of this pull request's diff, we can keep this local fix in Plugin.php for now, but it is highly recommended to refactor this in the Payment class as a follow-up to fix the display bug for unrefunded payments.

@remcotolsma

Copy link
Copy Markdown
Member

@copilot It does not seem correct to me to update the currency when a refund currency does not match the currency of the original payment.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes a currency-mismatch issue where a refunded amount could retain the default Money currency (EUR) for non-EUR payments by attempting to align the refunded amount currency with the payment total amount currency.

Changes:

  • Adds a currency alignment check in Payment::get_refunded_amount() to prevent EUR from “leaking” into non-EUR refunded amounts.

Comment thread src/Payments/Payment.php
Comment thread src/Payments/Payment.php Outdated
@remcotolsma

remcotolsma commented Jun 11, 2026

Copy link
Copy Markdown
Member

@knit-pay we prefer to avoid this kind of logic in a get method, see my earlier comment:

It does not seem correct to me to update the currency when a refund currency does not match the currency of the original payment.

When creating refunds, the correct refund currency must be set.

It is inconvenient that when creating a new payment, this is set to EUR by default by calling new Money() in the constructor:

/**
* Construct and initialize payment object.
*
* @param integer $post_id A payment post ID or null.
*/
public function __construct( $post_id = null ) {
parent::__construct( $post_id );
$this->meta_key_prefix = '_pronamic_payment_';
$this->subscriptions = [];
$this->set_status( PaymentStatus::OPEN );
$this->set_total_amount( new Money() );
$this->refunded_amount = new Money();
if ( null !== $post_id ) {
pronamic_pay_plugin()->payments_data_store->read( $this );
}
}

It is also inconvenient that we potentially just add different currencies together:

wp-pay-core/src/Plugin.php

Lines 1126 to 1161 in 8627aaa

/**
* Create refund.
*
* @param Refund $refund Refund.
* @return void
* @throws \Exception Throws exception on error.
*/
public static function create_refund( Refund $refund ) {
$payment = $refund->get_payment();
$gateway = $payment->get_gateway();
if ( null === $gateway ) {
throw new \Exception(
\esc_html__( 'Unable to process refund as gateway could not be found.', 'pronamic_ideal' )
);
}
try {
$gateway->create_refund( $refund );
$payment->refunds[] = $refund;
$refunded_amount = $payment->get_refunded_amount();
$refunded_amount = $refunded_amount->add( $refund->get_amount() );
$payment->set_refunded_amount( $refunded_amount );
} catch ( \Exception $exception ) {
$payment->add_note( $exception->getMessage() );
throw $exception;
} finally {
$payment->save();
}
}

Actually, you can't just add different currency amounts together; that might be a design flaw in https://github.com/pronamic/wp-money/blob/f2762d981b1d6ab8739b6f74a9f5ff21bd759efc/src/Money.php#L275-L286.

In https://github.com/moneyphp/money/blob/d49ee625c6ba79b9d7a228ce153b02fc1032152b/src/Money.php#L199-L220, for example, that is also not possible and results in a CurrencyMismatchException.

@knit-pay

Copy link
Copy Markdown
Contributor Author

Thanks @remcotolsma

I will check this and update the code accordingly. Or you may also do it, if it's a small change and if you have already checked how to do it properly.

@knit-pay

Copy link
Copy Markdown
Contributor Author

Today I got some time to review the comments and code again.

You are right about this. I also noticed this while reviewing the code today. I raised the issue at pronamic/wp-money#11

Actually, you can't just add different currency amounts together; that might be a design flaw in https://github.com/pronamic/wp-money/blob/f2762d981b1d6ab8739b6f74a9f5ff21bd759efc/src/Money.php#L275-L286.

You are right about this. It was my mistake to blindly follow the suggestion by Gemini and implement what it said during the review (#255 (review)). At first glance it was looking fine.

@knit-pay we prefer to avoid this kind of logic in a get method, see my earlier comment:

@knit-pay

Copy link
Copy Markdown
Contributor Author

@remcotolsma

Kindly check the new logic. What is your opinion on this?

@remcotolsma

Copy link
Copy Markdown
Member

Thanks for the update. After analyzing the problem, we've arrived at the following insights (with the help of AI):

1. How problematic is 0 EUR vs 0 USD really?

Mathematically, a zero amount has the same value regardless of currency. The problem only surfaces when you try to add this to an amount with a different currency:

$payment->get_refunded_amount()->add( $refund->get_amount() );
// 0 EUR + 50 USD = ❌ Currency mismatch

This is the real issue—not the fact that the initial refunded_amount is in EUR.

2. Current approach feels like patching

The current PR adds the same logic in two places:

  • src/Plugin.php in create_refund()
  • views/meta-box-payment-lines.php (UI rendering)

This feels defensive and scattered. If we add this logic, we'd prefer it to exist in one central place.

3. Single Responsibility Principle

We considered solving this in set_total_amount():

public function set_total_amount( Money $total_amount ) {
    $this->total_amount = $total_amount;
    // Auto-sync refunded_amount currency
}

This works, but doesn't feel quite right—a setter for total_amount shouldn't automatically update refunded_amount. That violates SRP.

Better approach

When creating and adding refunds, the correct currency should simply be set by the code responsible for doing so:

// In create_refund() or refund-creation flow:
$refund = new Refund( $payment, new Money( $amount, $payment->get_total_amount()->get_currency() ) );

This way:

  • ✅ Currency is correct from the start
  • ✅ No "magic" in setters
  • ✅ Responsibility lies with refund-creation code
  • ✅ Simpler and more explicit

What's your take on this?

@knit-pay

Copy link
Copy Markdown
Contributor Author

Hello @remcotolsma
Thanks for your review. Earlier, I forgot to explain the code change. I will do it now.

When creating and adding refunds, the correct currency should simply be set by the code responsible for doing so:

In WooCommerce, we are already passing the currency during refund. kindly refer
https://github.com/pronamic/wp-pronamic-pay-woocommerce/blob/9011600c8d5c206d2bd7dd9c08e97a37bc4c3de2/src/Gateway.php#L988:L1003

As per my understanding in the code below, these are the meaning of the variables used. Kindly correct me if I am wrong.
https://github.com/pronamic/wp-pay-core/blob/2d14a84f10df72fb528b993689ab9bf8361cd836/src/Plugin.php#L1133:L1161

  1. $refund -> is the current refund, having the correct currency code passed by WooCommerce or any other platform.
  2. $refunded_amount -> is the total of past refunds. Default currency is EUR, not set by WooCommerce. The value of the past refunded amount will be zero if not refunded in the past.
  3. $payment -> is current payment, having correct currency.

Because $refunded_amount is not set by WooCommerce, we are setting this in (https://github.com/pronamic/wp-pay-core/pull/255/changes)

			if ( $refunded_amount->is_zero() && $refunded_amount->get_currency() !== $refund->get_amount()->get_currency() ) {
				$refunded_amount->set_currency( $refund->get_amount()->get_currency() );
			}

Before setting the currency, I am considering 2 things.

  1. We can get currency from $payment and pass it to $refunded_amount. But I am assuming that, some plugins/payment gateway will have option to refund in different currency than the payment currency. For Example if payment is done in INR, payment gateway or plugins might allow admin to refund in USD. That's why instead of setting $payment currency in $refunded_amount, I am setting $refund currency ie $refund->get_amount()->get_currency() in $refunded_amount.
  2. As per my opinion, we should se $refunded_amount currency to $refund->get_amount()->get_currency() only if the $payment is not partially refunded in the past ie $refunded_amount->is_zero(). It means, currency of second refund should be equest to first currency otherwise addition of the refund will not be possible and we will get the CurrencyMismatchException from
			$refunded_amount = $refunded_amount->add( $refund->get_amount() );

I hope this explaination would help you understand the changes in a better way. What is your opinion on this? Do you still think we need to change anything?

@remcotolsma

Copy link
Copy Markdown
Member

Yes, I think we’re aligned on the immediate issue.
I’m leaning toward a slightly different, more structural fix: the Payment constructor currently initializes amounts with a default EUR currency, which is part of the root cause.

What do you think about this approach?

  1. Add an optional total_amount parameter to Payment::__construct(...).
  2. If provided, use its currency to initialize refunded_amount.
  3. In WooCommerce, pass the correct amount/currency directly when creating Payment (new Payment(...)).
  4. In Payment::from_json(...), pass total_amount into new self(...) when available.

This keeps the currency setup centralized in Payment, avoids patching in multiple call sites, and still keeps current behavior as fallback when total_amount is not provided.

@knit-pay

knit-pay commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

@remcotolsma, I agree, your structural approach is better. It fixes the root cause (EUR default in Payment::__construct) rather than patching at refund time. I have below concerns about this approach. Kindly share your opinion on this. I think we will still have to keep the changes in the file meta-box-payment-lines.php; without this, we may see incorrect currency on the payment edit page.
https://github.com/pronamic/wp-pay-core/pull/255/changes#diff-df877ce89d056af690d4f471162298eea13b7f067eaacf0b7da4c6e05238a371
What is your suggestion on this?

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.

3 participants