From 24830298b03272655e916af620d496f458a69c6b Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 8 Nov 2021 18:27:29 +0530 Subject: [PATCH 01/29] fix: Make owner, creation fields constant for docs --- frappe/model/meta.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/frappe/model/meta.py b/frappe/model/meta.py index 252c463d3d..a483f3f2d6 100644 --- a/frappe/model/meta.py +++ b/frappe/model/meta.py @@ -67,6 +67,10 @@ class Meta(Document): _metaclass = True default_fields = list(default_fields)[1:] special_doctypes = ("DocField", "DocPerm", "DocType", "Module Def", 'DocType Action', 'DocType Link', 'DocType State') + standard_set_once_fields = [ + frappe._dict(fieldname="creation", fieldtype="Datetime"), + frappe._dict(fieldname="owner", fieldtype="Data"), + ] def __init__(self, doctype): self._fields = {} @@ -154,6 +158,12 @@ class Meta(Document): '''Return fields with `set_only_once` set''' if not hasattr(self, "_set_only_once_fields"): self._set_only_once_fields = self.get("fields", {"set_only_once": 1}) + fieldnames = [d.fieldname for d in self._set_only_once_fields] + + for df in self.standard_set_once_fields: + if df.fieldname not in fieldnames: + self._set_only_once_fields.append(df) + return self._set_only_once_fields def get_table_fields(self): From 62499422a85a38b6dfbc16080674149d9108c852 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 8 Nov 2021 18:28:36 +0530 Subject: [PATCH 02/29] style: Make fieldname bold in user message --- frappe/model/document.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index 1f079feedc..891ad1d8de 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -562,8 +562,12 @@ class Document(BaseDocument): fail = value != original_value if fail: - frappe.throw(_("Value cannot be changed for {0}").format(self.meta.get_label(field.fieldname)), - frappe.CannotChangeConstantError) + frappe.throw( + _("Value cannot be changed for {0}").format( + frappe.bold(self.meta.get_label(field.fieldname)) + ), + exc=frappe.CannotChangeConstantError + ) return False From 568d9b94ed4b88e812a65fc2c1c001df89092152 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 8 Nov 2021 19:00:17 +0530 Subject: [PATCH 03/29] test: Use db_set to change creation, status --- .../test_personal_data_deletion_request.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/frappe/website/doctype/personal_data_deletion_request/test_personal_data_deletion_request.py b/frappe/website/doctype/personal_data_deletion_request/test_personal_data_deletion_request.py index 8fc8f38512..27dcfe5858 100644 --- a/frappe/website/doctype/personal_data_deletion_request/test_personal_data_deletion_request.py +++ b/frappe/website/doctype/personal_data_deletion_request/test_personal_data_deletion_request.py @@ -50,11 +50,10 @@ class TestPersonalDataDeletionRequest(unittest.TestCase): def test_unverified_record_removal(self): date_time_obj = datetime.strptime( self.delete_request.creation, "%Y-%m-%d %H:%M:%S.%f" - ) - date_time_obj += timedelta(days=-7) - self.delete_request.creation = date_time_obj - self.status = "Pending Verification" - self.delete_request.save() + ) + timedelta(days=-7) + self.delete_request.db_set("creation", date_time_obj) + self.delete_request.db_set("status", "Pending Verification") + remove_unverified_record() self.assertFalse( frappe.db.exists("Personal Data Deletion Request", self.delete_request.name) From 272cea49405cb5cbf3cc6be04659358d52c4968b Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 9 Nov 2021 01:17:26 +0530 Subject: [PATCH 04/29] test: Add test for owner, creation constants --- frappe/tests/test_permissions.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/frappe/tests/test_permissions.py b/frappe/tests/test_permissions.py index b4e7db9956..d83f2969ba 100644 --- a/frappe/tests/test_permissions.py +++ b/frappe/tests/test_permissions.py @@ -12,6 +12,7 @@ from frappe.core.page.permission_manager.permission_manager import update, reset from frappe.test_runner import make_test_records_for_doctype from frappe.core.doctype.user_permission.user_permission import clear_user_permissions from frappe.desk.form.load import getdoc +from frappe.utils.data import now_datetime test_dependencies = ['Blogger', 'Blog Post', "User", "Contact", "Salutation"] @@ -197,6 +198,17 @@ class TestPermissions(unittest.TestCase): doc = frappe.get_doc("Blog Post", "-test-blog-post") self.assertTrue(doc.has_permission("read")) + def test_dont_change_standard_constants(self): + # check that Document.creation cannot be changed + user = frappe.get_doc("User", frappe.session.user) + user.creation = now_datetime() + self.assertRaises(frappe.CannotChangeConstantError, user.save) + + # check that Document.owner cannot be changed + user.reload() + user.owner = frappe.db.get_value("User", {"name": ("!=", user.name)}) + self.assertRaises(frappe.CannotChangeConstantError, user.save) + def test_set_only_once(self): blog_post = frappe.get_meta("Blog Post") doc = frappe.get_doc("Blog Post", "-test-blog-post-1") From 881f3ad8c1bb7c88a275e8504ae183363985f674 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 30 Dec 2021 10:35:49 +0530 Subject: [PATCH 05/29] fix: Set owner & creation if new Document via https://github.com/frappe/frappe/pull/14918/commits/a323d624eb3a59942aaac70ec95412140e70b7cb --- frappe/model/document.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index 891ad1d8de..16a2d35290 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -396,6 +396,7 @@ class Document(BaseDocument): "parenttype": self.doctype, "parentfield": fieldname }) + def get_doc_before_save(self): return getattr(self, '_doc_before_save', None) @@ -468,9 +469,11 @@ class Document(BaseDocument): self._original_modified = self.modified self.modified = now() self.modified_by = frappe.session.user - if not self.creation: + + # We'd probably want the creation and owner to be set via API + # or Data import at some point, that'd have to be handled here + if self.is_new(): self.creation = self.modified - if not self.owner: self.owner = self.modified_by for d in self.get_all_children(): From ecb0cd41398d169a8eb849420f992cf086fc6872 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 30 Dec 2021 10:37:12 +0530 Subject: [PATCH 06/29] test: Add test for disallowing setting tandard fields Via https://github.com/frappe/frappe/pull/14918/commits/db008020f69408a1ce278a311645fb861b5a78d7 --- frappe/tests/test_permissions.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/frappe/tests/test_permissions.py b/frappe/tests/test_permissions.py index d83f2969ba..fdff4d103e 100644 --- a/frappe/tests/test_permissions.py +++ b/frappe/tests/test_permissions.py @@ -198,6 +198,21 @@ class TestPermissions(unittest.TestCase): doc = frappe.get_doc("Blog Post", "-test-blog-post") self.assertTrue(doc.has_permission("read")) + def test_set_standard_fields_manually(self): + # check that creation and owner cannot be set manually + from datetime import timedelta + + fake_creation = now_datetime() + timedelta(days=-7) + fake_owner = frappe.db.get_value("User", {"name": ("!=", frappe.session.user)}) + + d = frappe.new_doc("ToDo") + d.description = "ToDo created via test_set_standard_fields_manually" + d.creation = fake_creation + d.owner = fake_owner + d.save() + self.assertNotEqual(d.creation, fake_creation) + self.assertNotEqual(d.owner, fake_owner) + def test_dont_change_standard_constants(self): # check that Document.creation cannot be changed user = frappe.get_doc("User", frappe.session.user) From 289d7e7afa8c922d9069579d3a898ba98b529a9b Mon Sep 17 00:00:00 2001 From: Pruthvi Patel Date: Sat, 1 Jan 2022 17:49:59 +0530 Subject: [PATCH 07/29] fix: clear cache when filters passed as dn param in `set_value` --- frappe/database/database.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 7c147cd1d0..165dc729ed 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -699,6 +699,8 @@ class Database(object): self.sql("""update `tab{0}` set {1} where name=%(name)s""".format(dt, ', '.join(set_values)), values, debug=debug) + + frappe.clear_document_cache(dt, values['name']) else: # for singles keys = list(to_update) @@ -711,10 +713,11 @@ class Database(object): self.sql('''insert into `tabSingles` (doctype, field, value) values (%s, %s, %s)''', (dt, key, value), debug=debug) + frappe.clear_document_cache(dt, dn) + if dt in self.value_cache: del self.value_cache[dt] - frappe.clear_document_cache(dt, dn) @staticmethod def set(doc, field, val): From e3fc400fd4d9385995d4bf13d0bf586f0e4e675b Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 3 Jan 2022 19:01:01 +0530 Subject: [PATCH 08/29] refactor(minor): Override clear_cache instead of separate hooks Clear cached maps via Document.clear_cache --- .../assignment_rule/assignment_rule.py | 24 +++++++------------ 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/frappe/automation/doctype/assignment_rule/assignment_rule.py b/frappe/automation/doctype/assignment_rule/assignment_rule.py index a3e27d4da5..5a5581f2bc 100644 --- a/frappe/automation/doctype/assignment_rule/assignment_rule.py +++ b/frappe/automation/doctype/assignment_rule/assignment_rule.py @@ -3,14 +3,14 @@ # License: MIT. See LICENSE import frappe -from frappe.model.document import Document -from frappe.desk.form import assign_to -import frappe.cache_manager from frappe import _ +from frappe.cache_manager import clear_doctype_map, get_doctype_map +from frappe.desk.form import assign_to from frappe.model import log_types +from frappe.model.document import Document + class AssignmentRule(Document): - def validate(self): assignment_days = self.get_assignment_days() if not len(set(assignment_days)) == len(assignment_days): @@ -19,14 +19,10 @@ class AssignmentRule(Document): if self.document_type == 'ToDo': frappe.throw(_('Assignment Rule is not allowed on {0} document type').format(frappe.bold("ToDo"))) - def on_update(self): - clear_assignment_rule_cache(self) - - def after_rename(self, old, new, merge): - clear_assignment_rule_cache(self) - - def on_trash(self): - clear_assignment_rule_cache(self) + def clear_cache(self): + super().clear_cache() + clear_doctype_map(self.doctype, self.document_type) + clear_doctype_map(self.doctype, f"due_date_rules_for_{self.document_type}") def apply_unassign(self, doc, assignments): if (self.unassign_condition and @@ -35,7 +31,6 @@ class AssignmentRule(Document): return False - def apply_assign(self, doc): if self.safe_eval('assign_condition', doc): return self.do_assignment(doc) @@ -296,6 +291,3 @@ def get_repeated(values): diff.append(str(value)) return " ".join(diff) -def clear_assignment_rule_cache(rule): - frappe.cache_manager.clear_doctype_map('Assignment Rule', rule.document_type) - frappe.cache_manager.clear_doctype_map('Assignment Rule', 'due_date_rules_for_' + rule.document_type) From f72c445d41ad25f019ad713170885530b5a93e59 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 3 Jan 2022 19:02:41 +0530 Subject: [PATCH 09/29] fix: Clear Document cache on rename, delete --- frappe/model/delete_doc.py | 3 ++- frappe/model/rename_doc.py | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/frappe/model/delete_doc.py b/frappe/model/delete_doc.py index ac976e976c..2fddcf9e33 100644 --- a/frappe/model/delete_doc.py +++ b/frappe/model/delete_doc.py @@ -117,7 +117,8 @@ def delete_doc(doctype=None, name=None, force=0, ignore_doctypes=None, for_reloa doctype=doc.doctype, name=doc.name, is_async=False if frappe.flags.in_test else True) - + # clear cache for Document + doc.clear_cache() # delete global search entry delete_for_document(doc) # delete tag link entry diff --git a/frappe/model/rename_doc.py b/frappe/model/rename_doc.py index 651153876a..f84242626b 100644 --- a/frappe/model/rename_doc.py +++ b/frappe/model/rename_doc.py @@ -110,6 +110,7 @@ def rename_doc( if merge: frappe.delete_doc(doctype, old) + new_doc.clear_cache() frappe.clear_cache() if rebuild_search: frappe.enqueue('frappe.utils.global_search.rebuild_for_doctype', doctype=doctype) From 3a384565836ffb961e1db32189035582961f0b87 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 3 Jan 2022 19:03:56 +0530 Subject: [PATCH 10/29] refactor(minor): Assignment Rule * Simplified and refactored logic * Added type hints for where it gets really unbearable :') * Minor perf improvements & better API usages --- .../assignment_rule/assignment_rule.py | 201 ++++++++++++------ 1 file changed, 131 insertions(+), 70 deletions(-) diff --git a/frappe/automation/doctype/assignment_rule/assignment_rule.py b/frappe/automation/doctype/assignment_rule/assignment_rule.py index 5a5581f2bc..724c453972 100644 --- a/frappe/automation/doctype/assignment_rule/assignment_rule.py +++ b/frappe/automation/doctype/assignment_rule/assignment_rule.py @@ -1,7 +1,8 @@ -# -*- coding: utf-8 -*- -# Copyright (c) 2019, Frappe Technologies and contributors +# Copyright (c) 2022, Frappe Technologies and contributors # License: MIT. See LICENSE +from typing import Dict, Iterable, List + import frappe from frappe import _ from frappe.cache_manager import clear_doctype_map, get_doctype_map @@ -12,18 +13,36 @@ from frappe.model.document import Document class AssignmentRule(Document): def validate(self): - assignment_days = self.get_assignment_days() - if not len(set(assignment_days)) == len(assignment_days): - repeated_days = get_repeated(assignment_days) - frappe.throw(_("Assignment Day {0} has been repeated.").format(frappe.bold(repeated_days))) - if self.document_type == 'ToDo': - frappe.throw(_('Assignment Rule is not allowed on {0} document type').format(frappe.bold("ToDo"))) + self.validate_document_types() + self.validate_assignment_days() def clear_cache(self): super().clear_cache() clear_doctype_map(self.doctype, self.document_type) clear_doctype_map(self.doctype, f"due_date_rules_for_{self.document_type}") + def validate_document_types(self): + if self.document_type == "ToDo": + frappe.throw( + _('Assignment Rule is not allowed on {0} document type').format( + frappe.bold("ToDo") + ) + ) + + def validate_assignment_days(self): + assignment_days = self.get_assignment_days() + + if len(set(assignment_days)) != len(assignment_days): + repeated_days = get_repeated(assignment_days) + plural = "s" if len(repeated_days) > 1 else "" + + frappe.throw( + _("Assignment Day{0} {1} has been repeated.").format( + plural, + frappe.bold(", ".join(repeated_days)) + ) + ) + def apply_unassign(self, doc, assignments): if (self.unassign_condition and self.name in [d.assignment_rule for d in assignments]): @@ -136,65 +155,68 @@ class AssignmentRule(Document): def is_rule_not_applicable_today(self): today = frappe.flags.assignment_day or frappe.utils.get_weekday() assignment_days = self.get_assignment_days() - if assignment_days and not today in assignment_days: - return True + return assignment_days and today not in assignment_days - return False -def get_assignments(doc): +def get_assignments(doc) -> List[Dict]: return frappe.get_all('ToDo', fields = ['name', 'assignment_rule'], filters = dict( reference_type = doc.get('doctype'), reference_name = doc.get('name'), status = ('!=', 'Cancelled') - ), limit = 5) + ), limit=5) + @frappe.whitelist() def bulk_apply(doctype, docnames): - import json - docnames = json.loads(docnames) - + docnames = frappe.parse_json(docnames) background = len(docnames) > 5 + for name in docnames: if background: frappe.enqueue('frappe.automation.doctype.assignment_rule.assignment_rule.apply', doc=None, doctype=doctype, name=name) else: - apply(None, doctype=doctype, name=name) + apply(doctype=doctype, name=name) + def reopen_closed_assignment(doc): - todo_list = frappe.db.get_all('ToDo', filters = dict( - reference_type = doc.doctype, - reference_name = doc.name, - status = 'Closed' - )) - if not todo_list: - return False + todo_list = frappe.get_all("ToDo", filters={ + "reference_type": doc.doctype, + "reference_name": doc.name, + "status": "Closed", + }, pluck="name") + for todo in todo_list: - todo_doc = frappe.get_doc('ToDo', todo.name) + todo_doc = frappe.get_doc('ToDo', todo) todo_doc.status = 'Open' todo_doc.save(ignore_permissions=True) - return True -def apply(doc, method=None, doctype=None, name=None): - if not doctype: - doctype = doc.doctype + return bool(todo_list) - if (frappe.flags.in_patch + +def apply(doc=None, method=None, doctype=None, name=None): + doctype = doctype or doc.doctype + + skip_assignment_rules = ( + frappe.flags.in_patch or frappe.flags.in_install or frappe.flags.in_setup_wizard - or doctype in log_types): + or doctype in log_types + ) + + if skip_assignment_rules: return if not doc and doctype and name: doc = frappe.get_doc(doctype, name) - assignment_rules = frappe.cache_manager.get_doctype_map('Assignment Rule', doc.doctype, dict( - document_type = doc.doctype, disabled = 0), order_by = 'priority desc') - - assignment_rule_docs = [] + assignment_rules = get_doctype_map("Assignment Rule", doc.doctype, filters={ + "document_type": doc.doctype, "disabled": 0 + }, order_by="priority desc") # multiple auto assigns - for d in assignment_rules: - assignment_rule_docs.append(frappe.get_cached_doc('Assignment Rule', d.get('name'))) + assignment_rule_docs: List[AssignmentRule] = [ + frappe.get_cached_doc("Assignment Rule", d.get('name')) for d in assignment_rules + ] if not assignment_rule_docs: return @@ -230,6 +252,7 @@ def apply(doc, method=None, doctype=None, name=None): # apply close rule only if assignments exists assignments = get_assignments(doc) + if assignments: for assignment_rule in assignment_rule_docs: if assignment_rule.is_rule_not_applicable_today(): @@ -237,38 +260,74 @@ def apply(doc, method=None, doctype=None, name=None): if not new_apply: # only reopen if close condition is not satisfied - if not assignment_rule.safe_eval('close_condition', doc): - reopen = reopen_closed_assignment(doc) - if reopen: + to_close_todos = assignment_rule.safe_eval('close_condition', doc) + + if to_close_todos: + # close todo status + todos_to_close = frappe.get_all("ToDo", filters={ + "reference_type": doc.doctype, + "reference_name": doc.name, + }, pluck="name") + + for todo in todos_to_close: + _todo = frappe.get_doc("ToDo", todo) + _todo.status = "Closed" + _todo.save() + break + + else: + reopened = reopen_closed_assignment(doc) + if reopened: break + + # print(f"Rule:{assignment_rule}\nDoc: {doc}\nReOpened: {reopened}") + assignment_rule.close_assignments(doc) + def update_due_date(doc, state=None): - # called from hook - if (frappe.flags.in_patch - or frappe.flags.in_install - or frappe.flags.in_migrate + """Run on_update on every Document (via hooks.py) + """ + skip_document_update = ( + frappe.flags.in_migrate + or frappe.flags.in_patch or frappe.flags.in_import - or frappe.flags.in_setup_wizard): + or frappe.flags.in_setup_wizard + or frappe.flags.in_install + ) + + if skip_document_update: return - assignment_rules = frappe.cache_manager.get_doctype_map('Assignment Rule', 'due_date_rules_for_' + doc.doctype, dict( - document_type = doc.doctype, - disabled = 0, - due_date_based_on = ['is', 'set'] - )) + + assignment_rules = get_doctype_map( + doctype="Assignment Rule", + name=f"due_date_rules_for_{doc.doctype}", + filters={ + "due_date_based_on": ["is", "set"], + "document_type": doc.doctype, + "disabled": 0, + } + ) + for rule in assignment_rules: - rule_doc = frappe.get_cached_doc('Assignment Rule', rule.get('name')) + rule_doc = frappe.get_cached_doc("Assignment Rule", rule.get("name")) due_date_field = rule_doc.due_date_based_on - if doc.meta.has_field(due_date_field) and \ - doc.has_value_changed(due_date_field) and rule.get('name'): - assignment_todos = frappe.get_all('ToDo', { - 'assignment_rule': rule.get('name'), - 'status': 'Open', - 'reference_type': doc.doctype, - 'reference_name': doc.name - }) + field_updated = ( + doc.meta.has_field(due_date_field) + and doc.has_value_changed(due_date_field) + and rule.get("name") + ) + + if field_updated: + assignment_todos = frappe.get_all("ToDo", filters={ + "assignment_rule": rule.get("name"), + "reference_type": doc.doctype, + "reference_name": doc.name, + "status": "Open", + }, pluck="name") + for todo in assignment_todos: - todo_doc = frappe.get_doc('ToDo', todo.name) + todo_doc = frappe.get_doc('ToDo', todo) todo_doc.date = doc.get(due_date_field) todo_doc.flags.updater_reference = { 'doctype': 'Assignment Rule', @@ -277,17 +336,19 @@ def update_due_date(doc, state=None): } todo_doc.save(ignore_permissions=True) -def get_assignment_rules(): - return [d.document_type for d in frappe.db.get_all('Assignment Rule', fields=['document_type'], filters=dict(disabled = 0))] -def get_repeated(values): - unique_list = [] - diff = [] +def get_assignment_rules() -> List[str]: + return frappe.get_all("Assignment Rule", filters={"disabled": 0}, pluck="document_type") + + +def get_repeated(values: Iterable) -> List: + unique = set() + repeated = set() + for value in values: - if value not in unique_list: - unique_list.append(str(value)) + if value in unique: + repeated.add(value) else: - if value not in diff: - diff.append(str(value)) - return " ".join(diff) + unique.add(value) + return [str(x) for x in repeated] From f7b59829196a3e0594d2225b37eba8874d129bf4 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 3 Jan 2022 19:09:42 +0530 Subject: [PATCH 11/29] test(assign_rule): Setup and teardown for local testing --- .../assignment_rule/test_assignment_rule.py | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/frappe/automation/doctype/assignment_rule/test_assignment_rule.py b/frappe/automation/doctype/assignment_rule/test_assignment_rule.py index 1c9e177f94..f20253b779 100644 --- a/frappe/automation/doctype/assignment_rule/test_assignment_rule.py +++ b/frappe/automation/doctype/assignment_rule/test_assignment_rule.py @@ -1,12 +1,22 @@ -# -*- coding: utf-8 -*- -# Copyright (c) 2019, Frappe Technologies and Contributors +# Copyright (c) 2021, Frappe Technologies and Contributors # License: MIT. See LICENSE -import frappe + import unittest -from frappe.utils import random_string + +import frappe from frappe.test_runner import make_test_records +from frappe.utils import random_string + class TestAutoAssign(unittest.TestCase): + @classmethod + def setUpClass(cls): + frappe.db.delete("Assignment Rule") + + @classmethod + def tearDownClass(cls): + frappe.db.rollback() + def setUp(self): make_test_records("User") days = [ @@ -129,7 +139,7 @@ class TestAutoAssign(unittest.TestCase): reference_type = 'Note', reference_name = note.name, status = 'Open' - ))[0] + ), limit=1)[0] todo = frappe.get_doc('ToDo', todo['name']) self.assertEqual(todo.owner, 'test@example.com') @@ -151,7 +161,7 @@ class TestAutoAssign(unittest.TestCase): reference_type = 'Note', reference_name = note.name, status = 'Open' - ))[0] + ), limit=1)[0] todo = frappe.get_doc('ToDo', todo['name']) self.assertEqual(todo.owner, 'test@example.com') From 322a777b597990b8bfd66bda2531a4a97aeb35b9 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 3 Jan 2022 19:10:28 +0530 Subject: [PATCH 12/29] fix(ux): Auto Repeat's validation message has grammatical fixes --- frappe/automation/doctype/auto_repeat/auto_repeat.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/frappe/automation/doctype/auto_repeat/auto_repeat.py b/frappe/automation/doctype/auto_repeat/auto_repeat.py index 5ab6c86c00..0277b8e402 100644 --- a/frappe/automation/doctype/auto_repeat/auto_repeat.py +++ b/frappe/automation/doctype/auto_repeat/auto_repeat.py @@ -96,7 +96,15 @@ class AutoRepeat(Document): auto_repeat_days = self.get_auto_repeat_days() if not len(set(auto_repeat_days)) == len(auto_repeat_days): repeated_days = get_repeated(auto_repeat_days) - frappe.throw(_('Auto Repeat Day {0} has been repeated.').format(frappe.bold(repeated_days))) + plural = "s" if len(repeated_days) > 1 else "" + + frappe.throw( + _("Auto Repeat Day{0} {1} has been repeated.").format( + plural, + frappe.bold(", ".join(repeated_days)) + ) + ) + def update_auto_repeat_id(self): #check if document is already on auto repeat From d603aaf6018d5156063713f35a699f2e196854ff Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 2 Jan 2022 23:05:33 +0530 Subject: [PATCH 13/29] fix: define is_syntax_error for postgres --- frappe/database/postgres/database.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/frappe/database/postgres/database.py b/frappe/database/postgres/database.py index 008635b1b3..c163b72d14 100644 --- a/frappe/database/postgres/database.py +++ b/frappe/database/postgres/database.py @@ -138,6 +138,10 @@ class PostgresDatabase(Database): # http://initd.org/psycopg/docs/extensions.html?highlight=datatype#psycopg2.extensions.QueryCanceledError return isinstance(e, psycopg2.extensions.QueryCanceledError) + @staticmethod + def is_syntax_error(e): + return isinstance(e, psycopg2.errors.SyntaxError) + @staticmethod def is_table_missing(e): return getattr(e, 'pgcode', None) == '42P01' From 0fddbafd09f85a0754c40de534689d17fa359cbd Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 3 Jan 2022 17:42:11 +0530 Subject: [PATCH 14/29] fix(postgres)!: dont silently rollback on db exception --- frappe/database/database.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index a157343be6..856c85416f 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -164,10 +164,7 @@ class Database(object): frappe.errprint(("Execution time: {0} sec").format(round(time_end - time_start, 2))) except Exception as e: - if frappe.conf.db_type == 'postgres': - self.rollback() - - elif self.is_syntax_error(e): + if self.is_syntax_error(e): # only for mariadb frappe.errprint('Syntax error in query:') frappe.errprint(query) @@ -178,6 +175,9 @@ class Database(object): elif self.is_timedout(e): raise frappe.QueryTimeoutError(e) + elif frappe.conf.db_type == 'postgres': + raise + if ignore_ddl and (self.is_missing_column(e) or self.is_missing_table(e) or self.cant_drop_field_or_key(e)): pass else: From 5ac79925ef64d904bfd1df373e4d701e221948d9 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 4 Jan 2022 11:24:28 +0530 Subject: [PATCH 15/29] fix: postgres install that doesn't abort transactions --- frappe/core/doctype/doctype/doctype.py | 2 +- frappe/utils/install.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/core/doctype/doctype/doctype.py b/frappe/core/doctype/doctype/doctype.py index ad0c3e8e6f..cb62914fa0 100644 --- a/frappe/core/doctype/doctype/doctype.py +++ b/frappe/core/doctype/doctype/doctype.py @@ -1283,7 +1283,7 @@ def make_module_and_roles(doc, perm_fieldname="permissions"): roles = [p.role for p in doc.get("permissions") or []] + default_roles for role in list(set(roles)): - if not frappe.db.exists("Role", role): + if frappe.db.table_exists("Role") and not frappe.db.exists("Role", role): r = frappe.get_doc(dict(doctype= "Role", role_name=role, desk_access=1)) r.flags.ignore_mandatory = r.flags.ignore_permissions = True r.insert() diff --git a/frappe/utils/install.py b/frappe/utils/install.py index bdc5fdfbc5..5ca8c4878a 100644 --- a/frappe/utils/install.py +++ b/frappe/utils/install.py @@ -5,11 +5,11 @@ import getpass from frappe.utils.password import update_password def before_install(): + frappe.reload_doc("core", "doctype", "doctype_state") frappe.reload_doc("core", "doctype", "docfield") frappe.reload_doc("core", "doctype", "docperm") frappe.reload_doc("core", "doctype", "doctype_action") frappe.reload_doc("core", "doctype", "doctype_link") - frappe.reload_doc("core", "doctype", "doctype_state") frappe.reload_doc("desk", "doctype", "form_tour_step") frappe.reload_doc("desk", "doctype", "form_tour") frappe.reload_doc("core", "doctype", "doctype") From 43364cf89b5a9755f234c08433f1146daa1a30c2 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 4 Jan 2022 11:47:50 +0530 Subject: [PATCH 16/29] fix!: use repeatable read isolation level RR isolation is default in MariaDB, for sake of consistency use same isolation level in postgres --- frappe/database/postgres/database.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/database/postgres/database.py b/frappe/database/postgres/database.py index c163b72d14..e802b4f880 100644 --- a/frappe/database/postgres/database.py +++ b/frappe/database/postgres/database.py @@ -3,7 +3,7 @@ from typing import List, Tuple, Union import psycopg2 import psycopg2.extensions -from psycopg2.extensions import ISOLATION_LEVEL_AUTOCOMMIT +from psycopg2.extensions import ISOLATION_LEVEL_REPEATABLE_READ from psycopg2.errorcodes import STRING_DATA_RIGHT_TRUNCATION import frappe @@ -69,7 +69,7 @@ class PostgresDatabase(Database): conn = psycopg2.connect("host='{}' dbname='{}' user='{}' password='{}' port={}".format( self.host, self.user, self.user, self.password, self.port )) - conn.set_isolation_level(ISOLATION_LEVEL_AUTOCOMMIT) # TODO: Remove this + conn.set_isolation_level(ISOLATION_LEVEL_REPEATABLE_READ) return conn From 5fac8bb9653b0a5d80a431fee649c43aa9758b62 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 4 Jan 2022 12:36:23 +0530 Subject: [PATCH 17/29] test: don't hardcode test password --- frappe/tests/test_frappe_client.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/frappe/tests/test_frappe_client.py b/frappe/tests/test_frappe_client.py index 66e1160eea..e84163eb41 100644 --- a/frappe/tests/test_frappe_client.py +++ b/frappe/tests/test_frappe_client.py @@ -10,8 +10,9 @@ import requests import base64 class TestFrappeClient(unittest.TestCase): + PASSWORD = "admin" def test_insert_many(self): - server = FrappeClient(get_url(), "Administrator", "admin", verify=False) + server = FrappeClient(get_url(), "Administrator", self.PASSWORD, verify=False) frappe.db.delete("Note", {"title": ("in", ('Sing','a','song','of','sixpence'))}) frappe.db.commit() @@ -30,7 +31,7 @@ class TestFrappeClient(unittest.TestCase): self.assertTrue(frappe.db.get_value('Note', {'title': 'sixpence'})) def test_create_doc(self): - server = FrappeClient(get_url(), "Administrator", "admin", verify=False) + server = FrappeClient(get_url(), "Administrator", self.PASSWORD, verify=False) frappe.db.delete("Note", {"title": "test_create"}) frappe.db.commit() @@ -39,13 +40,13 @@ class TestFrappeClient(unittest.TestCase): self.assertTrue(frappe.db.get_value('Note', {'title': 'test_create'})) def test_list_docs(self): - server = FrappeClient(get_url(), "Administrator", "admin", verify=False) + server = FrappeClient(get_url(), "Administrator", self.PASSWORD, verify=False) doc_list = server.get_list("Note") self.assertTrue(len(doc_list)) def test_get_doc(self): - server = FrappeClient(get_url(), "Administrator", "admin", verify=False) + server = FrappeClient(get_url(), "Administrator", self.PASSWORD, verify=False) frappe.db.delete("Note", {"title": "get_this"}) frappe.db.commit() @@ -56,7 +57,7 @@ class TestFrappeClient(unittest.TestCase): self.assertTrue(doc) def test_get_value(self): - server = FrappeClient(get_url(), "Administrator", "admin", verify=False) + server = FrappeClient(get_url(), "Administrator", self.PASSWORD, verify=False) frappe.db.delete("Note", {"title": "get_value"}) frappe.db.commit() @@ -74,14 +75,14 @@ class TestFrappeClient(unittest.TestCase): self.assertRaises(FrappeException, server.get_value, "Note", "(select (password) from(__Auth) order by name desc limit 1)", {"title": "get_value"}) def test_get_single(self): - server = FrappeClient(get_url(), "Administrator", "admin", verify=False) + server = FrappeClient(get_url(), "Administrator", self.PASSWORD, verify=False) server.set_value('Website Settings', 'Website Settings', 'title_prefix', 'test-prefix') self.assertEqual(server.get_value('Website Settings', 'title_prefix', 'Website Settings').get('title_prefix'), 'test-prefix') self.assertEqual(server.get_value('Website Settings', 'title_prefix').get('title_prefix'), 'test-prefix') frappe.db.set_value('Website Settings', None, 'title_prefix', '') def test_update_doc(self): - server = FrappeClient(get_url(), "Administrator", "admin", verify=False) + server = FrappeClient(get_url(), "Administrator", self.PASSWORD, verify=False) frappe.db.delete("Note", {"title": ("in", ("Sing", "sing"))}) frappe.db.commit() @@ -93,7 +94,7 @@ class TestFrappeClient(unittest.TestCase): self.assertTrue(doc["title"] == changed_title) def test_update_child_doc(self): - server = FrappeClient(get_url(), "Administrator", "admin", verify=False) + server = FrappeClient(get_url(), "Administrator", self.PASSWORD, verify=False) frappe.db.delete("Contact", {"first_name": "George", "last_name": "Steevens"}) frappe.db.delete("Contact", {"first_name": "William", "last_name": "Shakespeare"}) frappe.db.delete("Communication", {"reference_doctype": "Event"}) @@ -130,7 +131,7 @@ class TestFrappeClient(unittest.TestCase): self.assertTrue(frappe.db.exists("Communication Link", {"link_name": "William Shakespeare"})) def test_delete_doc(self): - server = FrappeClient(get_url(), "Administrator", "admin", verify=False) + server = FrappeClient(get_url(), "Administrator", self.PASSWORD, verify=False) frappe.db.delete("Note", {"title": "delete"}) frappe.db.commit() From 08464553f309370aa5d0dd138291a58bfa5f4783 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 4 Jan 2022 13:00:57 +0530 Subject: [PATCH 18/29] fix: define check_transaction_status for postgres To implement repeatable reads it has to respect transactions. --- frappe/database/database.py | 9 ++++++--- frappe/database/postgres/database.py | 4 ++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 856c85416f..30689d44aa 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -267,9 +267,7 @@ class Database(object): """Raises exception if more than 20,000 `INSERT`, `UPDATE` queries are executed in one transaction. This is to ensure that writes are always flushed otherwise this could cause the system to hang.""" - if self.transaction_writes and \ - query and query.strip().split()[0].lower() in ['start', 'alter', 'drop', 'create', "begin", "truncate"]: - raise Exception('This statement can cause implicit commit') + self.check_implicit_commit(query) if query and query.strip().lower() in ('commit', 'rollback'): self.transaction_writes = 0 @@ -282,6 +280,11 @@ class Database(object): else: frappe.throw(_("Too many writes in one request. Please send smaller requests"), frappe.ValidationError) + def check_implicit_commit(self, query): + if self.transaction_writes and \ + query and query.strip().split()[0].lower() in ['start', 'alter', 'drop', 'create', "begin", "truncate"]: + raise Exception('This statement can cause implicit commit') + def fetch_as_dict(self, formatted=0, as_utf8=0): """Internal. Converts results to dict.""" result = self._cursor.fetchall() diff --git a/frappe/database/postgres/database.py b/frappe/database/postgres/database.py index e802b4f880..9fd033768d 100644 --- a/frappe/database/postgres/database.py +++ b/frappe/database/postgres/database.py @@ -259,8 +259,8 @@ class PostgresDatabase(Database): key=key ) - def check_transaction_status(self, query): - pass + def check_implicit_commit(self, query): + pass # postgres can run DDL in transactions without implicit commits def has_index(self, table_name, index_name): return self.sql("""SELECT 1 FROM pg_indexes WHERE tablename='{table_name}' From da9d07474af65c1fd43276d019076e51433762dc Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 4 Jan 2022 13:19:56 +0530 Subject: [PATCH 19/29] test: rollback tests that are aborting transactions --- frappe/core/doctype/doctype/test_doctype.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/frappe/core/doctype/doctype/test_doctype.py b/frappe/core/doctype/doctype/test_doctype.py index 4362a52c34..12c227464d 100644 --- a/frappe/core/doctype/doctype/test_doctype.py +++ b/frappe/core/doctype/doctype/test_doctype.py @@ -15,6 +15,10 @@ from frappe.core.doctype.doctype.doctype import (UniqueFieldnameError, # test_records = frappe.get_test_records('DocType') class TestDocType(unittest.TestCase): + + def tearDown(self): + frappe.db.rollback() + def test_validate_name(self): self.assertRaises(frappe.NameError, new_doctype("_Some DocType").insert) self.assertRaises(frappe.NameError, new_doctype("8Some DocType").insert) @@ -42,6 +46,7 @@ class TestDocType(unittest.TestCase): doc1.insert() self.assertRaises(frappe.UniqueValidationError, doc2.insert) + frappe.db.rollback() dt.fields[0].unique = 0 dt.save() From 4b9d8c025850a72e779e066be9858ec15da372b2 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 4 Jan 2022 15:31:08 +0530 Subject: [PATCH 20/29] fix: Remove extra Document.validate_owner validation --- frappe/model/document.py | 6 ------ frappe/tests/test_document.py | 19 ------------------- 2 files changed, 25 deletions(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index f199c96acd..e25469c68a 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -504,7 +504,6 @@ class Document(BaseDocument): self._sanitize_content() self._save_passwords() self.validate_workflow() - self.validate_owner() children = self.get_all_children() for d in children: @@ -547,11 +546,6 @@ class Document(BaseDocument): if not self._action == 'save': set_workflow_state_on_action(self, workflow, self._action) - def validate_owner(self): - """Validate if the owner of the Document has changed""" - if not self.is_new() and self.has_value_changed('owner'): - frappe.throw(_('Document owner cannot be changed')) - def validate_set_only_once(self): """Validate that fields are not changed if not in insert""" set_only_once_fields = self.meta.get_set_only_once_fields() diff --git a/frappe/tests/test_document.py b/frappe/tests/test_document.py index 46638f5bf2..29cec8b230 100644 --- a/frappe/tests/test_document.py +++ b/frappe/tests/test_document.py @@ -252,22 +252,3 @@ class TestDocument(unittest.TestCase): 'currency': 100000 }) self.assertEquals(d.get_formatted('currency', currency='INR', format="#,###.##"), '₹ 100,000.00') - - def test_owner_changed(self): - frappe.delete_doc_if_exists("User", "hello@example.com") - frappe.set_user("Administrator") - - d = frappe.get_doc({ - "doctype": "User", - "email": "hello@example.com", - "first_name": "John" - }) - d.insert() - self.assertEqual(frappe.db.get_value("User", d.owner), d.owner) - - d.set("owner", "johndoe@gmail.com") - self.assertRaises(frappe.ValidationError, d.save) - - d.reload() - d.save() - self.assertEqual(frappe.db.get_value("User", d.owner), d.owner) From 9ed95a2d75b2d36be0fa6bc53ed072ad49d2b0d8 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 4 Jan 2022 15:32:26 +0530 Subject: [PATCH 21/29] chore: Update caniuse-lite package --- yarn.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/yarn.lock b/yarn.lock index d5bc1d669f..ba58f6b719 100644 --- a/yarn.lock +++ b/yarn.lock @@ -554,9 +554,9 @@ caniuse-api@^3.0.0: lodash.uniq "^4.5.0" caniuse-lite@^1.0.0, caniuse-lite@^1.0.30001109, caniuse-lite@^1.0.30001196, caniuse-lite@^1.0.30001219: - version "1.0.30001272" - resolved "https://registry.npmjs.org/caniuse-lite/-/caniuse-lite-1.0.30001272.tgz" - integrity sha512-DV1j9Oot5dydyH1v28g25KoVm7l8MTxazwuiH3utWiAS6iL/9Nh//TGwqFEeqqN8nnWYQ8HHhUq+o4QPt9kvYw== + version "1.0.30001296" + resolved "https://registry.npmjs.org/caniuse-lite/-/caniuse-lite-1.0.30001296.tgz" + integrity sha512-WfrtPEoNSoeATDlf4y3QvkwiELl9GyPLISV5GejTbbQRtQx4LhsXmc9IQ6XCL2d7UxCyEzToEZNMeqR79OUw8Q== caseless@~0.12.0: version "0.12.0" From 22dd06101b25ace52f562624d8653735cf9df1dd Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Tue, 4 Jan 2022 17:19:10 +0530 Subject: [PATCH 22/29] fix(blog): Load more with category filter When you click "Load more" on a blog category page, it will now fetch blog posts for that category --- .../blog_post/templates/blog_post_list.html | 36 ++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/frappe/website/doctype/blog_post/templates/blog_post_list.html b/frappe/website/doctype/blog_post/templates/blog_post_list.html index 2b3d5e250c..1aa30316fe 100644 --- a/frappe/website/doctype/blog_post/templates/blog_post_list.html +++ b/frappe/website/doctype/blog_post/templates/blog_post_list.html @@ -52,5 +52,39 @@ {% endblock %} {% block script %} - + {% endblock %} From dc2a99b8d671e21f2a50765c3ef950a73fec883d Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 4 Jan 2022 18:57:49 +0530 Subject: [PATCH 23/29] refactor(minor): Use pluck instead of re-iteration --- frappe/desk/doctype/todo/todo.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/frappe/desk/doctype/todo/todo.py b/frappe/desk/doctype/todo/todo.py index eabb28a6f3..e689faafbe 100644 --- a/frappe/desk/doctype/todo/todo.py +++ b/frappe/desk/doctype/todo/todo.py @@ -69,15 +69,13 @@ class ToDo(Document): return try: - assignments = [d[0] for d in frappe.get_all("ToDo", - filters={ - "reference_type": self.reference_type, - "reference_name": self.reference_name, - "status": ("!=", "Cancelled") - }, - fields=["allocated_to"], as_list=True)] - + assignments = frappe.get_all("ToDo", filters={ + "reference_type": self.reference_type, + "reference_name": self.reference_name, + "status": ("!=", "Cancelled") + }, pluck="allocated_to") assignments.reverse() + frappe.db.set_value(self.reference_type, self.reference_name, "_assign", json.dumps(assignments), update_modified=False) From 1b57717058eb89f1f0b9b17ef462b3720dd43ce8 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 4 Jan 2022 18:58:17 +0530 Subject: [PATCH 24/29] fix: Return key name as owner for consistency --- frappe/desk/form/assign_to.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/frappe/desk/form/assign_to.py b/frappe/desk/form/assign_to.py index 7ea87b8d15..049d33c1ec 100644 --- a/frappe/desk/form/assign_to.py +++ b/frappe/desk/form/assign_to.py @@ -19,11 +19,11 @@ def get(args=None): if not args: args = frappe.local.form_dict - return frappe.get_all('ToDo', fields=['allocated_to', 'name'], filters=dict( - reference_type = args.get('doctype'), - reference_name = args.get('name'), - status = ('!=', 'Cancelled') - ), limit=5) + return frappe.get_all("ToDo", fields=["allocated_to as owner", "name"], filters={ + "reference_type": args.get("doctype"), + "reference_name": args.get("name"), + "status": ("!=", "Cancelled") + }, limit=5) @frappe.whitelist() def add(args=None): From 6e4aa52e2ecda5f8aaa61dad8461c1f519aa4053 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 4 Jan 2022 19:20:03 +0530 Subject: [PATCH 25/29] fix: Show assignments correctly for Forms --- frappe/desk/form/load.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/desk/form/load.py b/frappe/desk/form/load.py index 89e6598859..0e644c3cf5 100644 --- a/frappe/desk/form/load.py +++ b/frappe/desk/form/load.py @@ -253,7 +253,7 @@ def get_communication_data(doctype, name, start=0, limit=20, after=None, fields= def get_assignments(dt, dn): cl = frappe.get_all("ToDo", - fields=['name', 'owner', 'description', 'status'], + fields=['name', 'allocated_to as owner', 'description', 'status'], filters={ 'reference_type': dt, 'reference_name': dn, From f6d069730981f84c08dfe17279a8d67b83740f69 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Tue, 4 Jan 2022 19:54:41 +0530 Subject: [PATCH 26/29] test: Fix test_assign --- frappe/tests/test_assign.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/tests/test_assign.py b/frappe/tests/test_assign.py index 971f9ce071..05bf7e2fb3 100644 --- a/frappe/tests/test_assign.py +++ b/frappe/tests/test_assign.py @@ -13,7 +13,7 @@ class TestAssign(unittest.TestCase): added = assign(todo, "test@example.com") - self.assertTrue("test@example.com" in [d.allocated_to for d in added]) + self.assertTrue("test@example.com" in [d.owner for d in added]) removed = frappe.desk.form.assign_to.remove(todo.doctype, todo.name, "test@example.com") From c99e576e1b7e12154e4f1b174a93ea0ecf80899c Mon Sep 17 00:00:00 2001 From: Christoph Kappel Date: Mon, 3 Jan 2022 22:51:29 +0100 Subject: [PATCH 27/29] fix: offer all (also modern) supported tls versions (PROTOCOL_TLS_CLIENT [1]) to LDAP endpoints instead of only (deprecated) PROTOCOL_TLSv1 [2] Background: Currently, when connecting to a ldap backend, ssl.PROTOCOL_TLSv1 [2] is offered as only option to the backend. This leads to following issues: - LDAP Backends that do not support TLSv1.0 (because of security reasons [3]) cannot be used in ERPNext - erpnext can ONLY connect to LDAP Backends offering the insecure [3] TLSv1.0 protocol (see ldap_settings.py ln: 61, 63) With this change to ssl.PROTOCOL_TLS_CLIENT we allow erpnext customers to configure LDAP Backends that also support more modern/secure (TLSv1.2 and up) transport while still ensure backwards compatibility and allowing TLSv1.0, since ssl.PROTOCOL_TLS "Auto-negotiates the highest protocol version that both the client and server support" [1] [1]: https://docs.python.org/3/library/ssl.html#ssl.PROTOCOL_TLS_CLIENT [2]: https://docs.python.org/3/library/ssl.html#ssl.PROTOCOL_TLSv1 [3]: https://tools.ietf.org/id/draft-ietf-tls-oldversions-deprecate-02.html --- frappe/integrations/doctype/ldap_settings/ldap_settings.py | 4 ++-- .../integrations/doctype/ldap_settings/test_ldap_settings.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/frappe/integrations/doctype/ldap_settings/ldap_settings.py b/frappe/integrations/doctype/ldap_settings/ldap_settings.py index 1c5abb454c..7c9c64ba3c 100644 --- a/frappe/integrations/doctype/ldap_settings/ldap_settings.py +++ b/frappe/integrations/doctype/ldap_settings/ldap_settings.py @@ -58,9 +58,9 @@ class LDAPSettings(Document): import ssl if self.require_trusted_certificate == 'Yes': - tls_configuration = ldap3.Tls(validate=ssl.CERT_REQUIRED, version=ssl.PROTOCOL_TLSv1) + tls_configuration = ldap3.Tls(validate=ssl.CERT_REQUIRED, version=ssl.PROTOCOL_TLS_CLIENT) else: - tls_configuration = ldap3.Tls(validate=ssl.CERT_NONE, version=ssl.PROTOCOL_TLSv1) + tls_configuration = ldap3.Tls(validate=ssl.CERT_NONE, version=ssl.PROTOCOL_TLS_CLIENT) if self.local_private_key_file: tls_configuration.private_key_file = self.local_private_key_file diff --git a/frappe/integrations/doctype/ldap_settings/test_ldap_settings.py b/frappe/integrations/doctype/ldap_settings/test_ldap_settings.py index 7b0638876b..41997fb4c7 100644 --- a/frappe/integrations/doctype/ldap_settings/test_ldap_settings.py +++ b/frappe/integrations/doctype/ldap_settings/test_ldap_settings.py @@ -296,7 +296,7 @@ class LDAP_TestCase(): if local_doc['require_trusted_certificate'] == 'Yes': tls_validate = ssl.CERT_REQUIRED - tls_version = ssl.PROTOCOL_TLSv1 + tls_version = ssl.PROTOCOL_TLS_CLIENT tls_configuration = ldap3.Tls(validate=tls_validate, version=tls_version) self.assertTrue(kwargs['auto_bind'] == ldap3.AUTO_BIND_TLS_BEFORE_BIND, @@ -304,7 +304,7 @@ class LDAP_TestCase(): else: tls_validate = ssl.CERT_NONE - tls_version = ssl.PROTOCOL_TLSv1 + tls_version = ssl.PROTOCOL_TLS_CLIENT tls_configuration = ldap3.Tls(validate=tls_validate, version=tls_version) self.assertTrue(kwargs['auto_bind'], From 80d456ef7e9f0b96ac5081a11545773c2bc398c2 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 5 Jan 2022 11:53:03 +0530 Subject: [PATCH 28/29] fix: avoid cached results for `table_exists` during install --- frappe/core/doctype/doctype/doctype.py | 2 +- frappe/database/database.py | 4 ++-- frappe/database/postgres/database.py | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/frappe/core/doctype/doctype/doctype.py b/frappe/core/doctype/doctype/doctype.py index cb62914fa0..3754288145 100644 --- a/frappe/core/doctype/doctype/doctype.py +++ b/frappe/core/doctype/doctype/doctype.py @@ -1283,7 +1283,7 @@ def make_module_and_roles(doc, perm_fieldname="permissions"): roles = [p.role for p in doc.get("permissions") or []] + default_roles for role in list(set(roles)): - if frappe.db.table_exists("Role") and not frappe.db.exists("Role", role): + if frappe.db.table_exists("Role", cached=False) and not frappe.db.exists("Role", role): r = frappe.get_doc(dict(doctype= "Role", role_name=role, desk_access=1)) r.flags.ignore_mandatory = r.flags.ignore_permissions = True r.insert() diff --git a/frappe/database/database.py b/frappe/database/database.py index 30689d44aa..993e2eb51c 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -838,9 +838,9 @@ class Database(object): 'parent': dt }) - def table_exists(self, doctype): + def table_exists(self, doctype, cached=True): """Returns True if table for given doctype exists.""" - return ("tab" + doctype) in self.get_tables() + return ("tab" + doctype) in self.get_tables(cached=cached) def has_table(self, doctype): return self.table_exists(doctype) diff --git a/frappe/database/postgres/database.py b/frappe/database/postgres/database.py index 9fd033768d..0ce6fbb265 100644 --- a/frappe/database/postgres/database.py +++ b/frappe/database/postgres/database.py @@ -103,7 +103,7 @@ class PostgresDatabase(Database): return super(PostgresDatabase, self).sql(*args, **kwargs) - def get_tables(self): + def get_tables(self, cached=True): return [d[0] for d in self.sql("""select table_name from information_schema.tables where table_catalog='{0}' From ae5644c3e188522c5db2e0e73762f08f46961738 Mon Sep 17 00:00:00 2001 From: Saqib Ansari Date: Wed, 5 Jan 2022 14:23:44 +0530 Subject: [PATCH 29/29] fix: set `first_response_time` only if communication is sent --- frappe/core/doctype/communication/communication.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/frappe/core/doctype/communication/communication.py b/frappe/core/doctype/communication/communication.py index 3a78a6a599..96c8f271d9 100644 --- a/frappe/core/doctype/communication/communication.py +++ b/frappe/core/doctype/communication/communication.py @@ -488,10 +488,12 @@ def update_parent_document_on_communication(doc): def update_first_response_time(parent, communication): if parent.meta.has_field("first_response_time") and not parent.get("first_response_time"): if is_system_user(communication.sender): - first_responded_on = communication.creation - if parent.meta.has_field("first_responded_on") and communication.sent_or_received == "Sent": - parent.db_set("first_responded_on", first_responded_on) - parent.db_set("first_response_time", round(time_diff_in_seconds(first_responded_on, parent.creation), 2)) + if communication.sent_or_received == "Sent": + first_responded_on = communication.creation + if parent.meta.has_field("first_responded_on"): + parent.db_set("first_responded_on", first_responded_on) + first_response_time = round(time_diff_in_seconds(first_responded_on, parent.creation), 2) + parent.db_set("first_response_time", first_response_time) def set_avg_response_time(parent, communication): if parent.meta.has_field("avg_response_time") and communication.sent_or_received == "Sent":