invoiceninja / admin-portal

Invoice Ninja: Desktop/mobile admin portal built with Flutter
https://demo.invoiceninja.com
Other
1.6k stars 535 forks source link

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

Open EnriqCG opened 1 year ago

EnriqCG commented 1 year ago

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.

https://github.com/invoiceninja/admin-portal/blob/adc96d334eb2408a92d60f90d0bfc9833be6abd6/lib/ui/reports/invoice_report.dart#L218-L220

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 commented 1 year ago

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 commented 1 year ago

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

hillelcoren commented 1 year ago

@turbo124 I believe these are the required changes:

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

hillelcoren commented 1 year ago

@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:

turbo124 commented 1 year ago

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

hillelcoren commented 1 year ago

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

turbo124 commented 1 year ago

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