Merge pull request #23067 from rohitwaghchaure/profit-and-loss-report-not-working

fix: profit and loss report not working
diff --git a/erpnext/hr/doctype/department/department.json b/erpnext/hr/doctype/department/department.json
index a54c1d1..dcb6a74 100644
--- a/erpnext/hr/doctype/department/department.json
+++ b/erpnext/hr/doctype/department/department.json
@@ -17,10 +17,10 @@
   "payroll_cost_center",
   "column_break_9",
   "leave_block_list",
-  "leave_section",
+  "approvers",
   "leave_approvers",
-  "expense_section",
   "expense_approvers",
+  "shift_request_approver",
   "lft",
   "rgt",
   "old_parent"
@@ -33,14 +33,18 @@
    "label": "Department",
    "oldfieldname": "department_name",
    "oldfieldtype": "Data",
-   "reqd": 1
+   "reqd": 1,
+   "show_days": 1,
+   "show_seconds": 1
   },
   {
    "fieldname": "parent_department",
    "fieldtype": "Link",
    "in_list_view": 1,
    "label": "Parent Department",
-   "options": "Department"
+   "options": "Department",
+   "show_days": 1,
+   "show_seconds": 1
   },
   {
    "fieldname": "company",
@@ -48,7 +52,9 @@
    "in_standard_filter": 1,
    "label": "Company",
    "options": "Company",
-   "reqd": 1
+   "reqd": 1,
+   "show_days": 1,
+   "show_seconds": 1
   },
   {
    "bold": 1,
@@ -56,17 +62,23 @@
    "fieldname": "is_group",
    "fieldtype": "Check",
    "in_list_view": 1,
-   "label": "Is Group"
+   "label": "Is Group",
+   "show_days": 1,
+   "show_seconds": 1
   },
   {
    "default": "0",
    "fieldname": "disabled",
    "fieldtype": "Check",
-   "label": "Disabled"
+   "label": "Disabled",
+   "show_days": 1,
+   "show_seconds": 1
   },
   {
    "fieldname": "section_break_4",
-   "fieldtype": "Section Break"
+   "fieldtype": "Section Break",
+   "show_days": 1,
+   "show_seconds": 1
   },
   {
    "description": "Days for which Holidays are blocked for this department.",
@@ -74,31 +86,25 @@
    "fieldtype": "Link",
    "in_list_view": 1,
    "label": "Leave Block List",
-   "options": "Leave Block List"
+   "options": "Leave Block List",
+   "show_days": 1,
+   "show_seconds": 1
   },
   {
-   "fieldname": "leave_section",
-   "fieldtype": "Section Break",
-   "label": "Leave Approvers"
-  },
-  {
-   "description": "The first Leave Approver in the list will be set as the default Leave Approver.",
    "fieldname": "leave_approvers",
    "fieldtype": "Table",
    "label": "Leave Approver",
-   "options": "Department Approver"
+   "options": "Department Approver",
+   "show_days": 1,
+   "show_seconds": 1
   },
   {
-   "fieldname": "expense_section",
-   "fieldtype": "Section Break",
-   "label": "Expense Approvers"
-  },
-  {
-   "description": "The first Expense Approver in the list will be set as the default Expense Approver.",
    "fieldname": "expense_approvers",
    "fieldtype": "Table",
    "label": "Expense Approver",
-   "options": "Department Approver"
+   "options": "Department Approver",
+   "show_days": 1,
+   "show_seconds": 1
   },
   {
    "fieldname": "lft",
@@ -106,7 +112,9 @@
    "hidden": 1,
    "label": "lft",
    "print_hide": 1,
-   "read_only": 1
+   "read_only": 1,
+   "show_days": 1,
+   "show_seconds": 1
   },
   {
    "fieldname": "rgt",
@@ -114,7 +122,9 @@
    "hidden": 1,
    "label": "rgt",
    "print_hide": 1,
-   "read_only": 1
+   "read_only": 1,
+   "show_days": 1,
+   "show_seconds": 1
   },
   {
    "fieldname": "old_parent",
@@ -122,28 +132,52 @@
    "hidden": 1,
    "ignore_user_permissions": 1,
    "label": "Old Parent",
-   "print_hide": 1
+   "print_hide": 1,
+   "show_days": 1,
+   "show_seconds": 1
   },
   {
    "fieldname": "column_break_3",
-   "fieldtype": "Column Break"
+   "fieldtype": "Column Break",
+   "show_days": 1,
+   "show_seconds": 1
   },
   {
    "fieldname": "payroll_cost_center",
    "fieldtype": "Link",
    "label": "Payroll Cost Center",
-   "options": "Cost Center"
+   "options": "Cost Center",
+   "show_days": 1,
+   "show_seconds": 1
   },
   {
    "fieldname": "column_break_9",
-   "fieldtype": "Column Break"
+   "fieldtype": "Column Break",
+   "show_days": 1,
+   "show_seconds": 1
+  },
+  {
+   "description": "The first Approver in the list will be set as the default Approver.",
+   "fieldname": "approvers",
+   "fieldtype": "Section Break",
+   "label": "Approvers",
+   "show_days": 1,
+   "show_seconds": 1
+  },
+  {
+   "fieldname": "shift_request_approver",
+   "fieldtype": "Table",
+   "label": "Shift Request Approver",
+   "options": "Department Approver",
+   "show_days": 1,
+   "show_seconds": 1
   }
  ],
  "icon": "fa fa-sitemap",
  "idx": 1,
  "is_tree": 1,
  "links": [],
- "modified": "2020-05-05 18:49:28.503931",
+ "modified": "2020-06-23 15:42:00.563272",
  "modified_by": "Administrator",
  "module": "HR",
  "name": "Department",
diff --git a/erpnext/hr/doctype/department_approver/department_approver.py b/erpnext/hr/doctype/department_approver/department_approver.py
index afd54b8..9b2de0e 100644
--- a/erpnext/hr/doctype/department_approver/department_approver.py
+++ b/erpnext/hr/doctype/department_approver/department_approver.py
@@ -15,12 +15,12 @@
 def get_approvers(doctype, txt, searchfield, start, page_len, filters):
 
 	if not filters.get("employee"):
