From b57521a33719258ee3d9b41bc99ec50e4982ccaa Mon Sep 17 00:00:00 2001
From: barredterra <14891507+barredterra@users.noreply.github.com>
Date: Mon, 2 Aug 2021 18:37:45 +0200
Subject: [PATCH 1/4] feat: add `total_billing_hours` to Sales Invoice

---
 .../doctype/sales_invoice/sales_invoice.js    | 50 +++++++++----------
 .../doctype/sales_invoice/sales_invoice.json  | 10 +++-
 .../doctype/sales_invoice/sales_invoice.py    |  9 ++--
 3 files changed, 36 insertions(+), 33 deletions(-)

diff --git a/erpnext/accounts/doctype/sales_invoice/sales_invoice.js b/erpnext/accounts/doctype/sales_invoice/sales_invoice.js
index f813425e6b5..ca516439647 100644
--- a/erpnext/accounts/doctype/sales_invoice/sales_invoice.js
+++ b/erpnext/accounts/doctype/sales_invoice/sales_invoice.js
@@ -829,7 +829,7 @@ frappe.ui.form.on('Sales Invoice', {
 			'timesheet_detail': row.name
 		});
 		frm.refresh_field('timesheets');
-		calculate_total_billing_amount(frm);
+		frm.trigger("calculate_timesheet_totals");
 	},
 
 	refresh: function(frm) {
@@ -937,50 +937,46 @@ frappe.ui.form.on('Sales Invoice', {
 			frm: frm
 		});
 	},
+
 	create_dunning: function(frm) {
 		frappe.model.open_mapped_doc({
 			method: "erpnext.accounts.doctype.sales_invoice.sales_invoice.create_dunning",
 			frm: frm
 		});
-	}
-})
+	},
 
-frappe.ui.form.on('Sales Invoice Timesheet', {
+	calculate_timesheet_totals: function(frm) {
+		frm.set_value("total_billing_amount",
+			frm.doc.timesheets.reduce((a, b) => a + (b["billing_amount"] || 0.0), 0.0));
+		frm.set_value("total_billing_hours",
+			frm.doc.timesheets.reduce((a, b) => a + (b["billing_hours"] || 0.0), 0.0));
+	}
+});
+
+
+frappe.ui.form.on("Sales Invoice Timesheet", {
 	time_sheet: function(frm, cdt, cdn){
 		var d = locals[cdt][cdn];
 		if(d.time_sheet) {
 			frappe.call({
 				method: "erpnext.projects.doctype.timesheet.timesheet.get_timesheet_data",
 				args: {
-					'name': d.time_sheet,
-					'project': frm.doc.project || null
+					"name": d.time_sheet,
+					"project": frm.doc.project || null
 				},
-				callback: function(r, rt) {
-					if(r.message){
-						let data = r.message;
-						frappe.model.set_value(cdt, cdn, "billing_hours", data.billing_hours);
-						frappe.model.set_value(cdt, cdn, "billing_amount", data.billing_amount);
-						frappe.model.set_value(cdt, cdn, "timesheet_detail", data.timesheet_detail);
-						calculate_total_billing_amount(frm)
+				callback: function(r) {
+					if(r.message) {
+						frappe.model.set_value(cdt, cdn, "billing_hours", r.message.billing_hours);
+						frappe.model.set_value(cdt, cdn, "billing_amount", r.message.billing_amount);
+						frappe.model.set_value(cdt, cdn, "timesheet_detail", r.message.timesheet_detail);
+						frm.trigger("calculate_timesheet_totals");
 					}
 				}
-			})
+			});
 		}
 	}
-})
+});
 
