From b1e0c0370752f5dbc031162aba3a25e1102cd2fe Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=A9dric=20Krier?= <ced@b2ck.com>
Date: Sat, 28 Mar 2020 23:56:19 +0100
Subject: [PATCH] Simplify trigger action

We use now a selection field to choose the action to trigger.
It is safer and more user friendly.

issue9096
review272961003
---
 CHANGELOG                        |  1 +
 trytond/ir/trigger.py            | 26 ++++++++++++++++++++++----
 trytond/ir/view/trigger_form.xml |  6 ++----
 trytond/ir/view/trigger_list.xml |  1 +
 trytond/tests/test_trigger.py    | 30 +++++-------------------------
 trytond/tests/trigger.py         | 13 ++++++++++++-
 6 files changed, 43 insertions(+), 34 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index e030fa7aec..52bed505dc 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,3 +1,4 @@
+* Simplify trigger action
 * Run trigger in the queue
 * Do not copy Binary content with file_id
 * Replace memoize with functools.lru_cache
diff --git a/trytond/ir/trigger.py b/trytond/ir/trigger.py
index 4994febb04..794a8f7404 100644
--- a/trytond/ir/trigger.py
+++ b/trytond/ir/trigger.py
@@ -5,6 +5,7 @@ import time
 from sql import Literal, Null, Select
 from sql.aggregate import Count, Max
 from sql.functions import CurrentTimestamp
+from sql.operators import Concat
 
 from trytond.model.exceptions import ValidationError
 from trytond.i18n import gettext
@@ -55,8 +56,7 @@ class Trigger(DeactivableMixin, ModelSQL, ModelView):
         help='Set a minimum time delay between call to "Action Function" '
         'for the same record.\n'
         'empty for no delay.')
-    action_model = fields.Many2One('ir.model', 'Action Model', required=True)
-    action_function = fields.Char('Action Function', required=True)
+    action = fields.Selection([], "Action", required=True)
     _get_triggers_cache = Cache('ir_trigger.get_triggers')
 
     @classmethod
@@ -81,6 +81,8 @@ class Trigger(DeactivableMixin, ModelSQL, ModelView):
 
         super(Trigger, cls).__register__(module_name)
 
+        table_h = cls.__table_handler__(module_name)
+
         # Migration from 3.4:
         # change minimum_delay into timedelta minimum_time_delay
         if table.column_exist('minimum_delay'):
@@ -95,6 +97,21 @@ class Trigger(DeactivableMixin, ModelSQL, ModelView):
                         where=sql_table.id == id_))
             table.drop_column('minimum_delay')
 
+        # Migration from 5.4: merge action
+        if (table_h.column_exist('action_model')
+                and table_h.column_exist('action_function')):
+            pool = Pool()
+            Model = pool.get('ir.model')
+            model = Model.__table__()
+            action_model = model.select(
+                model.model, where=model.id == sql_table.action_model)
+            cursor.execute(*sql_table.update(
+                    [sql_table.action],
+                    [Concat(action_model, Concat(
+                                '|', sql_table.action_function))]))
+            table_h.drop_column('action_model')
+            table_h.drop_column('action_function')
+
     @classmethod
     def validate(cls, triggers):
         super(Trigger, cls).validate(triggers)
@@ -187,7 +204,8 @@ class Trigger(DeactivableMixin, ModelSQL, ModelView):
         pool = Pool()
         TriggerLog = pool.get('ir.trigger.log')
         Model = pool.get(self.model.model)
-        ActionModel = pool.get(self.action_model.model)
+        model, method = self.action.split('|')
+        ActionModel = pool.get(model)
         cursor = Transaction().connection.cursor()
         trigger_log = TriggerLog.__table__()
 
@@ -255,7 +273,7 @@ class Trigger(DeactivableMixin, ModelSQL, ModelView):
 
         records = Model.browse(ids)
         if records:
-            getattr(ActionModel, self.action_function)(records, self)
+            getattr(ActionModel, method)(records, self)
         if self.limit_number or self.minimum_time_delay:
             to_create = []
             for record in records:
diff --git a/trytond/ir/view/trigger_form.xml b/trytond/ir/view/trigger_form.xml
index 419c0b7c1c..e40079ad0a 100644
--- a/trytond/ir/view/trigger_form.xml
+++ b/trytond/ir/view/trigger_form.xml
@@ -24,8 +24,6 @@ this repository contains the full copyright notices and license terms. -->
     <field name="limit_number"/>
     <label name="minimum_time_delay"/>
     <field name="minimum_time_delay"/>
-    <label name="action_model"/>
-    <field name="action_model"/>
-    <label name="action_function"/>
-    <field name="action_function"/>
+    <label name="action"/>
+    <field name="action"/>
 </form>
