From 0331e37982b5513bc49ccfb8b840323bd9960041 Mon Sep 17 00:00:00 2001 From: ruthra kumar Date: Tue, 31 May 2022 15:38:08 +0530 Subject: [PATCH 01/22] fix: incorrect billed_qty when item has multiple Delivery note sales order analysis report returns incorrect billed_qty value for an SO item has multiple delivery notes --- .../sales_order_analysis.py | 68 ++++++++++++++++--- 1 file changed, 57 insertions(+), 11 deletions(-) diff --git a/erpnext/selling/report/sales_order_analysis/sales_order_analysis.py b/erpnext/selling/report/sales_order_analysis/sales_order_analysis.py index dcfb10a9d53..cc61594af4a 100644 --- a/erpnext/selling/report/sales_order_analysis/sales_order_analysis.py +++ b/erpnext/selling/report/sales_order_analysis/sales_order_analysis.py @@ -1,11 +1,13 @@ # Copyright (c) 2013, Frappe Technologies Pvt. Ltd. and contributors # For license information, please see license.txt - import copy +from collections import OrderedDict import frappe -from frappe import _ +from frappe import _, qb +from frappe.query_builder import CustomFunction +from frappe.query_builder.functions import Max from frappe.utils import date_diff, flt, getdate @@ -18,11 +20,12 @@ def execute(filters=None): columns = get_columns(filters) conditions = get_conditions(filters) data = get_data(conditions, filters) + so_elapsed_time = get_so_elapsed_time(data) if not data: return [], [], None, [] - data, chart_data = prepare_data(data, filters) + data, chart_data = prepare_data(data, so_elapsed_time, filters) return columns, data, None, chart_data @@ -65,7 +68,6 @@ def get_data(conditions, filters): IF(so.status in ('Completed','To Bill'), 0, (SELECT delay_days)) as delay, soi.qty, soi.delivered_qty, (soi.qty - soi.delivered_qty) AS pending_qty, - IF((SELECT pending_qty) = 0, (TO_SECONDS(Max(dn.posting_date))-TO_SECONDS(so.transaction_date)), 0) as time_taken_to_deliver, IFNULL(SUM(sii.qty), 0) as billed_qty, soi.base_amount as amount, (soi.delivered_qty * soi.base_rate) as delivered_qty_amount, @@ -76,13 +78,9 @@ def get_data(conditions, filters): soi.description as description FROM `tabSales Order` so, - (`tabSales Order Item` soi + `tabSales Order Item` soi LEFT JOIN `tabSales Invoice Item` sii - ON sii.so_detail = soi.name and sii.docstatus = 1) - LEFT JOIN `tabDelivery Note Item` dni - on dni.so_detail = soi.name - LEFT JOIN `tabDelivery Note` dn - on dni.parent = dn.name and dn.docstatus = 1 + ON sii.so_detail = soi.name and sii.docstatus = 1 WHERE soi.parent = so.name and so.status not in ('Stopped', 'Closed', 'On Hold') @@ -100,7 +98,48 @@ def get_data(conditions, filters): return data -def prepare_data(data, filters): +def get_so_elapsed_time(data): + """ + query SO's elapsed time till latest delivery note + """ + so_elapsed_time = OrderedDict() + if data: + sales_orders = [x.sales_order for x in data] + + so = qb.DocType("Sales Order") + soi = qb.DocType("Sales Order Item") + dn = qb.DocType("Delivery Note") + dni = qb.DocType("Delivery Note Item") + + to_seconds = CustomFunction("TO_SECONDS", ["date"]) + + query = ( + qb.from_(so) + .inner_join(soi) + .on(soi.parent == so.name) + .left_join(dni) + .on(dni.so_detail == soi.name) + .left_join(dn) + .on(dni.parent == dn.name) + .select( + so.name.as_("sales_order"), + soi.item_code.as_("so_item_code"), + (to_seconds(Max(dn.posting_date)) - to_seconds(so.transaction_date)).as_("elapsed_seconds"), + ) + .where((so.name.isin(sales_orders)) & (dn.docstatus == 1)) + .orderby(so.name, soi.name) + .groupby(soi.name) + ) + dn_elapsed_time = query.run(as_dict=True) + + for e in dn_elapsed_time: + key = (e.sales_order, e.so_item_code) + so_elapsed_time[key] = e.elapsed_seconds + + return so_elapsed_time + + +def prepare_data(data, so_elapsed_time, filters): completed, pending = 0, 0 if filters.get("group_by_so"): @@ -115,6 +154,13 @@ def prepare_data(data, filters): row["qty_to_bill"] = flt(row["qty"]) - flt(row["billed_qty"]) row["delay"] = 0 if row["delay"] and row["delay"] < 0 else row["delay"] + + row["time_taken_to_deliver"] = ( + so_elapsed_time.get((row.sales_order, row.item_code)) + if row["status"] in ("To Bill", "Completed") + else 0 + ) + if filters.get("group_by_so"): so_name = row["sales_order"] From 4f1bfbb93dc80f9f459d663cbe43433e47431ad5 Mon Sep 17 00:00:00 2001 From: ruthra kumar Date: Tue, 31 May 2022 12:10:04 +0530 Subject: [PATCH 02/22] test: multiple delivery notes and billed quantity --- .../test_sales_order_analysis.py | 106 ++++++++++++++++-- 1 file changed, 97 insertions(+), 9 deletions(-) diff --git a/erpnext/selling/report/sales_order_analysis/test_sales_order_analysis.py b/erpnext/selling/report/sales_order_analysis/test_sales_order_analysis.py index 25cbb734499..241f4358fba 100644 --- a/erpnext/selling/report/sales_order_analysis/test_sales_order_analysis.py +++ b/erpnext/selling/report/sales_order_analysis/test_sales_order_analysis.py @@ -11,7 +11,7 @@ test_dependencies = ["Sales Order", "Item", "Sales Invoice", "Delivery Note"] class TestSalesOrderAnalysis(FrappeTestCase): - def create_sales_order(self, transaction_date): + def create_sales_order(self, transaction_date, do_not_save=False, do_not_submit=False): item = create_item(item_code="_Test Excavator", is_stock_item=0) so = make_sales_order( transaction_date=transaction_date, @@ -24,25 +24,31 @@ class TestSalesOrderAnalysis(FrappeTestCase): so.taxes_and_charges = "" so.taxes = "" so.items[0].delivery_date = add_days(transaction_date, 15) - so.save() - so.submit() + if not do_not_save: + so.save() + if not do_not_submit: + so.submit() return item, so - def create_sales_invoice(self, so): + def create_sales_invoice(self, so, do_not_save=False, do_not_submit=False): sinv = make_sales_invoice(so.name) sinv.posting_date = so.transaction_date sinv.taxes_and_charges = "" sinv.taxes = "" - sinv.insert() - sinv.submit() + if not do_not_save: + sinv.save() + if not do_not_submit: + sinv.submit() return sinv - def create_delivery_note(self, so): + def create_delivery_note(self, so, do_not_save=False, do_not_submit=False): dn = make_delivery_note(so.name) dn.set_posting_time = True dn.posting_date = add_days(so.transaction_date, 1) - dn.save() - dn.submit() + if not do_not_save: + dn.save() + if not do_not_submit: + dn.submit() return dn def test_01_so_to_deliver_and_bill(self): @@ -164,3 +170,85 @@ class TestSalesOrderAnalysis(FrappeTestCase): ) # SO's from first 4 test cases should be in output self.assertEqual(len(data), 4) + + def test_06_so_pending_delivery_with_multiple_delivery_notes(self): + transaction_date = "2021-06-01" + item, so = self.create_sales_order(transaction_date) + + # bill 2 items + sinv1 = self.create_sales_invoice(so, do_not_save=True) + sinv1.items[0].qty = 2 + sinv1 = sinv1.save().submit() + # deliver 2 items + dn1 = self.create_delivery_note(so, do_not_save=True) + dn1.items[0].qty = 2 + dn1 = dn1.save().submit() + + # bill 2 items + sinv2 = self.create_sales_invoice(so, do_not_save=True) + sinv2.items[0].qty = 2 + sinv2 = sinv2.save().submit() + # deliver 1 item + dn2 = self.create_delivery_note(so, do_not_save=True) + dn2.items[0].qty = 1 + dn2 = dn2.save().submit() + + columns, data, message, chart = execute( + { + "company": "_Test Company", + "from_date": "2021-06-01", + "to_date": "2021-06-30", + "sales_order": [so.name], + } + ) + expected_value = { + "status": "To Deliver and Bill", + "sales_order": so.name, + "delay_days": frappe.utils.date_diff(frappe.utils.datetime.date.today(), so.delivery_date), + "qty": 10, + "delivered_qty": 3, + "pending_qty": 7, + "qty_to_bill": 6, + "billed_qty": 4, + "time_taken_to_deliver": 0, + } + self.assertEqual(len(data), 1) + for key, val in expected_value.items(): + with self.subTest(key=key, val=val): + self.assertEqual(data[0][key], val) + + def test_07_so_delivered_with_multiple_delivery_notes(self): + transaction_date = "2021-06-01" + item, so = self.create_sales_order(transaction_date) + + dn1 = self.create_delivery_note(so, do_not_save=True) + dn1.items[0].qty = 5 + dn1 = dn1.save().submit() + + dn2 = self.create_delivery_note(so, do_not_save=True) + dn2.items[0].qty = 5 + dn2 = dn2.save().submit() + + columns, data, message, chart = execute( + { + "company": "_Test Company", + "from_date": "2021-06-01", + "to_date": "2021-06-30", + "sales_order": [so.name], + } + ) + expected_value = { + "status": "To Bill", + "sales_order": so.name, + "delay_days": frappe.utils.date_diff(frappe.utils.datetime.date.today(), so.delivery_date), + "qty": 10, + "delivered_qty": 10, + "pending_qty": 0, + "qty_to_bill": 10, + "billed_qty": 0, + "time_taken_to_deliver": 86400, + } + self.assertEqual(len(data), 1) + for key, val in expected_value.items(): + with self.subTest(key=key, val=val): + self.assertEqual(data[0][key], val) From a200e7e1fbb14baf547e47f9644c8b2819916e41 Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Sun, 5 Jun 2022 11:16:12 +0530 Subject: [PATCH 03/22] fix: Remove redundant query --- .../item_wise_sales_register.py | 29 ++++++++++++------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/erpnext/accounts/report/item_wise_sales_register/item_wise_sales_register.py b/erpnext/accounts/report/item_wise_sales_register/item_wise_sales_register.py index 2e7213f42b1..ac706666547 100644 --- a/erpnext/accounts/report/item_wise_sales_register/item_wise_sales_register.py +++ b/erpnext/accounts/report/item_wise_sales_register/item_wise_sales_register.py @@ -443,12 +443,6 @@ def get_grand_total(filters, doctype): ] # nosec -def get_deducted_taxes(): - return frappe.db.sql_list( - "select name from `tabPurchase Taxes and Charges` where add_deduct_tax = 'Deduct'" - ) - - def get_tax_accounts( item_list, columns, @@ -462,6 +456,7 @@ def get_tax_accounts( tax_columns = [] invoice_item_row = {} itemised_tax = {} + add_deduct_tax = "charge_type" tax_amount_precision = ( get_field_precision( @@ -477,13 +472,13 @@ def get_tax_accounts( conditions = "" if doctype == "Purchase Invoice": conditions = " and category in ('Total', 'Valuation and Total') and base_tax_amount_after_discount_amount != 0" + add_deduct_tax = "add_deduct_tax" - deducted_tax = get_deducted_taxes() tax_details = frappe.db.sql( """ select name, parent, description, item_wise_tax_detail, - charge_type, base_tax_amount_after_discount_amount + charge_type, {add_deduct_tax}, base_tax_amount_after_discount_amount from `tab%s` where parenttype = %s and docstatus = 1 @@ -491,12 +486,22 @@ def get_tax_accounts( and parent in (%s) %s order by description - """ + """.format( + add_deduct_tax=add_deduct_tax + ) % (tax_doctype, "%s", ", ".join(["%s"] * len(invoice_item_row)), conditions), tuple([doctype] + list(invoice_item_row)), ) - for name, parent, description, item_wise_tax_detail, charge_type, tax_amount in tax_details: + for ( + name, + parent, + description, + item_wise_tax_detail, + charge_type, + add_deduct_tax, + tax_amount, + ) in tax_details: description = handle_html(description) if description not in tax_columns and tax_amount: # as description is text editor earlier and markup can break the column convention in reports @@ -529,7 +534,9 @@ def get_tax_accounts( if item_tax_amount: tax_value = flt(item_tax_amount, tax_amount_precision) tax_value = ( - tax_value * -1 if (doctype == "Purchase Invoice" and name in deducted_tax) else tax_value + tax_value * -1 + if (doctype == "Purchase Invoice" and add_deduct_tax == "Deduct") + else tax_value ) itemised_tax.setdefault(d.name, {})[description] = frappe._dict( From 61fa4eb6c947525a948ec3212e3d7af10eed815f Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Sun, 5 Jun 2022 18:23:24 +0530 Subject: [PATCH 04/22] fix: Reverse provisional entries on Purchase Invoice cancel --- .../doctype/purchase_invoice/purchase_invoice.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py b/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py index e6da6669ac2..deb905b0093 100644 --- a/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py +++ b/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py @@ -532,7 +532,10 @@ class PurchaseInvoice(BuyingController): def make_gl_entries(self, gl_entries=None, from_repost=False): if not gl_entries: - gl_entries = self.get_gl_entries() + if self.docstatus == 1: + gl_entries = self.get_gl_entries() + else: + gl_entries = self.get_gl_entries(cancel=1) if gl_entries: update_outstanding = "No" if (cint(self.is_paid) or self.write_off_account) else "Yes" @@ -545,7 +548,10 @@ class PurchaseInvoice(BuyingController): from_repost=from_repost, ) elif self.docstatus == 2: + provisional_entries = [a for a in gl_entries if a.voucher_type == "Purchase Receipt"] make_reverse_gl_entries(voucher_type=self.doctype, voucher_no=self.name) + if provisional_entries: + make_gl_entries(provisional_entries) if update_outstanding == "No": update_outstanding_amt( @@ -559,7 +565,7 @@ class PurchaseInvoice(BuyingController): elif self.docstatus == 2 and cint(self.update_stock) and self.auto_accounting_for_stock: make_reverse_gl_entries(voucher_type=self.doctype, voucher_no=self.name) - def get_gl_entries(self, warehouse_account=None): + def get_gl_entries(self, warehouse_account=None, cancel=0): self.auto_accounting_for_stock = erpnext.is_perpetual_inventory_enabled(self.company) if self.auto_accounting_for_stock: self.stock_received_but_not_billed = self.get_company_default("stock_received_but_not_billed") @@ -572,7 +578,7 @@ class PurchaseInvoice(BuyingController): gl_entries = [] self.make_supplier_gl_entry(gl_entries) - self.make_item_gl_entries(gl_entries) + self.make_item_gl_entries(gl_entries, cancel=cancel) self.make_discount_gl_entries(gl_entries) if self.check_asset_cwip_enabled(): @@ -639,7 +645,7 @@ class PurchaseInvoice(BuyingController): ) ) - def make_item_gl_entries(self, gl_entries): + def make_item_gl_entries(self, gl_entries, cancel=0): # item gl entries stock_items = self.get_stock_items() if self.update_stock and self.auto_accounting_for_stock: @@ -836,7 +842,7 @@ class PurchaseInvoice(BuyingController): if expense_booked_in_pr: # Intentionally passing purchase invoice item to handle partial billing purchase_receipt_doc.add_provisional_gl_entry( - item, gl_entries, self.posting_date, provisional_account, reverse=1 + item, gl_entries, self.posting_date, provisional_account, reverse=not cancel ) if not self.is_internal_transfer(): From 86a24f3d223c0ede3b8e9762bd166285b39a9b10 Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Sun, 5 Jun 2022 19:12:24 +0530 Subject: [PATCH 05/22] fix: Simply cancel reverse entries --- .../purchase_invoice/purchase_invoice.py | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py b/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py index deb905b0093..07173a31c2b 100644 --- a/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py +++ b/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py @@ -532,10 +532,7 @@ class PurchaseInvoice(BuyingController): def make_gl_entries(self, gl_entries=None, from_repost=False): if not gl_entries: - if self.docstatus == 1: - gl_entries = self.get_gl_entries() - else: - gl_entries = self.get_gl_entries(cancel=1) + gl_entries = self.get_gl_entries() if gl_entries: update_outstanding = "No" if (cint(self.is_paid) or self.write_off_account) else "Yes" @@ -551,7 +548,13 @@ class PurchaseInvoice(BuyingController): provisional_entries = [a for a in gl_entries if a.voucher_type == "Purchase Receipt"] make_reverse_gl_entries(voucher_type=self.doctype, voucher_no=self.name) if provisional_entries: - make_gl_entries(provisional_entries) + for entry in provisional_entries: + frappe.db.set_value( + "GL Entry", + {"voucher_type": "Purchase Receipt", "voucher_detail_no": entry.voucher_detail_no}, + "is_cancelled", + 1, + ) if update_outstanding == "No": update_outstanding_amt( @@ -565,7 +568,7 @@ class PurchaseInvoice(BuyingController): elif self.docstatus == 2 and cint(self.update_stock) and self.auto_accounting_for_stock: make_reverse_gl_entries(voucher_type=self.doctype, voucher_no=self.name) - def get_gl_entries(self, warehouse_account=None, cancel=0): + def get_gl_entries(self, warehouse_account=None): self.auto_accounting_for_stock = erpnext.is_perpetual_inventory_enabled(self.company) if self.auto_accounting_for_stock: self.stock_received_but_not_billed = self.get_company_default("stock_received_but_not_billed") @@ -578,7 +581,7 @@ class PurchaseInvoice(BuyingController): gl_entries = [] self.make_supplier_gl_entry(gl_entries) - self.make_item_gl_entries(gl_entries, cancel=cancel) + self.make_item_gl_entries(gl_entries) self.make_discount_gl_entries(gl_entries) if self.check_asset_cwip_enabled(): @@ -645,7 +648,7 @@ class PurchaseInvoice(BuyingController): ) ) - def make_item_gl_entries(self, gl_entries, cancel=0): + def make_item_gl_entries(self, gl_entries): # item gl entries stock_items = self.get_stock_items() if self.update_stock and self.auto_accounting_for_stock: @@ -842,7 +845,7 @@ class PurchaseInvoice(BuyingController): if expense_booked_in_pr: # Intentionally passing purchase invoice item to handle partial billing purchase_receipt_doc.add_provisional_gl_entry( - item, gl_entries, self.posting_date, provisional_account, reverse=not cancel + item, gl_entries, self.posting_date, provisional_account, reverse=1 ) if not self.is_internal_transfer(): From c3219ebad1cac35afc04cc051c9e215c70cd1e9b Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 6 Jun 2022 12:39:12 +0530 Subject: [PATCH 06/22] fix(Sales Register): incorrect query with dimensions If accounting dimension is also part of the default filters then same query is repeated with incorrect syntax. e.g. `item_group = (child1, child2)` instead of `in` query. fix: don't add default filter if they are part of dimensions to be added. --- .../report/sales_register/sales_register.py | 40 +++++++------------ 1 file changed, 15 insertions(+), 25 deletions(-) diff --git a/erpnext/accounts/report/sales_register/sales_register.py b/erpnext/accounts/report/sales_register/sales_register.py index 34b3f032068..777d96ced17 100644 --- a/erpnext/accounts/report/sales_register/sales_register.py +++ b/erpnext/accounts/report/sales_register/sales_register.py @@ -346,9 +346,13 @@ def get_columns(invoice_list, additional_table_columns): def get_conditions(filters): conditions = "" + accounting_dimensions = get_accounting_dimensions(as_list=False) or [] + accounting_dimensions_list = [d.fieldname for d in accounting_dimensions] + if filters.get("company"): conditions += " and company=%(company)s" - if filters.get("customer"): + + if filters.get("customer") and "customer" not in accounting_dimensions_list: conditions += " and customer = %(customer)s" if filters.get("from_date"): @@ -359,32 +363,18 @@ def get_conditions(filters): if filters.get("owner"): conditions += " and owner = %(owner)s" - if filters.get("mode_of_payment"): - conditions += """ and exists(select name from `tabSales Invoice Payment` + def get_sales_invoice_item_field_condition(field, table="Sales Invoice Item") -> str: + if not filters.get(field) or field in accounting_dimensions_list: + return "" + return f""" and exists(select name from `tab{table}` where parent=`tabSales Invoice`.name - and ifnull(`tabSales Invoice Payment`.mode_of_payment, '') = %(mode_of_payment)s)""" + and ifnull(`tab{table}`.{field}, '') = %({field})s)""" - if filters.get("cost_center"): - conditions += """ and exists(select name from `tabSales Invoice Item` - where parent=`tabSales Invoice`.name - and ifnull(`tabSales Invoice Item`.cost_center, '') = %(cost_center)s)""" - - if filters.get("warehouse"): - conditions += """ and exists(select name from `tabSales Invoice Item` - where parent=`tabSales Invoice`.name - and ifnull(`tabSales Invoice Item`.warehouse, '') = %(warehouse)s)""" - - if filters.get("brand"): - conditions += """ and exists(select name from `tabSales Invoice Item` - where parent=`tabSales Invoice`.name - and ifnull(`tabSales Invoice Item`.brand, '') = %(brand)s)""" - - if filters.get("item_group"): - conditions += """ and exists(select name from `tabSales Invoice Item` - where parent=`tabSales Invoice`.name - and ifnull(`tabSales Invoice Item`.item_group, '') = %(item_group)s)""" - - accounting_dimensions = get_accounting_dimensions(as_list=False) + conditions += get_sales_invoice_item_field_condition("mode_of_payments", "Sales Invoice Payment") + conditions += get_sales_invoice_item_field_condition("cost_center") + conditions += get_sales_invoice_item_field_condition("warehouse") + conditions += get_sales_invoice_item_field_condition("brand") + conditions += get_sales_invoice_item_field_condition("item_group") if accounting_dimensions: common_condition = """ From 4decb7a02be94a98ad8dea9d5d4db37f7410b1d2 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Fri, 3 Jun 2022 11:03:12 +0530 Subject: [PATCH 07/22] fix: Consider only Approved leave applications in LWP, Employee Benefit calculations - do not allow submitting leave applications with 'Cancelled' status --- erpnext/hr/doctype/leave_application/leave_application.py | 5 +++-- .../employee_benefit_application.py | 1 + erpnext/payroll/doctype/salary_slip/salary_slip.py | 1 + 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/erpnext/hr/doctype/leave_application/leave_application.py b/erpnext/hr/doctype/leave_application/leave_application.py index cd6b1686675..5a338bdfe83 100755 --- a/erpnext/hr/doctype/leave_application/leave_application.py +++ b/erpnext/hr/doctype/leave_application/leave_application.py @@ -88,7 +88,7 @@ class LeaveApplication(Document): share_doc_with_approver(self, self.leave_approver) def on_submit(self): - if self.status == "Open": + if self.status in ["Open", "Cancelled"]: frappe.throw( _("Only Leave Applications with status 'Approved' and 'Rejected' can be submitted") ) @@ -1117,7 +1117,7 @@ def add_leaves(events, start, end, filter_conditions=None): WHERE from_date <= %(end)s AND to_date >= %(start)s <= to_date AND docstatus < 2 - AND status != 'Rejected' + AND status in ('Approved', 'Open') """ if conditions: @@ -1206,6 +1206,7 @@ def get_approved_leaves_for_period(employee, leave_type, from_date, to_date): from `tabLeave Application` where employee=%(employee)s and docstatus=1 + and status='Approved' and (from_date between %(from_date)s and %(to_date)s or to_date between %(from_date)s and %(to_date)s or (from_date < %(from_date)s and to_date > %(to_date)s)) diff --git a/erpnext/payroll/doctype/employee_benefit_application/employee_benefit_application.py b/erpnext/payroll/doctype/employee_benefit_application/employee_benefit_application.py index 0acd44711b0..592e7dd6f09 100644 --- a/erpnext/payroll/doctype/employee_benefit_application/employee_benefit_application.py +++ b/erpnext/payroll/doctype/employee_benefit_application/employee_benefit_application.py @@ -216,6 +216,7 @@ def calculate_lwp(employee, start_date, holidays, working_days): where t2.name = t1.leave_type and t2.is_lwp = 1 and t1.docstatus = 1 + and t1.status = 'Approved' and t1.employee = %(employee)s and CASE WHEN t2.include_holiday != 1 THEN %(dt)s not in ('{0}') and %(dt)s between from_date and to_date WHEN t2.include_holiday THEN %(dt)s between from_date and to_date diff --git a/erpnext/payroll/doctype/salary_slip/salary_slip.py b/erpnext/payroll/doctype/salary_slip/salary_slip.py index 4c5fea1e759..378227f0463 100644 --- a/erpnext/payroll/doctype/salary_slip/salary_slip.py +++ b/erpnext/payroll/doctype/salary_slip/salary_slip.py @@ -477,6 +477,7 @@ class SalarySlip(TransactionBase): WHERE t2.name = t1.leave_type AND (t2.is_lwp = 1 or t2.is_ppl = 1) AND t1.docstatus = 1 + AND t1.status = 'Approved' AND t1.employee = %(employee)s AND ifnull(t1.salary_slip, '') = '' AND CASE From 59de303b131c49f5ffacb3142f58b688e2d1c926 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Fri, 3 Jun 2022 16:47:42 +0530 Subject: [PATCH 08/22] refactor: rewrite lwp queries using query builder --- .../leave_application/leave_application.py | 42 ++++++---- .../employee_benefit_application.py | 44 ++++++----- .../doctype/salary_slip/salary_slip.py | 77 ++++++++++++------- 3 files changed, 99 insertions(+), 64 deletions(-) diff --git a/erpnext/hr/doctype/leave_application/leave_application.py b/erpnext/hr/doctype/leave_application/leave_application.py index 5a338bdfe83..53c5df4210a 100755 --- a/erpnext/hr/doctype/leave_application/leave_application.py +++ b/erpnext/hr/doctype/leave_application/leave_application.py @@ -1201,25 +1201,33 @@ def get_mandatory_approval(doctype): def get_approved_leaves_for_period(employee, leave_type, from_date, to_date): - query = """ - select employee, leave_type, from_date, to_date, total_leave_days - from `tabLeave Application` - where employee=%(employee)s - and docstatus=1 - and status='Approved' - and (from_date between %(from_date)s and %(to_date)s - or to_date between %(from_date)s and %(to_date)s - or (from_date < %(from_date)s and to_date > %(to_date)s)) - """ - if leave_type: - query += "and leave_type=%(leave_type)s" - - leave_applications = frappe.db.sql( - query, - {"from_date": from_date, "to_date": to_date, "employee": employee, "leave_type": leave_type}, - as_dict=1, + LeaveApplication = frappe.qb.DocType("Leave Application") + query = ( + frappe.qb.from_(LeaveApplication) + .select( + LeaveApplication.employee, + LeaveApplication.leave_type, + LeaveApplication.from_date, + LeaveApplication.to_date, + LeaveApplication.total_leave_days, + ) + .where( + (LeaveApplication.employee == employee) + & (LeaveApplication.docstatus == 1) + & (LeaveApplication.status == "Approved") + & ( + (LeaveApplication.from_date.between(from_date, to_date)) + | (LeaveApplication.to_date.between(from_date, to_date)) + | ((LeaveApplication.from_date < from_date) & (LeaveApplication.to_date > to_date)) + ) + ) ) + if leave_type: + query = query.where(LeaveApplication.leave_type == leave_type) + + leave_applications = query.run(as_dict=True) + leave_days = 0 for leave_app in leave_applications: if leave_app.from_date >= getdate(from_date) and leave_app.to_date <= getdate(to_date): diff --git a/erpnext/payroll/doctype/employee_benefit_application/employee_benefit_application.py b/erpnext/payroll/doctype/employee_benefit_application/employee_benefit_application.py index 592e7dd6f09..8dad7cc8bc9 100644 --- a/erpnext/payroll/doctype/employee_benefit_application/employee_benefit_application.py +++ b/erpnext/payroll/doctype/employee_benefit_application/employee_benefit_application.py @@ -207,27 +207,35 @@ def get_max_benefits_remaining(employee, on_date, payroll_period): def calculate_lwp(employee, start_date, holidays, working_days): lwp = 0 holidays = "','".join(holidays) + for d in range(working_days): dt = add_days(cstr(getdate(start_date)), d) - leave = frappe.db.sql( - """ - select t1.name, t1.half_day - from `tabLeave Application` t1, `tabLeave Type` t2 - where t2.name = t1.leave_type - and t2.is_lwp = 1 - and t1.docstatus = 1 - and t1.status = 'Approved' - and t1.employee = %(employee)s - and CASE WHEN t2.include_holiday != 1 THEN %(dt)s not in ('{0}') and %(dt)s between from_date and to_date - WHEN t2.include_holiday THEN %(dt)s between from_date and to_date - END - """.format( - holidays - ), - {"employee": employee, "dt": dt}, + + LeaveApplication = frappe.qb.DocType("Leave Application") + LeaveType = frappe.qb.DocType("Leave Type") + + query = ( + frappe.qb.from_(LeaveApplication) + .inner_join(LeaveType) + .on((LeaveType.name == LeaveApplication.leave_type)) + .select(LeaveApplication.name, LeaveApplication.half_day) + .where( + (LeaveType.is_lwp == 1) + & (LeaveApplication.docstatus == 1) + & (LeaveApplication.status == "Approved") + & (LeaveApplication.employee == employee) + & ((LeaveApplication.from_date <= dt) & (dt <= LeaveApplication.to_date)) + ) ) - if leave: - lwp = cint(leave[0][1]) and (lwp + 0.5) or (lwp + 1) + + # if it's a holiday only include if leave type has "include holiday" enabled + if dt in holidays: + query = query.where((LeaveType.include_holiday == "1")) + leaves = query.run() + + if leaves: + lwp = cint(leaves[0][1]) and (lwp + 0.5) or (lwp + 1) + return lwp diff --git a/erpnext/payroll/doctype/salary_slip/salary_slip.py b/erpnext/payroll/doctype/salary_slip/salary_slip.py index 378227f0463..6a35985e64c 100644 --- a/erpnext/payroll/doctype/salary_slip/salary_slip.py +++ b/erpnext/payroll/doctype/salary_slip/salary_slip.py @@ -465,38 +465,14 @@ class SalarySlip(TransactionBase): ) for d in range(working_days): - dt = add_days(cstr(getdate(self.start_date)), d) - leave = frappe.db.sql( - """ - SELECT t1.name, - CASE WHEN (t1.half_day_date = %(dt)s or t1.to_date = t1.from_date) - THEN t1.half_day else 0 END, - t2.is_ppl, - t2.fraction_of_daily_salary_per_leave - FROM `tabLeave Application` t1, `tabLeave Type` t2 - WHERE t2.name = t1.leave_type - AND (t2.is_lwp = 1 or t2.is_ppl = 1) - AND t1.docstatus = 1 - AND t1.status = 'Approved' - AND t1.employee = %(employee)s - AND ifnull(t1.salary_slip, '') = '' - AND CASE - WHEN t2.include_holiday != 1 - THEN %(dt)s not in ('{0}') and %(dt)s between from_date and to_date - WHEN t2.include_holiday - THEN %(dt)s between from_date and to_date - END - """.format( - holidays - ), - {"employee": self.employee, "dt": dt}, - ) + date = add_days(cstr(getdate(self.start_date)), d) + leave = get_lwp_or_ppl_for_date(date, self.employee, holidays) if leave: equivalent_lwp_count = 0 - is_half_day_leave = cint(leave[0][1]) - is_partially_paid_leave = cint(leave[0][2]) - fraction_of_daily_salary_per_leave = flt(leave[0][3]) + is_half_day_leave = cint(leave[0].is_half_day) + is_partially_paid_leave = cint(leave[0].is_ppl) + fraction_of_daily_salary_per_leave = flt(leave[0].fraction_of_daily_salary_per_leave) equivalent_lwp_count = (1 - daily_wages_fraction_for_half_day) if is_half_day_leave else 1 @@ -1743,3 +1719,46 @@ def eval_tax_slab_condition(condition, eval_globals=None, eval_locals=None): except Exception as e: frappe.throw(_("Error in formula or condition: {0} in Income Tax Slab").format(e)) raise + + +def get_lwp_or_ppl_for_date(date, employee, holidays): + LeaveApplication = frappe.qb.DocType("Leave Application") + LeaveType = frappe.qb.DocType("Leave Type") + + is_half_day = ( + frappe.qb.terms.Case() + .when( + ( + (LeaveApplication.half_day_date == date) + | (LeaveApplication.from_date == LeaveApplication.to_date) + ), + LeaveApplication.half_day, + ) + .else_(0) + ).as_("is_half_day") + + query = ( + frappe.qb.from_(LeaveApplication) + .inner_join(LeaveType) + .on((LeaveType.name == LeaveApplication.leave_type)) + .select( + LeaveApplication.name, + LeaveType.is_ppl, + LeaveType.fraction_of_daily_salary_per_leave, + (is_half_day), + ) + .where( + (((LeaveType.is_lwp == 1) | (LeaveType.is_ppl == 1))) + & (LeaveApplication.docstatus == 1) + & (LeaveApplication.status == "Approved") + & (LeaveApplication.employee == employee) + & ((LeaveApplication.salary_slip.isnull()) | (LeaveApplication.salary_slip == "")) + & ((LeaveApplication.from_date <= date) & (date <= LeaveApplication.to_date)) + ) + ) + + # if it's a holiday only include if leave type has "include holiday" enabled + if date in holidays: + query = query.where((LeaveType.include_holiday == "1")) + + return query.run(as_dict=True) From cf04683ad333ca1d1b34d4bfb9a64b69e539657a Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Fri, 3 Jun 2022 17:00:01 +0530 Subject: [PATCH 09/22] fix(Leave Application): 'Cancelled' status shown as 'Open' in list view --- .../leave_application/leave_application_list.js | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/erpnext/hr/doctype/leave_application/leave_application_list.js b/erpnext/hr/doctype/leave_application/leave_application_list.js index a3c03b1bec7..157271a5a0e 100644 --- a/erpnext/hr/doctype/leave_application/leave_application_list.js +++ b/erpnext/hr/doctype/leave_application/leave_application_list.js @@ -1,13 +1,14 @@ -frappe.listview_settings['Leave Application'] = { +frappe.listview_settings["Leave Application"] = { add_fields: ["leave_type", "employee", "employee_name", "total_leave_days", "from_date", "to_date"], has_indicator_for_draft: 1, get_indicator: function (doc) { - if (doc.status === "Approved") { - return [__("Approved"), "green", "status,=,Approved"]; - } else if (doc.status === "Rejected") { - return [__("Rejected"), "red", "status,=,Rejected"]; - } else { - return [__("Open"), "red", "status,=,Open"]; - } + let status_color = { + "Approved": "green", + "Rejected": "red", + "Open": "orange", + "Cancelled": "red", + "Submitted": "blue" + }; + return [__(doc.status), status_color[doc.status], "status,=," + doc.status]; } }; From 3b8bc7d8e1446d8b48f7f95924842650cb3f74e3 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Mon, 6 Jun 2022 13:03:05 +0530 Subject: [PATCH 10/22] fix: incorrect LWP calculation for half days in employee benefit application --- .../employee_benefit_application.py | 38 +++++++++++++------ 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/erpnext/payroll/doctype/employee_benefit_application/employee_benefit_application.py b/erpnext/payroll/doctype/employee_benefit_application/employee_benefit_application.py index 8dad7cc8bc9..8df1bb6e87e 100644 --- a/erpnext/payroll/doctype/employee_benefit_application/employee_benefit_application.py +++ b/erpnext/payroll/doctype/employee_benefit_application/employee_benefit_application.py @@ -5,7 +5,7 @@ import frappe from frappe import _ from frappe.model.document import Document -from frappe.utils import add_days, cint, cstr, date_diff, getdate, rounded +from frappe.utils import add_days, cstr, date_diff, flt, getdate, rounded from erpnext.hr.utils import ( get_holiday_dates_for_employee, @@ -27,11 +27,14 @@ class EmployeeBenefitApplication(Document): validate_active_employee(self.employee) self.validate_duplicate_on_payroll_period() if not self.max_benefits: - self.max_benefits = get_max_benefits_remaining(self.employee, self.date, self.payroll_period) + self.max_benefits = flt( + get_max_benefits_remaining(self.employee, self.date, self.payroll_period), + self.precision("max_benefits"), + ) if self.max_benefits and self.max_benefits > 0: self.validate_max_benefit_for_component() self.validate_prev_benefit_claim() - if self.remaining_benefit > 0: + if self.remaining_benefit and self.remaining_benefit > 0: self.validate_remaining_benefit_amount() else: frappe.throw( @@ -110,7 +113,7 @@ class EmployeeBenefitApplication(Document): max_benefit_amount = 0 for employee_benefit in self.employee_benefits: self.validate_max_benefit(employee_benefit.earning_component) - max_benefit_amount += employee_benefit.amount + max_benefit_amount += flt(employee_benefit.amount) if max_benefit_amount > self.max_benefits: frappe.throw( _("Maximum benefit amount of employee {0} exceeds {1}").format( @@ -125,7 +128,8 @@ class EmployeeBenefitApplication(Document): benefit_amount = 0 for employee_benefit in self.employee_benefits: if employee_benefit.earning_component == earning_component_name: - benefit_amount += employee_benefit.amount + benefit_amount += flt(employee_benefit.amount) + prev_sal_slip_flexi_amount = get_sal_slip_total_benefit_given( self.employee, frappe.get_doc("Payroll Period", self.payroll_period), earning_component_name ) @@ -209,32 +213,44 @@ def calculate_lwp(employee, start_date, holidays, working_days): holidays = "','".join(holidays) for d in range(working_days): - dt = add_days(cstr(getdate(start_date)), d) + date = add_days(cstr(getdate(start_date)), d) LeaveApplication = frappe.qb.DocType("Leave Application") LeaveType = frappe.qb.DocType("Leave Type") + is_half_day = ( + frappe.qb.terms.Case() + .when( + ( + (LeaveApplication.half_day_date == date) + | (LeaveApplication.from_date == LeaveApplication.to_date) + ), + LeaveApplication.half_day, + ) + .else_(0) + ).as_("is_half_day") + query = ( frappe.qb.from_(LeaveApplication) .inner_join(LeaveType) .on((LeaveType.name == LeaveApplication.leave_type)) - .select(LeaveApplication.name, LeaveApplication.half_day) + .select(LeaveApplication.name, is_half_day) .where( (LeaveType.is_lwp == 1) & (LeaveApplication.docstatus == 1) & (LeaveApplication.status == "Approved") & (LeaveApplication.employee == employee) - & ((LeaveApplication.from_date <= dt) & (dt <= LeaveApplication.to_date)) + & ((LeaveApplication.from_date <= date) & (date <= LeaveApplication.to_date)) ) ) # if it's a holiday only include if leave type has "include holiday" enabled - if dt in holidays: + if date in holidays: query = query.where((LeaveType.include_holiday == "1")) - leaves = query.run() + leaves = query.run(as_dict=True) if leaves: - lwp = cint(leaves[0][1]) and (lwp + 0.5) or (lwp + 1) + lwp += 0.5 if leaves[0].is_half_day else 1 return lwp From edb775c3814ec94537f790d92b745e9672a45f21 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Mon, 6 Jun 2022 13:42:23 +0530 Subject: [PATCH 11/22] test: Employee Benefit Application - make `get_no_of_days` a function for reusability --- .../test_employee_benefit_application.py | 80 ++++++++++++++++++- .../doctype/salary_slip/test_salary_slip.py | 40 +++++----- 2 files changed, 99 insertions(+), 21 deletions(-) diff --git a/erpnext/payroll/doctype/employee_benefit_application/test_employee_benefit_application.py b/erpnext/payroll/doctype/employee_benefit_application/test_employee_benefit_application.py index 02149adfce5..de8f9b6a7ad 100644 --- a/erpnext/payroll/doctype/employee_benefit_application/test_employee_benefit_application.py +++ b/erpnext/payroll/doctype/employee_benefit_application/test_employee_benefit_application.py @@ -3,6 +3,82 @@ import unittest +import frappe +from frappe.tests.utils import FrappeTestCase +from frappe.utils import add_days, date_diff, get_year_ending, get_year_start, getdate -class TestEmployeeBenefitApplication(unittest.TestCase): - pass +from erpnext.hr.doctype.employee.test_employee import make_employee +from erpnext.hr.doctype.holiday_list.test_holiday_list import set_holiday_list +from erpnext.hr.doctype.leave_application.test_leave_application import get_first_sunday +from erpnext.hr.utils import get_holiday_dates_for_employee +from erpnext.payroll.doctype.employee_benefit_application.employee_benefit_application import ( + calculate_lwp, +) +from erpnext.payroll.doctype.employee_tax_exemption_declaration.test_employee_tax_exemption_declaration import ( + create_payroll_period, +) +from erpnext.payroll.doctype.salary_slip.test_salary_slip import ( + make_holiday_list, + make_leave_application, +) +from erpnext.payroll.doctype.salary_structure.salary_structure import make_salary_slip +from erpnext.payroll.doctype.salary_structure.test_salary_structure import make_salary_structure + + +class TestEmployeeBenefitApplication(FrappeTestCase): + def setUp(self): + date = getdate() + make_holiday_list(from_date=get_year_start(date), to_date=get_year_ending(date)) + + @set_holiday_list("Salary Slip Test Holiday List", "_Test Company") + def test_employee_benefit_application(self): + payroll_period = create_payroll_period(name="_Test Payroll Period 1", company="_Test Company") + employee = make_employee("test_employee_benefits@salary.com", company="_Test Company") + first_sunday = get_first_sunday("Salary Slip Test Holiday List") + + leave_application = make_leave_application( + employee, + add_days(first_sunday, 1), + add_days(first_sunday, 3), + "Leave Without Pay", + half_day=1, + half_day_date=add_days(first_sunday, 1), + submit=True, + ) + + frappe.db.set_value("Leave Type", "Leave Without Pay", "include_holiday", 0) + salary_structure = make_salary_structure( + "Test Employee Benefits", + "Monthly", + other_details={"max_benefits": 100000}, + include_flexi_benefits=True, + employee=employee, + payroll_period=payroll_period, + ) + salary_slip = make_salary_slip(salary_structure.name, employee=employee, posting_date=getdate()) + salary_slip.insert() + salary_slip.submit() + + application = make_employee_benefit_application( + employee, payroll_period.name, date=leave_application.to_date + ) + self.assertEqual(application.employee_benefits[0].max_benefit_amount, 15000) + + holidays = get_holiday_dates_for_employee(employee, payroll_period.start_date, application.date) + working_days = date_diff(application.date, payroll_period.start_date) + 1 + lwp = calculate_lwp(employee, payroll_period.start_date, holidays, working_days) + self.assertEqual(lwp, 2.5) + + +def make_employee_benefit_application(employee, payroll_period, date): + frappe.db.delete("Employee Benefit Application") + + return frappe.get_doc( + { + "doctype": "Employee Benefit Application", + "employee": employee, + "date": date, + "payroll_period": payroll_period, + "employee_benefits": [{"earning_component": "Medical Allowance", "amount": 1500}], + } + ).insert() diff --git a/erpnext/payroll/doctype/salary_slip/test_salary_slip.py b/erpnext/payroll/doctype/salary_slip/test_salary_slip.py index 5e3814b73cd..a8b6bb5714b 100644 --- a/erpnext/payroll/doctype/salary_slip/test_salary_slip.py +++ b/erpnext/payroll/doctype/salary_slip/test_salary_slip.py @@ -49,7 +49,7 @@ class TestSalarySlip(unittest.TestCase): "Payroll Settings", {"payroll_based_on": "Attendance", "daily_wages_fraction_for_half_day": 0.75} ) def test_payment_days_based_on_attendance(self): - no_of_days = self.get_no_of_days() + no_of_days = get_no_of_days() emp_id = make_employee("test_payment_days_based_on_attendance@salary.com") frappe.db.set_value("Employee", emp_id, {"relieving_date": None, "status": "Active"}) @@ -128,7 +128,7 @@ class TestSalarySlip(unittest.TestCase): }, ) def test_payment_days_for_mid_joinee_including_holidays(self): - no_of_days = self.get_no_of_days() + no_of_days = get_no_of_days() month_start_date, month_end_date = get_first_day(nowdate()), get_last_day(nowdate()) new_emp_id = make_employee("test_payment_days_based_on_joining_date@salary.com") @@ -196,7 +196,7 @@ class TestSalarySlip(unittest.TestCase): # tests mid month joining and relieving along with unmarked days from erpnext.hr.doctype.holiday_list.holiday_list import is_holiday - no_of_days = self.get_no_of_days() + no_of_days = get_no_of_days() month_start_date, month_end_date = get_first_day(nowdate()), get_last_day(nowdate()) new_emp_id = make_employee("test_payment_days_based_on_joining_date@salary.com") @@ -236,7 +236,7 @@ class TestSalarySlip(unittest.TestCase): def test_payment_days_for_mid_joinee_excluding_holidays(self): from erpnext.hr.doctype.holiday_list.holiday_list import is_holiday - no_of_days = self.get_no_of_days() + no_of_days = get_no_of_days() month_start_date, month_end_date = get_first_day(nowdate()), get_last_day(nowdate()) new_emp_id = make_employee("test_payment_days_based_on_joining_date@salary.com") @@ -267,7 +267,7 @@ class TestSalarySlip(unittest.TestCase): @change_settings("Payroll Settings", {"payroll_based_on": "Leave"}) def test_payment_days_based_on_leave_application(self): - no_of_days = self.get_no_of_days() + no_of_days = get_no_of_days() emp_id = make_employee("test_payment_days_based_on_leave_application@salary.com") frappe.db.set_value("Employee", emp_id, {"relieving_date": None, "status": "Active"}) @@ -366,7 +366,7 @@ class TestSalarySlip(unittest.TestCase): salary_slip.submit() salary_slip.reload() - no_of_days = self.get_no_of_days() + no_of_days = get_no_of_days() days_in_month = no_of_days[0] no_of_holidays = no_of_days[1] @@ -441,7 +441,7 @@ class TestSalarySlip(unittest.TestCase): @change_settings("Payroll Settings", {"include_holidays_in_total_working_days": 1}) def test_salary_slip_with_holidays_included(self): - no_of_days = self.get_no_of_days() + no_of_days = get_no_of_days() make_employee("test_salary_slip_with_holidays_included@salary.com") frappe.db.set_value( "Employee", @@ -473,7 +473,7 @@ class TestSalarySlip(unittest.TestCase): @change_settings("Payroll Settings", {"include_holidays_in_total_working_days": 0}) def test_salary_slip_with_holidays_excluded(self): - no_of_days = self.get_no_of_days() + no_of_days = get_no_of_days() make_employee("test_salary_slip_with_holidays_excluded@salary.com") frappe.db.set_value( "Employee", @@ -510,7 +510,7 @@ class TestSalarySlip(unittest.TestCase): create_salary_structure_assignment, ) - no_of_days = self.get_no_of_days() + no_of_days = get_no_of_days() # set joinng date in the same month employee = make_employee("test_payment_days@salary.com") @@ -984,17 +984,18 @@ class TestSalarySlip(unittest.TestCase): activity_type.wage_rate = 25 activity_type.save() - def get_no_of_days(self): - no_of_days_in_month = calendar.monthrange(getdate(nowdate()).year, getdate(nowdate()).month) - no_of_holidays_in_month = len( - [ - 1 - for i in calendar.monthcalendar(getdate(nowdate()).year, getdate(nowdate()).month) - if i[6] != 0 - ] - ) - return [no_of_days_in_month[1], no_of_holidays_in_month] +def get_no_of_days(): + no_of_days_in_month = calendar.monthrange(getdate(nowdate()).year, getdate(nowdate()).month) + no_of_holidays_in_month = len( + [ + 1 + for i in calendar.monthcalendar(getdate(nowdate()).year, getdate(nowdate()).month) + if i[6] != 0 + ] + ) + + return [no_of_days_in_month[1], no_of_holidays_in_month] def make_employee_salary_slip(user, payroll_frequency, salary_structure=None, posting_date=None): @@ -1136,6 +1137,7 @@ def make_earning_salary_component( "pay_against_benefit_claim": 0, "type": "Earning", "max_benefit_amount": 15000, + "depends_on_payment_days": 1, }, ] ) From c66c1e2215d251a904816436c8f99ebfbd83ff65 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 6 Jun 2022 15:27:29 +0530 Subject: [PATCH 12/22] chore(meta): disable stale bot on issues --- .github/stale.yml | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/.github/stale.yml b/.github/stale.yml index fbf64471567..cdce0aee082 100644 --- a/.github/stale.yml +++ b/.github/stale.yml @@ -23,15 +23,3 @@ pulls: activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing. - -issues: - daysUntilStale: 90 - daysUntilClose: 7 - exemptLabels: - - valid - - to-validate - - QA - markComment: > - This issue has been automatically marked as inactive because it has not had - recent activity and it wasn't validated by maintainer team. It will be - closed within a week if no further activity occurs. From ee5bc58e9ba8b4c4b4ab255101919974302068e6 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 6 Jun 2022 16:27:25 +0530 Subject: [PATCH 13/22] fix(job card): only hold during draft state (#31243) --- .../doctype/job_card/job_card.py | 2 +- erpnext/patches.txt | 1 + .../patches/v13_0/job_card_status_on_hold.py | 19 +++++++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 erpnext/patches/v13_0/job_card_status_on_hold.py diff --git a/erpnext/manufacturing/doctype/job_card/job_card.py b/erpnext/manufacturing/doctype/job_card/job_card.py index 0a9fd8a0996..0199a5c31ea 100644 --- a/erpnext/manufacturing/doctype/job_card/job_card.py +++ b/erpnext/manufacturing/doctype/job_card/job_card.py @@ -621,7 +621,7 @@ class JobCard(Document): self.set_status(update_status) def set_status(self, update_status=False): - if self.status == "On Hold": + if self.status == "On Hold" and self.docstatus == 0: return self.status = {0: "Open", 1: "Submitted", 2: "Cancelled"}[self.docstatus or 0] diff --git a/erpnext/patches.txt b/erpnext/patches.txt index 8594ebbe9d5..5a984635fdc 100644 --- a/erpnext/patches.txt +++ b/erpnext/patches.txt @@ -373,3 +373,4 @@ erpnext.patches.v13_0.create_accounting_dimensions_in_orders erpnext.patches.v13_0.set_per_billed_in_return_delivery_note execute:frappe.delete_doc("DocType", "Naming Series") erpnext.patches.v13_0.set_payroll_entry_status +erpnext.patches.v13_0.job_card_status_on_hold diff --git a/erpnext/patches/v13_0/job_card_status_on_hold.py b/erpnext/patches/v13_0/job_card_status_on_hold.py new file mode 100644 index 00000000000..8c67c3c858e --- /dev/null +++ b/erpnext/patches/v13_0/job_card_status_on_hold.py @@ -0,0 +1,19 @@ +import frappe + + +def execute(): + job_cards = frappe.get_all( + "Job Card", + {"status": "On Hold", "docstatus": ("!=", 0)}, + pluck="name", + ) + + for idx, job_card in enumerate(job_cards): + try: + doc = frappe.get_doc("Job Card", job_card) + doc.set_status() + doc.db_set("status", doc.status, update_modified=False) + if idx % 100 == 0: + frappe.db.commit() + except Exception: + continue From 91f9f37d64b54625888f2642edd1b11a8cde82a6 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Tue, 7 Jun 2022 09:51:30 +0530 Subject: [PATCH 14/22] fix: leave balance for earned leaves in backdated Leave Application dashboard (#31253) --- .../leave_application/leave_application.js | 2 +- .../leave_application/leave_application.py | 23 +-- .../test_leave_application.py | 139 ++++++++++++------ 3 files changed, 101 insertions(+), 63 deletions(-) diff --git a/erpnext/hr/doctype/leave_application/leave_application.js b/erpnext/hr/doctype/leave_application/leave_application.js index 85997a4087f..ee00e6719c0 100755 --- a/erpnext/hr/doctype/leave_application/leave_application.js +++ b/erpnext/hr/doctype/leave_application/leave_application.js @@ -173,7 +173,7 @@ frappe.ui.form.on("Leave Application", { date: frm.doc.from_date, to_date: frm.doc.to_date, leave_type: frm.doc.leave_type, - consider_all_leaves_in_the_allocation_period: true + consider_all_leaves_in_the_allocation_period: 1 }, callback: function (r) { if (!r.exc && r.message) { diff --git a/erpnext/hr/doctype/leave_application/leave_application.py b/erpnext/hr/doctype/leave_application/leave_application.py index 53c5df4210a..43c2bb37b21 100755 --- a/erpnext/hr/doctype/leave_application/leave_application.py +++ b/erpnext/hr/doctype/leave_application/leave_application.py @@ -757,22 +757,6 @@ def get_leave_details(employee, date): leave_allocation = {} for d in allocation_records: allocation = allocation_records.get(d, frappe._dict()) - - total_allocated_leaves = ( - frappe.db.get_value( - "Leave Allocation", - { - "from_date": ("<=", date), - "to_date": (">=", date), - "employee": employee, - "leave_type": allocation.leave_type, - "docstatus": 1, - }, - "SUM(total_leaves_allocated)", - ) - or 0 - ) - remaining_leaves = get_leave_balance_on( employee, d, date, to_date=allocation.to_date, consider_all_leaves_in_the_allocation_period=True ) @@ -782,10 +766,11 @@ def get_leave_details(employee, date): leaves_pending = get_leaves_pending_approval_for_period( employee, d, allocation.from_date, end_date ) + expired_leaves = allocation.total_leaves_allocated - (remaining_leaves + leaves_taken) leave_allocation[d] = { - "total_leaves": total_allocated_leaves, - "expired_leaves": total_allocated_leaves - (remaining_leaves + leaves_taken), + "total_leaves": allocation.total_leaves_allocated, + "expired_leaves": expired_leaves if expired_leaves > 0 else 0, "leaves_taken": leaves_taken, "leaves_pending_approval": leaves_pending, "remaining_leaves": remaining_leaves, @@ -830,7 +815,7 @@ def get_leave_balance_on( allocation_records = get_leave_allocation_records(employee, date, leave_type) allocation = allocation_records.get(leave_type, frappe._dict()) - end_date = allocation.to_date if consider_all_leaves_in_the_allocation_period else date + end_date = allocation.to_date if cint(consider_all_leaves_in_the_allocation_period) else date cf_expiry = get_allocation_expiry_for_cf_leaves(employee, leave_type, to_date, date) leaves_taken = get_leaves_for_period(employee, leave_type, allocation.from_date, end_date) diff --git a/erpnext/hr/doctype/leave_application/test_leave_application.py b/erpnext/hr/doctype/leave_application/test_leave_application.py index 7506c611083..27c54109dea 100644 --- a/erpnext/hr/doctype/leave_application/test_leave_application.py +++ b/erpnext/hr/doctype/leave_application/test_leave_application.py @@ -76,7 +76,14 @@ _test_records = [ class TestLeaveApplication(unittest.TestCase): def setUp(self): - for dt in ["Leave Application", "Leave Allocation", "Salary Slip", "Leave Ledger Entry"]: + for dt in [ + "Leave Application", + "Leave Allocation", + "Salary Slip", + "Leave Ledger Entry", + "Leave Period", + "Leave Policy Assignment", + ]: frappe.db.delete(dt) frappe.set_user("Administrator") @@ -702,59 +709,24 @@ class TestLeaveApplication(unittest.TestCase): self.assertEqual(details.leave_balance, 30) def test_earned_leaves_creation(self): - - frappe.db.sql("""delete from `tabLeave Period`""") - frappe.db.sql("""delete from `tabLeave Policy Assignment`""") - frappe.db.sql("""delete from `tabLeave Allocation`""") - frappe.db.sql("""delete from `tabLeave Ledger Entry`""") + from erpnext.hr.utils import allocate_earned_leaves leave_period = get_leave_period() employee = get_employee() leave_type = "Test Earned Leave Type" - frappe.delete_doc_if_exists("Leave Type", "Test Earned Leave Type", force=1) - frappe.get_doc( - dict( - leave_type_name=leave_type, - doctype="Leave Type", - is_earned_leave=1, - earned_leave_frequency="Monthly", - rounding=0.5, - max_leaves_allowed=6, - ) - ).insert() + make_policy_assignment(employee, leave_type, leave_period) - leave_policy = frappe.get_doc( - { - "doctype": "Leave Policy", - "title": "Test Leave Policy", - "leave_policy_details": [{"leave_type": leave_type, "annual_allocation": 6}], - } - ).insert() - - data = { - "assignment_based_on": "Leave Period", - "leave_policy": leave_policy.name, - "leave_period": leave_period.name, - } - - leave_policy_assignments = create_assignment_for_multiple_employees( - [employee.name], frappe._dict(data) - ) - - from erpnext.hr.utils import allocate_earned_leaves - - i = 0 - while i < 14: + for i in range(0, 14): allocate_earned_leaves() - i += 1 + self.assertEqual(get_leave_balance_on(employee.name, leave_type, nowdate()), 6) # validate earned leaves creation without maximum leaves frappe.db.set_value("Leave Type", leave_type, "max_leaves_allowed", 0) - i = 0 - while i < 6: + + for i in range(0, 6): allocate_earned_leaves() - i += 1 + self.assertEqual(get_leave_balance_on(employee.name, leave_type, nowdate()), 9) # test to not consider current leave in leave balance while submitting @@ -970,6 +942,54 @@ class TestLeaveApplication(unittest.TestCase): self.assertEqual(leave_allocation["leaves_pending_approval"], 1) self.assertEqual(leave_allocation["remaining_leaves"], 26) + @set_holiday_list("Salary Slip Test Holiday List", "_Test Company") + def test_get_earned_leave_details_for_dashboard(self): + from erpnext.hr.utils import allocate_earned_leaves + + leave_period = get_leave_period() + employee = get_employee() + leave_type = "Test Earned Leave Type" + leave_policy_assignments = make_policy_assignment(employee, leave_type, leave_period) + allocation = frappe.db.get_value( + "Leave Allocation", + {"leave_policy_assignment": leave_policy_assignments[0]}, + "name", + ) + allocation = frappe.get_doc("Leave Allocation", allocation) + allocation.new_leaves_allocated = 2 + allocation.save() + + for i in range(0, 6): + allocate_earned_leaves() + + first_sunday = get_first_sunday(self.holiday_list) + make_leave_application( + employee.name, add_days(first_sunday, 1), add_days(first_sunday, 1), leave_type + ) + + details = get_leave_details(employee.name, allocation.from_date) + leave_allocation = details["leave_allocation"][leave_type] + expected = { + "total_leaves": 2.0, + "expired_leaves": 0.0, + "leaves_taken": 1.0, + "leaves_pending_approval": 0.0, + "remaining_leaves": 1.0, + } + self.assertEqual(leave_allocation, expected) + + details = get_leave_details(employee.name, getdate()) + leave_allocation = details["leave_allocation"][leave_type] + + expected = { + "total_leaves": 5.0, + "expired_leaves": 0.0, + "leaves_taken": 1.0, + "leaves_pending_approval": 0.0, + "remaining_leaves": 4.0, + } + self.assertEqual(leave_allocation, expected) + @set_holiday_list("Salary Slip Test Holiday List", "_Test Company") def test_get_leave_allocation_records(self): employee = get_employee() @@ -1100,3 +1120,36 @@ def get_first_sunday(holiday_list, for_date=None): )[0][0] return first_sunday + + +def make_policy_assignment(employee, leave_type, leave_period): + frappe.delete_doc_if_exists("Leave Type", leave_type, force=1) + frappe.get_doc( + dict( + leave_type_name=leave_type, + doctype="Leave Type", + is_earned_leave=1, + earned_leave_frequency="Monthly", + rounding=0.5, + max_leaves_allowed=6, + ) + ).insert() + + leave_policy = frappe.get_doc( + { + "doctype": "Leave Policy", + "title": "Test Leave Policy", + "leave_policy_details": [{"leave_type": leave_type, "annual_allocation": 6}], + } + ).insert() + + data = { + "assignment_based_on": "Leave Period", + "leave_policy": leave_policy.name, + "leave_period": leave_period.name, + } + + leave_policy_assignments = create_assignment_for_multiple_employees( + [employee.name], frappe._dict(data) + ) + return leave_policy_assignments From c3f2201c45fa2bdbffdd31ba506120cc73658bfd Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 7 Jun 2022 10:04:35 +0530 Subject: [PATCH 15/22] chore(meta): apply stale rules to pull only --- .github/stale.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/stale.yml b/.github/stale.yml index cdce0aee082..da15d32680f 100644 --- a/.github/stale.yml +++ b/.github/stale.yml @@ -23,3 +23,5 @@ pulls: activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing. + +only: pulls From dc8e80ea815d5684b56376330500f8dccdd38816 Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Tue, 7 Jun 2022 11:35:03 +0530 Subject: [PATCH 16/22] test: Add test coverage for cancellation --- .../purchase_invoice/test_purchase_invoice.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/erpnext/accounts/doctype/purchase_invoice/test_purchase_invoice.py b/erpnext/accounts/doctype/purchase_invoice/test_purchase_invoice.py index 30d26acf3a2..9b7b88973fa 100644 --- a/erpnext/accounts/doctype/purchase_invoice/test_purchase_invoice.py +++ b/erpnext/accounts/doctype/purchase_invoice/test_purchase_invoice.py @@ -1526,6 +1526,18 @@ class TestPurchaseInvoice(unittest.TestCase): check_gl_entries(self, pr.name, expected_gle_for_purchase_receipt, pr.posting_date) + # Cancel purchase invoice to check reverse provisional entry cancellation + pi.cancel() + + expected_gle_for_purchase_receipt_post_pi_cancel = [ + ["Provision Account - _TC", 0, 250, pi.posting_date], + ["_Test Account Cost for Goods Sold - _TC", 250, 0, pi.posting_date], + ] + + check_gl_entries( + self, pr.name, expected_gle_for_purchase_receipt_post_pi_cancel, pr.posting_date + ) + company.enable_provisional_accounting_for_non_stock_items = 0 company.save() From 293eb8d722c773864eef6ef45ce36a8bda25340e Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 10 May 2022 21:30:31 +0530 Subject: [PATCH 17/22] test: create stock test mixin for assertion/utils --- .../doctype/stock_entry/stock_entry_utils.py | 26 +++++++++++ .../test_stock_ledger_entry.py | 27 +----------- .../test_stock_reconciliation.py | 15 ++++--- erpnext/stock/tests/test_utils.py | 44 ++++++++++++++++--- 4 files changed, 75 insertions(+), 37 deletions(-) diff --git a/erpnext/stock/doctype/stock_entry/stock_entry_utils.py b/erpnext/stock/doctype/stock_entry/stock_entry_utils.py index c5c0cefe51c..41a3b8916de 100644 --- a/erpnext/stock/doctype/stock_entry/stock_entry_utils.py +++ b/erpnext/stock/doctype/stock_entry/stock_entry_utils.py @@ -2,11 +2,37 @@ # See license.txt +from typing import TYPE_CHECKING, Optional, overload + import frappe from frappe.utils import cint, flt import erpnext +if TYPE_CHECKING: + from erpnext.stock.doctype.stock_entry.stock_entry import StockEntry + + +@overload +def make_stock_entry( + *, + item_code: str, + qty: float, + company: Optional[str] = None, + from_warehouse: Optional[str] = None, + to_warehouse: Optional[str] = None, + rate: Optional[float] = None, + serial_no: Optional[str] = None, + batch_no: Optional[str] = None, + posting_date: Optional[str] = None, + posting_time: Optional[str] = None, + purpose: Optional[str] = None, + do_not_save: bool = False, + do_not_submit: bool = False, + inspection_required: bool = False, +) -> "StockEntry": + ... + @frappe.whitelist() def make_stock_entry(**args): diff --git a/erpnext/stock/doctype/stock_ledger_entry/test_stock_ledger_entry.py b/erpnext/stock/doctype/stock_ledger_entry/test_stock_ledger_entry.py index eb1e0fc25fd..55a213ccc3f 100644 --- a/erpnext/stock/doctype/stock_ledger_entry/test_stock_ledger_entry.py +++ b/erpnext/stock/doctype/stock_ledger_entry/test_stock_ledger_entry.py @@ -24,9 +24,10 @@ from erpnext.stock.doctype.stock_reconciliation.test_stock_reconciliation import create_stock_reconciliation, ) from erpnext.stock.stock_ledger import get_previous_sle +from erpnext.stock.tests.test_utils import StockTestMixin -class TestStockLedgerEntry(FrappeTestCase): +class TestStockLedgerEntry(FrappeTestCase, StockTestMixin): def setUp(self): items = create_items() reset("Stock Entry") @@ -541,30 +542,6 @@ class TestStockLedgerEntry(FrappeTestCase): "Incorrect 'Incoming Rate' values fetched for DN items", ) - def assertSLEs(self, doc, expected_sles, sle_filters=None): - """Compare sorted SLEs, useful for vouchers that create multiple SLEs for same line""" - - filters = {"voucher_no": doc.name, "voucher_type": doc.doctype, "is_cancelled": 0} - if sle_filters: - filters.update(sle_filters) - sles = frappe.get_all( - "Stock Ledger Entry", - fields=["*"], - filters=filters, - order_by="timestamp(posting_date, posting_time), creation", - ) - - for exp_sle, act_sle in zip(expected_sles, sles): - for k, v in exp_sle.items(): - act_value = act_sle[k] - if k == "stock_queue": - act_value = json.loads(act_value) - if act_value and act_value[0][0] == 0: - # ignore empty fifo bins - continue - - self.assertEqual(v, act_value, msg=f"{k} doesn't match \n{exp_sle}\n{act_sle}") - def test_batchwise_item_valuation_stock_reco(self): item, warehouses, batches = setup_item_valuation_test() state = {"stock_value": 0.0, "qty": 0.0} diff --git a/erpnext/stock/doctype/stock_reconciliation/test_stock_reconciliation.py b/erpnext/stock/doctype/stock_reconciliation/test_stock_reconciliation.py index 9088eb802b5..191c03f5f1c 100644 --- a/erpnext/stock/doctype/stock_reconciliation/test_stock_reconciliation.py +++ b/erpnext/stock/doctype/stock_reconciliation/test_stock_reconciliation.py @@ -10,7 +10,7 @@ from frappe.tests.utils import FrappeTestCase, change_settings from frappe.utils import add_days, cstr, flt, nowdate, nowtime, random_string from erpnext.accounts.utils import get_stock_and_account_balance -from erpnext.stock.doctype.item.test_item import create_item, make_item +from erpnext.stock.doctype.item.test_item import create_item from erpnext.stock.doctype.purchase_receipt.test_purchase_receipt import make_purchase_receipt from erpnext.stock.doctype.serial_no.serial_no import get_serial_nos from erpnext.stock.doctype.stock_reconciliation.stock_reconciliation import ( @@ -19,10 +19,11 @@ from erpnext.stock.doctype.stock_reconciliation.stock_reconciliation import ( ) from erpnext.stock.doctype.warehouse.test_warehouse import create_warehouse from erpnext.stock.stock_ledger import get_previous_sle, update_entries_after +from erpnext.stock.tests.test_utils import StockTestMixin from erpnext.stock.utils import get_incoming_rate, get_stock_value_on, get_valuation_method -class TestStockReconciliation(FrappeTestCase): +class TestStockReconciliation(FrappeTestCase, StockTestMixin): @classmethod def setUpClass(cls): create_batch_or_serial_no_items() @@ -40,7 +41,7 @@ class TestStockReconciliation(FrappeTestCase): self._test_reco_sle_gle("Moving Average") def _test_reco_sle_gle(self, valuation_method): - item_code = make_item(properties={"valuation_method": valuation_method}).name + item_code = self.make_item(properties={"valuation_method": valuation_method}).name se1, se2, se3 = insert_existing_sle(warehouse="Stores - TCP1", item_code=item_code) company = frappe.db.get_value("Warehouse", "Stores - TCP1", "company") @@ -392,7 +393,7 @@ class TestStockReconciliation(FrappeTestCase): SR4 | Reco | 0 | 6 (posting date: today-1) [backdated] PR3 | PR | 1 | 7 (posting date: today) # can't post future PR """ - item_code = make_item().name + item_code = self.make_item().name warehouse = "_Test Warehouse - _TC" frappe.flags.dont_execute_stock_reposts = True @@ -458,7 +459,7 @@ class TestStockReconciliation(FrappeTestCase): from erpnext.stock.doctype.delivery_note.test_delivery_note import create_delivery_note from erpnext.stock.stock_ledger import NegativeStockError - item_code = make_item().name + item_code = self.make_item().name warehouse = "_Test Warehouse - _TC" pr1 = make_purchase_receipt( @@ -506,7 +507,7 @@ class TestStockReconciliation(FrappeTestCase): from erpnext.stock.doctype.delivery_note.test_delivery_note import create_delivery_note from erpnext.stock.stock_ledger import NegativeStockError - item_code = make_item().name + item_code = self.make_item().name warehouse = "_Test Warehouse - _TC" sr = create_stock_reconciliation( @@ -549,7 +550,7 @@ class TestStockReconciliation(FrappeTestCase): # repost will make this test useless, qty should update in realtime without reposts frappe.flags.dont_execute_stock_reposts = True - item_code = make_item().name + item_code = self.make_item().name warehouse = "_Test Warehouse - _TC" sr = create_stock_reconciliation( diff --git a/erpnext/stock/tests/test_utils.py b/erpnext/stock/tests/test_utils.py index 9ee0c9f3b5a..e07e08c79e5 100644 --- a/erpnext/stock/tests/test_utils.py +++ b/erpnext/stock/tests/test_utils.py @@ -1,16 +1,50 @@ +import json + import frappe from frappe.tests.utils import FrappeTestCase -from erpnext.stock.doctype.item.test_item import make_item from erpnext.stock.utils import scan_barcode -class TestStockUtilities(FrappeTestCase): +class StockTestMixin: + """Mixin to simplfy stock ledger tests, useful for all stock transactions.""" + + def make_item(self, item_code=None, properties=None, *args, **kwargs): + from erpnext.stock.doctype.item.test_item import make_item + + return make_item(item_code, properties, *args, **kwargs) + + def assertSLEs(self, doc, expected_sles, sle_filters=None): + """Compare sorted SLEs, useful for vouchers that create multiple SLEs for same line""" + + filters = {"voucher_no": doc.name, "voucher_type": doc.doctype, "is_cancelled": 0} + if sle_filters: + filters.update(sle_filters) + sles = frappe.get_all( + "Stock Ledger Entry", + fields=["*"], + filters=filters, + order_by="timestamp(posting_date, posting_time), creation", + ) + + for exp_sle, act_sle in zip(expected_sles, sles): + for k, v in exp_sle.items(): + act_value = act_sle[k] + if k == "stock_queue": + act_value = json.loads(act_value) + if act_value and act_value[0][0] == 0: + # ignore empty fifo bins + continue + + self.assertEqual(v, act_value, msg=f"{k} doesn't match \n{exp_sle}\n{act_sle}") + + +class TestStockUtilities(FrappeTestCase, StockTestMixin): def test_barcode_scanning(self): - simple_item = make_item(properties={"barcodes": [{"barcode": "12399"}]}) + simple_item = self.make_item(properties={"barcodes": [{"barcode": "12399"}]}) self.assertEqual(scan_barcode("12399")["item_code"], simple_item.name) - batch_item = make_item(properties={"has_batch_no": 1, "create_new_batch": 1}) + batch_item = self.make_item(properties={"has_batch_no": 1, "create_new_batch": 1}) batch = frappe.get_doc(doctype="Batch", item=batch_item.name).insert() batch_scan = scan_barcode(batch.name) @@ -19,7 +53,7 @@ class TestStockUtilities(FrappeTestCase): self.assertEqual(batch_scan["has_batch_no"], 1) self.assertEqual(batch_scan["has_serial_no"], 0) - serial_item = make_item(properties={"has_serial_no": 1}) + serial_item = self.make_item(properties={"has_serial_no": 1}) serial = frappe.get_doc( doctype="Serial No", item_code=serial_item.name, serial_no=frappe.generate_hash() ).insert() From 7726271e2ac6776b29f795e6e54dd76aa6d581b8 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 1 Jun 2022 14:17:06 +0530 Subject: [PATCH 18/22] fix: purchase invoice return GLe voucher_wise_stock_value contains tuples and the condition was looking for string, so it's never triggered. Caused by https://github.com/frappe/erpnext/pull/24200 --- .../purchase_invoice/purchase_invoice.py | 2 +- .../purchase_invoice/test_purchase_invoice.py | 77 ++++++++++++++++++- .../controllers/sales_and_purchase_return.py | 2 +- erpnext/stock/tests/test_utils.py | 17 ++++ 4 files changed, 95 insertions(+), 3 deletions(-) diff --git a/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py b/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py index e6da6669ac2..5d11dffdef8 100644 --- a/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py +++ b/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py @@ -1127,7 +1127,7 @@ class PurchaseInvoice(BuyingController): # Stock ledger value is not matching with the warehouse amount if ( self.update_stock - and voucher_wise_stock_value.get(item.name) + and voucher_wise_stock_value.get((item.name, item.warehouse)) and warehouse_debit_amount != flt(voucher_wise_stock_value.get((item.name, item.warehouse)), net_amt_precision) ): diff --git a/erpnext/accounts/doctype/purchase_invoice/test_purchase_invoice.py b/erpnext/accounts/doctype/purchase_invoice/test_purchase_invoice.py index 30d26acf3a2..9c8a6dd6b26 100644 --- a/erpnext/accounts/doctype/purchase_invoice/test_purchase_invoice.py +++ b/erpnext/accounts/doctype/purchase_invoice/test_purchase_invoice.py @@ -27,12 +27,13 @@ from erpnext.stock.doctype.purchase_receipt.test_purchase_receipt import ( make_purchase_receipt, ) from erpnext.stock.doctype.stock_entry.test_stock_entry import get_qty_after_transaction +from erpnext.stock.tests.test_utils import StockTestMixin test_dependencies = ["Item", "Cost Center", "Payment Term", "Payment Terms Template"] test_ignore = ["Serial No"] -class TestPurchaseInvoice(unittest.TestCase): +class TestPurchaseInvoice(unittest.TestCase, StockTestMixin): @classmethod def setUpClass(self): unlink_payment_on_cancel_of_invoice() @@ -693,6 +694,80 @@ class TestPurchaseInvoice(unittest.TestCase): self.assertEqual(expected_values[gle.account][0], gle.debit) self.assertEqual(expected_values[gle.account][1], gle.credit) + def test_standalone_return_using_pi(self): + from erpnext.stock.doctype.stock_entry.test_stock_entry import make_stock_entry + + item = self.make_item().name + company = "_Test Company with perpetual inventory" + warehouse = "Stores - TCP1" + + make_stock_entry(item_code=item, target=warehouse, qty=50, rate=120) + + return_pi = make_purchase_invoice( + is_return=1, + item=item, + qty=-10, + update_stock=1, + rate=100, + company=company, + warehouse=warehouse, + cost_center="Main - TCP1", + ) + + # assert that stock consumption is with actual rate + self.assertGLEs( + return_pi, + [{"credit": 1200, "debit": 0}], + gle_filters={"account": "Stock In Hand - TCP1"}, + ) + + # assert loss booked in COGS + self.assertGLEs( + return_pi, + [{"credit": 0, "debit": 200}], + gle_filters={"account": "Cost of Goods Sold - TCP1"}, + ) + + def test_return_with_lcv(self): + from erpnext.controllers.sales_and_purchase_return import make_return_doc + from erpnext.stock.doctype.landed_cost_voucher.test_landed_cost_voucher import ( + create_landed_cost_voucher, + ) + + item = self.make_item().name + company = "_Test Company with perpetual inventory" + warehouse = "Stores - TCP1" + cost_center = "Main - TCP1" + + pi = make_purchase_invoice( + item=item, + company=company, + warehouse=warehouse, + cost_center=cost_center, + update_stock=1, + qty=10, + rate=100, + ) + + # Create landed cost voucher - will increase valuation of received item by 10 + create_landed_cost_voucher("Purchase Invoice", pi.name, pi.company, charges=100) + return_pi = make_return_doc(pi.doctype, pi.name) + return_pi.save().submit() + + # assert that stock consumption is with actual in rate + self.assertGLEs( + return_pi, + [{"credit": 1100, "debit": 0}], + gle_filters={"account": "Stock In Hand - TCP1"}, + ) + + # assert loss booked in COGS + self.assertGLEs( + return_pi, + [{"credit": 0, "debit": 100}], + gle_filters={"account": "Cost of Goods Sold - TCP1"}, + ) + def test_multi_currency_gle(self): pi = make_purchase_invoice( supplier="_Test Supplier USD", diff --git a/erpnext/controllers/sales_and_purchase_return.py b/erpnext/controllers/sales_and_purchase_return.py index bd4b59b3855..d24ac3f2cf3 100644 --- a/erpnext/controllers/sales_and_purchase_return.py +++ b/erpnext/controllers/sales_and_purchase_return.py @@ -316,7 +316,7 @@ def get_returned_qty_map_for_row(return_against, party, row_name, doctype): return data[0] -def make_return_doc(doctype, source_name, target_doc=None): +def make_return_doc(doctype: str, source_name: str, target_doc=None): from frappe.model.mapper import get_mapped_doc from erpnext.stock.doctype.serial_no.serial_no import get_serial_nos diff --git a/erpnext/stock/tests/test_utils.py b/erpnext/stock/tests/test_utils.py index e07e08c79e5..b046dbda24f 100644 --- a/erpnext/stock/tests/test_utils.py +++ b/erpnext/stock/tests/test_utils.py @@ -38,6 +38,23 @@ class StockTestMixin: self.assertEqual(v, act_value, msg=f"{k} doesn't match \n{exp_sle}\n{act_sle}") + def assertGLEs(self, doc, expected_gles, gle_filters=None, order_by=None): + filters = {"voucher_no": doc.name, "voucher_type": doc.doctype, "is_cancelled": 0} + + if gle_filters: + filters.update(gle_filters) + actual_gles = frappe.get_all( + "GL Entry", + fields=["*"], + filters=filters, + order_by=order_by or "posting_date, creation", + ) + + for exp_gle, act_gle in zip(expected_gles, actual_gles): + for k, exp_value in exp_gle.items(): + act_value = act_gle[k] + self.assertEqual(exp_value, act_value, msg=f"{k} doesn't match \n{exp_gle}\n{act_gle}") + class TestStockUtilities(FrappeTestCase, StockTestMixin): def test_barcode_scanning(self): From 815141bf57b3f7710993c0d0d871ea2457d0488f Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Tue, 7 Jun 2022 13:16:06 +0530 Subject: [PATCH 19/22] fix: Close unsecured terms loans --- erpnext/loan_management/doctype/loan/loan.js | 18 ++++++++++++++++ erpnext/loan_management/doctype/loan/loan.py | 22 +++++++++++++++++--- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/erpnext/loan_management/doctype/loan/loan.js b/erpnext/loan_management/doctype/loan/loan.js index 940a1bbc000..38328e69674 100644 --- a/erpnext/loan_management/doctype/loan/loan.js +++ b/erpnext/loan_management/doctype/loan/loan.js @@ -93,6 +93,12 @@ frappe.ui.form.on('Loan', { frm.trigger("make_loan_refund"); },__('Create')); } + + if (frm.doc.status == "Loan Closure Requested" && frm.doc.is_term_loan && !frm.doc.is_secured_loan) { + frm.add_custom_button(__('Close Loan'), function() { + frm.trigger("close_unsecured_term_loan"); + },__('Status')); + } } frm.trigger("toggle_fields"); }, @@ -174,6 +180,18 @@ frappe.ui.form.on('Loan', { }) }, + close_unsecured_term_loan: function(frm) { + frappe.call({ + args: { + "loan": frm.doc.name + }, + method: "erpnext.loan_management.doctype.loan.loan.close_unsecured_term_loan", + callback: function () { + frm.refresh(); + } + }) + }, + request_loan_closure: function(frm) { frappe.confirm(__("Do you really want to close this loan"), function() { diff --git a/erpnext/loan_management/doctype/loan/loan.py b/erpnext/loan_management/doctype/loan/loan.py index 3b76ba4edb6..ac8b3629d97 100644 --- a/erpnext/loan_management/doctype/loan/loan.py +++ b/erpnext/loan_management/doctype/loan/loan.py @@ -60,11 +60,11 @@ class Loan(AccountsController): ) def validate_cost_center(self): - if not self.cost_center and self.rate_of_interest != 0: + if not self.cost_center and self.rate_of_interest != 0.0: self.cost_center = frappe.db.get_value("Company", self.company, "cost_center") - if not self.cost_center: - frappe.throw(_("Cost center is mandatory for loans having rate of interest greater than 0")) + if not self.cost_center: + frappe.throw(_("Cost center is mandatory for loans having rate of interest greater than 0")) def on_submit(self): self.link_loan_security_pledge() @@ -342,6 +342,22 @@ def get_loan_application(loan_application): return loan.as_dict() +@frappe.whitelist() +def close_unsecured_term_loan(loan): + loan_details = frappe.db.get_value( + "Loan", {"name": loan}, ["status", "is_term_loan", "is_secured_loan"], as_dict=1 + ) + + if ( + loan_details.status == "Loan Closure Requested" + and loan_details.is_term_loan + and not loan_details.is_secured_loan + ): + frappe.db.set_value("Loan", loan, "status", "Closed") + else: + frappe.throw(_("Cannot close this loan until full repayment")) + + def close_loan(loan, total_amount_paid): frappe.db.set_value("Loan", loan, "total_amount_paid", total_amount_paid) frappe.db.set_value("Loan", loan, "status", "Closed") From f830b57fd481d46973b6874387529394fe659539 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 7 Jun 2022 15:23:32 +0530 Subject: [PATCH 20/22] test: sales register report with conditions --- erpnext/accounts/report/sales_register/sales_register.py | 4 ++-- erpnext/accounts/test/test_reports.py | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/erpnext/accounts/report/sales_register/sales_register.py b/erpnext/accounts/report/sales_register/sales_register.py index 777d96ced17..33bd3c74965 100644 --- a/erpnext/accounts/report/sales_register/sales_register.py +++ b/erpnext/accounts/report/sales_register/sales_register.py @@ -367,8 +367,8 @@ def get_conditions(filters): if not filters.get(field) or field in accounting_dimensions_list: return "" return f""" and exists(select name from `tab{table}` - where parent=`tabSales Invoice`.name - and ifnull(`tab{table}`.{field}, '') = %({field})s)""" + where parent=`tabSales Invoice`.name + and ifnull(`tab{table}`.{field}, '') = %({field})s)""" conditions += get_sales_invoice_item_field_condition("mode_of_payments", "Sales Invoice Payment") conditions += get_sales_invoice_item_field_condition("cost_center") diff --git a/erpnext/accounts/test/test_reports.py b/erpnext/accounts/test/test_reports.py index 19fe74fffc1..3f06c30adb6 100644 --- a/erpnext/accounts/test/test_reports.py +++ b/erpnext/accounts/test/test_reports.py @@ -28,6 +28,7 @@ REPORT_FILTER_TEST_CASES: List[Tuple[ReportName, ReportFilters]] = [ ("Item-wise Sales Register", {}), ("Item-wise Purchase Register", {}), ("Sales Register", {}), + ("Sales Register", {"item_group": "All Item Groups"}), ("Purchase Register", {}), ( "Tax Detail", From fb4f8d870be85037df1fd416be8fb93c1a85231f Mon Sep 17 00:00:00 2001 From: Saqib Ansari Date: Wed, 8 Jun 2022 09:36:33 +0530 Subject: [PATCH 21/22] fix(india): e-invoice eligibility if company gstin is not configured (#31247) --- erpnext/regional/india/e_invoice/utils.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/erpnext/regional/india/e_invoice/utils.py b/erpnext/regional/india/e_invoice/utils.py index 9add09beaf7..5eb14a5ddd3 100644 --- a/erpnext/regional/india/e_invoice/utils.py +++ b/erpnext/regional/india/e_invoice/utils.py @@ -55,6 +55,9 @@ def validate_eligibility(doc): return False invalid_company = not frappe.db.get_value("E Invoice User", {"company": doc.get("company")}) + invalid_company_gstin = not frappe.db.get_value( + "E Invoice User", {"gstin": doc.get("company_gstin")} + ) invalid_supply_type = doc.get("gst_category") not in [ "Registered Regular", "Registered Composition", @@ -71,6 +74,7 @@ def validate_eligibility(doc): if ( invalid_company + or invalid_company_gstin or invalid_supply_type or company_transaction or no_taxes_applied From 2832731601920b07c7083a20c49868e866640add Mon Sep 17 00:00:00 2001 From: Marica Date: Wed, 8 Jun 2022 15:52:13 +0530 Subject: [PATCH 22/22] fix: Use `frappe.as_unicode` to decode output of redis module list (#31282) - As of redis 7, a list is added to the result of fetching the module list - This list cannot be "decoded",so use `frappe.as_unicode` that handles bytes as well as other types --- erpnext/e_commerce/redisearch_utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/erpnext/e_commerce/redisearch_utils.py b/erpnext/e_commerce/redisearch_utils.py index 61b4b9ee1f4..1f649c7b486 100644 --- a/erpnext/e_commerce/redisearch_utils.py +++ b/erpnext/e_commerce/redisearch_utils.py @@ -38,7 +38,7 @@ def is_search_module_loaded(): out = cache.execute_command("MODULE LIST") parsed_output = " ".join( - (" ".join([s.decode() for s in o if not isinstance(s, int)]) for o in out) + (" ".join([frappe.as_unicode(s) for s in o if not isinstance(s, int)]) for o in out) ) return "search" in parsed_output except Exception: