Page MenuHomePhorge
Differential D2383

Convert to foreign currencies using openexchangerates.org
ClosedPublic
Actions

Authored by mollekopf on Mar 19 2021, 5:25 PM.
Tags
None
Referenced Files
F11777619: D2383.id6703.diff
Thu, Apr 18, 2:58 AM
Unknown Object (File)
Fri, Apr 12, 5:01 AM
Unknown Object (File)
Sun, Mar 24, 10:48 PM
Unknown Object (File)
Sat, Mar 23, 1:22 AM
Unknown Object (File)
Mar 12 2024, 8:49 AM
Unknown Object (File)
Mar 11 2024, 10:13 PM
Unknown Object (File)
Feb 9 2024, 8:55 PM
Unknown Object (File)
Feb 9 2024, 8:19 PM
View All 42 Files
Subscribers
None

Details

Reviewers
machniak
vanmeeuwen
Commits
rK69abc4fca320: Convert to foreign currencies using openexchangerates.org
rK0f05faf1a6c1: Convert to foreign currencies using openexchangerates.org
rK476f5b5041d0: Convert to foreign currencies using openexchangerates.org
Summary

This also enables EUR payment methods in mollie.

Diff Detail

Repository
rK kolab
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

mollekopf requested review of this revision.Mar 19 2021, 5:25 PM
mollekopf created this revision.
Harbormaster completed remote builds in B33625: Diff 6664.Mar 19 2021, 5:25 PM
mollekopf added a comment.Mar 19 2021, 5:29 PM

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.

mollekopf added reviewers: machniak, vanmeeuwen.Mar 19 2021, 5:31 PM

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

Harbormaster completed remote builds in B33673: Diff 6688.Mar 23 2021, 10:39 AM
mollekopf updated this revision to Diff 6691.Mar 23 2021, 10:45 AM
mollekopf marked 8 inline comments as done.

Addressed remaining comments

Harbormaster completed remote builds in B33676: Diff 6691.Mar 23 2021, 10:45 AM
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.

Harbormaster completed remote builds in B33688: Diff 6703.Mar 23 2021, 12:48 PM
mollekopf updated this revision to Diff 6742.Mar 24 2021, 12:33 PM

Test foreign currency payments and fixed refund handling

Harbormaster completed remote builds in B33724: Diff 6742.Mar 24 2021, 12:33 PM
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
Harbormaster completed remote builds in B33769: Diff 6787.Apr 2 2021, 9:10 AM
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

Harbormaster completed remote builds in B33775: Diff 6793.Apr 2 2021, 10:04 AM
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
Closed by commit rK69abc4fca320: Convert to foreign currencies using openexchangerates.org (authored by mollekopf, committed by vanmeeuwen). · Explain WhyJun 25 2021, 10:14 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
src/
app/
Backends/
52 lines
Console/
Commands/
Data/
Import/
48 lines
Providers/
Payment/
20 lines
62 lines
34 lines
config/
5 lines
5 lines
resources/
172 lines
tests/
Browser/
2 lines
Feature/
Controller/
142 lines
Unit/
14 lines

Diff 7393

View Options

src/app/Backends/OpenExchangeRates.php

Loading...
View Options

src/app/Console/Commands/Data/Import/ExchangerateCommand.php

Loading...
View Options

src/app/Providers/Payment/Mollie.php

Loading...
View Options

src/app/Providers/PaymentProvider.php

Loading...
View Options

src/app/Utils.php

Loading...
View Options

src/config/app.php

Loading...
View Options

src/config/currency.php

Loading...
View Options

src/config/services.php

Loading...
View Options

src/resources/exchangerates-CHF.php

Loading...
View Options

src/tests/Browser/PaymentMollieTest.php

Loading...
View Options

src/tests/Feature/Controller/PaymentsMollieTest.php

Loading...
View Options

src/tests/Unit/UtilsTest.php

Loading...