diff --git a/trytond/ir/view/trigger_list.xml b/trytond/ir/view/trigger_list.xml
index a4c46dfbe1..ebfe023b71 100644
--- a/trytond/ir/view/trigger_list.xml
+++ b/trytond/ir/view/trigger_list.xml
@@ -8,4 +8,5 @@ this repository contains the full copyright notices and license terms. -->
     <field name="on_create"/>
     <field name="on_write"/>
     <field name="on_delete"/>
+    <field name="action"/>
 </tree>
diff --git a/trytond/tests/test_trigger.py b/trytond/tests/test_trigger.py
index b0226c5e45..bfeab05910 100644
--- a/trytond/tests/test_trigger.py
+++ b/trytond/tests/test_trigger.py
@@ -45,17 +45,13 @@ class TriggerTestCase(unittest.TestCase):
         model, = Model.search([
                 ('model', '=', 'test.triggered'),
                 ])
-        action_model, = Model.search([
-                ('model', '=', 'test.trigger_action'),
-                ])
 
         values = {
             'name': 'Test',
             'model': model.id,
             'on_time': True,
             'condition': 'true',
-            'action_model': action_model.id,
-            'action_function': 'test',
+            'action': 'test.trigger_action|trigger',
             }
         self.assertTrue(Trigger.create([values]))
 
@@ -93,17 +89,13 @@ class TriggerTestCase(unittest.TestCase):
         model, = Model.search([
                 ('model', '=', 'test.triggered'),
                 ])
-        action_model, = Model.search([
-                ('model', '=', 'test.trigger_action'),
-                ])
 
         trigger, = Trigger.create([{
                     'name': 'Test',
                     'model': model.id,
                     'on_create': True,
                     'condition': 'true',
-                    'action_model': action_model.id,
-                    'action_function': 'trigger',
+                    'action': 'test.trigger_action|trigger',
                     }])
 
         triggered, = Triggered.create([{
@@ -175,17 +167,13 @@ class TriggerTestCase(unittest.TestCase):
         model, = Model.search([
                 ('model', '=', 'test.triggered'),
                 ])
-        action_model, = Model.search([
-                ('model', '=', 'test.trigger_action'),
-                ])
 
         trigger, = Trigger.create([{
                     'name': 'Test',
                     'model': model.id,
                     'on_write': True,
                     'condition': 'true',
-                    'action_model': action_model.id,
-                    'action_function': 'trigger',
+                    'action': 'test.trigger_action|trigger',
                     }])
 
         triggered, = Triggered.create([{
@@ -311,9 +299,6 @@ class TriggerTestCase(unittest.TestCase):
         model, = Model.search([
                 ('model', '=', 'test.triggered'),
                 ])
-        action_model, = Model.search([
-                ('model', '=', 'test.trigger_action'),
-                ])
 
         triggered, = Triggered.create([{
                     'name': 'Test',
@@ -324,8 +309,7 @@ class TriggerTestCase(unittest.TestCase):
                     'model': model.id,
                     'on_delete': True,
                     'condition': 'true',
-                    'action_model': action_model.id,
-                    'action_function': 'trigger',
+                    'action': 'test.trigger_action|trigger',
                     }])
 
         Triggered.delete([triggered])
@@ -406,17 +390,13 @@ class TriggerTestCase(unittest.TestCase):
         model, = Model.search([
                 ('model', '=', 'test.triggered'),
                 ])
-        action_model, = Model.search([
-                ('model', '=', 'test.trigger_action'),
-                ])
 
         trigger, = Trigger.create([{
                     'name': 'Test',
                     'model': model.id,
                     'on_time': True,
                     'condition': 'true',
-                    'action_model': action_model.id,
-                    'action_function': 'trigger',
+                    'action': 'test.trigger_action|trigger',
                     }])
 
         triggered, = Triggered.create([{
diff --git a/trytond/tests/trigger.py b/trytond/tests/trigger.py
index 035a27c4fe..ea11d08b9a 100644
--- a/trytond/tests/trigger.py
+++ b/trytond/tests/trigger.py
@@ -1,11 +1,21 @@
 # This file is part of Tryton.  The COPYRIGHT file at the top level of
 # this repository contains the full copyright notices and license terms.
 from trytond.model import ModelSQL, fields
-from trytond.pool import Pool
+from trytond.pool import Pool, PoolMeta
 
 TRIGGER_LOGS = []
 
 
+class Trigger(metaclass=PoolMeta):
+    __name__ = 'ir.trigger'
+
+    @classmethod
+    def __setup__(cls):
+        super().__setup__()
+        cls.action.selection.append(
+            ('test.trigger_action|trigger', "Test Trigger"))
+
+
 class Triggered(ModelSQL):
     'Triggered'
     __name__ = 'test.triggered'
@@ -26,6 +36,7 @@ class TriggerAction(ModelSQL):
 
 def register(module):
     Pool.register(
+        Trigger,
         Triggered,
         TriggerAction,
         module=module, type_='model')