This also enables EUR payment methods in mollie.
Details
Diff Detail
- Repository
- rK kolab
- Branch
- dev/paymentdialog
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 33676 Build 13285: arc lint + arc unit
Event Timeline
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)
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. |
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. |
Looks good to me. Two things:
- Some tests would be nice.
- 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.
Checked in the generated exchangerates-CHF.php file.
I'll add some tests based on this data for conversion between CHF and EUR.
src/app/Providers/PaymentProvider.php | ||
---|---|---|
242 | 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. |
- Move the openexchangerates api key to where the other api keys are
- Load payment method whitelist from config