-		frappe.throw(_("Please select Employee Record first."))
+		frappe.throw(_("Please select Employee first."))
 
 	approvers = []
 	department_details = {}
 	department_list = []
-	employee = frappe.get_value("Employee", filters.get("employee"), ["department", "leave_approver", "expense_approver"], as_dict=True)
+	employee = frappe.get_value("Employee", filters.get("employee"), ["department", "leave_approver", "expense_approver", "shift_request_approver"], as_dict=True)
 
 	employee_department = filters.get("department") or employee.department
 	if employee_department:
@@ -37,13 +37,18 @@
 	if filters.get("doctype") == "Expense Claim" and employee.expense_approver:
 		approvers.append(frappe.db.get_value("User", employee.expense_approver, ['name', 'first_name', 'last_name']))
 
+	if filters.get("doctype") == "Shift Request" and employee.shift_request_approver:
+		approvers.append(frappe.db.get_value("User", employee.shift_request_approver, ['name', 'first_name', 'last_name']))
 
 	if filters.get("doctype") == "Leave Application":
 		parentfield = "leave_approvers"
 		field_name = "Leave Approver"
-	else:
+	elif filters.get("doctype") == "Expense Claim":
 		parentfield = "expense_approvers"
 		field_name = "Expense Approver"
+	elif filters.get("doctype") == "Shift Request":
+		parentfield = "shift_request_approver"
+		field_name = "Shift Request Approver"
 	if department_list:
 		for d in department_list:
 			approvers += frappe.db.sql("""select user.name, user.first_name, user.last_name from
diff --git a/erpnext/hr/doctype/employee/employee.json b/erpnext/hr/doctype/employee/employee.json
index f2afe06..8c02e4f 100644
--- a/erpnext/hr/doctype/employee/employee.json
+++ b/erpnext/hr/doctype/employee/employee.json
@@ -51,10 +51,14 @@
   "column_break_31",
   "grade",
   "branch",
+  "approvers_section",
+  "expense_approver",
+  "leave_approver",
+  "column_break_45",
+  "shift_request_approver",
   "attendance_and_leave_details",
   "leave_policy",
   "attendance_device_id",
-  "leave_approver",
   "column_break_44",
   "holiday_list",
   "default_shift",
@@ -62,7 +66,6 @@
   "salary_mode",
   "payroll_cost_center",
   "column_break_52",
-  "expense_approver",
   "bank_name",
   "bank_ac_no",
   "health_insurance_section",
@@ -806,14 +809,37 @@
    "fieldname": "expense_approver",
    "fieldtype": "Link",
    "label": "Expense Approver",
-   "options": "User"
+   "options": "User",
+   "show_days": 1,
+   "show_seconds": 1
+  },
+  {
+   "fieldname": "approvers_section",
+   "fieldtype": "Section Break",
+   "label": "Approvers",
+   "show_days": 1,
+   "show_seconds": 1
+  },
+  {
+   "fieldname": "column_break_45",
+   "fieldtype": "Column Break",
+   "show_days": 1,
+   "show_seconds": 1
+  },
+  {
+   "fieldname": "shift_request_approver",
+   "fieldtype": "Link",
+   "label": "Shift Request Approver",
+   "options": "User",
+   "show_days": 1,
+   "show_seconds": 1
   }
  ],
  "icon": "fa fa-user",
  "idx": 24,
  "image_field": "image",
  "links": [],
- "modified": "2020-07-03 21:28:04.109189",
+ "modified": "2020-07-28 01:36:04.109189",
  "modified_by": "Administrator",
  "module": "HR",
  "name": "Employee",
diff --git a/erpnext/hr/doctype/shift_assignment/shift_assignment.json b/erpnext/hr/doctype/shift_assignment/shift_assignment.json
index 72cbba8..ce2a10f 100644
--- a/erpnext/hr/doctype/shift_assignment/shift_assignment.json
+++ b/erpnext/hr/doctype/shift_assignment/shift_assignment.json
@@ -10,9 +10,11 @@
   "employee",
   "employee_name",
   "shift_type",
+  "status",
   "column_break_3",
   "company",
-  "date",
+  "start_date",
+  "end_date",
   "shift_request",
   "department",
   "amended_from"
@@ -60,12 +62,6 @@
    "reqd": 1
   },
   {
-   "fieldname": "date",
-   "fieldtype": "Date",
-   "in_list_view": 1,
-   "label": "Date"
-  },
-  {
    "fieldname": "shift_request",
    "fieldtype": "Link",
    "label": "Shift Request",
@@ -80,11 +76,36 @@
    "options": "Shift Assignment",
    "print_hide": 1,
    "read_only": 1
+  },
+  {
+   "fieldname": "start_date",
+   "fieldtype": "Date",
+   "in_list_view": 1,
+   "label": "Start Date",
+   "reqd": 1
+  },
+  {
+   "allow_on_submit": 1,
+   "fieldname": "end_date",
+   "fieldtype": "Date",
+   "label": "End Date",
+   "show_days": 1,
+   "show_seconds": 1
+  },
+  {
+   "allow_on_submit": 1,
+   "default": "Active",
+   "fieldname": "status",
+   "fieldtype": "Select",
+   "label": "Status",
+   "options": "Active\nInactive",
+   "show_days": 1,
+   "show_seconds": 1
   }
  ],
  "is_submittable": 1,
  "links": [],
- "modified": "2019-12-12 15:49:06.956901",
+ "modified": "2020-06-15 14:27:54.310773",
  "modified_by": "Administrator",
  "module": "HR",
  "name": "Shift Assignment",
diff --git a/erpnext/hr/doctype/shift_assignment/shift_assignment.py b/erpnext/hr/doctype/shift_assignment/shift_assignment.py
index 40c78cd..f8b7334 100644
--- a/erpnext/hr/doctype/shift_assignment/shift_assignment.py
+++ b/erpnext/hr/doctype/shift_assignment/shift_assignment.py
@@ -11,38 +11,63 @@
 from erpnext.hr.doctype.holiday_list.holiday_list import is_holiday
 from datetime import timedelta, datetime
 
-class OverlapError(frappe.ValidationError): pass
-
 class ShiftAssignment(Document):
 	def validate(self):
 		self.validate_overlapping_dates()
 
+		if self.end_date and self.end_date <= self.start_date:
+			frappe.throw(_("End Date must not be lesser than Start Date"))
+
 	def validate_overlapping_dates(self):
-			if not self.name:
-				self.name = "New Shift Assignment"
+		if not self.name:
+			self.name = "New Shift Assignment"
 
-			d = frappe.db.sql("""
-				select
-					name, shift_type, date
-				from `tabShift Assignment`
-				where employee = %(employee)s and docstatus < 2
-				and date = %(date)s
-				and name != %(name)s""", {
-					"employee": self.employee,
-					"shift_type": self.shift_type,
-					"date": self.date,
-					"name": self.name
-				}, as_dict = 1)
+		condition = """and (
+				end_date is null
+				or
+					%(start_date)s between start_date and end_date
+		"""
 
-			for date_overlap in d:
-				if date_overlap['name']:
-					self.throw_overlap_error(date_overlap)
+		if self.end_date:
+			condition  += """ or
+					%(end_date)s between start_date and end_date
+					or
+					start_date between %(start_date)s and %(end_date)s
+				) """
+		else:
+			condition += """ ) """
 
-	def throw_overlap_error(self, d):
-		msg = _("Employee {0} has already applied for {1} on {2} : ").format(self.employee,
-			d['shift_type'], formatdate(d['date'])) \
-			+ """ <b><a href="#Form/Shift Assignment/{0}">{0}</a></b>""".format(d["name"])
-		frappe.throw(msg, OverlapError)
+		assigned_shifts = frappe.db.sql("""
+			select name, shift_type, start_date ,end_date, docstatus, status
+			from `tabShift Assignment`
+			where
+				employee=%(employee)s and docstatus = 1
+				and name != %(name)s
+				and status = "Active"
+				{0}
+		""".format(condition), {
+			"employee": self.employee,
+			"shift_type": self.shift_type,
+			"start_date": self.start_date,
+			"end_date": self.end_date,
+			"name": self.name
+		}, as_dict = 1)
+
+		if len(assigned_shifts):
+			self.throw_overlap_error(assigned_shifts[0])
+
+	def throw_overlap_error(self, shift_details):
+		shift_details = frappe._dict(shift_details)
+		if shift_details.docstatus == 1 and shift_details.status == "Active":
+			msg = _("Employee {0} already has Active Shift {1}: {2}").format(frappe.bold(self.employee), frappe.bold(self.shift_type), frappe.bold(shift_details.name))
+		if shift_details.start_date:
+			msg += _(" from {0}").format(getdate(self.start_date).strftime("%d-%m-%Y"))
+			title = "Ongoing Shift"
+			if shift_details.end_date:
+				msg += _(" to {0}").format(getdate(self.end_date).strftime("%d-%m-%Y"))
+				title = "Active Shift"
+		if msg:
+			frappe.throw(msg, title=title)
 
 @frappe.whitelist()
 def get_events(start, end, filters=None):
@@ -62,19 +87,22 @@
 	return events
 
 def add_assignments(events, start, end, conditions=None):
-	query = """select name, date, employee_name, 
+	query = """select name, start_date, end_date, employee_name,
 		employee, docstatus
 		from `tabShift Assignment` where
-		date <= %(date)s
-		and docstatus < 2"""
+		start_date >= %(start_date)s
+		or end_date <=  %(end_date)s
+		or (%(start_date)s between start_date and end_date and %(end_date)s between start_date and end_date)
+		and docstatus = 1"""
 	if conditions:
 		query += conditions
 
-	for d in frappe.db.sql(query, {"date":start, "date":end}, as_dict=True):
+	for d in frappe.db.sql(query, {"start_date":start, "end_date":end}, as_dict=True):
 		e = {
 			"name": d.name,
 			"doctype": "Shift Assignment",
-			"date": d.date,
+			"start_date": d.start_date,
+			"end_date": d.end_date if d.end_date else nowdate(),
 			"title": cstr(d.employee_name) + \
 				cstr(d.shift_type),
 			"docstatus": d.docstatus
@@ -92,7 +120,16 @@
 	:param next_shift_direction: One of: None, 'forward', 'reverse'. Direction to look for next shift if shift not found on given date.
 	"""
 	default_shift = frappe.db.get_value('Employee', employee, 'default_shift')
