Handle DTML errors in ZMI more gradefully. (#356)

Fixes #287.

CHANGES.rst

@@ -34,6 +34,12 @@ New features
   dependency.
   (`#307 <https://github.com/zopefoundation/Zope/pull/307>`_)
 
+- Render an error message when trying to save DTML code containing a
+  SyntaxError in ZMI of a DTMLMethod or DTMLDocument.
+
+- Render an error message when trying to upload a file without choosing one
+  in ZMI of a DTMLMethod or DTMLDocument.
+
 - Add support for Python 3.7.
 
 Bugfixes

src/App/dtml/manage_tabs.dtml

@@ -79,11 +79,11 @@
 		</nav>
 
 
-	<dtml-if manage_tabs_message
-		><div class="alert alert-success" role="alert">
-			<dtml-var manage_tabs_message newline_to_br html_quote
-			> (<dtml-var ZopeTime fmt="%Y-%m-%d %H:%M">)
-		</div>
+	<dtml-if manage_tabs_message>
+			<div class="alert alert-<dtml-var manage_tabs_type missing=success html_quote>" role="alert">
+				<dtml-var manage_tabs_message newline_to_br html_quote
+				> (<dtml-var ZopeTime fmt="%Y-%m-%d %H:%M">)
+			</div>
 	</dtml-if>
 
 </dtml-with>

src/OFS/DTMLDocument.py

@@ -42,6 +42,7 @@ class DTMLDocument(PropertyManager, DTMLMethod):
     """
     meta_type = 'DTML Document'
     zmi_icon = 'far fa-file-alt'
+    _locked_error_text = 'This document has been locked.'
 
     manage_options = DTMLMethod.manage_options
 
@@ -51,29 +52,6 @@ class DTMLDocument(PropertyManager, DTMLMethod):
         (change_dtml_documents, perms[1]) or perms
         for perms in DTMLMethod.__ac_permissions__])
 
-    def manage_upload(self, file='', REQUEST=None):
-        """ Replace the contents of the document with the text in 'file'.
-        """
-        self._validateProxy(REQUEST)
-        if self.wl_isLocked():
-            raise ResourceLockedError('This document has been locked.')
-
-        if REQUEST and not file:
-            raise ValueError('No file specified')
-
-        if hasattr(file, 'read'):
-            file = file.read()
-        if PY3 and isinstance(file, binary_type):
-            file = file.decode('utf-8')
-        if PY2 and isinstance(file, text_type):
-            file = file.encode('utf-8')
-
-        self.munge(file)
-        self.ZCacheable_invalidate()
-        if REQUEST:
-            message = "Content uploaded."
-            return self.manage_main(self, REQUEST, manage_tabs_message=message)
-
     def __call__(self, client=None, REQUEST={}, RESPONSE=None, **kw):
         """Render the document with the given client object.
 

src/OFS/DTMLMethod.py

@@ -24,6 +24,7 @@
 from Acquisition import Implicit
 from App.special_dtml import DTMLFile
 from App.special_dtml import HTML
+from DocumentTemplate.DT_Util import ParseError
 from DocumentTemplate.permissions import change_dtml_methods
 from DocumentTemplate.security import RestrictedDTML
 from OFS import bbb
@@ -67,6 +68,7 @@ class DTMLMethod(
     _proxy_roles = ()
     index_html = None  # Prevent accidental acquisition
     _cache_namespace_keys = ()
+    _locked_error_text = 'This DTML Method is locked.'
 
     security = ClassSecurityInfo()
     security.declareObjectProtected(View)
@@ -98,6 +100,10 @@ class DTMLMethod(
     # More reasonable default for content-type for http HEAD requests.
     default_content_type = 'text/html'
 
+    def errQuote(self, s):
+        # Quoting is done when rendering the error in the template.
+        return s
+
     @security.protected(View)
     def __call__(self, client=None, REQUEST={}, RESPONSE=None, **kw):
         """Render using the given client object
@@ -255,15 +261,23 @@ def manage_edit(self, data, title, SUBMIT='Change', REQUEST=None):
         """
         self._validateProxy(REQUEST)
         if self.wl_isLocked():
-            raise ResourceLockedError('This item is locked.')
+            raise ResourceLockedError(self._locked_error_text)
 
         self.title = str(title)
         if isinstance(data, TaintedString):
             data = data.quoted()
 
         if hasattr(data, 'read'):
             data = data.read()
-        self.munge(data)
+        try:
+            self.munge(data)
+        except ParseError as e:
+            if REQUEST:
+                return self.manage_main(
+                    self, REQUEST, manage_tabs_message=e,
+                    manage_tabs_type='warning')
+            else:
+                raise
         self.ZCacheable_invalidate()
         if REQUEST:
             message = "Saved changes."
@@ -277,10 +291,18 @@ def manage_upload(self, file='', REQUEST=None):
         """
         self._validateProxy(REQUEST)
         if self.wl_isLocked():
-            raise ResourceLockedError('This DTML Method is locked.')
-
-        if REQUEST and not file:
-            raise ValueError('No file specified')
+            if REQUEST is not None:
+                return self.manage_main(
+                    self, REQUEST,
+                    manage_tabs_message=self._locked_error_text,
+                    manage_tabs_type='warning')
+            raise ResourceLockedError(self._locked_error_text)
+
+        if REQUEST is not None and not file:
+            return self.manage_main(
+                self, REQUEST,
+                manage_tabs_message='No file specified',
+                manage_tabs_type='warning')
 
         if hasattr(file, 'read'):
             file = file.read()
@@ -291,8 +313,8 @@ def manage_upload(self, file='', REQUEST=None):
 
         self.munge(file)
         self.ZCacheable_invalidate()
-        if REQUEST:
-            message = "Saved changes."
+        if REQUEST is not None:
+            message = "Content uploaded."
             return self.manage_main(self, REQUEST, manage_tabs_message=message)
 
     def manage_haveProxy(self, r):

src/OFS/tests/test_DTMLMethod.py

@@ -1,6 +1,17 @@
 # -*- coding: utf-8 -*-
+import Testing.ZopeTestCase
+import Testing.testbrowser
+import Zope2.App.zcml
+import codecs
 import io
 import unittest
+import zExceptions
+
+
+def _lock_item(item):
+    from OFS.LockItem import LockItem
+    from AccessControl.SpecialUsers import nobody
+    item.wl_setLock('token', LockItem(nobody, token='token'))
 
 
 class DTMLMethodTests(unittest.TestCase):
@@ -59,6 +70,99 @@ def test_manage_upload__BytesIO(self):
         self.assertEqual(doc.read(), 'bÿtës')
         self.assertIsInstance(doc.read(), str)
 
+    def test_manage_upload__Locked(self):
+        """It raises an exception if the object is locked."""
+        doc = self._makeOne()
+        _lock_item(doc)
+        with self.assertRaises(zExceptions.ResourceLockedError) as err:
+            doc.manage_upload()
+        self.assertEqual('This DTML Method is locked.', str(err.exception))
+
+    def test_manage_edit__Locked(self):
+        """It raises an exception if the object is locked."""
+        doc = self._makeOne()
+        _lock_item(doc)
+        with self.assertRaises(zExceptions.ResourceLockedError) as err:
+            doc.manage_edit('data', 'title')
+        self.assertEqual('This DTML Method is locked.', str(err.exception))
+
+    def test_manage_edit__invalid_code(self):
+        """It raises an exception if the code is invalid."""
+        from DocumentTemplate.DT_Util import ParseError
+        doc = self._makeOne()
+        with self.assertRaises(ParseError) as err:
+            doc.manage_edit('</dtml-let>', 'title')
+        self.assertEqual(
+            'unexpected end tag, for tag </dtml-let>, on line 1 of <string>',
+            str(err.exception))
+
+
+class DTMLMethodBrowserTests(Testing.ZopeTestCase.FunctionalTestCase):
+    """Browser testing ..OFS.DTMLMethod"""
+
+    def setUp(self):
+        from OFS.DTMLMethod import addDTMLMethod
+        super(DTMLMethodBrowserTests, self).setUp()
+
+        Zope2.App.zcml.load_site(force=True)
+
+        uf = self.app.acl_users
+        uf.userFolderAddUser('manager', 'manager_pass', ['Manager'], [])
+        addDTMLMethod(self.app, 'dtml_meth')
+
+        self.browser = Testing.testbrowser.Browser()
+        self.browser.addHeader(
+            'Authorization',
+            'basic {}'.format(codecs.encode(
+                b'manager:manager_pass', 'base64').decode()))
+        self.browser.open('http://localhost/dtml_meth/manage_main')
+
+    def test_manage_upload__Locked__REQUEST(self):
+        """It renders an error message if the object is locked."""
+        _lock_item(self.app.dtml_meth)
+        file_contents = b'<dtml-var "Hello!">'
+        self.browser.getControl('file').add_file(
+            file_contents, 'text/plain', 'hello.dtml')
+        self.browser.getControl('Upload File').click()
+        self.assertIn('This DTML Method is locked.', self.browser.contents)
+        self.assertNotEqual(
+            self.browser.getControl(name='data:text').value, file_contents)
+
+    def test_manage_upload__no_file(self):
+        """It renders an error message if no file is uploaded."""
+        self.browser.getControl('Upload File').click()
+        self.assertIn('No file specified', self.browser.contents)
+
+    def test_manage_upload__file_uploaded(self):
+        """It renders a success message if a file is uploaded."""
+        file_contents = b'<dtml-var title_or_id>'
+        self.browser.getControl('file').add_file(
+            file_contents, 'text/plain', 'hello.dtml')
+        self.browser.getControl('Upload File').click()
+        self.assertIn('Content uploaded.', self.browser.contents)
+        self.assertEqual(
+            self.browser.getControl(name='data:text').value,
+            file_contents.decode())
+
+    def test_manage_edit__ParseError(self):
+        """It renders an error message if the DTML code is invalid."""
+        code = '</dtml-let>'
+        self.browser.getControl(name='data:text').value = code
+        self.browser.getControl('Save').click()
+        self.assertIn(
+            'unexpected end tag, for tag &lt;/dtml-let&gt;,'
+            ' on line 1 of dtml_meth', self.browser.contents)
+        # But the value gets stored:
+        self.assertEqual(self.browser.getControl(name='data:text').value, code)
+
+    def test_manage_edit__success(self):
+        """It renders a success message if the DTML code is valid."""
+        code = '<dtml-var title_or_id>'
+        self.browser.getControl(name='data:text').value = code
+        self.browser.getControl('Save').click()
+        self.assertIn('Saved changes.', self.browser.contents)
+        self.assertEqual(self.browser.getControl(name='data:text').value, code)
+
 
 class FactoryTests(unittest.TestCase):