Merge pull request #1567 from anandpdoshi/fix/add-comments-column-after-request

[fix] add _comments column after request

frappe/__init__.py

@@ -1047,7 +1047,7 @@ def attach_print(doctype, name, file_name=None, print_format=None, style=None, h
 	return out
 
 logging_setup_complete = False
-def get_logger(module=None):
+def get_logger(module=None, loglevel="DEBUG"):
 	from frappe.setup_logging import setup_logging
 	global logging_setup_complete
 
@@ -1055,7 +1055,10 @@ def get_logger(module=None):
 		setup_logging()
 		logging_setup_complete = True
 
-	return logging.getLogger(module or "frappe")
+	logger = logging.getLogger(module or "frappe")
+	logger.setLevel(logging.DEBUG)
+
+	return logger
 
 def publish_realtime(*args, **kwargs):
 	"""Publish real-time updates

frappe/app.py

@@ -13,7 +13,6 @@ from werkzeug.contrib.profiler import ProfilerMiddleware
 from werkzeug.wsgi import SharedDataMiddleware
 from werkzeug.serving import run_with_reloader
 
-
 import frappe
 import frappe.handler
 import frappe.auth
@@ -23,8 +22,8 @@ import frappe.utils.response
 import frappe.website.render
 from frappe.utils import get_site_name, get_site_path
 from frappe.middlewares import StaticDataMiddleware
-
 from frappe.utils.error import make_error_snapshot
+from frappe.core.doctype.communication.comment import update_comments_in_parent_after_request
 
 local_manager = LocalManager([frappe.local])
 
@@ -39,10 +38,7 @@ class RequestContext(object):
 		self.request = Request(environ)
 
 	def __enter__(self):
-		frappe.local.request = self.request
-		init_site(self.request)
-		make_form_dict(self.request)
-		frappe.local.http_request = frappe.auth.HTTPRequest()
+		init_request(self.request)
 
 	def __exit__(self, type, value, traceback):
 		frappe.destroy()
@@ -50,20 +46,12 @@ class RequestContext(object):
 
 @Request.application
 def application(request):
-	frappe.local.request = request
-	frappe.local.is_ajax = frappe.get_request_header("X-Requested-With")=="XMLHttpRequest"
 	response = None
 
 	try:
 		rollback = True
 
-		init_site(request)
-
-		if frappe.local.conf.get('maintenance_mode'):
-			raise frappe.SessionStopped
-
-		make_form_dict(request)
-		frappe.local.http_request = frappe.auth.HTTPRequest()
+		init_request(request)
 
 		if frappe.local.form_dict.cmd:
 			response = frappe.handler.handle()
@@ -90,52 +78,10 @@ def application(request):
 		response = frappe.utils.response.handle_session_stopped()
 
 	except Exception, e:
-		http_status_code = getattr(e, "http_status_code", 500)
-
-		if (http_status_code==500
-			and isinstance(e, MySQLdb.OperationalError)
-			and e.args[0] in (1205, 1213)):
-				# 1205 = lock wait timeout
-				# 1213 = deadlock
-				# code 409 represents conflict
-				http_status_code = 508
-
-		if frappe.local.is_ajax or 'application/json' in request.headers.get('Accept', ''):
-			response = frappe.utils.response.report_error(http_status_code)
-		else:
-			traceback = "<pre>"+frappe.get_traceback()+"</pre>"
-			if frappe.local.flags.disable_traceback:
-				traceback = ""
-
-			frappe.respond_as_web_page("Server Error",
-				traceback,
-				http_status_code=http_status_code)
-			response = frappe.website.render.render("message", http_status_code=http_status_code)
-
-		if e.__class__ == frappe.AuthenticationError:
-			if hasattr(frappe.local, "login_manager"):
-				frappe.local.login_manager.clear_cookies()
-
-		if http_status_code==500:
-			logger.error('Request Error')
-
-		make_error_snapshot(e)
+		response = handle_exception(e)
 
 	else:
-		if (frappe.local.request.method in ("POST", "PUT") or frappe.local.flags.commit) and frappe.db:
-			if frappe.db.transaction_writes:
-				frappe.db.commit()
-				rollback = False
-
-		# update session
-		if getattr(frappe.local, "session_obj", None):
-			updated_in_db = frappe.local.session_obj.update()
-			if updated_in_db:
-				frappe.db.commit()
-
-		# publish realtime
-		for args in frappe.local.realtime_log:
-			frappe.async.emit_via_redis(*args)
+		rollback = after_request(rollback)
 
 	finally:
 		if frappe.local.request.method in ("POST", "PUT") and frappe.db and rollback:
@@ -149,7 +95,10 @@ def application(request):
 
 	return response
 
-def init_site(request):
+def init_request(request):
+	frappe.local.request = request
+	frappe.local.is_ajax = frappe.get_request_header("X-Requested-With")=="XMLHttpRequest"
+
 	site = _site or request.headers.get('X-Frappe-Site-Name') or get_site_name(request.host)
 	frappe.init(site=site, sites_path=_sites_path)
 
@@ -157,6 +106,13 @@ def init_site(request):
 		# site does not exist
 		raise NotFound
 
+	if frappe.local.conf.get('maintenance_mode'):
+		raise frappe.SessionStopped
+
+	make_form_dict(request)
+
+	frappe.local.http_request = frappe.auth.HTTPRequest()
+
 def make_form_dict(request):
 	frappe.local.form_dict = frappe._dict({ k:v[0] if isinstance(v, (list, tuple)) else v \
 		for k, v in (request.form or request.args).iteritems() })
@@ -165,9 +121,62 @@ def make_form_dict(request):
 		# _ is passed by $.ajax so that the request is not cached by the browser. So, remove _ from form_dict
 		frappe.local.form_dict.pop("_")
 
-application = local_manager.make_middleware(application)
-application.debug = True
+def handle_exception(e):
+	http_status_code = getattr(e, "http_status_code", 500)
 
+	if (http_status_code==500
+		and isinstance(e, MySQLdb.OperationalError)
+		and e.args[0] in (1205, 1213)):
+			# 1205 = lock wait timeout
+			# 1213 = deadlock
+			# code 409 represents conflict
+			http_status_code = 508
+
+	if frappe.local.is_ajax or 'application/json' in frappe.local.request.headers.get('Accept', ''):
+		response = frappe.utils.response.report_error(http_status_code)
+	else:
+		traceback = "<pre>"+frappe.get_traceback()+"</pre>"
+		if frappe.local.flags.disable_traceback:
+			traceback = ""
+
+		frappe.respond_as_web_page("Server Error",
+			traceback,
+			http_status_code=http_status_code)
+		response = frappe.website.render.render("message", http_status_code=http_status_code)
+
+	if e.__class__ == frappe.AuthenticationError:
+		if hasattr(frappe.local, "login_manager"):
+			frappe.local.login_manager.clear_cookies()
+
+	if http_status_code==500:
+		logger.error('Request Error')
+
+	make_error_snapshot(e)
+
+	return response
+
+def after_request(rollback):
+	if (frappe.local.request.method in ("POST", "PUT") or frappe.local.flags.commit) and frappe.db:
+		if frappe.db.transaction_writes:
+			frappe.db.commit()
+			rollback = False
+
+	# update session
+	if getattr(frappe.local, "session_obj", None):
+		updated_in_db = frappe.local.session_obj.update()
+		if updated_in_db:
+			frappe.db.commit()
+			rollback = False
+
+	update_comments_in_parent_after_request()
+
+	# publish realtime
+	for args in frappe.local.realtime_log:
+		frappe.async.emit_via_redis(*args)
+
+	return rollback
+
+application = local_manager.make_middleware(application)
 
 def serve(port=8000, profile=False, site=None, sites_path='.'):
 	global application, _site, _sites_path

frappe/core/doctype/communication/comment.py

@@ -8,6 +8,7 @@ import json
 from frappe.core.doctype.user.user import extract_mentions
 from frappe.utils import get_fullname, get_link_to_form
 from frappe.website.render import clear_cache
+from frappe.model.db_schema import add_column
 
 def validate_comment(doc):
 	"""Raise exception for more than 50 comments."""
@@ -39,7 +40,7 @@ def on_trash(doc):
 		if c.get("name")==doc.name:
 			_comments.remove(c)
 
-	update_comments_in_parent(doc, _comments)
+	update_comments_in_parent(doc.reference_doctype, doc.reference_name, _comments)
 
 def update_comment_in_doc(doc):
 	"""Updates `_comments` (JSON) property in parent Document.
@@ -70,7 +71,7 @@ def update_comment_in_doc(doc):
 				"by": doc.sender or doc.owner,
 				"name": doc.name
 			})
