Merge pull request #15918 from resilient-tech/report_fix

frappe/core/doctype/report/test_report.py

@@ -4,7 +4,9 @@
 import frappe, json, os
 import unittest
 from frappe.desk.query_report import run, save_report
+from frappe.desk.reportview import delete_report, save_report as _save_report
 from frappe.custom.doctype.customize_form.customize_form import reset_customization
+from frappe.core.doctype.user_permission.test_user_permission import create_user
 
 test_records = frappe.get_test_records('Report')
 test_dependencies = ['User']
@@ -30,6 +32,60 @@ class TestReport(unittest.TestCase):
 		self.assertEqual(columns[1].get('label'), 'Module')
 		self.assertTrue('User' in [d.get('name') for d in data])
 
+	def test_save_or_delete_report(self):
+		'''Test for validations when editing / deleting report of type Report Builder'''
+
+		try:
+			report = frappe.get_doc({
+				'doctype': 'Report',
+				'ref_doctype': 'User',
+				'report_name': 'Test Delete Report',
+				'report_type': 'Report Builder',
+				'is_standard': 'No',
+			}).insert()
+
+			# Check for PermissionError
+			create_user("test_report_owner@example.com", "Website Manager")
+			frappe.set_user("test_report_owner@example.com")
+			self.assertRaises(frappe.PermissionError, delete_report, report.name)
+
+			# Check for Report Type
+			frappe.set_user("Administrator")
+			report.db_set("report_type", "Custom Report")
+			self.assertRaisesRegex(
+				frappe.ValidationError,
+				"Only reports of type Report Builder can be deleted",
+				delete_report,
+				report.name
+			)
+
+			# Check if creating and deleting works with proper validations
+			frappe.set_user("test@example.com")
+			report_name = _save_report(
+				'Dummy Report',
+				'User',
+				json.dumps([{
+					'fieldname': 'email',
+					'fieldtype': 'Data',
+					'label': 'Email',
+					'insert_after_index': 0,
+					'link_field': 'name',
+					'doctype': 'User',
+					'options': 'Email',
+					'width': 100,
+					'id':'email',
+					'name': 'Email'
+				}])
+			)
+
+			doc = frappe.get_doc("Report", report_name)
+			delete_report(doc.name)
+
+		finally:
+			frappe.set_user("Administrator")
+			frappe.db.rollback()
+
+
 	def test_custom_report(self):
 		reset_customization('User')
 		custom_report_name = save_report(

frappe/desk/reportview.py

@@ -262,22 +262,66 @@ def compress(data, args=None):
 	}
 
 @frappe.whitelist()
-def save_report():
-	"""save report"""
+def save_report(name, doctype, report_settings):
+	"""Save reports of type Report Builder from Report View"""
 
-	data = frappe.local.form_dict
-	if frappe.db.exists('Report', data['name']):
-		d = frappe.get_doc('Report', data['name'])
+	if frappe.db.exists('Report', name):
+		report = frappe.get_doc('Report', name)
+		if report.is_standard == "Yes":
+			frappe.throw(_("Standard Reports cannot be edited"))
+
+		if report.report_type != "Report Builder":
+			frappe.throw(_("Only reports of type Report Builder can be edited"))
+
+		if (
+			report.owner != frappe.session.user
+			and not frappe.has_permission("Report", "write")
+		):
+			frappe.throw(
+				_("Insufficient Permissions for editing Report"),
+				frappe.PermissionError
+			)
 	else:
-		d = frappe.new_doc('Report')
-		d.report_name = data['name']
-		d.ref_doctype = data['doctype']
+		report = frappe.new_doc('Report')
+		report.report_name = name
+		report.ref_doctype = doctype
 
