Repository: frappe/frappe
Pull request: feat: Allow all users to customize reports of type Report Builder
Resolves #15074
Continuation of #15494

This PR allows a report of type Report Builder to be created / edited / deleted by any user. This is accomplished without changing permissions for the Report DocType, since this PR only allows non-standard reports of type Report Builder to be saved. They can then be edited / deleted by owner or someone else with requisite permissions.

Delete button in Report View:

![image](https://user-images.githubusercontent.com/16315650/153154852-3a444e39-7fbd-4ad3-b412-c340f56c5f31.png)

Docs to be added after preliminary review. Until then: `no-docs`

Problem statement:
Allow all users to save customized reports
### User story
If a user can see a report (type: Report Builder), he can add or remove columns and filters to customize his view. Consequently he should also be able to save this current view for future use.

### Situation
At the moment, only users with role "Report Manager" can save customized reports. A report manager can create, edit, delete all custom reports. This is way too permissive for a normal user.

### Proposed solution
A user can always customize the reports he can see, for himself. So everybody should have the permission to create, edit, delete his _own reports_ as long as this only means narrowing down an existing report.

Hints from issue discussion:
https://github.com/frappe/frappe/pull/15494 is a good start.

Further recommendations:
- Save button should be visible only if User is Report's owner or Report Manager.
- A button to delete a report will be visible in Report View based on following conditions:
   - Report is of type Report Builder
   - User is owner / report manager
   - It is not a standard report

I think that the restriction to narrow down the existing report is not really necessary as permissions are already being checked when rendering the report using `frappe.get_list`.

Commit message: style: add missing semicolon

Patch to review:
diff --git a/frappe/core/doctype/report/test_report.py b/frappe/core/doctype/report/test_report.py
index 36e3b09254ac..5a22304f321a 100644
--- a/frappe/core/doctype/report/test_report.py
+++ b/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 @@ def test_query_report(self):
 		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(
diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py
index c45fc9bfdd03..880afae489c6 100644
--- a/frappe/desk/reportview.py
+++ b/frappe/desk/reportview.py
@@ -262,22 +262,62 @@ def compress(data, args=None):
 	}
 
 @frappe.whitelist()
-def save_report():
-	"""save report"""
-
-	data = frappe.local.form_dict
-	if frappe.db.exists('Report', data['name']):
-		d = frappe.get_doc('Report', data['name'])
+def save_report(name, doctype, report_settings):
+	"""Save reports of type Report Builder from Report View"""
+
+	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']
-
-	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 = frappe.new_doc('Report')
+		report.report_name = name
+		report.ref_doctype = doctype
+
+	report.report_type = "Report Builder"
+	report.json = report_settings
+	report.save(ignore_permissions=True)
+	frappe.msgprint(_("{0} saved").format(frappe.bold(report.name)), 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(
+		_("{0} deleted").format(frappe.bold(report.name)),
+		indicator="red",
+		alert=True,
+	)
 
 @frappe.whitelist()
 @frappe.read_only()
diff --git a/frappe/public/js/frappe/views/reports/report_view.js b/frappe/public/js/frappe/views/reports/report_view.js
index 23e415ed3e6a..1291e6354311 100644
--- a/frappe/public/js/frappe/views/reports/report_view.js
+++ b/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


Code review comments:
diff_hunk: @@ -262,22 +262,62 @@ def compress(data, args=None):
 	}
 
 @frappe.whitelist()
-def save_report():
-	"""save report"""
-
-	data = frappe.local.form_dict
-	if frappe.db.exists('Report', data['name']):
-		d = frappe.get_doc('Report', data['name'])
+def save_report(name, doctype, report_settings):
+	"""Save reports of type Report Builder from Report View"""
+
+	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']
-
-	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 = frappe.new_doc('Report')
+		report.report_name = name
+		report.ref_doctype = doctype
+
+	report.report_type = "Report Builder"
+	report.json = report_settings
+	report.save(ignore_permissions=True)
+	frappe.msgprint(_("{0} saved").format(frappe.bold(report.name)), 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(
+		_("{0} deleted").format(frappe.bold(report.name)),
+		indicator="red",
path: frappe/desk/reportview.py
text: @user1:
Should be green instead? Because the intended action has been successfully executed. Red usually means "something went wrong"

@author:
Changed:

![Screenshot-2022-02-09-154434](https://user-images.githubusercontent.com/16315650/153177688-64a709fc-37d1-48ce-a609-35c324a654d1.png)

![Screenshot-2022-02-09-154456](https://user-images.githubusercontent.com/16315650/153177686-87153c6e-7871-4e0d-af96-dfe9ec0eb8a2.png)

Have added Report at beginning of message as well, for more context.