-		update_comments_in_parent(doc, _comments)
+		update_comments_in_parent(doc.reference_doctype, doc.reference_name, _comments)
 
 def notify_mentions(doc):
 	if doc.communication_type != "Comment":
@@ -107,8 +108,9 @@ def get_comments_from_parent(doc):
 		_comments = frappe.db.get_value(doc.reference_doctype, doc.reference_name, "_comments") or "[]"
 
 	except Exception, e:
-		if e.args[0]==1146:
-			# no table
+		if e.args[0] in (1146, 1054):
+			# 1146 = no table
+			# 1054 = missing column
 			_comments = "[]"
 
 		else:
@@ -116,20 +118,31 @@ def get_comments_from_parent(doc):
 
 	return json.loads(_comments)
 
-def update_comments_in_parent(doc, _comments):
+def update_comments_in_parent(reference_doctype, reference_name, _comments):
 	"""Updates `_comments` property in parent Document with given dict.
 
 	:param _comments: Dict of comments."""
-	if not doc.reference_doctype or frappe.db.get_value("DocType", doc.reference_doctype, "issingle"):
+	if not reference_doctype or frappe.db.get_value("DocType", reference_doctype, "issingle"):
 		return
 
-	# use sql, so that we do not mess with the timestamp
-	frappe.db.sql("""update `tab%s` set `_comments`=%s where name=%s""" % (doc.reference_doctype,
-		"%s", "%s"), (json.dumps(_comments), doc.reference_name))
+	try:
+		# use sql, so that we do not mess with the timestamp
+		frappe.db.sql("""update `tab%s` set `_comments`=%s where name=%s""" % (reference_doctype,
+			"%s", "%s"), (json.dumps(_comments), reference_name))
 
