Currency import using 1forge.com API & add conversion loss % calculation #446
Conversation
…o fields to system table to support this
|
@AlexHeylin Thanks for the PR. We'll observe the implementation, analyze its impact on other features and flow and merge it once confirm. |
| $debugstring = ""; | ||
| foreach ($quotesData as $quote) { | ||
| $debugstring = $debugstring . "A," ; | ||
| if (isset(Common_model::$global_config ['system_config'] ['currency_conv_loss_pct']) |
There was a problem hiding this comment.
There was a problem hiding this comment.
When changing currency it's usual for there to be fees involved. For example 5% bank fee - in this case you'd set currency_conv_lost_pct to 5 (in Configuration > Settings > Global > Currency Conversion Loss Percentage) and when the system imports the market rate, it will allow for this 5% loss and put the rate you actually get into the DB. This helps ensure the system calculates conversion more accurately. Say the market rate is 2 USD = 1 GBP, and base currency is GBP, if I set currency_conv_lost_pct to 5 the rate in the DB will be 1.9 USD = 1 GBP.
If you don't want to allow for any loss (the way it used to be) then just set Currency Conversion Loss Percentage = 0 and it will use the rate directly from the API.
The two new system variables need to be created by importing the SQL also in the PR. If use manually runs currency update without setting the API key, if will return error message saying to set it in Configuration > Settings > Global > API key for currency rate import from 1forge.com
Please don't merge this yet - I've seen a couple of improvements (remove debug / check DB write worked / log to disk) I should make. Also this is failing on one of my systems and I've not yet found out why. If I make more changes to the code in the same branch, do I need to do a new PR or just let you know this one is OK to process?
Thanks @smrdoshi
There was a problem hiding this comment.
Thanks for explanation @AlexHeylin
I got the idea. Let's do it.
Once you will be done all the modifications in your branch, you will need to send new PR.
| function __construct() { | ||
| parent::__construct (); | ||
| // if(!defined( 'CRON' ) ) | ||
| // exit(); | ||
| $this->load->model ( "db_model" ); | ||
| $this->load->library ( "astpp/common" ); | ||
| $this->fp = fopen("/var/log/astpp/astpp-currency.log", "a+"); |
There was a problem hiding this comment.
Thanks for the update @AlexHeylin
Would you please use variable for log path in place of using static path?
Use Common_model::$global_config ['system_config'] ['log_path']
Thanks
|
@smrdoshi - Thanks for pointing that out. I actually copied it from |
|
Yes. Please, let's do it.
Also thanks for pointing out processinvoice.php with static path. We will
update that file. :)
[image: Mailtrack]
<https://mailtrack.io?utm_source=gmail&utm_medium=signature&utm_campaign=signaturevirality6&>
Sender
notified by
Mailtrack
<https://mailtrack.io?utm_source=gmail&utm_medium=signature&utm_campaign=signaturevirality6&>
07/20/18,
10:48:59 AM
Best Regards
--
Samir Doshi
*iNextrix Technologie**s Pvt. Ltd*.
http://www.inextrix.com
*Disclaimer:*
The information contained in this communication is confidential and may be
legally privileged. It is intended solely for the use of the individual or
entity to whom it is addressed and others authorized to receive it. If you
are not the intended recipient you are hereby notified that any disclosure,
copying, distribution or taking action in reliance on the contents of this
information is strictly prohibited and may be unlawful. Please notify the
sender immediately and destroy all copies of this message and any
attachments contained in it.
…On Fri, Jul 20, 2018 at 1:40 AM, AlexHeylin ***@***.***> wrote:
@smrdoshi <https://github.com/smrdoshi> - Thanks for pointing that out. I
actually copied it from web_interface/astpp/application/controllers/
ProcessInvoice.php
This tests OK on my test system, and I'm happy for you to commit if you
are happy.
Thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#446 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AA6gcdf8KLNkmb52Hv5_zSmWxfZ01Psbks5uIOevgaJpZM4U7-MH>
.
|
|
This has worked well in both my dev and prod environments so please merge. Will you merge this, or do you want me to submit a new request? Thanks |
|
Will do.
…On Sat, Aug 4, 2018, 12:10 AM AlexHeylin ***@***.***> wrote:
This has worked well in both my dev and prod environments so please merge.
Will you merge this, or do you want me to submit a new request? Thanks
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#446 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AA6gcS_5xehtmpbkoMmNYTeRo-65jWSVks5uNJkqgaJpZM4U7-MH>
.
|
Currency import using 1forge.com API
Add conversion loss % calculation
Add two fields to system table to support this.
Note: need to import SQL manually for now, as expecting this to be integrated into v3.7 upgrade file.