Enhance Currency Exchange Management (#10482)
* add new settings in Accouts Settings
* patch for new settings
* refactor `get_exchange_rate`
* adds validation
* tests validation
* disables conversion rate field if stale rates not allowed
* more test cases
more test case...
test `get_exchange_rate` behaviour with stale not allowed in sett..
fix currency exchange test case
do housekeeping after running accounts settings test
* clean up
* documentation
* make use of correct api url
* Fix tests failing due to wrong exchange rate from fixer.io
* remove mandatory constraint from `allow_stale`
* added info to documentation
diff --git a/erpnext/accounts/doctype/accounts_settings/accounts_settings.json b/erpnext/accounts/doctype/accounts_settings/accounts_settings.json
index 0ce3d5d..42cd44a 100644
--- a/erpnext/accounts/doctype/accounts_settings/accounts_settings.json
+++ b/erpnext/accounts/doctype/accounts_settings/accounts_settings.json
@@ -286,6 +286,99 @@
"search_index": 0,
"set_only_once": 0,
"unique": 0
+ },
+ {
+ "allow_bulk_edit": 0,
+ "allow_on_submit": 0,
+ "bold": 0,
+ "collapsible": 0,
+ "columns": 0,
+ "fieldname": "currency_exchange_section",
+ "fieldtype": "Section Break",
+ "hidden": 0,
+ "ignore_user_permissions": 0,
+ "ignore_xss_filter": 0,
+ "in_filter": 0,
+ "in_global_search": 0,
+ "in_list_view": 0,
+ "in_standard_filter": 0,
+ "label": "Currency Exchange Settings",
+ "length": 0,
+ "no_copy": 0,
+ "permlevel": 0,
+ "precision": "",
+ "print_hide": 0,
+ "print_hide_if_no_value": 0,
+ "read_only": 0,
+ "remember_last_selected_value": 0,
+ "report_hide": 0,
+ "reqd": 0,
+ "search_index": 0,
+ "set_only_once": 0,
+ "unique": 0
+ },
+ {
+ "allow_bulk_edit": 0,
+ "allow_on_submit": 0,
+ "bold": 0,
+ "collapsible": 0,
+ "columns": 0,
+ "default": "1",
+ "fieldname": "allow_stale",
+ "fieldtype": "Check",
+ "hidden": 0,
+ "ignore_user_permissions": 0,
+ "ignore_xss_filter": 0,
+ "in_filter": 0,
+ "in_global_search": 0,
+ "in_list_view": 1,
+ "in_standard_filter": 0,
+ "label": "Allow Stale Exchange Rates",
+ "length": 0,
+ "no_copy": 0,
+ "permlevel": 0,
+ "precision": "",
+ "print_hide": 0,
+ "print_hide_if_no_value": 0,
+ "read_only": 0,
+ "remember_last_selected_value": 0,
+ "report_hide": 0,
+ "reqd": 0,
+ "search_index": 0,
+ "set_only_once": 0,
+ "unique": 0
+ },
+ {
+ "allow_bulk_edit": 0,
+ "allow_on_submit": 0,
+ "bold": 0,
+ "collapsible": 0,
+ "columns": 0,
+ "default": "1",
+ "depends_on": "eval:doc.allow_stale==0",
+ "fieldname": "stale_days",
+ "fieldtype": "Int",
+ "hidden": 0,
+ "ignore_user_permissions": 0,
+ "ignore_xss_filter": 0,
+ "in_filter": 0,
+ "in_global_search": 0,
+ "in_list_view": 0,
+ "in_standard_filter": 0,
+ "label": "Stale Days",
+ "length": 0,
+ "no_copy": 0,
+ "permlevel": 0,
+ "precision": "",
+ "print_hide": 0,
+ "print_hide_if_no_value": 0,
+ "read_only": 0,
+ "remember_last_selected_value": 0,
+ "report_hide": 0,
+ "reqd": 0,
+ "search_index": 0,
+ "set_only_once": 0,
+ "unique": 0
}
],
"has_web_view": 0,
@@ -299,7 +392,7 @@
"issingle": 1,
"istable": 0,
"max_attachments": 0,
- "modified": "2017-06-16 17:39:50.614522",
+ "modified": "2017-09-05 10:10:03.117505",
"modified_by": "Administrator",
"module": "Accounts",
"name": "Accounts Settings",
diff --git a/erpnext/accounts/doctype/accounts_settings/accounts_settings.py b/erpnext/accounts/doctype/accounts_settings/accounts_settings.py
index dd33ff1..8431173 100644
--- a/erpnext/accounts/doctype/accounts_settings/accounts_settings.py
+++ b/erpnext/accounts/doctype/accounts_settings/accounts_settings.py
@@ -5,10 +5,20 @@
from __future__ import unicode_literals
import frappe
-from frappe import _
-from frappe.utils import cint, comma_and
+from frappe.utils import cint
from frappe.model.document import Document
+
class AccountsSettings(Document):
def on_update(self):
- pass
\ No newline at end of file
+ pass
+
+ def validate(self):
+ self.validate_stale_days()
+
+ def validate_stale_days(self):
+ if not self.allow_stale and cint(self.stale_days) <= 0:
+ frappe.msgprint(
+ "Stale Days should start from 1.", title='Error', indicator='red',
+ raise_exception=1)
+
diff --git a/erpnext/accounts/doctype/accounts_settings/test_accounts_settings.js b/erpnext/accounts/doctype/accounts_settings/test_accounts_settings.js
new file mode 100644
index 0000000..f9aa166
--- /dev/null
+++ b/erpnext/accounts/doctype/accounts_settings/test_accounts_settings.js
@@ -0,0 +1,35 @@
+QUnit.module('accounts');
+
+QUnit.test("test: Accounts Settings doesn't allow negatives", function (assert) {
+ let done = assert.async();
+
+ assert.expect(2);
+
+ frappe.run_serially([
+ () => frappe.set_route('Form', 'Accounts Settings', 'Accounts Settings'),
+ () => frappe.timeout(2),
+ () => unchecked_if_checked(cur_frm, 'Allow Stale Exchange Rates', frappe.click_check),
+ () => cur_frm.set_value('stale_days', 0),
+ () => frappe.click_button('Save'),
+ () => frappe.timeout(2),
+ () => {
+ assert.ok(cur_dialog);
+ },
+ () => frappe.click_button('Close'),
+ () => cur_frm.set_value('stale_days', -1),
+ () => frappe.click_button('Save'),
+ () => frappe.timeout(2),
+ () => {
+ assert.ok(cur_dialog);
+ },
+ () => frappe.click_button('Close'),
+ () => done()
+ ]);
+
+});
+
+const unchecked_if_checked = function(frm, field_name, fn){
+ if (frm.doc.allow_stale) {
+ return fn(field_name);
+ }
+};
diff --git a/erpnext/accounts/doctype/accounts_settings/test_accounts_settings.py b/erpnext/accounts/doctype/accounts_settings/test_accounts_settings.py
new file mode 100644
index 0000000..bf1e967
--- /dev/null
+++ b/erpnext/accounts/doctype/accounts_settings/test_accounts_settings.py
@@ -0,0 +1,22 @@
+import unittest
+
+import frappe
+
+
+class TestAccountsSettings(unittest.TestCase):
+ def tearDown(self):
+ # Just in case `save` method succeeds, we need to take things back to default so that other tests
+ # don't break
+ cur_settings = frappe.get_doc('Accounts Settings', 'Accounts Settings')
+ cur_settings.allow_stale = 1
+ cur_settings.save()
+
+ def test_stale_days(self):
+ cur_settings = frappe.get_doc('Accounts Settings', 'Accounts Settings')
+ cur_settings.allow_stale = 0
+ cur_settings.stale_days = 0
+
+ self.assertRaises(frappe.ValidationError, cur_settings.save)
+
+ cur_settings.stale_days = -1
+ self.assertRaises(frappe.ValidationError, cur_settings.save)
diff --git a/erpnext/accounts/doctype/payment_entry/payment_entry.js b/erpnext/accounts/doctype/payment_entry/payment_entry.js
index 61ede97..04db9e2 100644
--- a/erpnext/accounts/doctype/payment_entry/payment_entry.js
+++ b/erpnext/accounts/doctype/payment_entry/payment_entry.js
@@ -403,6 +403,13 @@
frm.events.set_difference_amount(frm);
}
+
+ // Make read only if Accounts Settings doesn't allow stale rates
+ frappe.model.get_value("Accounts Settings", null, "allow_stale",
+ function(d){
+ frm.set_df_property("source_exchange_rate", "read_only", cint(d.allow_stale) ? 0 : 1);
+ }
+ );
},
target_exchange_rate: function(frm) {
@@ -421,6 +428,13 @@
frm.events.set_difference_amount(frm);
}
frm.set_paid_amount_based_on_received_amount = false;
+
+ // Make read only if Accounts Settings doesn't allow stale rates
+ frappe.model.get_value("Accounts Settings", null, "allow_stale",
+ function(d){
+ frm.set_df_property("target_exchange_rate", "read_only", cint(d.allow_stale) ? 0 : 1);
+ }
+ );
},
paid_amount: function(frm) {
diff --git a/erpnext/docs/user/manual/en/accounts/setup/accounts-settings.md b/erpnext/docs/user/manual/en/accounts/setup/accounts-settings.md
index 3bada56..f47f6e6 100644
--- a/erpnext/docs/user/manual/en/accounts/setup/accounts-settings.md
+++ b/erpnext/docs/user/manual/en/accounts/setup/accounts-settings.md
@@ -13,4 +13,8 @@
* Unlink Payment on Cancellation of Invoice: If checked, system will unlink the payment against the invoice. Otherwise, it will show the link error.
+* Allow Stale Exchange Rate: This should be unchecked if you want ERPNext to check the age of records fetched from Currency Exchange in foreign currency transactions. If it is unchecked, the exchange rate field will be read-only in documents.
+
+* Stale Days: The number of days to use when deciding if a Currency Exchange record is stale. E.g If Currency Exchange records are to be updated every day, the Stale Days should be set as 1.
+
{next}
diff --git a/erpnext/patches.txt b/erpnext/patches.txt
index 4b0c538..f961a5b 100644
--- a/erpnext/patches.txt
+++ b/erpnext/patches.txt
@@ -439,8 +439,9 @@
erpnext.patches.v8_9.add_setup_progress_actions #08-09-2017
erpnext.patches.v8_9.rename_company_sales_target_field
erpnext.patches.v8_8.set_bom_rate_as_per_uom
-erpnext.patches.v9_0.remove_subscription_module
erpnext.patches.v8_7.make_subscription_from_recurring_data
+erpnext.patches.v8_8.add_new_fields_in_accounts_settings
+erpnext.patches.v9_0.remove_subscription_module
erpnext.patches.v8_9.set_print_zero_amount_taxes
erpnext.patches.v8_9.set_default_customer_group
erpnext.patches.v8_9.remove_employee_from_salary_structure_parent
diff --git a/erpnext/patches/v8_8/add_new_fields_in_accounts_settings.py b/erpnext/patches/v8_8/add_new_fields_in_accounts_settings.py
new file mode 100644
index 0000000..bd25f15
--- /dev/null
+++ b/erpnext/patches/v8_8/add_new_fields_in_accounts_settings.py
@@ -0,0 +1,9 @@
+from __future__ import unicode_literals
+import frappe
+
+
+def execute():
+ frappe.db.sql(
+ "INSERT INTO `tabSingles` (`doctype`, `field`, `value`) VALUES ('Accounts Settings', 'allow_stale', '1'), "
+ "('Accounts Settings', 'stale_days', '1')"
+ )
diff --git a/erpnext/public/js/controllers/transaction.js b/erpnext/public/js/controllers/transaction.js
index a5ef15e..5f35ea4 100644
--- a/erpnext/public/js/controllers/transaction.js
+++ b/erpnext/public/js/controllers/transaction.js
@@ -519,6 +519,7 @@
},
conversion_rate: function() {
+ const me = this.frm;
if(this.frm.doc.currency === this.get_company_currency()) {
this.frm.set_value("conversion_rate", 1.0);
}
@@ -536,6 +537,12 @@
}
}
+ // Make read only if Accounts Settings doesn't allow stale rates
+ frappe.model.get_value("Accounts Settings", null, "allow_stale",
+ function(d){
+ me.set_df_property("conversion_rate", "read_only", cint(d.allow_stale) ? 0 : 1);
+ }
+ );
},
set_actual_charges_based_on_currency: function() {
diff --git a/erpnext/setup/doctype/currency_exchange/test_currency_exchange.py b/erpnext/setup/doctype/currency_exchange/test_currency_exchange.py
index a477379..6d5848a 100644
--- a/erpnext/setup/doctype/currency_exchange/test_currency_exchange.py
+++ b/erpnext/setup/doctype/currency_exchange/test_currency_exchange.py
@@ -1,9 +1,9 @@
# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors
# License: GNU General Public License v3. See license.txt
from __future__ import unicode_literals
-
-
import frappe, unittest
+from erpnext.setup.utils import get_exchange_rate
+
test_records = frappe.get_test_records('Currency Exchange')
@@ -28,11 +28,21 @@
class TestCurrencyExchange(unittest.TestCase):
- def test_exchnage_rate(self):
- from erpnext.setup.utils import get_exchange_rate
+ def clear_cache(self):
+ cache = frappe.cache()
+ key = "currency_exchange_rate:{0}:{1}".format("USD", "INR")
+ cache.delete(key)
+ def tearDown(self):
+ frappe.db.set_value("Accounts Settings", None, "allow_stale", 1)
+ self.clear_cache()
+
+ def test_exchange_rate(self):
save_new_records(test_records)
+ frappe.db.set_value("Accounts Settings", None, "allow_stale", 1)
+
+ # Start with allow_stale is True
exchange_rate = get_exchange_rate("USD", "INR", "2016-01-01")
self.assertEqual(exchange_rate, 60.0)
@@ -43,6 +53,51 @@
self.assertEqual(exchange_rate, 62.9)
# Exchange rate as on 15th Dec, 2015, should be fetched from fixer.io
+ self.clear_cache()
exchange_rate = get_exchange_rate("USD", "INR", "2015-12-15")
self.assertFalse(exchange_rate == 60)
- self.assertEqual(exchange_rate, 66.894)
\ No newline at end of file
+ self.assertEqual(exchange_rate, 66.894)
+
+ def test_exchange_rate_strict(self):
+ # strict currency settings
+ frappe.db.set_value("Accounts Settings", None, "allow_stale", 0)
+ frappe.db.set_value("Accounts Settings", None, "stale_days", 1)
+
+ exchange_rate = get_exchange_rate("USD", "INR", "2016-01-01")
+ self.assertEqual(exchange_rate, 60.0)
+
+ # Will fetch from fixer.io
+ self.clear_cache()
+ exchange_rate = get_exchange_rate("USD", "INR", "2016-01-15")
+ self.assertEqual(exchange_rate, 67.79)
+
+ exchange_rate = get_exchange_rate("USD", "INR", "2016-01-30")
+ self.assertEqual(exchange_rate, 62.9)
+
+ # Exchange rate as on 15th Dec, 2015, should be fetched from fixer.io
+ self.clear_cache()
+ exchange_rate = get_exchange_rate("USD", "INR", "2015-12-15")
+ self.assertEqual(exchange_rate, 66.894)
+
+ exchange_rate = get_exchange_rate("INR", "NGN", "2016-01-10")
+ self.assertEqual(exchange_rate, 65.1)
+
+ # NGN is not available on fixer.io so these should return 0
+ exchange_rate = get_exchange_rate("INR", "NGN", "2016-01-09")
+ self.assertEqual(exchange_rate, 0)
+
+ exchange_rate = get_exchange_rate("INR", "NGN", "2016-01-11")
+ self.assertEqual(exchange_rate, 0)
+
+ def test_exchange_rate_strict_switched(self):
+ # Start with allow_stale is True
+ exchange_rate = get_exchange_rate("USD", "INR", "2016-01-15")
+ self.assertEqual(exchange_rate, 65.1)
+
+ frappe.db.set_value("Accounts Settings", None, "allow_stale", 0)
+ frappe.db.set_value("Accounts Settings", None, "stale_days", 1)
+
+ # Will fetch from fixer.io
+ self.clear_cache()
+ exchange_rate = get_exchange_rate("USD", "INR", "2016-01-15")
+ self.assertEqual(exchange_rate, 67.79)
\ No newline at end of file
diff --git a/erpnext/setup/doctype/currency_exchange/test_records.json b/erpnext/setup/doctype/currency_exchange/test_records.json
index d2f658b..0c9cfbb 100644
--- a/erpnext/setup/doctype/currency_exchange/test_records.json
+++ b/erpnext/setup/doctype/currency_exchange/test_records.json
@@ -33,5 +33,12 @@
"exchange_rate": 62.9,
"from_currency": "USD",
"to_currency": "INR"
+ },
+ {
+ "doctype": "Currency Exchange",
+ "date": "2016-01-10",
+ "exchange_rate": 65.1,
+ "from_currency": "INR",
+ "to_currency": "NGN"
}
]
\ No newline at end of file
diff --git a/erpnext/setup/utils.py b/erpnext/setup/utils.py
index bdbf3f4..f003ce4 100644
--- a/erpnext/setup/utils.py
+++ b/erpnext/setup/utils.py
@@ -4,7 +4,7 @@
from __future__ import unicode_literals
import frappe
from frappe import _
-from frappe.utils import flt
+from frappe.utils import flt, add_days
from frappe.utils import get_datetime_str, nowdate
def get_root_of(doctype):
@@ -56,8 +56,6 @@
@frappe.whitelist()
def get_exchange_rate(from_currency, to_currency, transaction_date=None):
- if not transaction_date:
- transaction_date = nowdate()
if not (from_currency and to_currency):
# manqala 19/09/2016: Should this be an empty return or should it throw and exception?
return
@@ -65,13 +63,27 @@
if from_currency == to_currency:
return 1
+ if not transaction_date:
+ transaction_date = nowdate()
+
+ currency_settings = frappe.get_doc("Accounts Settings").as_dict()
+ allow_stale_rates = currency_settings.get("allow_stale")
+
+ filters = [
+ ["date", "<=", get_datetime_str(transaction_date)],
+ ["from_currency", "=", from_currency],
+ ["to_currency", "=", to_currency]
+ ]
+
+ if not allow_stale_rates:
+ stale_days = currency_settings.get("stale_days")
+ checkpoint_date = add_days(transaction_date, -stale_days)
+ filters.append(["date", ">", get_datetime_str(checkpoint_date)])
+
# cksgb 19/09/2016: get last entry in Currency Exchange with from_currency and to_currency.
- entries = frappe.get_all("Currency Exchange", fields = ["exchange_rate"],
- filters=[
- ["date", "<=", get_datetime_str(transaction_date)],
- ["from_currency", "=", from_currency],
- ["to_currency", "=", to_currency]
- ], order_by="date desc", limit=1)
+ entries = frappe.get_all(
+ "Currency Exchange", fields=["exchange_rate"], filters=filters, order_by="date desc",
+ limit=1)
if entries:
return flt(entries[0].exchange_rate)