-	shift_type_name = frappe.db.get_value('Shift Assignment', {'employee':employee, 'date': for_date, 'docstatus': '1'}, 'shift_type')
+	shift_type_name = None
+	shift_assignment_details = frappe.db.get_value('Shift Assignment', {'employee':employee, 'start_date':('<=', for_date), 'docstatus': '1', 'status': "Active"}, ['shift_type', 'end_date'])
+
+	if shift_assignment_details:
+		shift_type_name = shift_assignment_details[0]
+
+		# if end_date present means that shift is over after end_date else it is a ongoing shift.
+		if shift_assignment_details[1] and for_date >= shift_assignment_details[1] :
+			shift_type_name = None
+
 	if not shift_type_name and consider_default_shift:
 		shift_type_name = default_shift
 	if shift_type_name:
@@ -117,16 +154,20 @@
 			direction = '<' if next_shift_direction == 'reverse' else '>'
 			sort_order = 'desc' if next_shift_direction == 'reverse' else 'asc'
 			dates = frappe.db.get_all('Shift Assignment',
-				'date',
-				{'employee':employee, 'date':(direction, for_date), 'docstatus': '1'},
+				['start_date', 'end_date'],
+				{'employee':employee, 'start_date':(direction, for_date), 'docstatus': '1', "status": "Active"},
 				as_list=True,
-				limit=MAX_DAYS, order_by="date "+sort_order)
-			for date in dates:
-				shift_details = get_employee_shift(employee, date[0], consider_default_shift, None)
-				if shift_details:
-					shift_type_name = shift_details.shift_type.name
-					for_date = date[0]
-					break
+				limit=MAX_DAYS, order_by="start_date "+sort_order)
+
+			if dates:
+				for date in dates:
+					if date[1] and date[1] < for_date:
+						continue
+					shift_details = get_employee_shift(employee, date[0], consider_default_shift, None)
+					if shift_details:
+						shift_type_name = shift_details.shift_type.name
+						for_date = date[0]
+						break
 
 	return get_shift_details(shift_type_name, for_date)
 
