-
Notifications
You must be signed in to change notification settings - Fork 78
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
Fix exchange rate issue with automated invoicing in qbo #1325
Fix exchange rate issue with automated invoicing in qbo #1325
Conversation
), | ||
) | ||
); | ||
Logger\log( 'remote_request', compact( 'currency_code', 'response' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unsure if this is debugging code left in or needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code appears in other parts as well. You can see logs on the terminal and can also check them using wp wc-misc format-log wp-content/debug.log
as specified in the QBO Readme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that it can also let you see the production logs. (ref)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok - but it's kind of pointless logging every time we do a lookup isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly about it; just following the practice here. Perhaps there was a discussion that every API call should have a logger and what benefits it would bring? I'm not sure. However, removing it here makes sense to me since it isn't a critical call. What do you think?
In the 2017 discussion mentioned in the issue, it can be seen that at that time, even if you didn't provide an exchange rate when calling the QBO API, QBO would automatically fill in the exchange rate for you. So I tested in this direction in the first place but then found that if you don't include the
ExchangeRate
in the API call now, QBO will automatically set the value to1
:"CurrencyRef":{"value":"EUR","name":"Euro"},"ExchangeRate":1
.After searching, I found discussions indicating that the QBO invoice API does not support automatically providing the
ExchangeRate
(perhaps they changed the rules at some point after 2017). You have to get the value yourself and include it in the payload. So here I used the get exchange rate API also from QBO (the sample on the page doesn't work, but the API actually works..).In addition, the local QBO connection is also fixed. It turned out that the relevant SDK was missing, and installing it solved the issue.
Fixes #1250
Screenshots
Exchange rate now correctly displays
How to test the changes in this Pull Request:
composer update
./wp-admin/post-new.php?post_type=wcb_sponsor_invoice
./wp-admin/network/admin.php?page=sponsor-invoices-dashboard§ion=submitted