From c02a7469aadff866ba0dd40ffbbc5e6d6dcefd48 Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Mon, 13 Nov 2017 17:17:20 +0530 Subject: [PATCH] [refactor] a better set-only-once implementation with child tables (#4475) * [refactor] a better set-only-once implementation with child tables * [refactor] document.is_child_table_same(fieldname) * [refactor] tests * [refactor] tests * [test] catch timeout reason * [minor] edit in full page more prominent * [minor] tests --- frappe/model/base_document.py | 10 ++- frappe/model/document.py | 72 +++++++++++++++++++-- frappe/model/meta.py | 6 ++ frappe/public/js/frappe/form/quick_entry.js | 14 ++-- frappe/tests/test_permissions.py | 37 +++++++++-- frappe/tests/ui/test_test_runner.py | 23 ++++--- 6 files changed, 130 insertions(+), 32 deletions(-) diff --git a/frappe/model/base_document.py b/frappe/model/base_document.py index a50f520425..0a33cbd0bb 100644 --- a/frappe/model/base_document.py +++ b/frappe/model/base_document.py @@ -3,6 +3,7 @@ from __future__ import unicode_literals from six import iteritems, string_types +import datetime import frappe, sys from frappe import _ from frappe.utils import (cint, flt, now, cstr, strip_html, getdate, get_datetime, to_timedelta, @@ -181,7 +182,7 @@ class BaseDocument(object): return value - def get_valid_dict(self, sanitize=True): + def get_valid_dict(self, sanitize=True, convert_dates_to_str=False): d = frappe._dict() for fieldname in self.meta.get_valid_columns(): d[fieldname] = self.get(fieldname) @@ -215,6 +216,9 @@ class BaseDocument(object): if isinstance(d[fieldname], list) and df.fieldtype != 'Table': frappe.throw(_('Value for {0} cannot be a list').format(_(df.label))) + if convert_dates_to_str and isinstance(d[fieldname], (datetime.datetime, datetime.time)): + d[fieldname] = str(d[fieldname]) + return d def init_valid_columns(self): @@ -244,8 +248,8 @@ class BaseDocument(object): def is_new(self): return self.get("__islocal") - def as_dict(self, no_nulls=False, no_default_fields=False): - doc = self.get_valid_dict() + def as_dict(self, no_nulls=False, no_default_fields=False, convert_dates_to_str=False): + doc = self.get_valid_dict(convert_dates_to_str=convert_dates_to_str) doc["doctype"] = self.doctype for df in self.meta.get_table_fields(): children = self.get(df.fieldname) or [] diff --git a/frappe/model/document.py b/frappe/model/document.py index 6d11a7607d..cdf2a20223 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -357,7 +357,10 @@ class Document(BaseDocument): def get_doc_before_save(self): if not getattr(self, '_doc_before_save', None): - self._doc_before_save = frappe.get_doc(self.doctype, self.name) + try: + self._doc_before_save = frappe.get_doc(self.doctype, self.name) + except frappe.DoesNotExistError: + self._doc_before_save = None return self._doc_before_save def set_new_name(self, force=False): @@ -435,7 +438,6 @@ class Document(BaseDocument): def _validate(self): self._validate_mandatory() self._validate_selects() - self._validate_constants() self._validate_length() self._extract_images_from_text_editor() self._sanitize_content() @@ -444,17 +446,67 @@ class Document(BaseDocument): children = self.get_all_children() for d in children: d._validate_selects() - d._validate_constants() d._validate_length() d._extract_images_from_text_editor() d._sanitize_content() d._save_passwords() + self.validate_set_only_once() + if self.is_new(): # don't set fields like _assign, _comments for new doc for fieldname in optional_fields: self.set(fieldname, None) + 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() + + if set_only_once_fields and self._doc_before_save: + # document exists before saving + for field in set_only_once_fields: + fail = False + value = self.get(field.fieldname) + original_value = self._doc_before_save.get(field.fieldname) + + if field.fieldtype=='Table': + fail = not self.is_child_table_same(field.fieldname) + elif field.fieldtype in ('Date', 'Datetime', 'Time'): + fail = str(value) != str(original_value) + else: + fail = value != original_value + + if fail: + frappe.throw(_("Value cannot be changed for {0}").format(self.meta.get_label(field.fieldname)), + frappe.CannotChangeConstantError) + + return False + + def is_child_table_same(self, fieldname): + '''Validate child table is same as original table before saving''' + value = self.get(fieldname) + original_value = self._doc_before_save.get(fieldname) + same = True + + if len(original_value) != len(value): + same = False + else: + # check all child entries + for i, d in enumerate(original_value): + new_child = value[i].as_dict(convert_dates_to_str = True) + original_child = d.as_dict(convert_dates_to_str = True) + + # all fields must be same other than modified and modified_by + for key in ('modified', 'modified_by', 'creation'): + del new_child[key] + del original_child[key] + + if original_child != new_child: + same = False + break + + return same + def apply_fieldlevel_read_permissions(self): '''Remove values the user is not allowed to read (called when loading in desk)''' has_higher_permlevel = False @@ -798,10 +850,7 @@ class Document(BaseDocument): self.set_title_field() self.reset_seen() - self._doc_before_save = None - if not self.is_new() and getattr(self.meta, 'track_changes', False): - self.get_doc_before_save() - + self.load_doc_before_save() if self.flags.ignore_validate: return @@ -816,6 +865,15 @@ class Document(BaseDocument): elif self._action=="update_after_submit": self.run_method("before_update_after_submit") + def load_doc_before_save(self): + '''Save load document from db before saving''' + self._doc_before_save = None + if not (self.is_new() + and (getattr(self.meta, 'track_changes', False) + or self.meta.get_set_only_once_fields())): + self.get_doc_before_save() + + def run_post_save_methods(self): """Run standard methods after `INSERT` or `UPDATE`. Standard Methods are: diff --git a/frappe/model/meta.py b/frappe/model/meta.py index 81f5708dc8..6b9654c1f4 100644 --- a/frappe/model/meta.py +++ b/frappe/model/meta.py @@ -96,6 +96,12 @@ class Meta(Document): def get_image_fields(self): return self.get("fields", {"fieldtype": "Attach Image"}) + def get_set_only_once_fields(self): + '''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}) + return self._set_only_once_fields + def get_table_fields(self): if not hasattr(self, "_table_fields"): if self.name!="DocType": diff --git a/frappe/public/js/frappe/form/quick_entry.js b/frappe/public/js/frappe/form/quick_entry.js index c63730599c..9aa0d0099e 100644 --- a/frappe/public/js/frappe/form/quick_entry.js +++ b/frappe/public/js/frappe/form/quick_entry.js @@ -152,7 +152,7 @@ frappe.ui.form.QuickEntryForm = Class.extend({ if(me.after_insert) { me.after_insert(me.dialog.doc); } else { - me.open_from_if_not_list(); + me.open_form_if_not_list(); } } }, @@ -168,11 +168,13 @@ frappe.ui.form.QuickEntryForm = Class.extend({ }); }, - open_from_if_not_list: function() { + open_form_if_not_list: function() { let route = frappe.get_route(); let doc = this.dialog.doc; - if(route && !(route[0]==='List' && route[1]===doc.doctype)) { - frappe.set_route('Form', doc.doctype, doc.name); + if (route && !(route[0]==='List' && route[1]===doc.doctype)) { + frappe.run_serially([ + () => frappe.set_route('Form', doc.doctype, doc.name) + ]); } }, @@ -199,8 +201,8 @@ frappe.ui.form.QuickEntryForm = Class.extend({ render_edit_in_full_page_link: function(){ var me = this; - var $link = $('
' + - __("Ctrl+enter to save") + ' | ' + __("Edit in full page") + '
').appendTo(this.dialog.body); + var $link = $('
' + + '
').appendTo(this.dialog.body); $link.find('.edit-full').on('click', function() { // edit in form diff --git a/frappe/tests/test_permissions.py b/frappe/tests/test_permissions.py index fecae8c6aa..7a03943016 100644 --- a/frappe/tests/test_permissions.py +++ b/frappe/tests/test_permissions.py @@ -56,11 +56,6 @@ class TestPermissions(unittest.TestCase): clear_user_permissions_for_doctype("Contact") clear_user_permissions_for_doctype("Salutation") - reset('Blogger') - reset('Blog Post') - reset('Contact') - reset('Salutation') - self.set_ignore_user_permissions_if_missing(0) @staticmethod @@ -197,12 +192,42 @@ class TestPermissions(unittest.TestCase): def test_set_only_once(self): blog_post = frappe.get_meta("Blog Post") - blog_post.get_field("title").set_only_once = 1 doc = frappe.get_doc("Blog Post", "-test-blog-post-1") + doc.db_set('title', 'Old') + blog_post.get_field("title").set_only_once = 1 doc.title = "New" self.assertRaises(frappe.CannotChangeConstantError, doc.save) blog_post.get_field("title").set_only_once = 0 + def test_set_only_once_child_table_rows(self): + doctype_meta = frappe.get_meta("DocType") + doctype_meta.get_field("fields").set_only_once = 1 + doc = frappe.get_doc("DocType", "Blog Post") + + # remove last one + doc.fields = doc.fields[:-1] + self.assertRaises(frappe.CannotChangeConstantError, doc.save) + frappe.clear_cache(doctype='DocType') + + def test_set_only_once_child_table_row_value(self): + doctype_meta = frappe.get_meta("DocType") + doctype_meta.get_field("fields").set_only_once = 1 + doc = frappe.get_doc("DocType", "Blog Post") + + # change one property from the child table + doc.fields[-1].fieldtype = 'HTML' + self.assertRaises(frappe.CannotChangeConstantError, doc.save) + frappe.clear_cache(doctype='DocType') + + def test_set_only_once_child_table_okay(self): + doctype_meta = frappe.get_meta("DocType") + doctype_meta.get_field("fields").set_only_once = 1 + doc = frappe.get_doc("DocType", "Blog Post") + + doc.load_doc_before_save() + self.assertFalse(doc.validate_set_only_once()) + frappe.clear_cache(doctype='DocType') + def test_user_permission_doctypes(self): add_user_permission("Blog Category", "_Test Blog Category 1", "test2@example.com") diff --git a/frappe/tests/ui/test_test_runner.py b/frappe/tests/ui/test_test_runner.py index 6607f995eb..5cc31af091 100644 --- a/frappe/tests/ui/test_test_runner.py +++ b/frappe/tests/ui/test_test_runner.py @@ -18,6 +18,7 @@ class TestTestRunner(unittest.TestCase): continue timeout = 60 + passed = False if '#' in test: test, comment = test.split('#') test = test.strip() @@ -30,17 +31,19 @@ class TestTestRunner(unittest.TestCase): frappe.db.commit() driver.refresh() driver.set_route('Form', 'Test Runner') - driver.click_primary_action() - driver.wait_for('#frappe-qunit-done', timeout=timeout) + try: + driver.click_primary_action() + driver.wait_for('#frappe-qunit-done', timeout=timeout) - console = driver.get_console() - passed = 'Tests Passed' in console - if frappe.flags.tests_verbose or not passed: - for line in console: - print(line) - print('-' * 40) - self.assertTrue(passed) - time.sleep(1) + console = driver.get_console() + passed = 'Tests Passed' in console + finally: + if frappe.flags.tests_verbose or not passed: + for line in console: + print(line) + print('-' * 40) + self.assertTrue(passed) + time.sleep(1) frappe.db.set_default('in_selenium', None) driver.close()