@@ -134,7 +175,7 @@
 def get_employee_shift_timings(employee, for_timestamp=now_datetime(), consider_default_shift=False):
 	"""Returns previous shift, current/upcoming shift, next_shift for the given timestamp and employee
 	"""
-	# write and verify a test case for midnight shift. 
+	# write and verify a test case for midnight shift.
 	prev_shift = curr_shift = next_shift = None
 	curr_shift = get_employee_shift(employee, for_timestamp.date(), consider_default_shift, 'forward')
 	if curr_shift:
diff --git a/erpnext/hr/doctype/shift_assignment/shift_assignment_calendar.js b/erpnext/hr/doctype/shift_assignment/shift_assignment_calendar.js
index c2c9bc0..17a986d 100644
--- a/erpnext/hr/doctype/shift_assignment/shift_assignment_calendar.js
+++ b/erpnext/hr/doctype/shift_assignment/shift_assignment_calendar.js
@@ -3,8 +3,8 @@
 
 frappe.views.calendar["Shift Assignment"] = {
 	field_map: {
-		"start": "date",
-		"end": "date",
+		"start": "start_date",
+		"end": "end_date",
 		"id": "name",
 		"docstatus": 1
 	},
diff --git a/erpnext/hr/doctype/shift_assignment/test_shift_assignment.py b/erpnext/hr/doctype/shift_assignment/test_shift_assignment.py
index 7fe80a2..4c3c1ed 100644
--- a/erpnext/hr/doctype/shift_assignment/test_shift_assignment.py
+++ b/erpnext/hr/doctype/shift_assignment/test_shift_assignment.py
@@ -5,7 +5,7 @@
 
 import frappe
 import unittest
-from frappe.utils import nowdate
+from frappe.utils import nowdate, add_days
 
 test_dependencies = ["Shift Type"]
 
@@ -20,8 +20,61 @@
 			"shift_type": "Day Shift",
 			"company": "_Test Company",
 			"employee": "_T-Employee-00001",
-			"date": nowdate()
+			"start_date": nowdate()
 		}).insert()
 		shift_assignment.submit()
 
 		self.assertEqual(shift_assignment.docstatus, 1)
+
+	def test_overlapping_for_ongoing_shift(self):
+		# shift should be Ongoing if Only start_date is present and status = Active
+
+		shift_assignment_1 = frappe.get_doc({
+			"doctype": "Shift Assignment",
+			"shift_type": "Day Shift",
+			"company": "_Test Company",
+			"employee": "_T-Employee-00001",
+			"start_date": nowdate(),
+			"status": 'Active'
+		}).insert()
+		shift_assignment_1.submit()
+
+		self.assertEqual(shift_assignment_1.docstatus, 1)
+
+		shift_assignment = frappe.get_doc({
+			"doctype": "Shift Assignment",
+			"shift_type": "Day Shift",
+			"company": "_Test Company",
+			"employee": "_T-Employee-00001",
+			"start_date": add_days(nowdate(), 2)
+		})
+
+		self.assertRaises(frappe.ValidationError, shift_assignment.save)
+
+	def test_overlapping_for_fixed_period_shift(self):
+		# shift should is for Fixed period if Only start_date and end_date both are present and status = Active
+
+			shift_assignment_1 = frappe.get_doc({
+				"doctype": "Shift Assignment",
+				"shift_type": "Day Shift",
+				"company": "_Test Company",
+				"employee": "_T-Employee-00001",
+				"start_date": nowdate(),
+				"end_date": add_days(nowdate(), 30),
+				"status": 'Active'
+			}).insert()
+			shift_assignment_1.submit()
+
+
+			# it should not allowed within period of any shift.
+			shift_assignment_3 = frappe.get_doc({
+				"doctype": "Shift Assignment",
+				"shift_type": "Day Shift",
+				"company": "_Test Company",
+				"employee": "_T-Employee-00001",
+				"start_date":add_days(nowdate(), 10),
+				"end_date": add_days(nowdate(), 35),
+				"status": 'Active'
+			})
+
+			self.assertRaises(frappe.ValidationError, shift_assignment_3.save)
\ No newline at end of file
diff --git a/erpnext/hr/doctype/shift_request/shift_request.js b/erpnext/hr/doctype/shift_request/shift_request.js
index 1db7c7d..b17a6f3 100644
--- a/erpnext/hr/doctype/shift_request/shift_request.js
+++ b/erpnext/hr/doctype/shift_request/shift_request.js
@@ -2,7 +2,16 @@
 // For license information, please see license.txt
 
 frappe.ui.form.on('Shift Request', {
-	refresh: function(frm) {
-
-	}
+	setup: function(frm) {
+		frm.set_query("approver", function() {
+			return {
+				query: "erpnext.hr.doctype.department_approver.department_approver.get_approvers",
+				filters: {
+					employee: frm.doc.employee,
+					doctype: frm.doc.doctype
+				}
+			};
+		});
+		frm.set_query("employee", erpnext.queries.employee);
+	},
 });
