Refactor Expense Claim (#12883)

* patch for custom workflow

* deleted field approval_status

* replaced approval_status with workflow_state

* updated test cases

* validation to check expense approver

* check if workflow_state_name already exists

* fixes

* modified notifications.py

* removed field exp_approval and modified test cases

erpnext/accounts/doctype/journal_entry/journal_entry.js

@@ -116,7 +116,6 @@ erpnext.accounts.JournalEntry = frappe.ui.form.Controller.extend({
 			if(jvd.reference_type==="Expense Claim") {
 				return {
 					filters: {
-						'approval_status': 'Approved',
 						'total_sanctioned_amount': ['>', 0],
 						'status': ['!=', 'Paid'],
 						'docstatus': 1

erpnext/accounts/doctype/payment_entry/payment_entry.js

@@ -96,7 +96,7 @@ frappe.ui.form.on('Payment Entry', {
 			}
 
 			if(child.reference_doctype == "Expense Claim") {
-				filters["status"] = "Approved";
+				filters["docstatus"] = 1;
 				filters["is_paid"] = 0;
 			}
 

erpnext/demo/user/hr.py

@@ -7,7 +7,7 @@ from erpnext.projects.doctype.timesheet.test_timesheet import make_timesheet
 from erpnext.projects.doctype.timesheet.timesheet import make_salary_slip, make_sales_invoice
 from frappe.utils.make_random import get_random
 from erpnext.hr.doctype.expense_claim.test_expense_claim import get_payable_account
-from erpnext.hr.doctype.expense_claim.expense_claim import get_expense_approver, make_bank_entry
+from erpnext.hr.doctype.expense_claim.expense_claim import make_bank_entry
 from erpnext.hr.doctype.leave_application.leave_application import (get_leave_balance_on,
 	OverlapError, AttendanceAlreadyMarkedError)
 
@@ -55,13 +55,11 @@ def work():
 		expense_claim.company = frappe.flags.company
 		expense_claim.payable_account = get_payable_account(expense_claim.company)
 		expense_claim.posting_date = frappe.flags.current_date
-		expense_claim.exp_approver = filter((lambda x: x[0] != 'Administrator'), get_expense_approver(None, '', None, 0, 20, None))[0][0]
 		expense_claim.insert()
 
 		rand = random.random()
 
 		if rand < 0.4:
-			expense_claim.approval_status = "Approved"
 			update_sanctioned_amount(expense_claim)
 			expense_claim.submit()
 
@@ -74,10 +72,6 @@ def work():
 				je.flags.ignore_permissions = 1
 				je.submit()
 
-		elif rand < 0.2:
-			expense_claim.approval_status = "Rejected"
-			expense_claim.submit()
-
 def get_expenses():
 	expenses = []
 	expese_types = frappe.db.sql("""select ect.name, eca.default_account from `tabExpense Claim Type` ect,

erpnext/hr/doctype/expense_claim/expense_claim.js

@@ -38,13 +38,8 @@ cur_frm.add_fetch('employee', 'company', 'company');
 cur_frm.add_fetch('employee','employee_name','employee_name');
 
 cur_frm.cscript.onload = function(doc) {
-	if(!doc.approval_status)
-		cur_frm.set_value("approval_status", "Draft");
-
 	if (doc.__islocal) {
 		cur_frm.set_value("posting_date", frappe.datetime.get_today());
-		if(doc.amended_from)
-			cur_frm.set_value("approval_status", "Draft");
 		cur_frm.cscript.clear_sanctioned(doc);
 	}
 
@@ -53,12 +48,6 @@ cur_frm.cscript.onload = function(doc) {
 			query: "erpnext.controllers.queries.employee_query"
 		};
 	};
-
-	cur_frm.set_query("exp_approver", function() {
-		return {
-			query: "erpnext.hr.doctype.expense_claim.expense_claim.get_expense_approver"
-		};
-	});
 };
 
 cur_frm.cscript.clear_sanctioned = function(doc) {
@@ -75,13 +64,8 @@ cur_frm.cscript.refresh = function(doc) {
 	cur_frm.cscript.set_help(doc);
 
 	if(!doc.__islocal) {
-		cur_frm.toggle_enable("exp_approver", doc.approval_status=="Draft");
-		cur_frm.toggle_enable("approval_status", (doc.exp_approver==frappe.session.user && doc.docstatus==0));
 
-		if (doc.docstatus==0 && doc.exp_approver==frappe.session.user && doc.approval_status=="Approved")
+		if (doc.docstatus===1) {
-			cur_frm.savesubmit();
-
-		if (doc.docstatus===1 && doc.approval_status=="Approved") {
 			/* eslint-disable */
 			// no idea how `me` works here
 			var entry_doctype, entry_reference_doctype, entry_reference_name;
@@ -114,14 +98,6 @@ cur_frm.cscript.set_help = function(doc) {
 	cur_frm.set_intro("");
 	if(doc.__islocal && !in_list(frappe.user_roles, "HR User")) {
 		cur_frm.set_intro(__("Fill the form and save it"));
-	} else {
-		if(doc.docstatus==0 && doc.approval_status=="Draft") {
-			if(frappe.session.user==doc.exp_approver) {
-				cur_frm.set_intro(__("You are the Expense Approver for this record. Please Update the 'Status' and Save"));
-			} else {
-				cur_frm.set_intro(__("Expense Claim is pending approval. Only the Expense Approver can update status."));
-			}
-		}
 	}
 };
 
@@ -183,7 +159,7 @@ frappe.ui.form.on("Expense Claim", {
 	refresh: function(frm) {
 		frm.trigger("toggle_fields");
 
-		if(frm.doc.docstatus == 1 && frm.doc.approval_status == 'Approved') {
+		if(frm.doc.docstatus == 1) {
 			frm.add_custom_button(__('Accounting Ledger'), function() {
 				frappe.route_options = {
 					voucher_no: frm.doc.name,
@@ -194,7 +170,7 @@ frappe.ui.form.on("Expense Claim", {
 			}, __("View"));
 		}
 
-		if (frm.doc.docstatus===1 && frm.doc.approval_status=="Approved"
+		if (frm.doc.docstatus===1
 				&& (cint(frm.doc.total_amount_reimbursed) < cint(frm.doc.total_sanctioned_amount))
 				&& frappe.model.can_create("Payment Entry")) {
 			frm.add_custom_button(__('Payment'),

erpnext/hr/doctype/expense_claim/expense_claim.json

@@ -44,104 +44,6 @@
    "set_only_once": 1, 
    "unique": 0
   }, 
-  {
-   "allow_bulk_edit": 0, 
-   "allow_on_submit": 0, 
-   "bold": 0, 
-   "collapsible": 0, 
-   "columns": 0, 
-   "default": "Draft", 
-   "depends_on": "eval:!doc.__islocal", 
-   "fieldname": "approval_status", 
-   "fieldtype": "Select", 
-   "hidden": 0, 
-   "ignore_user_permissions": 0, 
-   "ignore_xss_filter": 0, 
-   "in_filter": 0, 
-   "in_global_search": 0, 
-   "in_list_view": 0, 
-   "in_standard_filter": 0, 
-   "label": "Approval Status", 
-   "length": 0, 
-   "no_copy": 1, 
-   "oldfieldname": "approval_status", 
-   "oldfieldtype": "Select", 
-   "options": "Draft\nApproved\nRejected", 
-   "permlevel": 0, 
-   "print_hide": 0, 
-   "print_hide_if_no_value": 0, 
-   "read_only": 0, 
-   "remember_last_selected_value": 0, 
-   "report_hide": 0, 
-   "reqd": 0, 
-   "search_index": 1, 
-   "set_only_once": 0, 
-   "unique": 0
-  }, 
-  {
-   "allow_bulk_edit": 0, 
-   "allow_on_submit": 0, 
-   "bold": 0, 
-   "collapsible": 0, 
-   "columns": 0, 
-   "description": "A user with \"Expense Approver\" role", 
-   "fieldname": "exp_approver", 
-   "fieldtype": "Link", 
-   "hidden": 0, 
-   "ignore_user_permissions": 0, 
-   "ignore_xss_filter": 0, 
-   "in_filter": 0, 
-   "in_global_search": 0, 
-   "in_list_view": 0, 
-   "in_standard_filter": 0, 
-   "label": "Approver", 
-   "length": 0, 
-   "no_copy": 0, 
-   "oldfieldname": "exp_approver", 
-   "oldfieldtype": "Select", 
-   "options": "User", 
-   "permlevel": 0, 
-   "print_hide": 0, 
-   "print_hide_if_no_value": 0, 
-   "read_only": 0, 
-   "remember_last_selected_value": 0, 
-   "report_hide": 0, 
-   "reqd": 0, 
-   "search_index": 0, 
-   "set_only_once": 0, 
-   "unique": 0, 
-   "width": "160px"
-  }, 
-  {
-   "allow_bulk_edit": 0, 
-   "allow_on_submit": 0, 
-   "bold": 0, 
-   "collapsible": 0, 
-   "columns": 0, 
-   "fieldname": "column_break0", 
-   "fieldtype": "Column Break", 
-   "hidden": 0, 
-   "ignore_user_permissions": 0, 
-   "ignore_xss_filter": 0, 
-   "in_filter": 0, 
-   "in_global_search": 0, 
-   "in_list_view": 0, 
-   "in_standard_filter": 0, 
-   "length": 0, 
-   "no_copy": 0, 
-   "oldfieldtype": "Column Break", 
-   "permlevel": 0, 
-   "print_hide": 0, 
-   "print_hide_if_no_value": 0, 
-   "read_only": 0, 
-   "remember_last_selected_value": 0, 
-   "report_hide": 0, 
-   "reqd": 0, 
-   "search_index": 0, 
-   "set_only_once": 0, 
-   "unique": 0, 
-   "width": "50%"
-  }, 
   {
    "allow_bulk_edit": 0, 
    "allow_on_submit": 0, 
@@ -1058,8 +960,8 @@
  "istable": 0, 
  "max_attachments": 0, 
  "menu_index": 0, 
- "modified": "2017-12-14 17:40:02.959352", 
+ "modified": "2018-02-13 12:14:38.236072", 
- "modified_by": "nabinhait@gmail.com", 
+ "modified_by": "Administrator", 
  "module": "HR", 
  "name": "Expense Claim", 
  "name_case": "Title Case", 
@@ -1152,7 +1054,7 @@
  "quick_entry": 0, 
  "read_only": 0, 
  "read_only_onload": 0, 
- "search_fields": "approval_status,employee,employee_name", 
+ "search_fields": "employee,employee_name", 
  "show_name_in_global_search": 1, 
  "sort_field": "modified", 
  "sort_order": "DESC", 

erpnext/hr/doctype/expense_claim/expense_claim.py

@@ -14,20 +14,16 @@ from erpnext.controllers.accounts_controller import AccountsController
 from frappe.utils.csvutils import getlink
 
 class InvalidExpenseApproverError(frappe.ValidationError): pass
+class ExpenseApproverIdentityError(frappe.ValidationError): pass
 
 class ExpenseClaim(AccountsController):
 	def onload(self):
 		self.get("__onload").make_payment_via_journal_entry = frappe.db.get_single_value('Accounts Settings', 
 			'make_payment_via_journal_entry')
 
-	def get_feed(self):
-		return _("{0}: From {0} for {1}").format(self.approval_status,
-			self.employee_name, self.total_claimed_amount)
-
 	def validate(self):
 		self.validate_advances()
 		self.validate_sanctioned_amount()
-		self.validate_expense_approver()
 		self.calculate_total_amount()
 		set_employee_name(self)
 		self.set_expense_account()
@@ -46,12 +42,10 @@ class ExpenseClaim(AccountsController):
 
 		paid_amount = flt(self.total_amount_reimbursed) + flt(self.total_advance_amount)
 		if self.total_sanctioned_amount > 0 and self.total_sanctioned_amount ==  paid_amount\
-			and self.docstatus == 1 and self.approval_status == 'Approved':
+			and self.docstatus == 1:
 			self.status = "Paid"
-		elif self.total_sanctioned_amount > 0 and self.docstatus == 1 and self.approval_status == 'Approved':
+		elif self.total_sanctioned_amount > 0 and self.docstatus == 1:
 			self.status = "Unpaid"
-		elif self.docstatus == 1 and self.approval_status == 'Rejected':
-			self.status = 'Rejected'
 
 	def set_payable_account(self):
 		if not self.payable_account and not self.is_paid:
@@ -62,8 +56,6 @@ class ExpenseClaim(AccountsController):
 			self.cost_center = frappe.db.get_value('Company', self.company, 'cost_center')
 
 	def on_submit(self):
-		if self.approval_status=="Draft":
-			frappe.throw(_("""Approval Status must be 'Approved' or 'Rejected'"""))
 
 		self.update_task_and_project()
 		self.make_gl_entries()
@@ -189,17 +181,9 @@ class ExpenseClaim(AccountsController):
 		self.total_claimed_amount = 0
 		self.total_sanctioned_amount = 0
 		for d in self.get('expenses'):
-			if self.approval_status == 'Rejected':
-				d.sanctioned_amount = 0.0
-
 			self.total_claimed_amount += flt(d.claim_amount)
 			self.total_sanctioned_amount += flt(d.sanctioned_amount)
 
-	def validate_expense_approver(self):
-		if self.exp_approver and "Expense Approver" not in frappe.get_roles(self.exp_approver):
-			frappe.throw(_("{0} ({1}) must have role 'Expense Approver'")\
-				.format(get_fullname(self.exp_approver), self.exp_approver), InvalidExpenseApproverError)
-
 	def update_task(self):
 		task = frappe.get_doc("Task", self.task)
 		task.update_total_expense_claim()
@@ -249,15 +233,6 @@ def update_reimbursed_amount(doc):
 	doc.set_status()
 	frappe.db.set_value("Expense Claim", doc.name , "status", doc.status)
 
-@frappe.whitelist()
-def get_expense_approver(doctype, txt, searchfield, start, page_len, filters):
-	return frappe.db.sql("""
-		select u.name, concat(u.first_name, ' ', u.last_name)
-		from tabUser u, `tabHas Role` r
-		where u.name = r.parent and r.role = 'Expense Approver' 
-		and u.enabled = 1 and u.name like %s
-	""", ("%" + txt + "%"))
-
 @frappe.whitelist()
 def make_bank_entry(dt, dn):
 	from erpnext.accounts.doctype.journal_entry.journal_entry import get_default_bank_cash_account

erpnext/hr/doctype/expense_claim/expense_claim_list.js

@@ -1,6 +1,5 @@
 frappe.listview_settings['Expense Claim'] = {
-	add_fields: ["approval_status", "total_claimed_amount", "docstatus"],
+	add_fields: ["total_claimed_amount", "docstatus"],
-	filters:[["approval_status","!=", "Rejected"]],
 	get_indicator: function(doc) {
 		if(doc.status == "Paid") {
 			return [__("Paid"), "green", "status,=,'Paid'"];

erpnext/hr/doctype/expense_claim/test_expense_claim.js

@@ -9,9 +9,8 @@ QUnit.test("Test: Expense Claim [HR]", function (assert) {
 		// Creating Expense Claim
 		() => frappe.set_route('List','Expense Claim','List'),
 		() => frappe.timeout(0.3),
-		() => frappe.click_button('Make a new Expense Claim'),
+		() => frappe.click_button('New'),
 		() => {
-			cur_frm.set_value('exp_approver','Administrator'),
 			cur_frm.set_value('is_paid',1),
 			cur_frm.set_value('expenses',[]),
 			d = frappe.model.add_child(cur_frm.doc,'Expense Claim Detail','expenses'),
@@ -22,36 +21,23 @@ QUnit.test("Test: Expense Claim [HR]", function (assert) {
 			d.sanctioned_amount=2000,
 			refresh_field('expenses');
 		},
-		() => frappe.timeout(2),
-		() => frappe.db.get_value('Employee', {'employee_name': 'Test Employee 1'}, 'name'),
-		(r) => {
-			employee_name = r.message.name;
-		},
 		() => frappe.timeout(1),
-		() => cur_frm.set_value('employee',employee_name),
+		() => cur_frm.set_value('employee','Test Employee 1'),
-		() => cur_frm.set_value('employee_name','Test Employee 1'),
 		() => cur_frm.set_value('company','For Testing'),
 		() => cur_frm.set_value('payable_account','Creditors - FT'),
 		() => cur_frm.set_value('cost_center','Main - FT'),
 		() => cur_frm.set_value('mode_of_payment','Cash'),
 		() => cur_frm.save(),
-		() => frappe.timeout(1),
-		() => cur_frm.set_value('approval_status','Approved'),
-		() => frappe.timeout(1),
-		() => cur_frm.save(),
-		// Submitting the Expense Claim
 		() => frappe.click_button('Submit'),
 		() => frappe.click_button('Yes'),
 		() => frappe.timeout(3),
 
 		// Checking if the amount is correctly reimbursed for the employee
 		() => {
-			assert.equal(employee_name,cur_frm.get_field('employee').value,
+			assert.equal("Test Employee 1",cur_frm.doc.employee, 'Employee name set correctly');
-				'Expense Claim is created for correct employee');
+			assert.equal(1, cur_frm.doc.is_paid, 'Expense is paid as required');
-			assert.equal(1,cur_frm.get_field('is_paid').value,
+			assert.equal(2000, cur_frm.doc.total_amount_reimbursed, 'Amount is reimbursed correctly');
-				'Expense is paid as required');
+
-			assert.equal(2000,cur_frm.get_field('total_amount_reimbursed').value,
-				'Amount is reimbursed correctly');
 		},
 		() => done()
 	]);

erpnext/hr/doctype/expense_claim/test_expense_claim.py

@@ -81,24 +81,6 @@ class TestExpenseClaim(unittest.TestCase):
 			self.assertEquals(expected_values[gle.account][1], gle.debit)
 			self.assertEquals(expected_values[gle.account][2], gle.credit)
 
-	def test_rejected_expense_claim(self):
-		payable_account = get_payable_account("Wind Power LLC")
-		expense_claim = frappe.get_doc({
-			 "doctype": "Expense Claim",
-			 "employee": "_T-Employee-0001",
-			 "payable_account": payable_account,
-			 "approval_status": "Rejected",
-			 "expenses":
-			 	[{ "expense_type": "Travel", "default_account": "Travel Expenses - WP", "claim_amount": 300, "sanctioned_amount": 200 }]
-		})
-		expense_claim.submit()
-
-		self.assertEquals(expense_claim.status, 'Rejected')
-		self.assertEquals(expense_claim.total_sanctioned_amount, 0.0)
-
-		gl_entry = frappe.get_all('GL Entry', {'voucher_type': 'Expense Claim', 'voucher_no': expense_claim.name})
-		self.assertEquals(len(gl_entry), 0)
-
 def get_payable_account(company):
 	return frappe.db.get_value('Company', company, 'default_payable_account')
 
@@ -107,7 +89,6 @@ def make_expense_claim(payable_account,claim_amount, sanctioned_amount, company,
 		 "doctype": "Expense Claim",
 		 "employee": "_T-Employee-0001",
 		 "payable_account": payable_account,
-		 "approval_status": "Approved",
 		 "company": company,
 		 "expenses":
 			[{ "expense_type": "Travel", "default_account": account, "claim_amount": claim_amount, "sanctioned_amount": sanctioned_amount }]

erpnext/patches.txt

@@ -494,6 +494,7 @@ erpnext.patches.v10_0.set_default_payment_terms_based_on_company
 erpnext.patches.v10_0.update_sales_order_link_to_purchase_order
 erpnext.patches.v10_0.added_extra_gst_custom_field_in_gstr2 #2018-02-13
 erpnext.patches.v10_0.item_barcode_childtable_migrate
+erpnext.patches.v10_0.workflow_expense_claim
 erpnext.patches.v10_0.set_b2c_limit
 erpnext.patches.v10_0.update_translatable_fields
-erpnext.patches.v10_0.rename_offer_letter_to_job_offer
+erpnext.patches.v10_0.rename_offer_letter_to_job_offer

erpnext/patches/v10_0/workflow_expense_claim.py

@@ -0,0 +1,65 @@
+# Copyright (c) 2017, Frappe and Contributors
+# License: GNU General Public License v3. See license.txt
+
+from __future__ import unicode_literals
+import frappe
+
+def execute():
+	if frappe.db.a_row_exists("Expense Claim"):
+		frappe.reload_doc("hr", "doctype", "expense_claim")
+		frappe.reload_doc("hr", "doctype", "vehicle_log")
+		frappe.reload_doc("hr", "doctype", "expense_claim_advance")
+		frappe.reload_doc("workflow", "doctype", "workflow")
+
+		states = {'Approved': 'Success', 'Rejected': 'Danger', 'Draft': 'Warning'}
+		for state, style in states.items():
+			if not frappe.db.exists("Workflow State", state):
+				frappe.get_doc({
+					'doctype': 'Workflow State',
+					'workflow_state_name': state,
+					'style': style
+				}).insert(ignore_permissions=True)
+
+		for action in ['Approve', 'Reject']:
+			if not frappe.db.exists("Workflow Action", action):
+				frappe.get_doc({
+					'doctype': 'Workflow Action',
+					'workflow_action_name': action
+				}).insert(ignore_permissions=True)
+
+		if not frappe.db.exists("Workflow", "Expense Approval"):
+			frappe.get_doc({
+				'doctype': 'Workflow',
+				'workflow_name': 'Expense Approval',
+				'document_type': 'Expense Claim',
+				'is_active': 1,
+				'workflow_state_field': 'workflow_state',
+				'states': [{
+					"state": 'Draft',
+					"doc_status": 0,
+					"allow_edit": 'Employee'
+				}, {
+					"state": 'Approved',
+					"doc_status": 1,
+					"allow_edit": 'Expense Approver'
+				}, {
+					"state": 'Rejected',
+					"doc_status": 0,
+					"allow_edit": 'Expense Approver'
+				}],
+				'transitions': [{
+					"state": 'Draft',
+					"action": 'Approve',
+					"next_state": 'Approved',
+					"allowed": 'Expense Approver'
+				},
+				{
+					"state": 'Draft',
+					"action": 'Reject',
+					"next_state": 'Rejected',
+					"allowed": 'Expense Approver'
+				}]
+			}).insert(ignore_permissions=True)
+
+		if frappe.db.has_column("Expense Claim", "status"):
+			frappe.db.sql("""update `tabExpense Claim` set workflow_state = approval_status""")

erpnext/projects/doctype/project/project.py

@@ -170,7 +170,7 @@ class Project(Document):
 
 		from_expense_claim = frappe.db.sql("""select
 			sum(total_sanctioned_amount) as total_sanctioned_amount
-			from `tabExpense Claim` where project = %s and approval_status='Approved'
+			from `tabExpense Claim` where project = %s
 			and docstatus = 1""",
 			self.name, as_dict=1)[0]
 

erpnext/projects/doctype/task/task.py

@@ -77,7 +77,7 @@ class Task(NestedSet):
 
 	def update_total_expense_claim(self):
 		self.total_expense_claim = frappe.db.sql("""select sum(total_sanctioned_amount) from `tabExpense Claim`
-			where project = %s and task = %s and approval_status = "Approved" and docstatus=1""",(self.project, self.name))[0][0]
+			where project = %s and task = %s and docstatus=1""",(self.project, self.name))[0][0]
 
 	def update_time_and_costing(self):
 		tl = frappe.db.sql("""select min(from_time) as start_date, max(to_time) as end_date,

erpnext/startup/notifications.py

@@ -30,7 +30,7 @@ def get_notification_config():
 			},
 			"Payment Entry": {"docstatus": 0},
 			"Leave Application": {"docstatus": 0},
-			"Expense Claim": {"approval_status": "Draft"},
+			"Expense Claim": {"docstatus": 0},
 			"Job Applicant": {"status": "Open"},
 			"Delivery Note": {
 				"status": ("not in", ("Completed", "Closed")),