Page MenuHomekolab.org

Convert to foreign currencies using openexchangerates.org
ClosedPublic

Authored by mollekopf on Mar 19 2021, 5:25 PM.

Diff Detail

Repository
rK kolab
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mollekopf requested review of this revision.Mar 19 2021, 5:25 PM
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 requested changes to this revision.Mar 22 2021, 10:26 AM
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
403

Again, the phpdoc formatting is not correct.

420

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
mollekopf added inline comments.Mar 22 2021, 11:16 AM
src/app/Utils.php
420

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

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

mollekopf added inline comments.Mar 22 2021, 11:19 AM
src/app/Utils.php
420

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.

machniak added inline comments.Mar 22 2021, 11:45 AM
src/app/Utils.php
420

I think a file would be fine.

mollekopf updated this revision to Diff 6688.Mar 23 2021, 10:38 AM

Import conversion rates with a command

mollekopf updated this revision to Diff 6691.Mar 23 2021, 10:45 AM
mollekopf marked 8 inline comments as done.

Addressed remaining comments

machniak requested changes to this revision.Mar 23 2021, 11:46 AM

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 updated this revision to Diff 6703.EditedMar 23 2021, 12:48 PM
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.

mollekopf updated this revision to Diff 6742.Mar 24 2021, 12:33 PM

Test foreign currency payments and fixed refund handling

mollekopf added inline comments.Mar 24 2021, 4:12 PM
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 requested changes to this revision.Mar 25 2021, 9:24 AM
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
mollekopf updated this revision to Diff 6787.Apr 2 2021, 9:10 AM
  • Move the openexchangerates api key to where the other api keys are
  • Load payment method whitelist from config
mollekopf updated this revision to Diff 6793.Apr 2 2021, 10:04 AM
mollekopf marked an inline comment as done.

Simplified and tested reverse currency conversion

machniak accepted this revision.Apr 6 2021, 11:56 AM

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.