Free Currency Converter API requires API key now. #22461

Hailong · 2019-04-23T04:27:26Z

Description (*)

Free Currency Converter API now requires API key, this change is to add the API key setting to backend and append the key to request url.

Fixed Issues (if relevant)

#22468

Manual testing scenarios (*)

Navigate to Stores -> Configuration -> General -> Currency Setup
Make sure the new API Key field is added for Currency Converter API.
Navigate to Stores -> Currency Rates
Select Currency Converter API as Import Service and click Import.
Make sure the rated value(s) are refreshed.

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

m2-assistant · 2019-04-23T04:27:28Z

Hi @Hailong. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento-engcom-team give me test instance - deploy test instance based on PR changes
@magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

orlangur · 2019-04-23T07:18:20Z

app/code/Magento/Directory/Model/Currency/Import/AbstractImport.php

+     * @param array $currenciesTo
+     * @return array
+     */
+    protected function _makeEmptyResponse(array $currenciesTo): array


This method was originally in FixerIo.php, moved it into this base class so that it can be reused by CurrencyConverterApi. But I just see the AbstractImport class is marked by @api annotation, so probably I can not add any new method in it. I can remove it if needed.

@Hailong yep, please let this method be private as it was initially.

I just reverted the changes in AbstractImport and FixerIo, but cloned makeEmptyResponse method in CurrencyConverterApi class.

orlangur · 2019-04-23T07:18:37Z

app/code/Magento/Directory/Model/Currency/Import/CurrencyConverterApi.php

@@ -12,7 +12,7 @@ class CurrencyConverterApi extends AbstractImport
    /**
     * @var string
     */
-    const CURRENCY_CONVERTER_URL = 'http://free.currencyconverterapi.com/api/v3/convert?q={{CURRENCY_FROM}}_{{CURRENCY_TO}}&compact=ultra'; //@codingStandardsIgnoreLine
+    const CURRENCY_CONVERTER_URL = 'http://free.currencyconverterapi.com/api/v6/convert?q={{CURRENCY_FROM}}_{{CURRENCY_TO}}&compact=ultra&apiKey={{API_KEY}}'; //@codingStandardsIgnoreLine


What happened to V3 API?

Both v3 and v6 require API Key, but since the latest version is v6, I changed it to v6.

Then I suppose some functionality is broken, please report steps to reproduce as an issue and mention it in description.

Let me add the steps in description now.

@orlangur I just added the steps to reproduce the original error in description. Thanks!

@Hailong please report an issue separately in https://github.com/magento/magento2/issues and then add it here to description

orlangur · 2019-04-23T17:20:27Z

app/code/Magento/Directory/Model/Currency/Import/CurrencyConverterApi.php