diff --git a/erpnext/hr/doctype/shift_request/shift_request.json b/erpnext/hr/doctype/shift_request/shift_request.json
index dd05647..64cbdff 100644
--- a/erpnext/hr/doctype/shift_request/shift_request.json
+++ b/erpnext/hr/doctype/shift_request/shift_request.json
@@ -1,396 +1,155 @@
 {
- "allow_copy": 0, 
- "allow_guest_to_view": 0, 
- "allow_import": 1, 
- "allow_rename": 0, 
- "autoname": "HR-SHR-.YY.-.MM.-.#####", 
- "beta": 0, 
- "creation": "2018-04-13 16:32:27.974273", 
- "custom": 0, 
- "docstatus": 0, 
- "doctype": "DocType", 
- "document_type": "", 
- "editable_grid": 1, 
- "engine": "InnoDB", 
+ "actions": [],
+ "allow_import": 1,
+ "autoname": "HR-SHR-.YY.-.MM.-.#####",
+ "creation": "2018-04-13 16:32:27.974273",
+ "doctype": "DocType",
+ "editable_grid": 1,
+ "engine": "InnoDB",
+ "field_order": [
+  "shift_type",
+  "employee",
+  "employee_name",
+  "department",
+  "status",
+  "column_break_4",
+  "company",
+  "approver",
+  "from_date",
+  "to_date",
+  "amended_from"
+ ],
  "fields": [
   {
-   "allow_bulk_edit": 0, 
-   "allow_in_quick_entry": 0, 
-   "allow_on_submit": 0, 
-   "bold": 0, 
-   "collapsible": 0, 
-   "columns": 0, 
-   "fieldname": "shift_type", 
-   "fieldtype": "Link", 
-   "hidden": 0, 
-   "ignore_user_permissions": 0, 
-   "ignore_xss_filter": 0, 
-   "in_filter": 0, 
-   "in_global_search": 0, 
-   "in_list_view": 1, 
-   "in_standard_filter": 0, 
-   "label": "Shift Type", 
-   "length": 0, 
-   "no_copy": 0, 
-   "options": "Shift Type", 
-   "permlevel": 0, 
-   "precision": "", 
-   "print_hide": 0, 
-   "print_hide_if_no_value": 0, 
-   "read_only": 0, 
-   "remember_last_selected_value": 0, 
-   "report_hide": 0, 
-   "reqd": 1, 
-   "search_index": 0, 
-   "set_only_once": 0, 
-   "translatable": 0, 
-   "unique": 0
-  }, 
+   "fieldname": "shift_type",
+   "fieldtype": "Link",
+   "in_list_view": 1,
+   "label": "Shift Type",
+   "options": "Shift Type",
+   "reqd": 1
+  },
   {
-   "allow_bulk_edit": 0, 
-   "allow_in_quick_entry": 0, 
-   "allow_on_submit": 0, 
-   "bold": 0, 
-   "collapsible": 0, 
-   "columns": 0, 
-   "fieldname": "employee", 
-   "fieldtype": "Link", 
-   "hidden": 0, 
-   "ignore_user_permissions": 0, 
-   "ignore_xss_filter": 0, 
-   "in_filter": 0, 
-   "in_global_search": 0, 
-   "in_list_view": 1, 
-   "in_standard_filter": 0, 
-   "label": "Employee", 
-   "length": 0, 
-   "no_copy": 0, 
-   "options": "Employee", 
-   "permlevel": 0, 
-   "precision": "", 
-   "print_hide": 0, 
-   "print_hide_if_no_value": 0, 
-   "read_only": 0, 
-   "remember_last_selected_value": 0, 
-   "report_hide": 0, 
-   "reqd": 1, 
-   "search_index": 0, 
-   "set_only_once": 0, 
-   "translatable": 0, 
-   "unique": 0
-  }, 
+   "fieldname": "employee",
+   "fieldtype": "Link",
+   "in_list_view": 1,
+   "label": "Employee",
+   "options": "Employee",
+   "reqd": 1
+  },
   {
-   "allow_bulk_edit": 0, 
-   "allow_in_quick_entry": 0, 
-   "allow_on_submit": 0, 
-   "bold": 0, 
-   "collapsible": 0, 
-   "columns": 0, 
-   "fetch_from": "employee.employee_name", 
-   "fieldname": "employee_name", 
-   "fieldtype": "Data", 
-   "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": "Employee Name", 
-   "length": 0, 
-   "no_copy": 0, 
-   "options": "", 
-   "permlevel": 0, 
-   "precision": "", 
-   "print_hide": 0, 
-   "print_hide_if_no_value": 0, 
-   "read_only": 1, 
-   "remember_last_selected_value": 0, 
-   "report_hide": 0, 
-   "reqd": 0, 
-   "search_index": 0, 
-   "set_only_once": 0, 
-   "translatable": 0, 
-   "unique": 0
-  }, 
+   "fetch_from": "employee.employee_name",
+   "fieldname": "employee_name",
+   "fieldtype": "Data",
+   "label": "Employee Name",
+   "read_only": 1
+  },
   {
-   "allow_bulk_edit": 0, 
-   "allow_in_quick_entry": 0, 
-   "allow_on_submit": 0, 
-   "bold": 0, 
-   "collapsible": 0, 
-   "columns": 0, 
-   "fetch_from": "employee.department", 
-   "fieldname": "department", 
-   "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": "Department", 
-   "length": 0, 
-   "no_copy": 0, 
-   "options": "Department", 
-   "permlevel": 0, 
-   "precision": "", 
-   "print_hide": 0, 
-   "print_hide_if_no_value": 0, 
-   "read_only": 1, 
-   "remember_last_selected_value": 0, 
-   "report_hide": 0, 
-   "reqd": 0, 
-   "search_index": 0, 
-   "set_only_once": 0, 
-   "translatable": 0, 
-   "unique": 0
-  }, 
+   "fetch_from": "employee.department",
+   "fieldname": "department",
+   "fieldtype": "Link",
+   "label": "Department",
+   "options": "Department",
+   "read_only": 1
+  },
   {
-   "allow_bulk_edit": 0, 
-   "allow_in_quick_entry": 0, 
-   "allow_on_submit": 0, 
-   "bold": 0, 
-   "collapsible": 0, 
-   "columns": 0, 
-   "fieldname": "column_break_4", 
-   "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, 
-   "permlevel": 0, 
-   "precision": "", 
-   "print_hide": 0, 
-   "print_hide_if_no_value": 0, 
-   "read_only": 0, 
-   "remember_last_selected_value": 0, 
-   "report_hide": 0, 
-   "reqd": 0, 
-   "search_index": 0, 
-   "set_only_once": 0, 
-   "translatable": 0, 
-   "unique": 0
-  }, 
+   "fieldname": "column_break_4",
+   "fieldtype": "Column Break"
+  },
   {
-   "allow_bulk_edit": 0, 
-   "allow_in_quick_entry": 0, 
-   "allow_on_submit": 0, 
-   "bold": 0, 
-   "collapsible": 0, 
-   "columns": 0, 
-   "fieldname": "company", 
-   "fieldtype": "Link", 
-   "hidden": 0, 
-   "ignore_user_permissions": 0, 
-   "ignore_xss_filter": 0, 
-   "in_filter": 0, 
-   "in_global_search": 0, 
-   "in_list_view": 1, 
-   "in_standard_filter": 0, 
-   "label": "Company", 
-   "length": 0, 
-   "no_copy": 0, 
-   "options": "Company", 
-   "permlevel": 0, 
-   "precision": "", 
-   "print_hide": 0, 
-   "print_hide_if_no_value": 0, 
-   "read_only": 0, 
-   "remember_last_selected_value": 0, 
-   "report_hide": 0, 
-   "reqd": 1, 
-   "search_index": 0, 
-   "set_only_once": 0, 
-   "translatable": 0, 
-   "unique": 0
-  }, 
+   "fieldname": "company",
+   "fieldtype": "Link",
+   "in_list_view": 1,
+   "label": "Company",
+   "options": "Company",
+   "reqd": 1
+  },
   {
-   "allow_bulk_edit": 0, 
-   "allow_in_quick_entry": 0, 
-   "allow_on_submit": 0, 
-   "bold": 0, 
-   "collapsible": 0, 
-   "columns": 0, 
-   "fieldname": "from_date", 
-   "fieldtype": "Date", 
-   "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": "From Date", 
-   "length": 0, 
-   "no_copy": 0, 
-   "permlevel": 0, 
-   "precision": "", 
-   "print_hide": 0, 
-   "print_hide_if_no_value": 0, 
-   "read_only": 0, 
-   "remember_last_selected_value": 0, 
-   "report_hide": 0, 
-   "reqd": 0, 
-   "search_index": 0, 
-   "set_only_once": 0, 
-   "translatable": 0, 
-   "unique": 0
-  }, 
+   "fieldname": "from_date",
+   "fieldtype": "Date",
+   "label": "From Date",
+   "reqd": 1
+  },
   {
-   "allow_bulk_edit": 0, 
-   "allow_in_quick_entry": 0, 
-   "allow_on_submit": 0, 
-   "bold": 0, 
-   "collapsible": 0, 
-   "columns": 0, 
-   "fieldname": "to_date", 
-   "fieldtype": "Date", 
-   "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": "To Date", 
-   "length": 0, 
-   "no_copy": 0, 
-   "permlevel": 0, 
-   "precision": "", 
-   "print_hide": 0, 
-   "print_hide_if_no_value": 0, 
-   "read_only": 0, 
-   "remember_last_selected_value": 0, 
-   "report_hide": 0, 
-   "reqd": 0, 
-   "search_index": 0, 
-   "set_only_once": 0, 
-   "translatable": 0, 
-   "unique": 0
-  }, 
+   "fieldname": "to_date",
+   "fieldtype": "Date",
+   "label": "To Date"
+  },
   {
-   "allow_bulk_edit": 0, 
-   "allow_in_quick_entry": 0, 
-   "allow_on_submit": 0, 
-   "bold": 0, 
-   "collapsible": 0, 
-   "columns": 0, 
-   "fieldname": "amended_from", 
-   "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": "Amended From", 
-   "length": 0, 
-   "no_copy": 1, 
-   "options": "Shift Request", 
-   "permlevel": 0, 
-   "print_hide": 1, 
-   "print_hide_if_no_value": 0, 
-   "read_only": 1, 
-   "remember_last_selected_value": 0, 
-   "report_hide": 0, 
-   "reqd": 0, 
-   "search_index": 0, 
-   "set_only_once": 0, 
-   "translatable": 0, 
-   "unique": 0
+   "fieldname": "amended_from",
+   "fieldtype": "Link",
+   "label": "Amended From",
+   "no_copy": 1,
+   "options": "Shift Request",
+   "print_hide": 1,
+   "read_only": 1
+  },
+  {
+   "default": "Draft",
+   "fieldname": "status",
+   "fieldtype": "Select",
+   "label": "Status",
+   "options": "Draft\nApproved\nRejected",
+   "reqd": 1
+  },
+  {
+   "fetch_from": "employee.shift_request_approver",
+   "fetch_if_empty": 1,
+   "fieldname": "approver",
+   "fieldtype": "Link",
+   "label": "Approver",
+   "options": "User",
+   "reqd": 1
   }