-var calculate_total_billing_amount =  function(frm) {
-	var doc = frm.doc;
-
-	doc.total_billing_amount = 0.0
-	if(doc.timesheets) {
-		$.each(doc.timesheets, function(index, data){
-			doc.total_billing_amount += data.billing_amount
-		})
-	}
-
-	refresh_field('total_billing_amount')
-}
 
 var select_loyalty_program = function(frm, loyalty_programs) {
 	var dialog = new frappe.ui.Dialog({
diff --git a/erpnext/accounts/doctype/sales_invoice/sales_invoice.json b/erpnext/accounts/doctype/sales_invoice/sales_invoice.json
index 0a9a105b7ca..6f16cd29223 100644
--- a/erpnext/accounts/doctype/sales_invoice/sales_invoice.json
+++ b/erpnext/accounts/doctype/sales_invoice/sales_invoice.json
@@ -74,6 +74,7 @@
   "time_sheet_list",
   "timesheets",
   "total_billing_amount",
+  "total_billing_hours",
   "section_break_30",
   "total_qty",
   "base_total",
@@ -1983,6 +1984,13 @@
    "fieldtype": "Small Text",
    "label": "Dispatch Address",
    "read_only": 1
+  },
+  {
+   "fieldname": "total_billing_hours",
+   "fieldtype": "Float",
+   "label": "Total Billing Hours",
+   "print_hide": 1,
+   "read_only": 1
   }
  ],
  "icon": "fa fa-file-text",
@@ -1995,7 +2003,7 @@
    "link_fieldname": "consolidated_invoice"
   }
  ],
- "modified": "2021-07-08 14:03:55.502522",
+ "modified": "2021-08-02 18:36:51.978581",
  "modified_by": "Administrator",
  "module": "Accounts",
  "name": "Sales Invoice",
diff --git a/erpnext/accounts/doctype/sales_invoice/sales_invoice.py b/erpnext/accounts/doctype/sales_invoice/sales_invoice.py
index 6d1f6249c13..c6beaabe19b 100644
--- a/erpnext/accounts/doctype/sales_invoice/sales_invoice.py
+++ b/erpnext/accounts/doctype/sales_invoice/sales_invoice.py
@@ -764,12 +764,11 @@ class SalesInvoice(SellingController):
 			self.calculate_billing_amount_for_timesheet()
 
 	def calculate_billing_amount_for_timesheet(self):
-		total_billing_amount = 0.0
-		for data in self.timesheets:
-			if data.billing_amount:
-				total_billing_amount += data.billing_amount
+		def timesheet_sum(field):
+			return sum((ts.get(field) or 0.0) for ts in self.timesheets)
 