-	d.report_type = "Report Builder"
-	d.json = data['json']
-	frappe.get_doc(d).save()
-	frappe.msgprint(_("{0} is saved").format(d.name), alert=True)
-	return d.name
+	report.report_type = "Report Builder"
+	report.json = report_settings
+	report.save(ignore_permissions=True)
+	frappe.msgprint(
+		_("Report {0} saved").format(frappe.bold(report.name)),
+		indicator="green",
+		alert=True,
+	)
+	return report.name
+
+@frappe.whitelist()
+def delete_report(name):
+	"""Delete reports of type Report Builder from Report View"""
+
+	report = frappe.get_doc("Report", name)
+	if report.is_standard == "Yes":
+		frappe.throw(_("Standard Reports cannot be deleted"))
+
+	if report.report_type != "Report Builder":
+		frappe.throw(_("Only reports of type Report Builder can be deleted"))
+
+	if (
+		report.owner != frappe.session.user
+		and not frappe.has_permission("Report", "delete")
+	):
+		frappe.throw(
+			_("Insufficient Permissions for deleting Report"),
+			frappe.PermissionError
+		)
+
+	report.delete(ignore_permissions=True)
+	frappe.msgprint(
+		_("Report {0} deleted").format(frappe.bold(report.name)),
+		indicator="green",
+		alert=True,
+	)
 
 @frappe.whitelist()
 @frappe.read_only()

frappe/public/js/frappe/views/reports/report_view.js

@@ -18,7 +18,6 @@ frappe.views.ReportView = class ReportView extends frappe.views.ListView {
 	setup_defaults() {
 		super.setup_defaults();
 		this.page_title = __('Report:') + ' ' + this.page_title;
-		this.menu_items = this.report_menu_items();
 		this.view = 'Report';
 
 		const route = frappe.get_route();
@@ -52,6 +51,11 @@ frappe.views.ReportView = class ReportView extends frappe.views.ListView {
 		this.page.main.addClass('report-view');
 	}
 
+	setup_page() {
+		this.menu_items = this.report_menu_items();
+		super.setup_page();
+	}
+
 	toggle_side_bar() {
 		super.toggle_side_bar();
 		// refresh datatable when sidebar is toggled to accomodate extra space
@@ -1207,7 +1211,7 @@ frappe.views.ReportView = class ReportView extends frappe.views.ListView {
 				args: {
 					name: name,
 					doctype: this.doctype,
-					json: JSON.stringify(report_settings)
+					report_settings: JSON.stringify(report_settings)
 				},
 				callback:(r) => {
 					if(r.exc) {
@@ -1244,6 +1248,17 @@ frappe.views.ReportView = class ReportView extends frappe.views.ListView {
 		}
 	}
 
+	delete_report() {
+		return frappe.call({
+			method: 'frappe.desk.reportview.delete_report',
+			args: { name: this.report_name },
+			callback(response) {
+				if (response.exc) return;
+				window.history.back();
+			}
+		});
+	}
+
 	get_column_widths() {
 		if (this.datatable) {
 			return this.datatable
@@ -1465,12 +1480,42 @@ frappe.views.ReportView = class ReportView extends frappe.views.ListView {
 			}
 		});
 
-		// save buttons
-		if(frappe.user.is_report_manager()) {
-			items = items.concat([
-				{ label: __('Save'), action: () => this.save_report('save') },
-				{ label: __('Save As'), action: () => this.save_report('save_as') }
-			]);
+		const can_edit_or_delete = (action) => {
+			const method = action == "delete" ? "can_delete" : "can_write";
+			return (
+				this.report_doc
+				&& this.report_doc.is_standard !== "Yes"
+				&& (
+					frappe.model[method]("Report")
+					|| this.report_doc.owner === frappe.session.user
+				)
+			);
+		};
+
+		// A user with role Report Manager or Report Owner can save
+		if (can_edit_or_delete()) {
+			items.push({
+				label: __("Save"),
+				action: () => this.save_report('save')
+			});
+		}
+
+		// anyone can save as
+		items.push({
+			label: __('Save As'),
+			action: () => this.save_report('save_as')
+		});
+
+		// A user with role Report Manager or Report Owner can delete
+		if (can_edit_or_delete("delete")) {
+			items.push({
+				label: __("Delete"),
+				action: () => frappe.confirm(
+					"Are you sure you want to delete this report?",
+					() => this.delete_report(),
+				),
+				shortcut: "Shift+Ctrl+D"
+			});
 		}
 
 		// user permissions