- ], 
- "has_web_view": 0, 
- "hide_heading": 0, 
- "hide_toolbar": 0, 
- "idx": 0, 
- "image_view": 0, 
- "in_create": 0, 
- "is_submittable": 1, 
- "issingle": 0, 
- "istable": 0, 
- "max_attachments": 0, 
- "modified": "2018-08-21 16:15:36.577448", 
- "modified_by": "Administrator", 
- "module": "HR", 
- "name": "Shift Request", 
- "name_case": "", 
- "owner": "Administrator", 
+ ],
+ "is_submittable": 1,
+ "links": [],
+ "modified": "2020-08-10 17:59:31.550558",
+ "modified_by": "Administrator",
+ "module": "HR",
+ "name": "Shift Request",
+ "owner": "Administrator",
  "permissions": [
   {
-   "amend": 0, 
-   "cancel": 0, 
-   "create": 1, 
-   "delete": 0, 
-   "email": 1, 
-   "export": 1, 
-   "if_owner": 0, 
-   "import": 0, 
-   "permlevel": 0, 
-   "print": 1, 
-   "read": 1, 
-   "report": 1, 
-   "role": "Employee", 
-   "set_user_permissions": 0, 
-   "share": 1, 
-   "submit": 1, 
+   "create": 1,
+   "email": 1,
+   "export": 1,
+   "print": 1,
+   "read": 1,
+   "report": 1,
+   "role": "Employee",
+   "share": 1,
    "write": 1
-  }, 
+  },
   {
-   "amend": 1, 
-   "cancel": 1, 
-   "create": 1, 
-   "delete": 1, 
-   "email": 1, 
-   "export": 1, 
-   "if_owner": 0, 
-   "import": 0, 
-   "permlevel": 0, 
-   "print": 1, 
-   "read": 1, 
-   "report": 1, 
-   "role": "HR Manager", 
-   "set_user_permissions": 0, 
-   "share": 1, 
-   "submit": 1, 
+   "amend": 1,
+   "cancel": 1,
+   "create": 1,
+   "delete": 1,
+   "email": 1,
+   "export": 1,
+   "print": 1,
+   "read": 1,
+   "report": 1,
+   "role": "HR Manager",
+   "share": 1,
+   "submit": 1,
    "write": 1
-  }, 
+  },
   {
-   "amend": 0, 
-   "cancel": 0, 
-   "create": 1, 
-   "delete": 0, 
-   "email": 1, 
-   "export": 1, 
-   "if_owner": 0, 
-   "import": 0, 
-   "permlevel": 0, 
-   "print": 1, 
-   "read": 1, 
-   "report": 1, 
-   "role": "HR User", 
-   "set_user_permissions": 0, 
-   "share": 1, 
-   "submit": 1, 
+   "create": 1,
+   "email": 1,
+   "export": 1,
+   "print": 1,
+   "read": 1,
+   "report": 1,
+   "role": "HR User",
+   "share": 1,
+   "submit": 1,
    "write": 1
   }
