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

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", 

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
+		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)
+

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);
+	}
+};

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)

erpnext/accounts/doctype/payment_entry/payment_entry.js

@@ -403,6 +403,13 @@ frappe.ui.form.on('Payment Entry', {
 			
 			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 @@ frappe.ui.form.on('Payment Entry', {
 			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) {

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}

erpnext/patches.txt

@@ -439,8 +439,9 @@ erpnext.patches.v8_7.set_offline_in_pos_settings #11-09-17
 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

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')"
+	)

erpnext/public/js/controllers/transaction.js

@@ -519,6 +519,7 @@ erpnext.TransactionController = erpnext.taxes_and_totals.extend({
 	},
 
 	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 @@ erpnext.TransactionController = erpnext.taxes_and_totals.extend({
 			}
 
 		}
+		// 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() {

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 @@ def save_new_records(test_records):
 
 
 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 @@ class TestCurrencyExchange(unittest.TestCase):
 		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)
+		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)

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"
  }
 ]

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 @@ def before_tests():
 
 @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 @@ def get_exchange_rate(from_currency, to_currency, transaction_date=None):
 	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)