[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

@@ -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 []

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:
 

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":

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 = $('<div class="text-muted small" style="padding-left: 10px; padding-top: 15px;">' +
-			__("Ctrl+enter to save") + ' | <a class="edit-full">' + __("Edit in full page") + '</a></div>').appendTo(this.dialog.body);
+		var $link = $('<div style="padding-left: 7px; padding-top: 15px; padding-bottom: 10px;">' +
+			'<button class="edit-full btn-default btn-sm">' + __("Edit in full page") + '</button></div>').appendTo(this.dialog.body);
 
 		$link.find('.edit-full').on('click', function() {
 			// edit in form

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")

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()