From b00efb06c0c34133e8055aa4ce926ce191dca39b Mon Sep 17 00:00:00 2001 From: David Arnold Date: Sun, 4 Feb 2024 12:20:10 +0100 Subject: [PATCH 1/7] feat: generalize receiver logic in notifications --- .../doctype/notification/notification.js | 39 ++++++----- .../doctype/notification/notification.py | 67 +++++++++++++++---- 2 files changed, 74 insertions(+), 32 deletions(-) diff --git a/frappe/email/doctype/notification/notification.js b/frappe/email/doctype/notification/notification.js index bede0b429b..6f628b117a 100644 --- a/frappe/email/doctype/notification/notification.js +++ b/frappe/email/doctype/notification/notification.js @@ -14,10 +14,11 @@ frappe.notification = { let get_select_options = function (df, parent_field) { // Append parent_field name along with fieldname for child table fields let select_value = parent_field ? df.fieldname + "," + parent_field : df.fieldname; + let path = parent_field ? parent_field + " > " + df.fieldname : df.fieldname; return { value: select_value, - label: df.fieldname + " (" + __(df.label, null, df.parent) + ")", + label: path + " (" + __(df.label, null, df.parent) + ")", }; }; @@ -49,28 +50,30 @@ frappe.notification = { frm.set_df_property("date_changed", "options", get_date_change_options()); let receiver_fields = []; - if (frm.doc.channel === "Email") { - receiver_fields = $.map(fields, function (d) { - // Add User and Email fields from child into select dropdown - if (frappe.model.table_fields.includes(d.fieldtype)) { - let child_fields = frappe.get_doc("DocType", d.options).fields; - return $.map(child_fields, function (df) { - return df.options == "Email" || - (df.options == "User" && df.fieldtype == "Link") - ? get_select_options(df, d.fieldname) - : null; + let find_receiver_fields = function (extra) { + let predicate = function (df) { + return extra(df) || (df.options == "User" && df.fieldtype == "Link"); + }; + let extract = function (df) { + // Add recipients from child doctypes into select dropdown + if (frappe.model.table_fields.includes(df.fieldtype)) { + let child_fields = frappe.get_doc("DocType", df.options).fields; + return $.map(child_fields, function (cdf) { + return predicate(cdf) ? get_select_options(cdf, df.fieldname) : null; }); - // Add User and Email fields from parent into select dropdown } else { - return d.options == "Email" || - (d.options == "User" && d.fieldtype == "Link") - ? get_select_options(d) - : null; + return predicate(df) ? get_select_options(df) : null; } + }; + return $.map(fields, extract); + }; + if (frm.doc.channel === "Email") { + receiver_fields = find_receiver_fields(function (df) { + return df.options == "Email"; }); } else if (["WhatsApp", "SMS"].includes(frm.doc.channel)) { - receiver_fields = $.map(fields, function (d) { - return d.options == "Phone" ? get_select_options(d) : null; + receiver_fields = find_receiver_fields(function (df) { + return df.options == "Phone" || df.options == "Mobile"; }); } diff --git a/frappe/email/doctype/notification/notification.py b/frappe/email/doctype/notification/notification.py index c9e9f44a53..bae12191fd 100644 --- a/frappe/email/doctype/notification/notification.py +++ b/frappe/email/doctype/notification/notification.py @@ -176,7 +176,7 @@ def get_context(context): """Build recipients and send Notification""" context = get_context(doc) - context = {"doc": doc, "alert": self, "comments": None} + context.update({"alert": self, "comments": None}) if doc.get("_comments"): context["comments"] = json.loads(doc.get("_comments")) @@ -315,8 +315,27 @@ def get_context(context): ) def send_sms(self, doc, context): + def get_phone_no(d, field): + option = d.meta.get_field(field).options.strip() + if option == "Phone" or option == "Mobile": + phone_no = d.get(field) + if not phone_no: + self.log_error(_("Field {0} on document {1} has no Mobile No set").format(field, d.name)) + elif option == "User": + user = d.get(field) + phone_no = frappe.get_value("User", user, "mobile_no") + if not phone_no: + self.log_error(_("User {0} has no Mobile No set").format(user)) + else: + frappe.throw( + _("Field {0} on document {1} is neither a Mobile No data field nor a User link").format( + field, d.name + ) + ) + return phone_no + send_sms( - receiver_list=self.get_receiver_list(doc, context), + receiver_list=self.get_receiver_list(doc, context, "mobile_no", get_phone_no), msg=frappe.utils.strip_html_tags(frappe.render_template(self.message, context)), ) @@ -329,16 +348,17 @@ def get_context(context): if not frappe.safe_eval(recipient.condition, None, context): continue if recipient.receiver_by_document_field: - fields = recipient.receiver_by_document_field.split(",") - # fields from child table - if len(fields) > 1: - for d in doc.get(fields[1]): - email_id = d.get(fields[0]) + data_field, child_field = _parse_receiver_by_document_field( + recipient.receiver_by_document_field + ) + if child_field: + for d in doc.get(child_field): + email_id = d.get(data_field) if validate_email_address(email_id): recipients.append(email_id) - # field from parent doc + # field from current doc else: - email_ids_value = doc.get(fields[0]) + email_ids_value = doc.get(data_field) if validate_email_address(email_ids_value): email_ids = email_ids_value.replace(",", "\n") recipients = recipients + email_ids.split("\n") @@ -358,7 +378,7 @@ def get_context(context): return list(set(recipients)), list(set(cc)), list(set(bcc)) - def get_receiver_list(self, doc, context): + def get_receiver_list(self, doc, context, user_field, field_extractor_func): """return receiver list based on the doc field and role specified""" receiver_list = [] for recipient in self.recipients: @@ -368,18 +388,28 @@ def get_context(context): # For sending messages to the owner's mobile phone number if recipient.receiver_by_document_field == "owner": - receiver_list += get_user_info([dict(user_name=doc.get("owner"))], "mobile_no") + receiver_list += get_user_info([dict(user_name=doc.get("owner"))], user_field) # For sending messages to the number specified in the receiver field elif recipient.receiver_by_document_field: - receiver_list.append(doc.get(recipient.receiver_by_document_field)) + data_field, child_field = _parse_receiver_by_document_field( + recipient.receiver_by_document_field + ) + if child_field: + for d in doc.get(child_field): + if recv := field_extractor_func(d, data_field): + receiver_list.append(recv) + # field from current doc + else: + if recv := field_extractor_func(doc, data_field): + receiver_list.append(recv) # For sending messages to specified role if recipient.receiver_by_role: receiver_list += get_info_based_on_role( - recipient.receiver_by_role, "mobile_no", ignore_permissions=True + recipient.receiver_by_role, user_field, ignore_permissions=True ) - return receiver_list + return list(set(receiver_list)) def get_attachment(self, doc): """check print settings are attach the pdf""" @@ -555,3 +585,12 @@ def get_reference_doctype(doc): def get_reference_name(doc): return doc.parent if doc.meta.istable else doc.name + +def _parse_receiver_by_document_field(s): + fragments = s.split(",") + # fields from child table or linked doctype + if len(fragments) > 1: + data_field, child_field = fragments + else: + data_field, child_field = fragments[0], None + return data_field, child_field From 19e03592be807113d0fbf590e7126acd671cfb40 Mon Sep 17 00:00:00 2001 From: David Arnold Date: Mon, 12 Feb 2024 15:40:10 +0100 Subject: [PATCH 2/7] style: be more explicit and docstring --- .../doctype/notification/notification.js | 54 ++++++++++++------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/frappe/email/doctype/notification/notification.js b/frappe/email/doctype/notification/notification.js index 6f628b117a..d224faf3ca 100644 --- a/frappe/email/doctype/notification/notification.js +++ b/frappe/email/doctype/notification/notification.js @@ -35,6 +35,37 @@ frappe.notification = { ]); }; + let get_receiver_fields = function ( + is_extra_receiver_field = (_) => { + return false; + } + ) { + // finds receiver fields from the fields or any child table + // by default finds any link to the User doctype + // however an additional optional predicate can be passed as argument + // to find additional fields + let is_receiver_field = function (df) { + return ( + is_extra_receiver_field(df) || + (df.options == "User" && df.fieldtype == "Link") + ); + }; + let extract_receiver_field = function (df) { + // Add recipients from child doctypes into select dropdown + if (frappe.model.table_fields.includes(df.fieldtype)) { + let child_fields = frappe.get_doc("DocType", df.options).fields; + return $.map(child_fields, function (cdf) { + return is_receiver_field(cdf) + ? get_select_options(cdf, df.fieldname) + : null; + }); + } else { + return is_receiver_field(df) ? get_select_options(df) : null; + } + }; + return $.map(fields, extract_receiver_field); + }; + let fields = frappe.get_doc("DocType", frm.doc.document_type).fields; let options = $.map(fields, function (d) { return frappe.model.no_value_type.includes(d.fieldtype) @@ -50,30 +81,13 @@ frappe.notification = { frm.set_df_property("date_changed", "options", get_date_change_options()); let receiver_fields = []; - let find_receiver_fields = function (extra) { - let predicate = function (df) { - return extra(df) || (df.options == "User" && df.fieldtype == "Link"); - }; - let extract = function (df) { - // Add recipients from child doctypes into select dropdown - if (frappe.model.table_fields.includes(df.fieldtype)) { - let child_fields = frappe.get_doc("DocType", df.options).fields; - return $.map(child_fields, function (cdf) { - return predicate(cdf) ? get_select_options(cdf, df.fieldname) : null; - }); - } else { - return predicate(df) ? get_select_options(df) : null; - } - }; - return $.map(fields, extract); - }; if (frm.doc.channel === "Email") { - receiver_fields = find_receiver_fields(function (df) { + receiver_fields = get_receiver_fields(fields, function (df) { return df.options == "Email"; }); } else if (["WhatsApp", "SMS"].includes(frm.doc.channel)) { - receiver_fields = find_receiver_fields(function (df) { - return df.options == "Phone" || df.options == "Mobile"; + receiver_fields = get_receiver_fields(fields, function (df) { + df.options == "Phone" || df.options == "Mobile"; }); } From f0a28b51ee3d051be876ecc18da2762996ff15f1 Mon Sep 17 00:00:00 2001 From: David Date: Mon, 17 Jun 2024 09:38:44 +0200 Subject: [PATCH 3/7] fix: add customer to eligible indirections --- frappe/email/doctype/notification/notification.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/frappe/email/doctype/notification/notification.py b/frappe/email/doctype/notification/notification.py index bae12191fd..1b2ecb536b 100644 --- a/frappe/email/doctype/notification/notification.py +++ b/frappe/email/doctype/notification/notification.py @@ -326,11 +326,16 @@ def get_context(context): phone_no = frappe.get_value("User", user, "mobile_no") if not phone_no: self.log_error(_("User {0} has no Mobile No set").format(user)) + elif option == "Customer": + customer = d.get(field) + phone_no = frappe.get_value("Customer", customer, "mobile_no") + if not phone_no: + self.log_error(_("Customer {0} has no Mobile No set").format(customer)) else: frappe.throw( - _("Field {0} on document {1} is neither a Mobile No data field nor a User link").format( - field, d.name - ) + _( + "Field {0} on document {1} is neither a Mobile No data field nor a Customer or User link" + ).format(field, d.name) ) return phone_no From 69ac3b7f1669a2f18f73b27e63c46151631f116c Mon Sep 17 00:00:00 2001 From: David Date: Mon, 17 Jun 2024 10:12:43 +0200 Subject: [PATCH 4/7] fix: add customer to eligible indirections 2 --- frappe/email/doctype/notification/notification.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frappe/email/doctype/notification/notification.js b/frappe/email/doctype/notification/notification.js index d224faf3ca..4017c9e40f 100644 --- a/frappe/email/doctype/notification/notification.js +++ b/frappe/email/doctype/notification/notification.js @@ -34,8 +34,8 @@ frappe.notification = { { value: "modified", label: `modified (${__("Last Modified Date")})` }, ]); }; - let get_receiver_fields = function ( + fields, is_extra_receiver_field = (_) => { return false; } @@ -47,7 +47,8 @@ frappe.notification = { let is_receiver_field = function (df) { return ( is_extra_receiver_field(df) || - (df.options == "User" && df.fieldtype == "Link") + (df.options == "User" && df.fieldtype == "Link") || + (df.options == "Customer" && df.fieldtype == "Link") ); }; let extract_receiver_field = function (df) { From c4da76579c9c1994aa0baf131996854ff2af520d Mon Sep 17 00:00:00 2001 From: David Date: Fri, 21 Jun 2024 12:35:17 +0200 Subject: [PATCH 5/7] fix: error log context on notification failure --- frappe/email/doctype/notification/notification.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/frappe/email/doctype/notification/notification.py b/frappe/email/doctype/notification/notification.py index 1b2ecb536b..906152f7a6 100644 --- a/frappe/email/doctype/notification/notification.py +++ b/frappe/email/doctype/notification/notification.py @@ -320,17 +320,21 @@ def get_context(context): if option == "Phone" or option == "Mobile": phone_no = d.get(field) if not phone_no: - self.log_error(_("Field {0} on document {1} has no Mobile No set").format(field, d.name)) + doc.log_error( + _("Notification: field {0} on document {1} has no Mobile No set").format( + field, d.name + ) + ) elif option == "User": user = d.get(field) phone_no = frappe.get_value("User", user, "mobile_no") if not phone_no: - self.log_error(_("User {0} has no Mobile No set").format(user)) + doc.log_error(_("Notification: user {0} has no Mobile No set").format(user)) elif option == "Customer": customer = d.get(field) phone_no = frappe.get_value("Customer", customer, "mobile_no") if not phone_no: - self.log_error(_("Customer {0} has no Mobile No set").format(customer)) + doc.log_error(_("Notification: customer {0} has no Mobile No set").format(customer)) else: frappe.throw( _( From 7c1abdf1aaebd70ebbe3e47a750ae9df13c95f8b Mon Sep 17 00:00:00 2001 From: David Date: Mon, 1 Jul 2024 19:18:04 +0200 Subject: [PATCH 6/7] fix: implement feedback --- .../doctype/notification/notification.py | 73 ++++++++++--------- 1 file changed, 39 insertions(+), 34 deletions(-) diff --git a/frappe/email/doctype/notification/notification.py b/frappe/email/doctype/notification/notification.py index 906152f7a6..ae915759c0 100644 --- a/frappe/email/doctype/notification/notification.py +++ b/frappe/email/doctype/notification/notification.py @@ -315,39 +315,42 @@ def get_context(context): ) def send_sms(self, doc, context): - def get_phone_no(d, field): - option = d.meta.get_field(field).options.strip() - if option == "Phone" or option == "Mobile": - phone_no = d.get(field) - if not phone_no: - doc.log_error( - _("Notification: field {0} on document {1} has no Mobile No set").format( - field, d.name - ) - ) - elif option == "User": - user = d.get(field) - phone_no = frappe.get_value("User", user, "mobile_no") - if not phone_no: - doc.log_error(_("Notification: user {0} has no Mobile No set").format(user)) - elif option == "Customer": - customer = d.get(field) - phone_no = frappe.get_value("Customer", customer, "mobile_no") - if not phone_no: - doc.log_error(_("Notification: customer {0} has no Mobile No set").format(customer)) - else: - frappe.throw( - _( - "Field {0} on document {1} is neither a Mobile No data field nor a Customer or User link" - ).format(field, d.name) - ) - return phone_no - send_sms( - receiver_list=self.get_receiver_list(doc, context, "mobile_no", get_phone_no), + receiver_list=self.get_receiver_list(doc, context, "mobile_no", self.get_mobile_no), msg=frappe.utils.strip_html_tags(frappe.render_template(self.message, context)), ) + @staticmethod + def get_mobile_no(doc, field): + option = doc.meta.get_field(field).options.strip() + # users may sometimes register mobile numbers under Phone type fields + if option == "Phone" or option == "Mobile": + mobile_no = doc.get(field) + if not mobile_no: + doc.log_error( + _("Notification: document {0} has no {1} number set (field: {2})").format( + field, doc.name, option, field + ) + ) + # but on user & customer it's expected to be set on the proper field + elif option == "User": + user = doc.get(field) + mobile_no = frappe.get_value("User", user, "mobile_no") + if not mobile_no: + doc.log_error(_("Notification: user {0} has no Mobile number set").format(user)) + elif option == "Customer": + customer = doc.get(field) + mobile_no = frappe.get_value("Customer", customer, "mobile_no") + if not mobile_no: + doc.log_error(_("Notification: customer {0} has no Mobile number set").format(customer)) + else: + frappe.throw( + _( + "Field {0} on document {1} is neither a Mobile number field nor a Customer or User link" + ).format(field, doc.name) + ) + return mobile_no + def get_list_of_recipients(self, doc, context): recipients = [] cc = [] @@ -387,8 +390,10 @@ def get_context(context): return list(set(recipients)), list(set(cc)), list(set(bcc)) - def get_receiver_list(self, doc, context, user_field, field_extractor_func): + def get_receiver_list(self, doc, context, field_on_user="mobile_no", recipient_extractor_func=None): """return receiver list based on the doc field and role specified""" + if not recipient_extractor_func: + recipient_extractor_func = self.get_mobile_no receiver_list = [] for recipient in self.recipients: if recipient.condition: @@ -397,7 +402,7 @@ def get_context(context): # For sending messages to the owner's mobile phone number if recipient.receiver_by_document_field == "owner": - receiver_list += get_user_info([dict(user_name=doc.get("owner"))], user_field) + receiver_list += get_user_info([dict(user_name=doc.get("owner"))], field_on_user) # For sending messages to the number specified in the receiver field elif recipient.receiver_by_document_field: data_field, child_field = _parse_receiver_by_document_field( @@ -405,17 +410,17 @@ def get_context(context): ) if child_field: for d in doc.get(child_field): - if recv := field_extractor_func(d, data_field): + if recv := recipient_extractor_func(d, data_field): receiver_list.append(recv) # field from current doc else: - if recv := field_extractor_func(doc, data_field): + if recv := recipient_extractor_func(doc, data_field): receiver_list.append(recv) # For sending messages to specified role if recipient.receiver_by_role: receiver_list += get_info_based_on_role( - recipient.receiver_by_role, user_field, ignore_permissions=True + recipient.receiver_by_role, field_on_user, ignore_permissions=True ) return list(set(receiver_list)) From 4a4e25f9887b0b1e1fb8b43bdd48c88c0865683a Mon Sep 17 00:00:00 2001 From: David Date: Thu, 18 Jul 2024 11:48:17 +0200 Subject: [PATCH 7/7] style: format --- frappe/email/doctype/notification/notification.py | 1 + 1 file changed, 1 insertion(+) diff --git a/frappe/email/doctype/notification/notification.py b/frappe/email/doctype/notification/notification.py index ae915759c0..9725846496 100644 --- a/frappe/email/doctype/notification/notification.py +++ b/frappe/email/doctype/notification/notification.py @@ -600,6 +600,7 @@ def get_reference_doctype(doc): def get_reference_name(doc): return doc.parent if doc.meta.istable else doc.name + def _parse_receiver_by_document_field(s): fragments = s.split(",") # fields from child table or linked doctype