-		self.total_billing_amount = total_billing_amount
+		self.total_billing_amount = timesheet_sum("billing_amount")
+		self.total_billing_hours = timesheet_sum("billing_hours")
 
 	def get_warehouse(self):
 		user_pos_profile = frappe.db.sql("""select name, warehouse from `tabPOS Profile`

From 1110f88e5a94cd3e3a61df25c6c47e6940757f82 Mon Sep 17 00:00:00 2001
From: barredterra <14891507+barredterra@users.noreply.github.com>
Date: Mon, 2 Aug 2021 23:06:37 +0200
Subject: [PATCH 2/4] feat: refactor and enhance sales invoice timesheet

---
 .../doctype/sales_invoice/sales_invoice.js    | 180 +++++++++---------
 .../doctype/sales_invoice/sales_invoice.py    |   2 +-
 .../sales_invoice_timesheet.json              |  42 +++-
 .../projects/doctype/timesheet/timesheet.py   |  38 +++-
 4 files changed, 160 insertions(+), 102 deletions(-)

diff --git a/erpnext/accounts/doctype/sales_invoice/sales_invoice.js b/erpnext/accounts/doctype/sales_invoice/sales_invoice.js
index ca516439647..ec9c3aec7b7 100644
--- a/erpnext/accounts/doctype/sales_invoice/sales_invoice.js
+++ b/erpnext/accounts/doctype/sales_invoice/sales_invoice.js
@@ -694,19 +694,6 @@ frappe.ui.form.on('Sales Invoice', {
 		}
 	},
 
-	project: function(frm){
-		if (!frm.doc.is_return) {
-			frm.call({
-				method: "add_timesheet_data",
-				doc: frm.doc,
-				callback: function(r, rt) {
-					refresh_field(['timesheets'])
-				}
-			})
-			frm.refresh();
-		}
-	},
-
 	onload: function(frm) {
 		frm.redemption_conversion_factor = null;
 	},
@@ -819,24 +806,91 @@ frappe.ui.form.on('Sales Invoice', {
 		}
 	},
 
-	add_timesheet_row: function(frm, row, exchange_rate) {
-		frm.add_child('timesheets', {
-			'activity_type': row.activity_type,
-			'description': row.description,
-			'time_sheet': row.parent,
-			'billing_hours': row.billing_hours,
-			'billing_amount': flt(row.billing_amount) * flt(exchange_rate),
-			'timesheet_detail': row.name
+	project: function(frm) {
+		if (frm.doc.project) {
+			frm.events.add_timesheet_data(frm, {
+				project: frm.doc.project
+			});
+		}
+	},
+
+	async add_timesheet_data(frm, kwargs) {
+		if (kwargs === "Sales Invoice") {
+			// called via frm.trigger()
+			kwargs = Object();
+		}
+
+		if (!kwargs.hasOwnProperty("project") && frm.doc.project) {
+			kwargs.project = frm.doc.project;
+		}
+
+		const timesheets = await frm.events.get_timesheet_data(frm, kwargs);
+		return frm.events.set_timesheet_data(frm, timesheets);
+	},
+
+	async get_timesheet_data(frm, kwargs) {
+		return frappe.call({
+			method: "erpnext.projects.doctype.timesheet.timesheet.get_projectwise_timesheet_data",
+			args: kwargs
+		}).then(r => {
+			if (!r.exc && r.message.length > 0) {
+				return r.message
+			} else {
+				return []
+			}
 		});
-		frm.refresh_field('timesheets');
+	},
+
+	set_timesheet_data: function(frm, timesheets) {
+		frm.clear_table("timesheets")
+		timesheets.forEach(timesheet => {
+			if (frm.doc.currency != timesheet.currency) {
+				frappe.call({
+					method: "erpnext.setup.utils.get_exchange_rate",
+					args: {
+						from_currency: timesheet.currency,
+						to_currency: frm.doc.currency
+					},
+					callback: function(r) {
+						if (r.message) {
+							exchange_rate = r.message;
+							frm.events.append_time_log(frm, timesheet, exchange_rate);
+						}
+					}
+				});
+			} else {
+				frm.events.append_time_log(frm, timesheet, 1.0);
+			}
+		});
+	},
+
+	append_time_log: function(frm, time_log, exchange_rate) {
+		const row = frm.add_child("timesheets");
+		row.activity_type = time_log.activity_type;
+		row.description = time_log.description;
+		row.time_sheet = time_log.time_sheet;
+		row.from_time = time_log.from_time;
+		row.to_time = time_log.to_time;
+		row.billing_hours = time_log.billing_hours;
+		row.billing_amount = flt(time_log.billing_amount) * flt(exchange_rate);
+		row.timesheet_detail = time_log.name;
+
+		frm.refresh_field("timesheets");
 		frm.trigger("calculate_timesheet_totals");
 	},
 
+	calculate_timesheet_totals: function(frm) {
+		frm.set_value("total_billing_amount",
+			frm.doc.timesheets.reduce((a, b) => a + (b["billing_amount"] || 0.0), 0.0));
+		frm.set_value("total_billing_hours",
+			frm.doc.timesheets.reduce((a, b) => a + (b["billing_hours"] || 0.0), 0.0));
+	},
+
 	refresh: function(frm) {
 		if (frm.doc.docstatus===0 && !frm.doc.is_return) {
-			frm.add_custom_button(__('Fetch Timesheet'), function() {
+			frm.add_custom_button(__("Fetch Timesheet"), function() {
 				let d = new frappe.ui.Dialog({
-					title: __('Fetch Timesheet'),
+					title: __("Fetch Timesheet"),
 					fields: [
 						{
 							"label" : __("From"),
@@ -845,8 +899,8 @@ frappe.ui.form.on('Sales Invoice', {
 							"reqd": 1,
 						},
 						{
-							fieldtype: 'Column Break',
-							fieldname: 'col_break_1',
+							fieldtype: "Column Break",
+							fieldname: "col_break_1",
 						},
 						{
 							"label" : __("To"),
@@ -863,48 +917,18 @@ frappe.ui.form.on('Sales Invoice', {
 						},
 					],
 					primary_action: function() {
-						let data = d.get_values();
-						frappe.call({
-							method: "erpnext.projects.doctype.timesheet.timesheet.get_projectwise_timesheet_data",
-							args: {
-								from_time: data.from_time,
-								to_time: data.to_time,
-								project: data.project
-							},
-							callback: function(r) {
-								if (!r.exc && r.message.length > 0) {
-									frm.clear_table('timesheets')
-									r.message.forEach((d) => {
-										let exchange_rate = 1.0;
-										if (frm.doc.currency != d.currency) {
-											frappe.call({
-												method: 'erpnext.setup.utils.get_exchange_rate',
-												args: {
-													from_currency: d.currency,
-													to_currency: frm.doc.currency
-												},
-												callback: function(r) {
-													if (r.message) {
-														exchange_rate = r.message;
-														frm.events.add_timesheet_row(frm, d, exchange_rate);
-													}
-												}
-											});
-										} else {
-											frm.events.add_timesheet_row(frm, d, exchange_rate);
-										}
-									});
-								} else {
-									frappe.msgprint(__('No Timesheets found with the selected filters.'))
-								}
-								d.hide();
-							}
+						const data = d.get_values();
+						frm.events.add_timesheet_data(frm, {
+							from_time: data.from_time,
+							to_time: data.to_time,
+							project: data.project
 						});
+						d.hide();
 					},
-					primary_action_label: __('Get Timesheets')
+					primary_action_label: __("Get Timesheets")
 				});
 				d.show();
-			})
+			});
 		}
 
 		if (frm.doc.is_debit_note) {
@@ -943,37 +967,13 @@ frappe.ui.form.on('Sales Invoice', {
 			method: "erpnext.accounts.doctype.sales_invoice.sales_invoice.create_dunning",
 			frm: frm
 		});
-	},
-
-	calculate_timesheet_totals: function(frm) {
-		frm.set_value("total_billing_amount",
-			frm.doc.timesheets.reduce((a, b) => a + (b["billing_amount"] || 0.0), 0.0));
-		frm.set_value("total_billing_hours",
-			frm.doc.timesheets.reduce((a, b) => a + (b["billing_hours"] || 0.0), 0.0));
 	}
 });
 
 
 frappe.ui.form.on("Sales Invoice Timesheet", {
-	time_sheet: function(frm, cdt, cdn){
-		var d = locals[cdt][cdn];
-		if(d.time_sheet) {
-			frappe.call({
-				method: "erpnext.projects.doctype.timesheet.timesheet.get_timesheet_data",
-				args: {
-					"name": d.time_sheet,
-					"project": frm.doc.project || null
-				},
-				callback: function(r) {
-					if(r.message) {
-						frappe.model.set_value(cdt, cdn, "billing_hours", r.message.billing_hours);
-						frappe.model.set_value(cdt, cdn, "billing_amount", r.message.billing_amount);
-						frappe.model.set_value(cdt, cdn, "timesheet_detail", r.message.timesheet_detail);
-						frm.trigger("calculate_timesheet_totals");
-					}
-				}
-			});
-		}
+	timesheets_remove(frm, cdt, cdn) {
+		frm.trigger("calculate_timesheet_totals");
 	}
 });
 
diff --git a/erpnext/accounts/doctype/sales_invoice/sales_invoice.py b/erpnext/accounts/doctype/sales_invoice/sales_invoice.py
index c6beaabe19b..673dcf42d27 100644
--- a/erpnext/accounts/doctype/sales_invoice/sales_invoice.py
+++ b/erpnext/accounts/doctype/sales_invoice/sales_invoice.py
@@ -753,7 +753,7 @@ class SalesInvoice(SellingController):
 		if self.project:
 			for data in get_projectwise_timesheet_data(self.project):
 				self.append('timesheets', {
-						'time_sheet': data.parent,
+						'time_sheet': data.time_sheet,
 						'billing_hours': data.billing_hours,
 						'billing_amount': data.billing_amount,
 						'timesheet_detail': data.name,
diff --git a/erpnext/accounts/doctype/sales_invoice_timesheet/sales_invoice_timesheet.json b/erpnext/accounts/doctype/sales_invoice_timesheet/sales_invoice_timesheet.json
index f069e8dd0b8..a8364d3d50b 100644
--- a/erpnext/accounts/doctype/sales_invoice_timesheet/sales_invoice_timesheet.json
+++ b/erpnext/accounts/doctype/sales_invoice_timesheet/sales_invoice_timesheet.json
@@ -7,8 +7,15 @@
  "field_order": [
   "activity_type",
   "description",
+  "section_break_3",
+  "from_time",
+  "column_break_5",
+  "to_time",
+  "section_break_7",
   "billing_hours",
+  "column_break_9",
   "billing_amount",
+  "section_break_11",
   "time_sheet",
   "timesheet_detail"
  ],
@@ -61,11 +68,44 @@
    "in_list_view": 1,
    "label": "Description",
    "read_only": 1
+  },
+  {
+   "fieldname": "from_time",
+   "fieldtype": "Datetime",
+   "label": "From Time"
+  },
+  {
+   "fieldname": "to_time",
+   "fieldtype": "Datetime",
+   "label": "To Time"
+  },
+  {
+   "fieldname": "section_break_3",
+   "fieldtype": "Section Break",
+   "label": "Time"
+  },
+  {
+   "fieldname": "column_break_5",
+   "fieldtype": "Column Break"
+  },
+  {
+   "fieldname": "section_break_7",
+   "fieldtype": "Section Break",
+   "label": "Totals"
+  },
+  {
+   "fieldname": "column_break_9",
+   "fieldtype": "Column Break"
+  },
+  {
+   "fieldname": "section_break_11",
+   "fieldtype": "Section Break",
+   "label": "Reference"
   }
  ],
  "istable": 1,
  "links": [],
- "modified": "2021-05-20 22:33:57.234846",
+ "modified": "2021-08-02 23:03:08.084930",
  "modified_by": "Administrator",
  "module": "Accounts",
  "name": "Sales Invoice Timesheet",
diff --git a/erpnext/projects/doctype/timesheet/timesheet.py b/erpnext/projects/doctype/timesheet/timesheet.py
index ae38d4ca192..1e11f73cfdc 100644
--- a/erpnext/projects/doctype/timesheet/timesheet.py
+++ b/erpnext/projects/doctype/timesheet/timesheet.py
@@ -224,16 +224,34 @@ def get_projectwise_timesheet_data(project=None, parent=None, from_time=None, to
 	if from_time and to_time:
 		condition += "AND CAST(tsd.from_time as DATE) BETWEEN %(from_time)s AND %(to_time)s"
 
-	return frappe.db.sql("""SELECT tsd.name as name,
-				tsd.parent as parent, tsd.billing_hours as billing_hours,
-				tsd.billing_amount as billing_amount, tsd.activity_type as activity_type,
-				tsd.description as description, ts.currency as currency
-			FROM `tabTimesheet Detail` tsd
-			INNER JOIN `tabTimesheet` ts ON ts.name = tsd.parent
-			WHERE tsd.parenttype = 'Timesheet'
-				and tsd.docstatus=1 {0}
-				and tsd.is_billable = 1
-				and tsd.sales_invoice is null""".format(condition), {'project': project, 'parent': parent, 'from_time': from_time, 'to_time': to_time}, as_dict=1)
+	return frappe.db.sql("""
+		SELECT
+
+			tsd.name as name,
+			tsd.parent as time_sheet,
+			tsd.from_time as from_time,
+			tsd.to_time as to_time,
+			tsd.billing_hours as billing_hours,
+			tsd.billing_amount as billing_amount,
+			tsd.activity_type as activity_type,
+			tsd.description as description,
+			ts.currency as currency
+
+		FROM `tabTimesheet Detail` tsd
+
+			INNER JOIN `tabTimesheet` ts
+			ON ts.name = tsd.parent
+
+		WHERE tsd.parenttype = 'Timesheet'
+			AND tsd.docstatus=1 {0}
+			AND tsd.is_billable = 1
+			AND tsd.sales_invoice is null
+		""".format(condition), {
+			'project': project,
+			'parent': parent,
+			'from_time': from_time,
+			'to_time': to_time
+		}, as_dict=1)
 
 @frappe.whitelist()
 @frappe.validate_and_sanitize_search_inputs

From c82611aa6260e303995d9ed09b59799ef6b44c03 Mon Sep 17 00:00:00 2001
From: barredterra <14891507+barredterra@users.noreply.github.com>
Date: Mon, 2 Aug 2021 23:19:57 +0200
Subject: [PATCH 3/4] feat: sort timesheets by start time

---
 .../projects/doctype/timesheet/timesheet.py   | 35 ++++++++++++-------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/erpnext/projects/doctype/timesheet/timesheet.py b/erpnext/projects/doctype/timesheet/timesheet.py
index 1e11f73cfdc..f82072e6318 100644
--- a/erpnext/projects/doctype/timesheet/timesheet.py
+++ b/erpnext/projects/doctype/timesheet/timesheet.py
@@ -216,15 +216,15 @@ class Timesheet(Document):
 
 @frappe.whitelist()
 def get_projectwise_timesheet_data(project=None, parent=None, from_time=None, to_time=None):
-	condition = ''
+	condition = ""
 	if project:
-		condition += "and tsd.project = %(project)s"
+		condition += "AND tsd.project = %(project)s "
 	if parent:
-		condition += "AND tsd.parent = %(parent)s"
+		condition += "AND tsd.parent = %(parent)s "
 	if from_time and to_time:
 		condition += "AND CAST(tsd.from_time as DATE) BETWEEN %(from_time)s AND %(to_time)s"
 
-	return frappe.db.sql("""
+	query = f"""
 		SELECT
 
 			tsd.name as name,
@@ -242,16 +242,25 @@ def get_projectwise_timesheet_data(project=None, parent=None, from_time=None, to
 			INNER JOIN `tabTimesheet` ts
 			ON ts.name = tsd.parent
 
-		WHERE tsd.parenttype = 'Timesheet'
-			AND tsd.docstatus=1 {0}
+		WHERE
+
+			tsd.parenttype = 'Timesheet'
+			AND tsd.docstatus = 1
 			AND tsd.is_billable = 1
-			AND tsd.sales_invoice is null
-		""".format(condition), {
-			'project': project,
-			'parent': parent,
-			'from_time': from_time,
-			'to_time': to_time
-		}, as_dict=1)
+			AND tsd.sales_invoice is NULL
+			{condition}
+
+		ORDER BY tsd.from_time ASC
+	"""
+
+	filters = {
+		"project": project,
+		"parent": parent,
+		"from_time": from_time,
+		"to_time": to_time
+	}
+
+	return frappe.db.sql(query, filters, as_dict=1)
 
 @frappe.whitelist()
 @frappe.validate_and_sanitize_search_inputs

From 0f2f11cb33fa3911b85d22676e2b2c0c87fdbdf3 Mon Sep 17 00:00:00 2001
From: Raffael Meyer <14891507+barredterra@users.noreply.github.com>
Date: Sun, 15 Aug 2021 18:47:51 +0200
Subject: [PATCH 4/4] fix: typo

---
 erpnext/accounts/doctype/sales_invoice/sales_invoice.json | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/erpnext/accounts/doctype/sales_invoice/sales_invoice.json b/erpnext/accounts/doctype/sales_invoice/sales_invoice.json
index f500e80545b..1b0f96076c0 100644
--- a/erpnext/accounts/doctype/sales_invoice/sales_invoice.json
+++ b/erpnext/accounts/doctype/sales_invoice/sales_invoice.json
@@ -2385,7 +2385,7 @@
    "link_fieldname": "consolidated_invoice"
   }
  ],
- "modified": "2021-08-15 18:40:20.445127"
+ "modified": "2021-08-15 18:40:20.445127",
  "modified_by": "Administrator",
  "module": "Accounts",
  "name": "Sales Invoice",
@@ -2439,4 +2439,4 @@
  "title_field": "title",
  "track_changes": 1,
  "track_seen": 1
-}
\ No newline at end of file
+}