Browse Source

Merge "Avoid duplicated message in Batch/DeleteAction"

Zuul 1 year ago
parent
commit
f1eba8073d
2 changed files with 175 additions and 4 deletions
  1. 91
    4
      horizon/tables/actions.py
  2. 84
    0
      horizon/test/unit/tables/test_tables.py

+ 91
- 4
horizon/tables/actions.py View File

@@ -15,6 +15,7 @@
15 15
 from collections import defaultdict
16 16
 from collections import OrderedDict
17 17
 import copy
18
+import functools
18 19
 import logging
19 20
 import types
20 21
 
@@ -28,6 +29,7 @@ from django.utils.translation import ugettext_lazy as _
28 29
 from django.utils.translation import ungettext_lazy
29 30
 import six
30 31
 
32
+from horizon import exceptions
31 33
 from horizon import messages
32 34
 from horizon.utils import functions
33 35
 from horizon.utils import html
@@ -775,10 +777,22 @@ class BatchAction(Action):
775 777
                          {'action': self._get_action_name(past=True),
776 778
                           'datum_display': datum_display})
777 779
             except Exception as ex:
778
-                # Handle the exception but silence it since we'll display
779
-                # an aggregate error message later. Otherwise we'd get
780
-                # multiple error messages displayed to the user.
781
-                action_failure.append(datum_display)
780
+                handled_exc = isinstance(ex, exceptions.HandledException)
781
+                if handled_exc:
782
+                    # In case of HandledException, an error message should be
783
+                    # handled in exceptions.handle() or other logic,
784
+                    # so we don't need to handle the error message here.
785
+                    # NOTE(amotoki): To raise HandledException from the logic,
786
+                    # pass escalate=True and do not pass redirect argument
787
+                    # to exceptions.handle().
788
+                    # If an exception is handled, the original exception object
789
+                    # is stored in ex.wrapped[1].
790
+                    ex = ex.wrapped[1]
791
+                else:
792
+                    # Handle the exception but silence it since we'll display
793
+                    # an aggregate error message later. Otherwise we'd get
794
+                    # multiple error messages displayed to the user.
795
+                    action_failure.append(datum_display)
782 796
                 action_description = (
783 797
                     self._get_action_name(past=True).lower(), datum_display)