-	reference_doc = frappe.get_doc(doc.reference_doctype, doc.reference_name)
-	if getattr(reference_doc, "get_route", None):
-		clear_cache(reference_doc.get_route())
+	except Exception, e:
+		if e.args[0] == 1054 and frappe.local.request:
+			# missing column and in request, add column and update after commit
+			frappe.local._comments = (getattr(frappe.local, "_comments", [])
+				+ [(reference_doctype, reference_name, _comments)])
+
+		else:
+			raise
+
+	else:
+		reference_doc = frappe.get_doc(reference_doctype, reference_name)
+		if getattr(reference_doc, "get_route", None):
+			clear_cache(reference_doc.get_route())
 
 def add_info_comment(**kwargs):
 	kwargs.update({
@@ -138,3 +151,12 @@ def add_info_comment(**kwargs):
 		"comment_type": "Info"
 	})
 	return frappe.get_doc(kwargs).insert(ignore_permissions=True)
+
+def update_comments_in_parent_after_request():
+	"""update _comments in parent if _comments column is missing"""
+	if hasattr(frappe.local, "_comments"):
+		for (reference_doctype, reference_name, _comments) in frappe.local._comments:
+			add_column(reference_doctype, "_comments", "Text")
+			update_comments_in_parent(reference_doctype, reference_name, _comments)
+
+		frappe.db.commit()

frappe/core/doctype/communication/communication.py

@@ -165,10 +165,6 @@ def on_doctype_update():
 	"""Add index in `tabCommunication` for `(reference_doctype, reference_name)`"""
 	frappe.db.add_index("Communication", ["reference_doctype", "reference_name"])
 
-	if "_liked_by" not in frappe.db.get_table_columns("Communication"):
-		add_column("Communication", "_liked_by", "Text")
-
-
 def has_permission(doc, ptype, user):
 	if ptype=="read" and doc.reference_doctype and doc.reference_name:
 		if frappe.has_permission(doc.reference_doctype, ptype="read", doc=doc.reference_name):

frappe/model/db_schema.py

@@ -579,6 +579,10 @@ def get_definition(fieldtype, precision=None, length=None):
 	return coltype
 
 def add_column(doctype, column_name, fieldtype, precision=None):
+	if column_name in frappe.db.get_table_columns(doctype):
+		# already exists
+		return
+
 	frappe.db.commit()
 	frappe.db.sql("alter table `tab%s` add column %s %s" % (doctype,
 		column_name, get_definition(fieldtype, precision)))

frappe/setup_logging.py

@@ -25,6 +25,7 @@ def setup_logging():
 		logging_conf = {
 			"version": 1,
 			"disable_existing_loggers": True,
+			"level": logging.DEBUG,
 			"filters": {
 				"context_filter": {
 					"()": "frappe.setup_logging.ContextFilter"