+            \Magento\Store\Model\ScopeInterface::SCOPE_STORE
+        );
+        if (empty($apiKey)) {
+            $this->_messages[] = __('No API Key was specified or an invalid API Key was specified.');


I don't see how it can be not empty here.

If user gives an API Key from Currency Setup of backend, $apiKey would be not empty. Otherwise, it would be empty.

orlangur · 2019-04-23T17:21:57Z

app/code/Magento/Directory/etc/adminhtml/system.xml

@@ -47,7 +47,12 @@
            </group>
            <group id="currencyconverterapi" translate="label" sortOrder="45" showInDefault="1" showInWebsite="0" showInStore="0">
                <label>Currency Converter API</label>
-                <field id="timeout" translate="label" type="text" sortOrder="0" showInDefault="1" showInWebsite="0" showInStore="0">
+                <field id="api_key" translate="label" type="obscure" sortOrder="5" showInDefault="1" showInWebsite="0" showInStore="0">


Please add validation as this field seems to be mandatory.

User may input the API key for either Fixer.io or Currency Converter API on the backend, so not mandatory seems more user friendly.

orlangur · 2019-04-23T17:22:22Z

app/code/Magento/Directory/Model/Currency/Import/CurrencyConverterApi.php

+        );
+        if (empty($apiKey)) {
+            $this->_messages[] = __('No API Key was specified or an invalid API Key was specified.');
+            $data[$currencyFrom] = $this->makeEmptyResponse($currenciesTo);


Returning such response is not a option, please throw some error instead.

Let me see how to throw a proper error from there, and change it accordingly.

@orlangur by reviewing the code again, I found setting the _messages[] array here seems a good idea. Because the caller will check the _messages[] array and report error to users. For this part I actually cloned the code from the FixerIo class.

Here is a screenshot of the warning message: https://pasteboard.co/IbuUilQ.png

Thanks, good to know. Still, we should not produce an empty response.

I agree with you. Let me check what kind of response is better for this case later today, and refine the code properly.

@orlangur I have changed it to throw a LocalizedException, in this way there would be a red error message shown in the UI when the key is empty. Screenshot

magento-engcom-team · 2019-04-27T15:27:37Z

Hi @orlangur, thank you for the review.
ENGCOM-4835 has been created to process this Pull Request

VasylShvorak · 2019-04-30T08:54:29Z

✔️ QA passed

…for-free-currency-converter-api

sivaschenko · 2019-06-09T20:07:00Z

@magento run all tests

orlangur

@ysapiga please squash all build-fixing commits into one.

ysapiga · 2019-06-11T09:53:13Z

@orlangur please review.

sidolov · 2019-08-16T20:21:23Z

Hi @Hailong , looks like the same changes were merged in the scope of PR: #24008
I'm closing this PR as not relevant, I'm sorry for the inconvenience.

m2-assistant · 2019-08-16T20:21:25Z

Hi @Hailong, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

magento-engcom-team added Component: Directory Release Line: 2.3 labels Apr 23, 2019

orlangur requested changes Apr 23, 2019

View reviewed changes

ghost assigned orlangur Apr 23, 2019

ghost added the Progress: needs update label Apr 23, 2019

Hailong force-pushed the add-api-key-for-free-currency-converter-api branch from 8164bc9 to 7baa877 Compare April 23, 2019 13:26

orlangur requested changes Apr 23, 2019

View reviewed changes

Hailong force-pushed the add-api-key-for-free-currency-converter-api branch 2 times, most recently from 314c0f6 to 1cbf202 Compare April 26, 2019 00:20

Free Currency Converter API requires API key now.

10518b7

Hailong force-pushed the add-api-key-for-free-currency-converter-api branch from 1cbf202 to 10518b7 Compare April 26, 2019 00:36

Hailong requested a review from orlangur April 27, 2019 11:41

orlangur approved these changes Apr 27, 2019

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Apr 27, 2019

VasylShvorak self-assigned this Apr 30, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels Apr 30, 2019

nmalevanec self-assigned this Apr 30, 2019

ghost added Progress: extended testing and removed Progress: testing in progress labels Apr 30, 2019

Merge remote-tracking branch 'mainline/2.3-develop' into add-api-key-…

3714c72

…for-free-currency-converter-api

ghost added Progress: accept and removed Progress: extended testing labels Apr 30, 2019

magento-engcom-team added the Award: bug fix label May 1, 2019

mikebox mentioned this pull request May 4, 2019

Handling Currency Converter API Error Responses #22698

Closed

4 tasks

ghost added Progress: extended testing and removed Progress: accept labels Jun 9, 2019

ysapiga assigned ysapiga and unassigned nmalevanec Jun 10, 2019

orlangur requested changes Jun 10, 2019

View reviewed changes

ghost added Progress: needs update and removed Progress: extended testing labels Jun 10, 2019

Fix static tests.

12f6b88

ysapiga force-pushed the add-api-key-for-free-currency-converter-api branch from 1976e27 to 12f6b88 Compare June 11, 2019 09:51

sidolov closed this Aug 16, 2019

Free Currency Converter API requires API key now. #22461

Free Currency Converter API requires API key now. #22461

Uh oh!

Conversation

Hailong commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

Uh oh!

m2-assistant bot commented Apr 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magento-engcom-team commented Apr 27, 2019

Uh oh!

VasylShvorak commented Apr 30, 2019

Uh oh!

sivaschenko commented Jun 9, 2019

Uh oh!

orlangur left a comment

Choose a reason for hiding this comment

Uh oh!

ysapiga commented Jun 11, 2019

Uh oh!

sidolov commented Aug 16, 2019

Uh oh!

m2-assistant bot commented Aug 16, 2019

Uh oh!

Uh oh!

Hailong commented Apr 23, 2019 •

edited

Loading