fix(accounts): keep price list within user permissions (#59231)

Co-authored-by: Mihir Kandoi <kandoimihir@gmail.com>
(cherry picked from commit e814d13126)

erpnext/accounts/party.py

@@ -5,8 +5,9 @@
 import frappe
 from frappe import _, msgprint, qb, scrub
 from frappe.contacts.doctype.address.address import get_company_address, get_default_address
-from frappe.core.doctype.user_permission.user_permission import get_permitted_documents
+from frappe.core.doctype.user_permission.user_permission import get_user_permissions
 from frappe.model.utils import get_fetch_values
+from frappe.permissions import get_allowed_docs_for_doctype
 from frappe.query_builder.functions import Abs, Date, Sum
 from frappe.utils import (
 	add_days,
@@ -158,7 +159,7 @@ def _get_party_details(
 	)
 	set_contact_details(party_details, party, party_type)
 	set_other_values(party_details, party, party_type)
-	set_price_list(party_details, party, party_type, price_list, pos_profile)
+	set_price_list(party_details, party, party_type, price_list, pos_profile, doctype)
 
 	tax_template = set_taxes(
 		party.name,
@@ -384,13 +385,33 @@ def get_default_price_list(party):
 		return price_list
 
 
-def set_price_list(party_details, party, party_type, given_price_list, pos=None):
+def get_permitted_price_lists(doctype=None):
+	permissions = sorted(
+		get_user_permissions().get("Price List", []), key=lambda p: p.get("is_default"), reverse=True
+	)
+
+	# a permission applicable for another doctype doesn't restrict this transaction
+	return get_allowed_docs_for_doctype(permissions, doctype)
+
+
+def get_usable_price_list(price_lists, party_doctype):
+	transaction_side = "selling" if party_doctype == "Customer" else "buying"
+
+	for price_list in price_lists:
+		details = frappe.get_cached_value(
+			"Price List", price_list, ["enabled", transaction_side], as_dict=True
+		)
+		if details.enabled and details[transaction_side]:
+			return price_list
+
+
+def set_price_list(party_details, party, party_type, given_price_list, pos=None, doctype=None):
 	# price list
-	price_list = get_permitted_documents("Price List")
+	permitted_price_lists = get_permitted_price_lists(doctype)
 
 	# if there is only one permitted document based on user permissions, set it
-	if price_list and len(price_list) == 1:
-		price_list = price_list[0]
+	if len(permitted_price_lists) == 1:
+		price_list = get_usable_price_list(permitted_price_lists, party.doctype)
 	elif pos and party_type == "Customer":
 		customer_price_list = frappe.get_value("Customer", party.name, "default_price_list")
 
@@ -402,6 +423,10 @@ def set_price_list(party_details, party, party_type, given_price_list, pos=None)
 	else:
 		price_list = get_default_price_list(party) or given_price_list
 
+		# don't set a price list the user has no permission for, the transaction can't be saved with it
+		if price_list and permitted_price_lists and price_list not in permitted_price_lists:
+			price_list = get_usable_price_list(permitted_price_lists, party.doctype)
+
 	if price_list and not is_price_list_enabled(price_list):
 		price_list = None
 

erpnext/accounts/test_party.py

@@ -1,5 +1,6 @@
 import frappe
 
+from erpnext.accounts.doctype.pos_profile.test_pos_profile import make_pos_profile
 from erpnext.accounts.party import get_default_price_list, set_price_list
 from erpnext.tests.utils import ERPNextTestSuite
 
@@ -34,19 +35,159 @@ class PartyTestCase(ERPNextTestSuite):
 
 		self.assertIsNone(party_details.selling_price_list)
 
-	def create_price_list(self, enabled):
+	def test_fallback_should_not_pick_an_unpermitted_price_list(self):
+		permitted_default = self.create_price_list(enabled=1)
+		permitted_other = self.create_price_list(enabled=1)
+		user = self.create_user_with_price_list_permissions([permitted_default, permitted_other])
+		customer = self.create_customer()
+
+		party_details = frappe._dict()
+		with self.set_user(user):
+			set_price_list(
+				party_details, customer, "Customer", self.create_price_list(enabled=1), doctype="Sales Order"
+			)
+
+		self.assertEqual(party_details.selling_price_list, permitted_default)
+
+	def test_permitted_given_price_list_should_be_kept(self):
+		permitted_default = self.create_price_list(enabled=1)
+		permitted_other = self.create_price_list(enabled=1)
+		user = self.create_user_with_price_list_permissions([permitted_default, permitted_other])
+		customer = self.create_customer()
+
+		party_details = frappe._dict()
+		with self.set_user(user):
+			set_price_list(party_details, customer, "Customer", permitted_other, doctype="Sales Order")
+
+		self.assertEqual(party_details.selling_price_list, permitted_other)
+
+	def test_permission_for_another_doctype_should_not_apply(self):
+		permitted = [self.create_price_list(enabled=1), self.create_price_list(enabled=1)]
+		user = self.create_user_with_price_list_permissions(permitted, applicable_for="Quotation")
+		customer = self.create_customer()
+		given_price_list = self.create_price_list(enabled=1)
+
+		party_details = frappe._dict()
+		with self.set_user(user):
+			set_price_list(party_details, customer, "Customer", given_price_list, doctype="Sales Order")
+
+		self.assertEqual(party_details.selling_price_list, given_price_list)
+
+	def test_a_single_permitted_price_list_should_fit_the_transaction(self):
+		buying_price_list = self.create_price_list(enabled=1, selling=0, buying=1)
+		user = self.create_user_with_price_list_permissions([buying_price_list])
+		customer = self.create_customer()
+		given_price_list = self.create_price_list(enabled=1)
+
+		party_details = frappe._dict()
+		with self.set_user(user):
+			set_price_list(party_details, customer, "Customer", given_price_list, doctype="Sales Order")
+
+		self.assertIsNone(party_details.selling_price_list)
+
+	def test_buying_transaction_should_not_take_a_selling_price_list(self):
+		permitted = [self.create_price_list(enabled=1) for _ in range(2)]
+		user = self.create_user_with_price_list_permissions(permitted)
+		supplier_price_list = self.create_price_list(enabled=1, selling=0, buying=1)
+		supplier = self.create_supplier(default_price_list=supplier_price_list)
+
+		party_details = frappe._dict()
+		with self.set_user(user):
+			set_price_list(party_details, supplier, "Supplier", None, doctype="Purchase Order")
+
+		self.assertIsNone(party_details.buying_price_list)
+
+	def test_permission_for_another_doctype_should_not_apply_without_a_doctype(self):
+		permitted = [self.create_price_list(enabled=1), self.create_price_list(enabled=1)]
+		user = self.create_user_with_price_list_permissions(permitted, applicable_for="Quotation")
+		customer = self.create_customer()
+		given_price_list = self.create_price_list(enabled=1)
+
+		party_details = frappe._dict()
+		with self.set_user(user):
+			set_price_list(party_details, customer, "Customer", given_price_list)
+
+		self.assertEqual(party_details.selling_price_list, given_price_list)
+
+	def test_pos_price_list_should_be_kept(self):
+		permitted = [self.create_price_list(enabled=1), self.create_price_list(enabled=1)]
+		user = self.create_user_with_price_list_permissions(permitted)
+		pos_price_list = self.create_price_list(enabled=1)
+		pos_profile = make_pos_profile(selling_price_list=pos_price_list)
+		customer = self.create_customer()
+
+		party_details = frappe._dict()
+		with self.set_user(user):
+			set_price_list(
+				party_details, customer, "Customer", None, pos=pos_profile.name, doctype="POS Invoice"
+			)
+
+		self.assertEqual(party_details.selling_price_list, pos_price_list)
+
+	def test_disabled_permitted_price_lists_should_clear_the_price_list(self):
+		permitted = [self.create_price_list(enabled=0), self.create_price_list(enabled=0)]
+		user = self.create_user_with_price_list_permissions(permitted)
+		customer = self.create_customer()
+		given_price_list = self.create_price_list(enabled=1)
+
+		party_details = frappe._dict()
+		with self.set_user(user):
+			set_price_list(party_details, customer, "Customer", given_price_list, doctype="Sales Order")
+
+		self.assertIsNone(party_details.selling_price_list)
+
+	def create_user_with_price_list_permissions(self, price_lists, applicable_for=None):
+		user = frappe.get_doc(
+			{
+				"doctype": "User",
+				"email": f"{frappe.generate_hash(length=10)}@example.com",
+				"first_name": "Price List Test",
+				"send_welcome_email": 0,
+				"roles": [{"role": "Sales User"}],
+			}
+		).insert(ignore_permissions=True)
+
+		for idx, price_list in enumerate(price_lists):
+			frappe.get_doc(
+				{
+					"doctype": "User Permission",
+					"user": user.name,
+					"allow": "Price List",
+					"for_value": price_list,
+					"is_default": int(idx == 0),
+					"apply_to_all_doctypes": int(not applicable_for),
+					"applicable_for": applicable_for,
+				}
+			).insert(ignore_permissions=True)
+
+		frappe.clear_cache(user=user.name)
+		self.addCleanup(frappe.clear_cache, user=user.name)
+
+		return user.name
+
+	def create_price_list(self, enabled, selling=1, buying=0):
 		price_list = frappe.get_doc(
 			{
 				"doctype": "Price List",
 				"price_list_name": frappe.generate_hash(length=10),
 				"currency": "INR",
-				"selling": 1,
+				"selling": selling,
+				"buying": buying,
 				"enabled": enabled,
 			}
 		).insert(ignore_permissions=True)
 
 		return price_list.name
 
+	def create_supplier(self, **values):
+		return frappe.get_doc(
+			{
+				"doctype": "Supplier",
+				"supplier_name": frappe.generate_hash(length=10),
+				**values,
+			}
+		).insert(ignore_permissions=True, ignore_mandatory=True)
+
 	def create_customer(self, **values):
 		customer = frappe.get_doc(
 			{

erpnext/controllers/accounts_controller.py

@@ -405,12 +405,26 @@ class AccountsController(TransactionBase):
 		return any(item.delivered_by_supplier for item in items)
 
 	def validate_price_list(self):
-		price_list_field = "selling_price_list" if self.get("selling_price_list") else "buying_price_list"
+		if self.get("selling_price_list"):
+			price_list_field, transaction_side = "selling_price_list", "selling"
+		else:
+			price_list_field, transaction_side = "buying_price_list", "buying"
+
 		price_list = self.get(price_list_field)
-		if not price_list or frappe.db.get_value("Price List", price_list, "enabled"):
+		if not price_list:
 			return
 
-		# Returns retain a submitted voucher's pricing even if its price list is now disabled.
+		details = (
+			frappe.db.get_value("Price List", price_list, ["enabled", transaction_side], as_dict=True)
+			or frappe._dict()
+		)
+
+		# An internal transfer carries the price list of the outward document into the inward one.
+		fits_transaction = details.get(transaction_side) or self.is_internal_transfer()
+		if details.enabled and fits_transaction:
+			return
+
+		# Returns retain a submitted voucher's pricing even if its price list no longer fits.
 		if (
 			self.get("is_return")
 			and self.get("return_against")
@@ -421,9 +435,20 @@ class AccountsController(TransactionBase):
 		):
 			return
 
+		if not details.enabled:
+			frappe.throw(
+				_("Price List {0} is disabled").format(get_link_to_form("Price List", price_list)),
+				title=_("Disabled Price List"),
+			)
+
+		if transaction_side == "selling":
+			message = _("Price List {0} cannot be used on a selling transaction")
+		else:
+			message = _("Price List {0} cannot be used on a buying transaction")
+
 		frappe.throw(
-			_("Price List {0} is disabled").format(get_link_to_form("Price List", price_list)),
-			title=_("Disabled Price List"),
+			message.format(get_link_to_form("Price List", price_list)),
+			title=_("Invalid Price List"),
 		)
 
 	def set_default_letter_head(self):

erpnext/controllers/tests/test_price_list_validation.py

@@ -0,0 +1,93 @@
+import frappe
+
+from erpnext.accounts.doctype.purchase_invoice.test_purchase_invoice import make_purchase_invoice
+from erpnext.accounts.doctype.sales_invoice.test_sales_invoice import create_sales_invoice
+from erpnext.tests.utils import ERPNextTestSuite
+
+
+class TestPriceListValidation(ERPNextTestSuite):
+	def create_price_list(self, selling=0, buying=0, enabled=1):
+		return (
+			frappe.get_doc(
+				{
+					"doctype": "Price List",
+					"price_list_name": frappe.generate_hash(length=10),
+					"currency": "INR",
+					"selling": selling,
+					"buying": buying,
+					"enabled": enabled,
+				}
+			)
+			.insert()
+			.name
+		)
+
+	def test_selling_transaction_should_reject_a_buying_price_list(self):
+		invoice = create_sales_invoice(do_not_save=1)
+		invoice.selling_price_list = self.create_price_list(buying=1)
+
+		with self.assertRaisesRegex(frappe.ValidationError, "selling transaction"):
+			invoice.save()
+
+	def test_buying_transaction_should_reject_a_selling_price_list(self):
+		invoice = make_purchase_invoice(do_not_save=1)
+		invoice.buying_price_list = self.create_price_list(selling=1)
+
+		with self.assertRaisesRegex(frappe.ValidationError, "buying transaction"):
+			invoice.save()
+
+	def test_a_price_list_for_both_sides_should_be_accepted(self):
+		price_list = self.create_price_list(selling=1, buying=1)
+
+		invoice = create_sales_invoice(do_not_save=1)
+		invoice.selling_price_list = price_list
+		invoice.save()
+
+		self.assertEqual(invoice.selling_price_list, price_list)
+
+	def test_a_missing_price_list_should_report_rather_than_crash(self):
+		invoice = create_sales_invoice(do_not_save=1)
+		invoice.selling_price_list = frappe.generate_hash(length=10)
+
+		with self.assertRaises(frappe.ValidationError):
+			invoice.validate_price_list()
+
+	def test_internal_transfer_should_keep_the_outward_price_list(self):
+		"""The inward document of an internal transfer takes the price list of the outward one, which
+		is flagged for the opposite side."""
+		from erpnext.stock.doctype.delivery_note.mapper import make_inter_company_purchase_receipt
+		from erpnext.stock.doctype.delivery_note.test_delivery_note import create_delivery_note
+		from erpnext.stock.doctype.purchase_receipt.test_purchase_receipt import (
+			prepare_data_for_internal_transfer,
+		)
+		from erpnext.stock.doctype.warehouse.test_warehouse import create_warehouse
+
+		prepare_data_for_internal_transfer()
+		company = "_Test Company with perpetual inventory"
+		selling_only = self.create_price_list(selling=1)
+
+		delivery_note = create_delivery_note(
+			company=company,
+			customer="_Test Internal Customer 2",
+			cost_center="Main - TCP1",
+			expense_account="Cost of Goods Sold - TCP1",
+			warehouse="Stores - TCP1",
+			target_warehouse=create_warehouse("_Test Transit For Price List", company=company),
+			do_not_submit=1,
+		)
+		delivery_note.selling_price_list = selling_only
+		delivery_note.save()
+		delivery_note.submit()
+
+		receipt = make_inter_company_purchase_receipt(delivery_note.name)
+		receipt.items[0].warehouse = "Stores - TCP1"
+		receipt.save()
+
+		self.assertEqual(receipt.buying_price_list, selling_only)
+
+	def test_disabled_price_list_should_still_report_as_disabled(self):
+		invoice = create_sales_invoice(do_not_save=1)
+		invoice.selling_price_list = self.create_price_list(selling=1, enabled=0)
+
+		with self.assertRaisesRegex(frappe.ValidationError, "is disabled"):
+			invoice.save()