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

Inconsistent use of Exchange Rate in Invoices vs. Payments #588

Open
EnriqCG opened this issue Jul 18, 2023 · 7 comments
Open

Inconsistent use of Exchange Rate in Invoices vs. Payments #588

EnriqCG opened this issue Jul 18, 2023 · 7 comments
Labels
bug Something isn't working

Comments

@EnriqCG
Copy link

EnriqCG commented Jul 18, 2023

I've observed an inconsistency in the application of the Exchange Rate when calculating the final "Converted Balance" value. This inconsistency is noticeable when comparing the Payments section to the Invoice report.

Example

Let's say my primary currency is EUR and I invoice an international client in USD, specifically USD 10,000. I need to manage the currency conversion for reporting purposes to my local tax authority.

Behavior in Payments

The invoice is issued in USD, but I record the Payment I receive in EUR, accounting for currency conversions, fees, etc. The application correctly calculates the exchange rate as (EUR 9204 / USD 10000) = 0.9204.

image

And correctly shows up the Original Balance AND Converted Balance columns in the Payments table.

image

✅ This is the expected behavior

Behavior in Invoices

After obtaining the exchange rate from the payment, I can then input this rate in the Invoice > Settings.

image

However, when I navigate to the Invoice Reports tab, I find that the exchange rate is presented in an inverse format (1 / x). This representation causes an inconsistency in the Converted Balance when compared to the Payments table. To rectify this, I could adjust the Invoice Exchange Rate to its inverse form (1 / 0.9204 = 1.0865). However, this would result in the Payment and the Invoice having two different exchange rate values, which, to my understanding, is not the intended outcome.

image

Looking at the code, I find lines that manage the Invoice Report. I observe that the inverse of the exchange rate is used to determine the final value. Is this the expected behavior? If so, I don't understand why it would be beneficial to input the inverse of the Exchange Rate in the Invoice Settings.

case InvoiceReportFields.converted_amount:
value = round(invoice.amount * 1 / invoice.exchangeRate, 2);
break;

I realize that addressing this might result in a breaking change, but before I proceed with opening a pull request, I wanted to inquire whether this inconsistency was intentionally designed or not.

@hillelcoren
Copy link
Member

I believe the current approach is inconsistent but correct for each case, @EnriqCG does that align with what you're seeing?

Correcting this would require backend work, @turbo124 do you think this is worth the effort to correct? If not now maybe at some point in the future.

@turbo124
Copy link
Member

@hillelcoren can you provide a spec for what the backend needs to do here please.

@hillelcoren
Copy link
Member

@turbo124 I believe these are the required changes:

  • Run a database migration to set the payment exchange rate to 1 divided by the current value
  • Adjust the Flutter and React apps to use the reversed rate

One challenge here is the backend and apps will all need to be updated at the same time.

@hillelcoren
Copy link
Member

hillelcoren commented Jul 19, 2023

@turbo124 I've given this a bit more thought, there are two problems with the suggestion above:

  1. The changes need to be coordinated
  2. The Flutter app persists data locally which will become invalid

Here's an adjusted process to account for this:

  • Add a "payment_exchange_rate_corrected" bool (default false) to the account transformer
  • Adjust the Flutter and React app to use this bool to calculate the rate
  • Run the database migration to correct the data and set the bool to true
  • The Flutter app would check for changes to the bool and when found would force refresh all data
  • Once we assume everyone has updated we can remove the bool
  • Note: the v4/v5 migration would also need to be adjusted

@hillelcoren hillelcoren added the bug Something isn't working label Jul 19, 2023
@turbo124
Copy link
Member

If a user has not updated all their client apps, they would potentially corrupt their dataset if they continue using older clients, correct?

@hillelcoren
Copy link
Member

I believe we could use the x-minimum-client-version header in the response to prevent this.

@turbo124
Copy link
Member

This was my thought also you user the API to the prevent older clients from causing any corruption

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants