Skip to content

Currency import Undefined index error #21487: #21497

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

Jitheesh
Copy link
Contributor

  • API key is required for free version of currencyconverterapi

Description (*)

CurrencyConverterApi is using http://free.currencyconverterapi.com service for importing currency rates. API key is now required for the free server requests and this is not implemented on current Magento core code.
Because of missing API key, https://free.currencyconverterapi.com/api/v6/convert?q=USD_EUR,USD_UAH&compact=ultra
it return following response {"status":400,"error":"API Key is required."}

Fixed Issues (if relevant)

  1. Currency import Undefined index error #21487: Currency import Undefined index error

Manual testing scenarios (*)

  1. Setup multiple currencies from Stores >> Settings>> Configuration > Currency Setup
  2. Go to Admin > Stores > Currency Rates
  3. Select Currency Converter API in the dropdown and click the import button
    3.1 It will return API key missing error instead of "Notice: Undefined index:" warning
  4. Create https://free.currencyconverterapi.com API key and setup that key from Stores >> Settings>> Configuration > Currency Setup > Currency Converter API
  5. Select Currency Converter API in the dropdown and click the import button
    5.1 Currency rates imported

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)

- API key is required for free version of currencyconverterapi
@magento-engcom-team
Copy link
Contributor

Hi @Jitheesh. 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

@Jitheesh
Copy link
Contributor Author

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @Jitheesh. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @Jitheesh, here is your new Magento instance.
Admin access: https://pr-21497.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@Jitheesh Jitheesh requested a review from sidolov March 20, 2019 10:15
@josefbehr josefbehr self-requested a review March 25, 2019 11:28
@josefbehr josefbehr self-assigned this Mar 25, 2019
Copy link
Contributor

@josefbehr josefbehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jitheesh Thank you for this contribution, very nice. Can you please have a look at my comments?

@Jitheesh
Copy link
Contributor Author

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @Jitheesh. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @Jitheesh, here is your new Magento instance.
Admin access: https://pr-21497.instances.magento-community.engineering/admin
Login: admin Password: 123123q

- API version and protocol update
@ghost ghost assigned sidolov Jul 15, 2019
@Jitheesh
Copy link
Contributor Author

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @Jitheesh. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @Jitheesh, here is your new Magento instance.
Admin access: https://pr-21497.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@josefbehr
Copy link
Contributor

@Jitheesh
please fix the failing tests, it's just static stuff:

PHP Code Sniffer detected 2 violation(s):

FILE: ...Magento/Directory/Model/Currency/Import/CurrencyConverterApi.php

FOUND 9 ERRORS AND 3 WARNINGS AFFECTING 11 LINES

12 | WARNING | [ ] Missing doc comment for class
| | CurrencyConverterApi
12 | ERROR | [x] Class is missing annotation block
17 | WARNING | [ ] Line exceeds maximum limit of 120 characters;
| | contains 125 characters
49 | ERROR | [x] If the @inheritdoc not inline it shouldn’t
| | have braces
78 | ERROR | [x] $data parameter is not in order
79 | ERROR | [x] $currencyFrom parameter is not in order
80 | ERROR | [x] $currenciesTo parameter is not in order
81 | ERROR | [x] $accessKey parameter is not in order
87 | WARNING | [ ] The use of function set_time_limit() is
| | discouraged
160 | ERROR | [x] If the @inheritdoc not inline it shouldn’t
| | have braces
162 | ERROR | [x] {@inheritdoc} does not import parameter
| | annotation
169 | ERROR | [x] There must be exactly one blank line before tags

PHPCBF CAN FIX THE 9 MARKED SNIFF VIOLATIONS AUTOMATICALLY

@magento-engcom-team
Copy link
Contributor

Hi @sidolov, thank you for the review.
ENGCOM-5615 has been created to process this Pull Request
✳️ @sidolov, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@engcom-Alfa engcom-Alfa self-assigned this Aug 23, 2019
@sidolov
Copy link
Contributor

sidolov commented Aug 23, 2019

Hi @Jitheesh , looks like the issue resolved in the scope of #24008
I'm closing this PR as duplicate.
Thank you for the collaboration!

@sidolov sidolov closed this Aug 23, 2019
@m2-assistant
Copy link

m2-assistant bot commented Aug 23, 2019

Hi @Jitheesh, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants