[fixes] removed frappe.throw from feedback trigger (#2838)

* [fixes] removed frappe.throw from feedback trigger

* Update feedback_trigger.py

frappe/core/doctype/feedback_trigger/feedback_trigger.py

@@ -24,14 +24,32 @@ class FeedbackTrigger(Document):
 			except:
 				frappe.throw(_("The condition '{0}' is invalid").format(self.condition))
 
+def trigger_feedback_request(doc, method):
+	""" trigger the feedback alert"""
+
+	if doc.flags.in_delete:
+		frappe.enqueue('frappe.core.doctype.feedback_trigger.feedback_trigger.delete_feedback_request_and_feedback',
+			reference_doctype=doc.doctype, reference_name=doc.name, now=frappe.flags.in_test)
+	else:
+		feedback_trigger = frappe.db.get_value("Feedback Trigger", { "enabled": 1, "document_type": doc.doctype })
+		if feedback_trigger:
+			frappe.enqueue('frappe.core.doctype.feedback_trigger.feedback_trigger.send_feedback_request',
+				trigger=feedback_trigger, reference_doctype=doc.doctype, reference_name=doc.name, now=frappe.flags.in_test)
+
 @frappe.whitelist()
 def send_feedback_request(reference_doctype, reference_name, trigger="Manual", details=None, is_manual=False):
 	""" send feedback alert """
 
-	is_feedback_request_already_sent(reference_doctype, reference_name, is_manual=is_manual)
+	if is_feedback_request_already_sent(reference_doctype, reference_name, is_manual=is_manual):
+		frappe.msgprint(_("Feedback Request is already sent to user"))
+		return None
+
 	details = json.loads(details) if details else \
 		get_feedback_request_details(reference_doctype, reference_name, trigger=trigger)
 
+	if not details:
+		return None
+
 	feedback_request, url = get_feedback_request_url(reference_doctype,
 		reference_name, details.get("recipients"), trigger)
 
@@ -47,17 +65,6 @@ def send_feedback_request(reference_doctype, reference_name, trigger="Manual", d
 		frappe.sendmail(**details)
 		frappe.db.set_value("Feedback Request", feedback_request, "is_sent", 1)
 
-def trigger_feedback_request(doc, method):
-	""" trigger the feedback alert"""
-
-	if doc.flags.in_delete:
-		frappe.enqueue('frappe.core.doctype.feedback_trigger.feedback_trigger.delete_feedback_request_and_feedback',
-			reference_doctype=doc.doctype, reference_name=doc.name, now=frappe.flags.in_test)
-	else:
-		feedback_trigger = frappe.db.get_value("Feedback Trigger", { "enabled": 1, "document_type": doc.doctype })
-		if feedback_trigger:
-			frappe.enqueue('frappe.core.doctype.feedback_trigger.feedback_trigger.send_feedback_request',
-				trigger=feedback_trigger, reference_doctype=doc.doctype, reference_name=doc.name, now=frappe.flags.in_test)
 
 @frappe.whitelist()
 def get_feedback_request_details(reference_doctype, reference_name, trigger="Manual", request=None):
@@ -88,7 +95,8 @@ def get_feedback_request_details(reference_doctype, reference_name, trigger="Man
 		}, fields=["name"])
 
 		if len(communications) < 1:
-			frappe.throw(_("No communication found for the document"))
+			frappe.msgprint(_("At least one reply is mandatory before requesting feedback"))
+			return None
 
 	if recipients and eval(feedback_trigger.condition, context):
 		subject = feedback_trigger.subject
@@ -107,7 +115,8 @@ def get_feedback_request_details(reference_doctype, reference_name, trigger="Man
 			"message": feedback_request_message,
 		}
 	else:
-		frappe.throw("Feedback conditions does not match !!")
+		frappe.msgprint("Feedback conditions do not match")
+		return None
 
 def get_feedback_request_url(reference_doctype, reference_name, recipients, trigger="Manual"):
 	""" prepare the feedback request url """
@@ -135,6 +144,7 @@ def is_feedback_request_already_sent(reference_doctype, reference_name, is_manua
 		check if feedback request mail is already sent but feedback is not submitted
 		to avoid sending multiple feedback request mail
 	"""
+	is_request_sent = False
 	filters = {
 		"is_sent": 1,
 		"reference_name": reference_name,
@@ -147,8 +157,8 @@ def is_feedback_request_already_sent(reference_doctype, reference_name, is_manua
 
 	feedback_request = frappe.get_all("Feedback Request", filters=filters, fields=["name"])
 
-	if feedback_request:
-		frappe.throw(_("Feedback request mail has been already sent to the recipient"))
+	if feedback_request: is_request_sent = True
+	return is_request_sent
 
 def get_enabled_feedback_trigger():
 	""" get mapper of all the enable feedback trigger """
@@ -183,4 +193,4 @@ def delete_feedback_request_and_feedback(reference_doctype, reference_name):
 		frappe.delete_doc("Feedback Request", request.get("name"), ignore_permissions=True)
 
 	for communication in communications:
-		frappe.delete_doc("Communication", communication.get("name"), ignore_permissions=True)
+		frappe.delete_doc("Communication", communication.get("name"), ignore_permissions=True)

frappe/core/doctype/feedback_trigger/test_feedback_trigger.py

@@ -54,11 +54,9 @@ class TestFeedbackTrigger(unittest.TestCase):
 			"owner": "test-feedback@example.com",
 			"assigned_by": "test-feedback@example.com",
 			"description": "Unable To Submit Sales Order #SO-00001"
-		})
+		}).insert(ignore_permissions=True)
 
 		# feedback alert mail should be sent only on 'Closed' status
-		self.assertRaises(frappe.ValidationError, todo.insert, ignore_permissions=True)
-
 		email_queue = frappe.db.sql("""select name from `tabEmail Queue` where
 			reference_doctype='ToDo' and reference_name='{0}'""".format(todo.name))
 		self.assertFalse(email_queue)
@@ -79,16 +77,19 @@ class TestFeedbackTrigger(unittest.TestCase):
 
 		email_queue = frappe.db.sql("""select name from `tabEmail Queue` where
 			reference_doctype='ToDo' and reference_name='{0}'""".format(todo.name))
-
 		self.assertTrue(email_queue)
-		frappe.db.sql('delete from `tabEmail Queue`')
 
 		# test if feedback is submitted for the todo
 		feedback_request, request_key = get_feedback_request(todo.name, feedback_trigger.name)
 		self.assertTrue(feedback_request)
 
 		# test if mail alerts are triggered multiple times for same document
-		self.assertRaises(Exception, todo.save, ignore_permissions=True) 
+		todo.save(ignore_permissions=True)
+		email_queue = frappe.db.sql("""select name from `tabEmail Queue` where
+			reference_doctype='ToDo' and reference_name='{0}'""".format(todo.name))
+		self.assertTrue(len(email_queue) == 1)
+		frappe.db.sql('delete from `tabEmail Queue`')
+
 
 		# Test if feedback is submitted sucessfully
 		result = accept(request_key, "test-feedback@example.com", "ToDo", todo.name, "Great Work !!", 4, fullname="Test User")
@@ -111,8 +112,10 @@ class TestFeedbackTrigger(unittest.TestCase):
 			reference_doctype="ToDo", reference_name=todo.name, feedback="Thank You !!", rating=4, fullname="Test User")
 
 		# auto feedback request should trigger only once
-		self.assertRaises(Exception, todo.save, ignore_permissions=True)
-
+		todo.save(ignore_permissions=True)
+		email_queue = frappe.db.sql("""select name from `tabEmail Queue` where
+			reference_doctype='ToDo' and reference_name='{0}'""".format(todo.name))
+		self.assertFalse(email_queue)
 		frappe.delete_doc("ToDo", todo.name)
 
 		# test if feedback requests and feedback communications are deleted?

frappe/public/js/frappe/feedback.js

@@ -91,10 +91,10 @@ frappe.utils.Feedback = Class.extend({
 			'args': args,
 			freeze: true,
 			callback: function(r) {
-				docname = args.reference_name;
-				recipients = args.details.recipients || ""
-				frappe.msgprint(__("Feedback Request for {0} is sent to {1}",
-					[docname, recipients]));
+				if(r.message) {
+					frappe.msgprint(__("Feedback Request for {0} is sent to {1}",
+						[args.reference_name, args.details.recipients]));
+				}
 			}
 		});
 	}