784 798
                 LOG.warning(
@@ -879,6 +893,79 @@ class DeleteAction(BatchAction):
879 893
         """
880 894
 
881 895
 
896
+class handle_exception_with_detail_message(object):
897
+    """Decorator to allow special exception handling in BatchAction.action().
898
+
899
+    An exception from BatchAction.action() or DeleteAction.delete() is
900
+    normally caught by BatchAction.handle() and BatchAction.handle() displays
901
+    an aggregated error message. However, there are cases where we would like
902
+    to provide an error message which explains a failure reason if some
903
+    exception occurs so that users can understand situation better.
904
+
905
+    This decorator allows us to do this kind of special handling easily.
906
+    This can be applied to BatchAction.action() and DeleteAction.delete()
907
+    methods.
908
+
909
+    :param normal_log_message: Log message template when an exception other
910
+        than ``target_exception`` is detected. Keyword substituion "%(id)s"
911
+        and "%(exc)s" can be used.
912
+
913
+    :param target_exception: Exception class should be handled specially.
914
+        If this exception is caught, a log message will be logged using
915
+        ``target_log_message`` and a user visible will be shown using
916
+        ``target_user_message``. In this case, an aggregated error message
917
+        generated by BatchAction.handle() does not include an object which
918
+        causes this exception.
919
+
920
+    :param target_log_message: Log message template when an exception specified
921
+        in ``target_exception`` is detected. Keyword substituion "%(id)s"
922
+        and "%(exc)s" can be used.
923
+
924
+    :param target_user_message: User visible message template when an exception
925
+        specified in ``target_exception`` is detected. It is recommended to
926
+        use an internationalized string. Keyword substituion "%(name)s"
927
+        and "%(exc)s" can be used.
928
+
929
+    :param logger_name: (optional) Logger name to be used.
930
+        The usual pattern is to pass __name__ of a caller.
931
+        This allows us to show a module name of a caller in a logged message.
932
+    """
933
+
934
+    def __init__(self, normal_log_message, target_exception,
935
+                 target_log_message, target_user_message, logger_name=None):
936
+        self.logger = logging.getLogger(logger_name or __name__)
937
+        self.normal_log_message = normal_log_message
938
+        self.target_exception = target_exception
939
+        self.target_log_message = target_log_message
940
+        self.target_user_message = target_user_message
941
+
942
+    def __call__(self, fn):
943
+        @functools.wraps(fn)
944
+        def decorated(instance, request, obj_id):
945
+            try:
946
+                fn(instance, request, obj_id)
947
+            except self.target_exception as e:
948
+                self.logger.info(self.target_log_message,
949
+                                 {'id': obj_id, 'exc': e})
950
+                obj = instance.table.get_object_by_id(obj_id)
951
+                name = instance.table.get_object_display(obj)
952
+                msg = self.target_user_message % {'name': name, 'exc': e}
953
+                # 'escalate=True' is required to notify the caller
954
+                # (DeleteAction) of the failure. exceptions.handle() will
955
+                # raise a wrapped exception of HandledException and BatchAction
956
+                # will handle it. 'redirect' should not be passed here as
957
+                # 'redirect' has a priority over 'escalate' argument.
958
+                exceptions.handle(request, msg, escalate=True)
959
+            except Exception as e:
960
+                self.logger.info(self.normal_log_message,
961
+                                 {'id': obj_id, 'exc': e})
962
+                # NOTE: No exception handling is required here because
963
+                # BatchAction.handle() does it. What we need to do is
964
+                # just to re-raise the exception.
965
+                raise
966
+        return decorated
967
+
968
+
882 969
 class Deprecated(type):
883 970
     # TODO(lcastell) Replace class with similar functionality from
884 971
     # oslo_log.versionutils when it's finally added in 11.0

+ 84
- 0
horizon/test/unit/tables/test_tables.py View File

@@ -15,6 +15,9 @@
15 15
 #    License for the specific language governing permissions and limitations
16 16
 #    under the License.
17 17
 
18
+import unittest
19
+import uuid
20
+
18 21
 from django.core.urlresolvers import reverse
19 22
 from django import forms
20 23
 from django import http
@@ -23,10 +26,13 @@ from django.template import defaultfilters
23 26
 from django.test.utils import override_settings
24 27
 from django.utils.translation import ungettext_lazy
25 28
 
29
+import mock
26 30
 from mox3.mox import IsA
27 31
 import six
28 32
 
33
+from horizon import exceptions
29 34
 from horizon import tables
35
+from horizon.tables import actions
30 36
 from horizon.tables import formset as table_formset
31 37
 from horizon.tables import views as table_views
32 38
 from horizon.test import helpers as test
@@ -1603,3 +1609,81 @@ class FormsetTableTests(test.TestCase):
1603 1609
         form_data = form.initial
1604 1610
         self.assertEqual('object_1', form_data['name'])
1605 1611
         self.assertEqual(2, form_data['value'])
1612
+
1613
+
1614
+class MyException(Exception):
1615
+    pass
1616
+
1617
+
1618
+class OtherException(Exception):
1619
+    pass
1620
+
1621
+
1622
+class BatchActionDecoratorTests(unittest.TestCase):
1623
+
1624
+    def setUp(self):
1625
+        obj = uuid.uuid4()
1626
+        self.obj_id = 'id-%s' % str(obj)
1627
+        self.obj_name = 'name-%s' % str(obj)
1628
+        table = mock.Mock()
1629
+        table.get_object_by_id.return_value = obj
1630
+        table.get_object_display.return_value = self.obj_name
1631
+
1632
+        # getLogger is called insdie handle_exception_with_detail_message
1633
+        # decorator, so this needs to be mocked before using the decorator.
1634
+        self.logger = mock.Mock()
1635
+        with mock.patch('logging.getLogger',
1636
+                        return_value=self.logger) as mock_getlogger:
1637
+            class MyAction(object):
1638
+                def __init__(self, table):
1639
+                    self.table = table
1640
+
1641
+                @actions.handle_exception_with_detail_message(
1642
+                    'normal log message %(id)s %(exc)s',
1643
+                    MyException,
1644
+                    'target log message %(id)s %(exc)s',
1645
+                    'target user message %(name)s %(exc)s',
1646
+                    'mylogger')
1647
+                def action(self, request, obj_id):
1648
+                    self._to_be_mocked()
1649
+
1650
+                # This is required because if mock.patch replaces
1651
+                # a decorated method. We are testing a decorated method
1652
+                # so we need a separate method to mock,
1653
+                def _to_be_mocked(self):
1654
+                    pass
1655
+
1656
+        self.action = MyAction(table)
1657
+        self.mock_getlogger = mock_getlogger
1658
+
1659
+    def test_normal_exception(self):
1660
+        myexc = OtherException()
1661
+        with mock.patch.object(self.action, '_to_be_mocked',
1662
+                               side_effect=myexc):
1663
+            self.assertRaises(OtherException,
1664
+                              self.action.action,
1665
+                              mock.sentinel.request, self.obj_id)
1666
+        self.mock_getlogger.assert_called_once_with('mylogger')
1667
+        self.logger.info.assert_called_once_with(
1668
+            'normal log message %(id)s %(exc)s',
1669
+            {'id': self.obj_id, 'exc': myexc})
1670
+
1671
+    def test_target_exception(self):
1672
+        myexc = MyException()
1673
+        handled = exceptions.HandledException(myexc)
1674
+        with mock.patch.object(self.action, '_to_be_mocked',
1675
+                               side_effect=myexc), \
1676
+                mock.patch.object(exceptions, 'handle',
1677
+                                  side_effect=handled) as mocked_handle:
1678
+            self.assertRaises(exceptions.HandledException,
1679
+                              self.action.action,
1680
+                              mock.sentinel.request, self.obj_id)
1681
+        self.mock_getlogger.assert_called_once_with('mylogger')
1682
+        self.logger.info.assert_called_once_with(
1683
+            'target log message %(id)s %(exc)s',
1684
+            {'id': self.obj_id, 'exc': myexc})
1685
+        mocked_handle.assert_called_once_with(
1686
+            mock.sentinel.request,
1687
+            'target user message %(name)s %(exc)s' %
1688
+            {'name': self.obj_name, 'exc': myexc},
1689
+            escalate=True)

Loading…
Cancel
Save