- ], 
- "quick_entry": 0, 
- "read_only": 0, 
- "read_only_onload": 0, 
- "show_name_in_global_search": 0, 
- "sort_field": "modified", 
- "sort_order": "DESC", 
- "title_field": "employee_name", 
- "track_changes": 1, 
- "track_seen": 0, 
- "track_views": 0
+ ],
+ "sort_field": "modified",
+ "sort_order": "DESC",
+ "title_field": "employee_name",
+ "track_changes": 1
 }
\ No newline at end of file
diff --git a/erpnext/hr/doctype/shift_request/shift_request.py b/erpnext/hr/doctype/shift_request/shift_request.py
index ff5de08..1c2801b 100644
--- a/erpnext/hr/doctype/shift_request/shift_request.py
+++ b/erpnext/hr/doctype/shift_request/shift_request.py
@@ -14,19 +14,26 @@
 	def validate(self):
 		self.validate_dates()
 		self.validate_shift_request_overlap_dates()
+		self.validate_approver()
+		self.validate_default_shift()
 
 	def on_submit(self):
-		date_list = self.get_working_days(self.from_date, self.to_date)
-		for date in date_list:
+		if self.status not in ["Approved", "Rejected"]:
+			frappe.throw(_("Only Shift Request with status 'Approved' and 'Rejected' can be submitted"))
+		if self.status == "Approved":
 			assignment_doc = frappe.new_doc("Shift Assignment")
 			assignment_doc.company = self.company
 			assignment_doc.shift_type = self.shift_type
 			assignment_doc.employee = self.employee
-			assignment_doc.date = date
+			assignment_doc.start_date = self.from_date
+			if self.to_date:
+				assignment_doc.end_date = self.to_date
 			assignment_doc.shift_request = self.name
 			assignment_doc.insert()
 			assignment_doc.submit()
 
+			frappe.msgprint(_("Shift Assignment: {0} created for Employee: {1}").format(frappe.bold(assignment_doc.name), frappe.bold(self.employee)))
+
 	def on_cancel(self):
 		shift_assignment_list = frappe.get_list("Shift Assignment", {'employee': self.employee, 'shift_request': self.name})
 		if shift_assignment_list:
@@ -34,6 +41,19 @@
 				shift_assignment_doc = frappe.get_doc("Shift Assignment", shift['name'])
 				shift_assignment_doc.cancel()
 
+	def validate_default_shift(self):
+		default_shift = frappe.get_value("Employee", self.employee, "default_shift")
+		if self.shift_type == default_shift:
+			frappe.throw(_("You can not request for your Default Shift: {0}").format(frappe.bold(self.shift_type)))
+
+	def validate_approver(self):
+		department = frappe.get_value("Employee", self.employee, "department")
+		shift_approver = frappe.get_value("Employee", self.employee, "shift_request_approver")
+		approvers = frappe.db.sql("""select approver from `tabDepartment Approver` where parent= %s and parentfield = 'shift_request_approver'""", (department))
+		approvers = [approver[0] for approver in approvers]
+		approvers.append(shift_approver)
+		if self.approver not in approvers:
+			frappe.throw(_("Only Approvers can Approve this Request."))
 
 	def validate_dates(self):
 		if self.from_date and self.to_date and (getdate(self.to_date) < getdate(self.from_date)):
@@ -68,28 +88,4 @@
 		msg = _("Employee {0} has already applied for {1} between {2} and {3} : ").format(self.employee,
 			d['shift_type'], formatdate(d['from_date']), formatdate(d['to_date'])) \
 			+ """ <b><a href="#Form/Shift Request/{0}">{0}</a></b>""".format(d["name"])
