Page MenuHomePhorge

Convert to foreign currencies using openexchangerates.org
ClosedPublic

Authored by mollekopf on Mar 19 2021, 5:25 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 24, 10:48 PM
Unknown Object (File)
Sat, Mar 23, 1:22 AM
Unknown Object (File)
Tue, Mar 12, 8:49 AM
Unknown Object (File)
Mon, Mar 11, 10:13 PM
Unknown Object (File)
Feb 9 2024, 8:55 PM
Unknown Object (File)
Feb 9 2024, 8:19 PM
Unknown Object (File)
Feb 7 2024, 9:46 PM
Unknown Object (File)
Jan 24 2024, 11:46 PM
Subscribers
None

Diff Detail

Repository
rK kolab
Branch
dev/paymentdialog
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 33775
Build 13378: arc lint + arc unit

Event Timeline

mollekopf created this revision.

This now just converts directly using the exchange rate from openexchangerates.org, without applying any safety margin.

I removed the "Application Currency" from the config because it doesn't seem to make sense in our case (we have per wallet currencies), and I assume it was initially added due to some copy/paste work. May be that some other cleanup could be done in that config file.

Exchange rates are cached for 24h so we can live on the free OpenExchangeRates plan (1000 per month)

machniak added inline comments.
src/app/Backends/OpenExchangeRates.php
6

I was going to say that it would probably be better to move it to app\Providers, but thinking more, I'm not sure. Maybe it's ok and we should rather move payment providers code here, and leave app\Providers for the Laravel framework stuff only.

10

Missing argument doc.

15

Short array syntax, please (here and below). Shouldn't it be 'base' => $base_currency?

48

I don't remember if we ever do both Log::error() and throw. Looks redundant.

src/app/Providers/Payment/Mollie.php
565–573

A space after the cast would be nice.

src/app/Utils.php
416

Again, the phpdoc formatting is not correct.

433

I think Jeroen and I would prefer to do this differently. When fetching the exchange rates we rather use cron and store the rates in a file (or DB). This way a failing request to openexchangerates website does not cause issues for our users. In such cases we'd just use the old rates instead of throwing an exception.

src/config/currency.php
16

This config file was added when Jeroen did some testing with "torann/currency" package. We got rid of it since, but forgot about the file. So, you can remove what we don't need from it.

This revision now requires changes to proceed.Mar 22 2021, 10:26 AM
src/app/Utils.php
433

So is that a data:import:exchangerate artisan command?

How do I store and load something like the exchangerates in the db best?

src/app/Utils.php
433

I think I could follow either what ip4net does and store it in the db or what countriescommand does and store it in a file. Up to you.

src/app/Utils.php
433

I think a file would be fine.

Import conversion rates with a command

mollekopf marked 8 inline comments as done.

Addressed remaining comments

Looks good to me. Two things:

  1. Some tests would be nice.
  2. In bin/quickstart.php we should probably run the command if the api key is set somewhere. And/or maybe we should check-in some already fetched data into the repository. It will be needed for tests.

@vanmeeuwen should also review this.

This revision now requires changes to proceed.Mar 23 2021, 11:46 AM
mollekopf retitled this revision from Initial OpenExchangeRates support to Convert to foreign currencies using openexchangerates.org.
mollekopf edited the summary of this revision. (Show Details)

Checked in the generated exchangerates-CHF.php file.

I'll add some tests based on this data for conversion between CHF and EUR.

Test foreign currency payments and fixed refund handling

src/app/Backends/OpenExchangeRates.php
6

Moving payment providers to backends could make sense indeed.

15

The base currency for the request is fixed to USD for the free plan. That's why we calculate the conversion rate manually below.

machniak added inline comments.
src/app/Providers/PaymentProvider.php
226

This code does not look nice. We should make exchange() to be able to work with any exchange direction. Actually exchangeRate() should.

Also, how about adding a couple of simple tests for exchange() and/or exchangeRate() itself? Also maybe remove exchangeRate() from the payment provider and use the one from Utils directly, the wrapper should not be needed.

This revision now requires changes to proceed.Mar 25 2021, 9:24 AM
  • Move the openexchangerates api key to where the other api keys are
  • Load payment method whitelist from config
mollekopf marked an inline comment as done.

Simplified and tested reverse currency conversion

I was expecting a single whitelist. We don't really need two, do we?

This revision is now accepted and ready to land.Apr 6 2021, 11:56 AM
This revision was automatically updated to reflect the committed changes.