From 0ecd1cec2ee251d2fdb964aeb9791418c8b75aa4 Mon Sep 17 00:00:00 2001
From: Jens Vagelpohl <jens@netz.ooo>
Date: Tue, 11 May 2021 07:37:02 +0200
Subject: [PATCH] Provide friendlier ZMI error message for the Transaction Undo
 form (#969)

* - Provide friendlier ZMI error message for the Transaction Undo form

* - enclosing test in try...finally to always ensure the cleanup runs

* - fix long line
---
 CHANGES.rst                              |  3 +
 src/App/Undo.py                          | 14 ++++-
 src/App/dtml/undo.dtml                   |  4 +-
 src/App/tests/test_ApplicationManager.py | 76 ++++++++++++++++++++++++
 4 files changed, 95 insertions(+), 2 deletions(-)

diff --git a/CHANGES.rst b/CHANGES.rst
index ab8f87e8a2..942752d1d6 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -11,6 +11,9 @@ https://github.com/zopefoundation/Zope/blob/4.x/CHANGES.rst
 5.2 (unreleased)
 ----------------
 
+- Provide friendlier ZMI error message for the Transaction Undo form
+  (`#964 <https://github.com/zopefoundation/Zope/issues/964>`_)
+
 - Updated/fixed the poll application tutorial in the Zope Developers Guide
   (`#958 <https://github.com/zopefoundation/Zope/issues/958>`_)
 
diff --git a/src/App/Undo.py b/src/App/Undo.py
index 0a374c5f87..a162858f4d 100644
--- a/src/App/Undo.py
+++ b/src/App/Undo.py
@@ -110,8 +110,20 @@ def manage_undo_transactions(self, transaction_info=(), REQUEST=None):
                 descriptions.append(tid[-1])
 
         if tids:
-            transaction.get().note("Undo %s" % ' '.join(descriptions))
+            ts = transaction.get()
+            ts.note("Undo %s" % ' '.join(descriptions))
             self._p_jar.db().undoMultiple(tids)
+            try:
+                ts.commit()
+            except Exception as exc:
+                if REQUEST is None:
+                    raise
+
+                ts.abort()
+                error = '{}: {}'.format(exc.__class__.__name__, str(exc))
+                return self.manage_UndoForm(self, REQUEST,
+                                            manage_tabs_message=error,
+                                            manage_tabs_type='danger')
 
         if REQUEST is not None:
             REQUEST.RESPONSE.redirect("%s/manage_UndoForm" % REQUEST['URL1'])
diff --git a/src/App/dtml/undo.dtml b/src/App/dtml/undo.dtml
index 1414664f50..bf53e5df4c 100644
--- a/src/App/dtml/undo.dtml
+++ b/src/App/dtml/undo.dtml
@@ -1,6 +1,8 @@
 <dtml-var manage_page_header>
 
-<dtml-var manage_tabs>
+<dtml-with "_(management_view='Undo')">
+  <dtml-var manage_tabs>
+</dtml-with>
 
 <main class="container-fluid">
 
diff --git a/src/App/tests/test_ApplicationManager.py b/src/App/tests/test_ApplicationManager.py
index d54f109764..ec227e16ab 100644
--- a/src/App/tests/test_ApplicationManager.py
+++ b/src/App/tests/test_ApplicationManager.py
@@ -9,6 +9,12 @@
 import Testing.ZopeTestCase
 
 
+class DummyForm:
+
+    def __call__(self, *args, **kw):
+        return kw
+
+
 class DummyConnection:
 
     def __init__(self, db):
@@ -53,6 +59,40 @@ def getCacheSize(self):
     def pack(self, when):
         self._packed = when
 
+    def undoInfo(self, first_transaction, last_transaction):
+        return [{'time': 1,
+                 'description': 'Transaction 1',
+                 'id': b'id1'}]
+
+    def undoMultiple(self, tids):
+        pass
+
+
+class DummyTransaction:
+
+    def __init__(self, raises=False):
+        self.raises = raises
+        self.aborted = False
+
+    def note(self, note):
+        self._note = note
+
+    def commit(self):
+        if self.raises:
+            raise RuntimeError('This did not work')
+
+    def abort(self):
+        self.aborted = True
+
+
+class DummyTransactionModule:
+
+    def __init__(self, raises=False):
+        self.ts = DummyTransaction(raises=raises)
+
+    def get(self):
+        return self.ts
+
 
 class ConfigTestBase:
 
@@ -366,6 +406,42 @@ def test_manage_pack(self):
         # The dummy storage value should not change because pack was not called
         self.assertIsNone(am._getDB()._packed)
 
+    def test_manage_undoTransactions_raises(self):
+        # Patch in fake transaction module that will raise RuntimeError
+        # on transaction commit
+        try:
+            import App.Undo
+            trs_module = App.Undo.transaction
+            App.Undo.transaction = DummyTransactionModule(raises=True)
+
+            am = self._makeOne()
+            am._p_jar = self._makeJar('foo', '')
+            am.manage_UndoForm = DummyForm()
+            undoable_tids = [x['id'] for x in am.undoable_transactions()]
+            current_transaction = App.Undo.transaction.get()
+
+            # If no REQUEST is passed in, the exception is re-raised unchanged
+            # and the current transaction is left unchanged.
+            self.assertRaises(RuntimeError,
+                              am.manage_undo_transactions,
+                              transaction_info=undoable_tids)
+            self.assertFalse(current_transaction.aborted)
+
+            # If a REQUEST is passed in, the transaction will be aborted and
+            # the exception is caught. The DummyForm instance shows the
+            # call arguments so the exception data is visible.
+            res = am.manage_undo_transactions(transaction_info=undoable_tids,
+                                              REQUEST={})
+            expected = {
+                'manage_tabs_message': 'RuntimeError: This did not work',
+                'manage_tabs_type': 'danger'}
+            self.assertDictEqual(res, expected)
+            self.assertTrue(current_transaction.aborted)
+
+        finally:
+            # Cleanup
+            App.Undo.transaction = trs_module
+
 
 class DebugManagerTests(unittest.TestCase):