-		frappe.throw(msg, OverlapError)
-
-	def get_working_days(self, start_date, end_date):
-		start_date, end_date = getdate(start_date), getdate(end_date)
-
-		from datetime import timedelta
-
-		date_list = []
-		employee_holiday_list = []
-
-		employee_holidays = frappe.db.sql("""select holiday_date from `tabHoliday`
-								where parent in (select holiday_list from `tabEmployee`
-								where name = %s)""",self.employee,as_dict=1)
-
-		for d in employee_holidays:
-			employee_holiday_list.append(d.holiday_date)
-
-		reference_date = start_date
-		
-		while reference_date <= end_date:
-			if reference_date not in employee_holiday_list:
-				date_list.append(reference_date)
-			reference_date += timedelta(days=1)
-
-		return date_list
\ No newline at end of file
+		frappe.throw(msg, OverlapError)
\ No newline at end of file
diff --git a/erpnext/hr/doctype/shift_request/test_shift_request.py b/erpnext/hr/doctype/shift_request/test_shift_request.py
index 1d0cf71..3dcfcbf 100644
--- a/erpnext/hr/doctype/shift_request/test_shift_request.py
+++ b/erpnext/hr/doctype/shift_request/test_shift_request.py
@@ -5,7 +5,7 @@
 
 import frappe
 import unittest
-from frappe.utils import nowdate
+from frappe.utils import nowdate, add_days
 
 class TestShiftRequest(unittest.TestCase):
 	def setUp(self):
@@ -13,14 +13,20 @@
 			frappe.db.sql("delete from `tab{doctype}`".format(doctype=doctype))
 
 	def test_make_shift_request(self):
+		department = frappe.get_value("Employee", "_T-Employee-00001", 'department')
+		set_shift_approver(department)
+		approver = frappe.db.sql("""select approver from `tabDepartment Approver` where parent= %s and parentfield = 'shift_request_approver'""", (department))[0][0]
+
 		shift_request = frappe.get_doc({
 			"doctype": "Shift Request",
 			"shift_type": "Day Shift",
 			"company": "_Test Company",
 			"employee": "_T-Employee-00001",
 			"employee_name": "_Test Employee",
-			"start_date": nowdate(),
-			"end_date": nowdate()
+			"from_date": nowdate(),
+			"to_date": add_days(nowdate(), 10),
+			"approver": approver,
+			"status": "Approved"
 		})
 		shift_request.insert()
 		shift_request.submit()
@@ -34,4 +40,10 @@
 			self.assertEqual(shift_request.employee, employee)
 			shift_request.cancel()
 			shift_assignment_doc = frappe.get_doc("Shift Assignment", {"shift_request": d.get('shift_request')})
-			self.assertEqual(shift_assignment_doc.docstatus, 2)
\ No newline at end of file
+			self.assertEqual(shift_assignment_doc.docstatus, 2)
+
+def set_shift_approver(department):
+	department_doc = frappe.get_doc("Department", department)
+	department_doc.append('shift_request_approver',{'approver': "test1@example.com"})
+	department_doc.save()
+	department_doc.reload()
\ No newline at end of file
diff --git a/erpnext/hr/doctype/shift_type/shift_type.js b/erpnext/hr/doctype/shift_type/shift_type.js
index e633545..ba53312 100644
--- a/erpnext/hr/doctype/shift_type/shift_type.js
+++ b/erpnext/hr/doctype/shift_type/shift_type.js
@@ -4,7 +4,7 @@
 frappe.ui.form.on('Shift Type', {
 	refresh: function(frm) {
 		frm.add_custom_button(
-			'Mark Auto Attendance',
+			'Mark Attendance',
 			() => frm.call({
 				doc: frm.doc,
 				method: 'process_auto_attendance',
diff --git a/erpnext/hr/doctype/shift_type/shift_type.py b/erpnext/hr/doctype/shift_type/shift_type.py
index 1973564..054e7e3 100644
--- a/erpnext/hr/doctype/shift_type/shift_type.py
+++ b/erpnext/hr/doctype/shift_type/shift_type.py
@@ -79,9 +79,10 @@
 				mark_attendance(employee, date, 'Absent', self.name)
 
 	def get_assigned_employee(self, from_date=None, consider_default_shift=False):
-		filters = {'date':('>=', from_date), 'shift_type': self.name, 'docstatus': '1'}
+		filters = {'start_date':('>', from_date), 'shift_type': self.name, 'docstatus': '1'}
 		if not from_date:
-			del filters['date']
+			del filters["start_date"]
+
 		assigned_employees = frappe.get_all('Shift Assignment', 'employee', filters, as_list=True)
 		assigned_employees = [x[0] for x in assigned_employees]
 
diff --git a/erpnext/patches.txt b/erpnext/patches.txt
index 361fe83..e17e949 100644
--- a/erpnext/patches.txt
+++ b/erpnext/patches.txt
@@ -721,3 +721,4 @@
 erpnext.patches.v13_0.stock_entry_enhancements
 erpnext.patches.v12_0.update_state_code_for_daman_and_diu
 erpnext.patches.v12_0.rename_lost_reason_detail
+erpnext.patches.v13_0.update_start_end_date_for_old_shift_assignment
diff --git a/erpnext/patches/v13_0/update_start_end_date_for_old_shift_assignment.py b/erpnext/patches/v13_0/update_start_end_date_for_old_shift_assignment.py
new file mode 100644
index 0000000..7c07b98
--- /dev/null
+++ b/erpnext/patches/v13_0/update_start_end_date_for_old_shift_assignment.py
@@ -0,0 +1,10 @@
+# Copyright (c) 2019, Frappe and Contributors
+# License: GNU General Public License v3. See license.txt
+
+from __future__ import unicode_literals
+
+import frappe
+
+def execute():
+    frappe.reload_doc('hr', 'doctype', 'shift_assignment')
+    frappe.db.sql("update `tabShift Assignment` set end_date=date, start_date=date where date IS NOT NULL and start_date IS NULL and end_date IS NULL;")