[Openerp-community] [Merge] lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

[Openerp-community] [Merge] lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server

Raphael Valyi
Raphaël Valyi - http://www.akretion.com has proposed merging lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server.

Requested reviews:
  Raphaël Valyi - http://www.akretion.com (rvalyi)
  OpenERP Core Team (openerp)

For more details, see:
https://code.launchpad.net/~openerp-community/openobject-server/context-in-workflows/+merge/85518

Work in progress, seem my merge message.
A way to test the current limitation where a bug is caused because the workflow engine would pass some unexpected context param is just to create an order with manual invoicing. Validate it and then create the invoice from the sale order.

A bug like that will result:
[..]
  File "/home/rvalyi/DEV/openerp/openerp6.1/addons/sale/sale.py", line 457, in manual_invoice
[..]
  File "/home/rvalyi/DEV/openerp/openerp6.1/server/openerp/workflow/wkf_expr.py", line 85, in check
    return _eval_expr(cr, ident, workitem, transition['condition'], context)
  File "/home/rvalyi/DEV/openerp/openerp6.1/server/openerp/workflow/wkf_expr.py", line 59, in _eval_expr
    ret = eval(line, env, nocopy=True)
  File "/home/rvalyi/DEV/openerp/openerp6.1/server/openerp/tools/safe_eval.py", line 294, in safe_eval
    return eval(test_expr(expr,_SAFE_OPCODES, mode=mode), globals_dict, locals_dict)
  File "", line 1, in <module>
  File "/home/rvalyi/DEV/openerp/openerp6.1/server/openerp/osv/orm.py", line 396, in function_proxy
    return attr(self._cr, self._uid, [self._id], *args, **kwargs)
TypeError: test_state() got an unexpected keyword argument 'context'


Probably, to make it compatible we should inspect the method signature a bit like Albert did in it former "browse_executer" here:
https://code.launchpad.net/~albert-nan/openobject-server/workflow-context/+merge/12256
(in orm.py)
--
https://code.launchpad.net/~openerp-community/openobject-server/context-in-workflows/+merge/85518
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/context-in-workflows.

=== modified file 'openerp/osv/osv.py'
--- openerp/osv/osv.py 2011-10-01 01:22:10 +0000
+++ openerp/osv/osv.py 2011-12-13 17:01:27 +0000
@@ -180,16 +180,16 @@
             cr.close()
         return res
 
-    def exec_workflow_cr(self, cr, uid, obj, method, *args):
+    def exec_workflow_cr(self, cr, uid, obj, method, id, context=None):
         wf_service = netsvc.LocalService("workflow")
-        return wf_service.trg_validate(uid, obj, args[0], method, cr)
+        return wf_service.trg_validate(uid, obj, id, method, cr, context)
 
     @check
-    def exec_workflow(self, db, uid, obj, method, *args):
+    def exec_workflow(self, db, uid, obj, method, id, context=None):
         cr = pooler.get_db(db).cursor()
         try:
             try:
-                res = self.exec_workflow_cr(cr, uid, obj, method, *args)
+                res = self.exec_workflow_cr(cr, uid, obj, method, id, context)
                 cr.commit()
             except Exception:
                 cr.rollback()

=== modified file 'openerp/workflow/instance.py'
--- openerp/workflow/instance.py 2011-02-07 12:57:23 +0000
+++ openerp/workflow/instance.py 2011-12-13 17:01:27 +0000
@@ -25,14 +25,14 @@
 import openerp.netsvc as netsvc
 import openerp.pooler as pooler
 
-def create(cr, ident, wkf_id):
+def create(cr, ident, wkf_id, context=None):
     (uid,res_type,res_id) = ident
     cr.execute('insert into wkf_instance (res_type,res_id,uid,wkf_id) values (%s,%s,%s,%s) RETURNING id', (res_type,res_id,uid,wkf_id))
     id_new = cr.fetchone()[0]
     cr.execute('select * from wkf_activity where flow_start=True and wkf_id=%s', (wkf_id,))
     res = cr.dictfetchall()
     stack = []
-    workitem.create(cr, res, id_new, ident, stack=stack)
+    workitem.create(cr, res, id_new, ident, stack=stack, context=context)
     update(cr, id_new, ident)
     return id_new
 
@@ -40,24 +40,24 @@
     (uid,res_type,res_id) = ident
     cr.execute('delete from wkf_instance where res_id=%s and res_type=%s', (res_id,res_type))
 
-def validate(cr, inst_id, ident, signal, force_running=False):
+def validate(cr, inst_id, ident, signal, force_running=False, context=None):
     cr.execute("select * from wkf_workitem where inst_id=%s", (inst_id,))
     stack = []
     for witem in cr.dictfetchall():
         stack = []
-        workitem.process(cr, witem, ident, signal, force_running, stack=stack)
+        workitem.process(cr, witem, ident, signal, force_running, stack=stack, context=context)
         # An action is returned
-    _update_end(cr, inst_id, ident)
+    _update_end(cr, inst_id, ident, context)
     return stack and stack[0] or False
 
-def update(cr, inst_id, ident):
+def update(cr, inst_id, ident, context=None):
     cr.execute("select * from wkf_workitem where inst_id=%s", (inst_id,))
     for witem in cr.dictfetchall():
         stack = []
-        workitem.process(cr, witem, ident, stack=stack)
-    return _update_end(cr, inst_id, ident)
+        workitem.process(cr, witem, ident, stack=stack, context=context)
+    return _update_end(cr, inst_id, ident, context)
 
-def _update_end(cr, inst_id, ident):
+def _update_end(cr, inst_id, ident, context=None):
     cr.execute('select wkf_id from wkf_instance where id=%s', (inst_id,))
     wkf_id = cr.fetchone()[0]
     cr.execute('select state,flow_stop from wkf_workitem w left join wkf_activity a on (a.id=w.act_id) where w.inst_id=%s', (inst_id,))
@@ -74,7 +74,7 @@
         cr.execute("select i.id,w.osv,i.res_id from wkf_instance i left join wkf w on (i.wkf_id=w.id) where i.id IN (select inst_id from wkf_workitem where subflow_id=%s)", (inst_id,))
         for i in cr.fetchall():
             for act_name in act_names:
-                validate(cr, i[0], (ident[0],i[1],i[2]), 'subflow.'+act_name[0])
+                validate(cr, i[0], (ident[0],i[1],i[2]), 'subflow.'+act_name[0], context)
     return ok
 
 

=== modified file 'openerp/workflow/wkf_expr.py'
--- openerp/workflow/wkf_expr.py 2011-09-14 10:25:05 +0000
+++ openerp/workflow/wkf_expr.py 2011-12-13 17:01:27 +0000
@@ -26,22 +26,23 @@
 from openerp.tools.safe_eval import safe_eval as eval
 
 class Env(dict):
-    def __init__(self, cr, uid, model, ids):
+    def __init__(self, cr, uid, model, ids, context=None):
         self.cr = cr
         self.uid = uid
         self.model = model
         self.ids = ids
+        self.context = context
         self.obj = pooler.get_pool(cr.dbname).get(model)
         self.columns = self.obj._columns.keys() + self.obj._inherit_fields.keys()
 
     def __getitem__(self, key):
         if (key in self.columns) or (key in dir(self.obj)):
-            res = self.obj.browse(self.cr, self.uid, self.ids[0])
+            res = self.obj.browse(self.cr, self.uid, self.ids[0], self.context)
             return res[key]
         else:
             return super(Env, self).__getitem__(key)
 
-def _eval_expr(cr, ident, workitem, action):
+def _eval_expr(cr, ident, workitem, action, context=None):
     ret=False
     assert action, 'You used a NULL action in a workflow, use dummy node instead.'
     for line in action.split('\n'):
@@ -54,20 +55,23 @@
         elif line =='False':
             ret=False
         else:
-            env = Env(cr, uid, model, ids)
+            env = Env(cr, uid, model, ids, context)
             ret = eval(line, env, nocopy=True)
     return ret
 
-def execute_action(cr, ident, workitem, activity):
+def execute_action(cr, ident, workitem, activity, context=None):
     obj = pooler.get_pool(cr.dbname).get('ir.actions.server')
+    ctx = {}
+    if context is not None:
+        ctx.update( context )
     ctx = {'active_model':ident[1], 'active_id':ident[2], 'active_ids':[ident[2]]}
     result = obj.run(cr, ident[0], [activity['action_id']], ctx)
     return result
 
-def execute(cr, ident, workitem, activity):
-    return _eval_expr(cr, ident, workitem, activity['action'])
+def execute(cr, ident, workitem, activity, context=None):
+    return _eval_expr(cr, ident, workitem, activity['action'], context)
 
-def check(cr, workitem, ident, transition, signal):
+def check(cr, workitem, ident, transition, signal, context=None):
     if transition['signal'] and signal != transition['signal']:
         return False
 
@@ -78,8 +82,7 @@
         if not transition['group_id'] in user_groups:
             return False
 
-    return _eval_expr(cr, ident, workitem, transition['condition'])
-
+    return _eval_expr(cr, ident, workitem, transition['condition'], context)
 
 # vim:expandtab:smartindent:tabstop=4:softtabstop=4:shiftwidth=4:
 

=== modified file 'openerp/workflow/wkf_service.py'
--- openerp/workflow/wkf_service.py 2011-09-24 14:52:58 +0000
+++ openerp/workflow/wkf_service.py 2011-12-13 17:01:27 +0000
@@ -44,7 +44,10 @@
     def clear_cache(self, cr, uid):
         self.wkf_on_create_cache[cr.dbname]={}
 
-    def trg_write(self, uid, res_type, res_id, cr):
+    def trg_write(self, uid, res_type, res_id, cr, context=None):
+        print "WWWWWWWWWWWWW", context
+        import traceback
+        traceback.print_stack()
         """
         Reevaluates the specified workflow instance. Thus if any condition for
         a transition have been changed in the backend, then running ``trg_write``
@@ -57,9 +60,10 @@
         ident = (uid,res_type,res_id)
         cr.execute('select id from wkf_instance where res_id=%s and res_type=%s and state=%s', (res_id or None,res_type or None, 'active'))
         for (id,) in cr.fetchall():
-            instance.update(cr, id, ident)
+            instance.update(cr, id, ident, context)
 
-    def trg_trigger(self, uid, res_type, res_id, cr):
+    def trg_trigger(self, uid, res_type, res_id, cr, context=None):
+        print "TTTTTTTTTTTTTTT", context
         """
         Activate a trigger.
 
@@ -75,7 +79,7 @@
         for (instance_id,) in res:
             cr.execute('select %s,res_type,res_id from wkf_instance where id=%s', (uid, instance_id,))
             ident = cr.fetchone()
-            instance.update(cr, instance_id, ident)
+            instance.update(cr, instance_id, ident, context)
 
     def trg_delete(self, uid, res_type, res_id, cr):
         """
@@ -88,7 +92,7 @@
         ident = (uid,res_type,res_id)
         instance.delete(cr, ident)
 
-    def trg_create(self, uid, res_type, res_id, cr):
+    def trg_create(self, uid, res_type, res_id, cr, context=None):
         """
         Create a new workflow instance
 
@@ -105,9 +109,10 @@
             wkf_ids = cr.fetchall()
             self.wkf_on_create_cache[cr.dbname][res_type] = wkf_ids
         for (wkf_id,) in wkf_ids:
-            instance.create(cr, ident, wkf_id)
+            instance.create(cr, ident, wkf_id, context)
 
-    def trg_validate(self, uid, res_type, res_id, signal, cr):
+    def trg_validate(self, uid, res_type, res_id, signal, cr, context=None):
+        print "VVVVVVVVVVV", context
         """
         Fire a signal on a given workflow instance
 
@@ -121,7 +126,7 @@
         # ids of all active workflow instances for a corresponding resource (id, model_nam)
         cr.execute('select id from wkf_instance where res_id=%s and res_type=%s and state=%s', (res_id, res_type, 'active'))
         for (id,) in cr.fetchall():
-            res2 = instance.validate(cr, id, ident, signal)
+            res2 = instance.validate(cr, id, ident, signal, context=context)
             result = result or res2
         return result
 

=== modified file 'openerp/workflow/workitem.py'
--- openerp/workflow/workitem.py 2011-12-11 10:21:40 +0000
+++ openerp/workflow/workitem.py 2011-12-13 17:01:27 +0000
@@ -30,7 +30,7 @@
 import wkf_expr
 import wkf_logs
 
-def create(cr, act_datas, inst_id, ident, stack):
+def create(cr, act_datas, inst_id, ident, stack, context=None):
     for act in act_datas:
         cr.execute("select nextval('wkf_workitem_id_seq')")
         id_new = cr.fetchone()[0]
@@ -38,9 +38,9 @@
         cr.execute('select * from wkf_workitem where id=%s',(id_new,))
         res = cr.dictfetchone()
         wkf_logs.log(cr,ident,act['id'],'active')
-        process(cr, res, ident, stack=stack)
+        process(cr, res, ident, stack=stack, context=context)
 
-def process(cr, workitem, ident, signal=None, force_running=False, stack=None):
+def process(cr, workitem, ident, signal=None, force_running=False, stack=None, context=None):
     if stack is None:
         raise 'Error !!!'
     result = True
@@ -50,7 +50,7 @@
     triggers = False
     if workitem['state']=='active':
         triggers = True
-        result = _execute(cr, workitem, activity, ident, stack)
+        result = _execute(cr, workitem, activity, ident, stack, context)
         if not result:
             return False
 
@@ -58,7 +58,7 @@
         pass
 
     if workitem['state']=='complete' or force_running:
-        ok = _split_test(cr, workitem, activity['split_mode'], ident, signal, stack)
+        ok = _split_test(cr, workitem, activity['split_mode'], ident, signal, stack, context)
         triggers = triggers and not ok
 
     if triggers:
@@ -66,7 +66,7 @@
         alltrans = cr.dictfetchall()
         for trans in alltrans:
             if trans['trigger_model']:
-                ids = wkf_expr._eval_expr(cr,ident,workitem,trans['trigger_expr_id'])
+                ids = wkf_expr._eval_expr(cr,ident,workitem,trans['trigger_expr_id'], context)
                 for res_id in ids:
                     cr.execute('select nextval(\'wkf_triggers_id_seq\')')
                     id =cr.fetchone()[0]
@@ -82,7 +82,7 @@
     workitem['state'] = state
     wkf_logs.log(cr,ident,activity['id'],state)
 
-def _execute(cr, workitem, activity, ident, stack):
+def _execute(cr, workitem, activity, ident, stack, context=None):
     result = True
     #
     # send a signal to parent workflow (signal: subflow.signal_name)
@@ -97,18 +97,18 @@
         if workitem['state']=='active':
             _state_set(cr, workitem, activity, 'complete', ident)
             if activity['action_id']:
-                res2 = wkf_expr.execute_action(cr, ident, workitem, activity)
+                res2 = wkf_expr.execute_action(cr, ident, workitem, activity, context)
                 if res2:
                     stack.append(res2)
                     result=res2
     elif activity['kind']=='function':
         if workitem['state']=='active':
             _state_set(cr, workitem, activity, 'running', ident)
-            returned_action = wkf_expr.execute(cr, ident, workitem, activity)
+            returned_action = wkf_expr.execute(cr, ident, workitem, activity, context)
             if type(returned_action) in (dict,):
                 stack.append(returned_action)
             if activity['action_id']:
-                res2 = wkf_expr.execute_action(cr, ident, workitem, activity)
+                res2 = wkf_expr.execute_action(cr, ident, workitem, activity, context)
                 # A client action has been returned
                 if res2:
                     stack.append(res2)
@@ -119,13 +119,13 @@
             _state_set(cr, workitem, activity, 'running', ident)
             cr.execute('delete from wkf_workitem where inst_id=%s and id<>%s', (workitem['inst_id'], workitem['id']))
             if activity['action']:
-                wkf_expr.execute(cr, ident, workitem, activity)
+                wkf_expr.execute(cr, ident, workitem, activity, context)
             _state_set(cr, workitem, activity, 'complete', ident)
     elif activity['kind']=='subflow':
         if workitem['state']=='active':
             _state_set(cr, workitem, activity, 'running', ident)
             if activity.get('action', False):
-                id_new = wkf_expr.execute(cr, ident, workitem, activity)
+                id_new = wkf_expr.execute(cr, ident, workitem, activity, context)
                 if not (id_new):
                     cr.execute('delete from wkf_workitem where id=%s', (workitem['id'],))
                     return False
@@ -133,7 +133,7 @@
                 cr.execute('select id from wkf_instance where res_id=%s and wkf_id=%s', (id_new,activity['subflow_id']))
                 id_new = cr.fetchone()[0]
             else:
-                id_new = instance.create(cr, ident, activity['subflow_id'])
+                id_new = instance.create(cr, ident, activity['subflow_id'], context)
             cr.execute('update wkf_workitem set subflow_id=%s where id=%s', (id_new, workitem['id']))
             workitem['subflow_id'] = id_new
         if workitem['state']=='running':
@@ -142,11 +142,11 @@
             if state=='complete':
                 _state_set(cr, workitem, activity, 'complete', ident)
     for t in signal_todo:
-        instance.validate(cr, t[0], t[1], t[2], force_running=True)
+        instance.validate(cr, t[0], t[1], t[2], force_running=True, context=context)
 
     return result
 
-def _split_test(cr, workitem, split_mode, ident, signal=None, stack=None):
+def _split_test(cr, workitem, split_mode, ident, signal=None, stack=None, context=None):
     if stack is None:
         raise 'Error !!!'
     cr.execute('select * from wkf_transition where act_from=%s', (workitem['act_id'],))
@@ -155,7 +155,7 @@
     alltrans = cr.dictfetchall()
     if split_mode=='XOR' or split_mode=='OR':
         for transition in alltrans:
-            if wkf_expr.check(cr, workitem, ident, transition,signal):
+            if wkf_expr.check(cr, workitem, ident, transition, signal, context):
                 test = True
                 transitions.append((transition['id'], workitem['inst_id']))
                 if split_mode=='XOR':
@@ -163,7 +163,7 @@
     else:
         test = True
         for transition in alltrans:
-            if not wkf_expr.check(cr, workitem, ident, transition,signal):
+            if not wkf_expr.check(cr, workitem, ident, transition, signal, context):
                 test = False
                 break
             cr.execute('select count(*) from wkf_witm_trans where trans_id=%s and inst_id=%s', (transition['id'], workitem['inst_id']))
@@ -173,15 +173,15 @@
         cr.executemany('insert into wkf_witm_trans (trans_id,inst_id) values (%s,%s)', transitions)
         cr.execute('delete from wkf_workitem where id=%s', (workitem['id'],))
         for t in transitions:
-            _join_test(cr, t[0], t[1], ident, stack)
+            _join_test(cr, t[0], t[1], ident, stack, context)
         return True
     return False
 
-def _join_test(cr, trans_id, inst_id, ident, stack):
+def _join_test(cr, trans_id, inst_id, ident, stack, context=None):
     cr.execute('select * from wkf_activity where id=(select act_to from wkf_transition where id=%s)', (trans_id,))
     activity = cr.dictfetchone()
     if activity['join_mode']=='XOR':
-        create(cr,[activity], inst_id, ident, stack)
+        create(cr,[activity], inst_id, ident, stack, context)
         cr.execute('delete from wkf_witm_trans where inst_id=%s and trans_id=%s', (inst_id,trans_id))
     else:
         cr.execute('select id from wkf_transition where act_to=%s', (activity['id'],))
@@ -196,7 +196,7 @@
         if ok:
             for (id,) in trans_ids:
                 cr.execute('delete from wkf_witm_trans where trans_id=%s and inst_id=%s', (id,inst_id))
-            create(cr, [activity], inst_id, ident, stack)
+            create(cr, [activity], inst_id, ident, stack, context)
 
 # vim:expandtab:smartindent:tabstop=4:softtabstop=4:shiftwidth=4:
 


_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server

Raphael Valyi
Review: Needs Fixing


--
https://code.launchpad.net/~openerp-community/openobject-server/context-in-workflows/+merge/85518
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/context-in-workflows.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

[Openerp-community] [Merge] lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server

Olivier Dony (OpenERP)
In reply to this post by Raphael Valyi
The proposal to merge lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server has been updated.

    Status: Needs review => Work in progress

For more details, see:
https://code.launchpad.net/~openerp-community/openobject-server/context-in-workflows/+merge/85518
--
https://code.launchpad.net/~openerp-community/openobject-server/context-in-workflows/+merge/85518
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/context-in-workflows.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server

Christophe Chauvet - http://www.syleam.fr/
In reply to this post by Raphael Valyi
Review: Needs Information

+1 to have context on workflow

It does also transmit context to the workflow, if it define on button element as

<button name="wkf_test" context="{'test': 'ok'}"/>

Christophe.
--
https://code.launchpad.net/~openerp-community/openobject-server/context-in-workflows/+merge/85518
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/context-in-workflows.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server

Olivier Dony (OpenERP)
In reply to this post by Raphael Valyi
FWIW, you can have a look at the branch where I tested merging the same thing[1].

As I explain on my merge prop there, I think we need more work on the addons side in order to get any benefit from this work, and that will be much more tricky. I started a wip of changing addons in this manner[2] but it is far from complete or working yet, and perhaps it's too much at this point.


[1] lp:~openerp-dev/openobject-server/trunk-workflow-context-odo
[2] lp:~openerp-dev/openobject-addons/trunk-workflow-context-odo
--
https://code.launchpad.net/~openerp-community/openobject-server/context-in-workflows/+merge/85518
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/context-in-workflows.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server

Stefan Rijnhart (Therp)
In reply to this post by Raphael Valyi
Review: Needs Fixing

Hi Raphaël,

thanks for the effort! A tiny detail in openerp/workflow/wkf_expr.py, line 133: looks like you lose the context here.

I see the problem with the API change and existing methods that Olivier also points out. However, it is one thing to make all models pass the context when calling the workflow service API, but for 6.1 it might minimally suffice to just make the methods mentioned in wkf_activity.action accept the context keyword argument. That should be doable.

Cheers,
Stefan.


--
https://code.launchpad.net/~openerp-community/openobject-server/context-in-workflows/+merge/85518
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/context-in-workflows.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server

Fabien (Open ERP)
In reply to this post by Olivier Dony (OpenERP)
Olivier,

Are you sure it's a good idea to allow context in workflows ?

Workflows are not used for user interface interactions, but for
controlling the
evolution of the object. It has no sense to put context on constraints
and, for
the same reason, I don't think we should allow context in workflows.

An activity should not depend on a context but on data stored on the object.
The goal of the workflow is to easily modify it, add/remove some
transitions, etc.
If we depend on contexts, we may loose this modularity.

I am still not sure about this, but I would not allow the context to be
used in
workflows, for the same reason that it has no sense to put context on
constraints.


On 13/12/11 19:05, Olivier Dony (OpenERP) wrote:
> FWIW, you can have a look at the branch where I tested merging the same thing[1].
>
> As I explain on my merge prop there, I think we need more work on the addons side in order to get any benefit from this work, and that will be much more tricky. I started a wip of changing addons in this manner[2] but it is far from complete or working yet, and perhaps it's too much at this point.
>
>
> [1] lp:~openerp-dev/openobject-server/trunk-workflow-context-odo
> [2] lp:~openerp-dev/openobject-addons/trunk-workflow-context-odo
>    


--
https://code.launchpad.net/~openerp-community/openobject-server/context-in-workflows/+merge/85518
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/context-in-workflows.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server

Raphael Valyi
Hello Fabien,

1) If I'm not wrong about it, on issue is that if you don't pass the
context you'll for instance miss important things like translations. So for
instance if I create an invoice upon the order (through the workflow) but
if if that invoice fails to create for some reason, the error message it
will display will not be localized properly (you'll eventually find out the
sale_order.user_id lang or something like that but won't be very precise.
Or am I wrong? Also I believe you were using some Python frame inspection
for that but is was extremely slow, am I correct, was it related?

2) we also have an issue with the methods the workflow call: those methods
are easily called from different paths and easily they need the standard
context system to be modular.
Look at such methods:
http://bazaar.launchpad.net/~akretion-team/openobject-addons/sale-modular-picking-better-context/revision/5761
or
http://bazaar.launchpad.net/~akretion-team/openobject-addons/sale-modular-picking-better-context/revision/5760

Actually for a modular API, we really have the case were Sebastien Beau
would need to override the beginning of the method chain to inject some
custom context key.
Later in the call chain, the will override again and hope he will get his
context back so he can take some custom action. Usually it's done in the
same transaction and it seems overkill to write some data in the database
to just re-read it a few method calls later while it could come from memory.

The issue is that is seems pointless then to have all those clean methods
correctly propagating the context and using it normally being usually
called by some piggy workflow method that wouldn't pass them the context
and would instead use a useless meaningless arg* signature.

Wouldn't it be better to just propagate the context all the way through?

IMHO it was right to say constraints should not depend on context. But for
the workflow I'm really not sure. I see many modularity benefit of having
that. A system like Christophe gives with things like <button
name="wkf_test" context="{'test': 'ok'}"/> could be extremely powerful with
context being propagated.

So in any case I'm not 100% what is best here. But probably if we decide
workflows don't propagate the context, we should still refactor the methods
it call to support the standard context arguments so it can be used in a
more modular way, no?

So at the end what would we decide:
1) workflow propagate the context (it doesn't mean it should use it itself;
this can be checked) ?
2) the methods called by workflow don't propagate the context at all either
(sounds it sucks to me, cause would force overkill database accesses when
we are already not that fast in transactions)
3) the methods called by the workflow engine, pass the context if they have
some passed. However the workflow engine don't give them the context, so
it's useful only in overrides or when those methods are called outside from
the workflow engine.

What do you guys think?



On Tue, Dec 13, 2011 at 5:58 PM, Fabien (Open ERP) <[hidden email]> wrote:

> Olivier,
>
> Are you sure it's a good idea to allow context in workflows ?
>
> Workflows are not used for user interface interactions, but for
> controlling the
> evolution of the object. It has no sense to put context on constraints
> and, for
> the same reason, I don't think we should allow context in workflows.
>
> An activity should not depend on a context but on data stored on the
> object.
> The goal of the workflow is to easily modify it, add/remove some
> transitions, etc.
> If we depend on contexts, we may loose this modularity.
>
> I am still not sure about this, but I would not allow the context to be
> used in
> workflows, for the same reason that it has no sense to put context on
> constraints.
>
>
> On 13/12/11 19:05, Olivier Dony (OpenERP) wrote:
> > FWIW, you can have a look at the branch where I tested merging the same
> thing[1].
> >
> > As I explain on my merge prop there, I think we need more work on the
> addons side in order to get any benefit from this work, and that will be
> much more tricky. I started a wip of changing addons in this manner[2] but
> it is far from complete or working yet, and perhaps it's too much at this
> point.
> >
> >
> > [1] lp:~openerp-dev/openobject-server/trunk-workflow-context-odo
> > [2] lp:~openerp-dev/openobject-addons/trunk-workflow-context-odo
> >
>
>
> --
>
> https://code.launchpad.net/~openerp-community/openobject-server/context-in-workflows/+merge/85518
> You proposed lp:~openerp-community/openobject-server/context-in-workflows
> for merging.
>

--
https://code.launchpad.net/~openerp-community/openobject-server/context-in-workflows/+merge/85518
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/context-in-workflows.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server

Olivier Dony (OpenERP)
In reply to this post by Stefan Rijnhart (Therp)
On 12/13/2011 08:07 PM, Stefan Rijnhart (Therp) wrote:
> I see the problem with the API change and existing methods that
> Olivier also points out. However, it is one thing to make all models
> pass the context when calling the workflow service API, but for 6.1
> it might minimally suffice to just make the methods mentioned in
> wkf_activity.action accept the context keyword argument. That should
> be doable.

Yes, that's one option I think would be acceptable for 6.1.
(See my comment on
https://code.launchpad.net/~openerp-dev/openobject-addons/trunk-workflow-context-odo/+merge/85530
)

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server

Olivier Dony (OpenERP)
In reply to this post by Raphael Valyi
On 12/13/2011 11:49 PM, Raphaël Valyi - http://www.akretion.com wrote:
> 2) we also have an issue with the methods the workflow call: those methods
> are easily called from different paths and easily they need the standard
> context system to be modular.
> Look at such methods:
> http://bazaar.launchpad.net/~akretion-team/openobject-addons/sale-modular-picking-better-context/revision/5761
> or
> http://bazaar.launchpad.net/~akretion-team/openobject-addons/sale-modular-picking-better-context/revision/5760

To me this is a separate issue, and I don't see any reason why we should
not have these business methods conform to the API, and take a proper
context, just like all the others.
Not having the context provided in some cases is something else.


> So at the end what would we decide:
> 1) workflow propagate the context (it doesn't mean it should use it itself;
> this can be checked) ?

If we can have the transaction context propagate to business code
without being available inside the workflow engine, then why not.
But if the easiest method to do that in 6.1 is to have the wkf engine
propagate it, then perhaps it's worth it.


> 2) the methods called by workflow don't propagate the context at all either
> (sounds it sucks to me, cause would force overkill database accesses when
> we are already not that fast in transactions)

Many of these methods will be or will call normal business methods, so I
don't see why they should follow a different contract.


> 3) the methods called by the workflow engine, pass the context if they have
> some passed. However the workflow engine don't give them the context, so
> it's useful only in overrides or when those methods are called outside from
> the workflow engine.

Could be a partial answer to the issue, but it looks to me this wouldn't
be worth the trouble for such poor outcome. In that case we can accept
MPs that add the context where people need it for special purposes, but
not bother with adding it to all workflow-called methods by default.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server

Olivier Dony (OpenERP)
In reply to this post by Fabien (Open ERP)
On 12/13/2011 08:58 PM, Fabien (Open ERP) wrote:
> Olivier,
>
> Are you sure it's a good idea to allow context in workflows ?
>
> Workflows are not used for user interface interactions, but for
> controlling the
> evolution of the object. It has no sense to put context on constraints
> and, for
> the same reason, I don't think we should allow context in workflows.

Actually, workflows execute business operations requested by users, and
are mostly called in the context of user transactions. When a workflow
button sends a signal, the workflow engine actually produces the result
to send back to the user, by invoking business code.
Granted, the workflow activities themselves don't need the context, but
the business code they are calling does.


> An activity should not depend on a context but on data stored on the object.
> The goal of the workflow is to easily modify it, add/remove some
> transitions, etc.
> If we depend on contexts, we may loose this modularity.

Indeed the goal is not to make the workflow itself depend on the
context, but to have it propagate it to the business code that it calls.
The workflow is a controller that invokes business code in the context
of a user transaction, and business code is in general context-aware, so
why would the workflow engine break this contract?


> I am still not sure about this, but I would not allow the context to
> be used in workflows, for the same reason that it has no sense to
> put context on constraints.

We don't want constraints to be context-aware because it would be a bad
incentive for developers. And they really don't need it, because
constraints should never alter the data in any way (not call mutating
business code) and have their error messages properly localized by the
ORM. I think this is different for the workflow engine, which acts as a
controller.

BTW, we'll have to address this in a more global way for next release if
we change the API to have a thread-local (or similar) handling of the
transaction context. If we can allow that transaction context to be
propagated to business code in all circumstances, and that without
making it available to the actual workflow engine, I'm happy with it.
However if we have to choose, it looks to me that having business code
behave properly in all circumstances (even when called via workflows) is
a more important goal than preventing the design of context-dependent
workflows.

--
https://code.launchpad.net/~openerp-community/openobject-server/context-in-workflows/+merge/85518
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/context-in-workflows.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server

Raphael Valyi
Ok,

So Olivier and Fabien, I'm happy that it seems we totally share the same
opinion: none of us wants workflow behavior to be context driven, this
would be bad. BUT, there is not valid reason usual business methods to be
half broken because today when they are invoked through the workflow they
receive no context.

What I propose is:

1) as a first step: change the signature of those common business methods
such as:
http://bazaar.launchpad.net/~akretion-team/openobject-addons/sale-modular-picking-better-context/revision/5761
or
http://bazaar.launchpad.net/~akretion-team/openobject-addons/sale-modular-picking-better-context/revision/5760
So they accept the standard context argument and properly propagate it.

Honestly there aren't too many such actions, if we quickly review stock,
sale, purchase and account, with probably a change in around 20 methods I'm
pretty sure we would cover 90% of the use cases.

I insist today those methods usually have a nonsense useless *args argument
that limit a lot their capabilities when overridden or when calling other
normal business methods. So I mean this is not like if we were breaking
something already self consistent: it's not consistent today.
Today that *args is not used, so it's quite safe I think to replace it by a
"context" arg, basic testing would ensure we are not breaking anything.


2) Eventually, still up to debate, the workflow engine propagate the
context (I'm not 100% sure but I would go for that while still having the
strong policy that workflow themselves should not be context dependent).
If the workflow engine starts passing the context, we should avoid passing
the context where it's not expected. May be one way is to simply pass it as
the last argument (would not break where *args is expected) rather than a
keyword argument, OR, eventually we do some signature checking before
calling as Albert did (sounds less good to me but not sure).


3) Eventually workflow calls and clients can be modified to properly pass
the context when triggering workflow actions. But once most of the common
business signatures are in place, to me this is no API change so this could
be gradually added during the 6.1 release life cycle.


As for keeping the context and the cursor and uid in thread local, of
course, I think this will need to be done in the future. But this may be
effective may be only in 2 years, so we also need a decent solution for the
next 2 years.



On Wed, Dec 14, 2011 at 8:29 AM, Olivier Dony (OpenERP) <[hidden email]>wrote:

> On 12/13/2011 08:58 PM, Fabien (Open ERP) wrote:
> > Olivier,
> >
> > Are you sure it's a good idea to allow context in workflows ?
> >
> > Workflows are not used for user interface interactions, but for
> > controlling the
> > evolution of the object. It has no sense to put context on constraints
> > and, for
> > the same reason, I don't think we should allow context in workflows.
>
> Actually, workflows execute business operations requested by users, and
> are mostly called in the context of user transactions. When a workflow
> button sends a signal, the workflow engine actually produces the result
> to send back to the user, by invoking business code.
> Granted, the workflow activities themselves don't need the context, but
> the business code they are calling does.
>
>
> > An activity should not depend on a context but on data stored on the
> object.
> > The goal of the workflow is to easily modify it, add/remove some
> > transitions, etc.
> > If we depend on contexts, we may loose this modularity.
>
> Indeed the goal is not to make the workflow itself depend on the
> context, but to have it propagate it to the business code that it calls.
> The workflow is a controller that invokes business code in the context
> of a user transaction, and business code is in general context-aware, so
> why would the workflow engine break this contract?
>
>
> > I am still not sure about this, but I would not allow the context to
> > be used in workflows, for the same reason that it has no sense to
> > put context on constraints.
>
> We don't want constraints to be context-aware because it would be a bad
> incentive for developers. And they really don't need it, because
> constraints should never alter the data in any way (not call mutating
> business code) and have their error messages properly localized by the
> ORM. I think this is different for the workflow engine, which acts as a
> controller.
>
> BTW, we'll have to address this in a more global way for next release if
> we change the API to have a thread-local (or similar) handling of the
> transaction context. If we can allow that transaction context to be
> propagated to business code in all circumstances, and that without
> making it available to the actual workflow engine, I'm happy with it.
> However if we have to choose, it looks to me that having business code
> behave properly in all circumstances (even when called via workflows) is
> a more important goal than preventing the design of context-dependent
> workflows.
>
> --
>
> https://code.launchpad.net/~openerp-community/openobject-server/context-in-workflows/+merge/85518
> You proposed lp:~openerp-community/openobject-server/context-in-workflows
> for merging.
>

--
https://code.launchpad.net/~openerp-community/openobject-server/context-in-workflows/+merge/85518
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/context-in-workflows.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server

Stefan Rijnhart (Therp)

> 2) Eventually, still up to debate, the workflow engine propagate the
> context (I'm not 100% sure but I would go for that while still having the
> strong policy that workflow themselves should not be context dependent).
> If the workflow engine starts passing the context, we should avoid passing
> the context where it's not expected. May be one way is to simply pass it as
> the last argument (would not break where *args is expected) rather than a
> keyword argument, OR, eventually we do some signature checking before
> calling as Albert did (sounds less good to me but not sure).

The simplest is just to add the keyword argument. If you install all addons from openobject-addons/trunk (well, n10l_de breaks for me) you can get an idea of the scale of the impact. The query "SELECT wkf.osv, wkf_activity.action FROM wkf, wkf_activity WHERE wkf_id = wkf.id AND kind =  'function' AND action LIKE '%()%'" gives you some 80 methods to convert.

The person to prepare such a merge could temporarily hack in Nan's introspection check to iterate over these methods at loading time to see if all have been covered.

The question is whether this 'last minute'(?) API change is fair towards 3rd party addon developers but AFAIK stranger things have been pulled off in this community ;-) In that respect it would be informative when a proper RC1 is planned.

Cheers,
Stefan.

--
https://code.launchpad.net/~openerp-community/openobject-server/context-in-workflows/+merge/85518
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/context-in-workflows.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server

Olivier Dony (OpenERP)
On 12/14/2011 02:28 PM, Stefan Rijnhart (Therp) wrote:
> The simplest is just to add the keyword argument. If you install all
> addons from openobject-addons/trunk (well, n10l_de breaks for me) you
> can get an idea of the scale of the impact. The query "SELECT
> wkf.osv, wkf_activity.action FROM wkf, wkf_activity WHERE wkf_id =
> wkf.id AND kind =  'function' AND action LIKE '%()%'" gives you some
> 80 methods to convert.

You also need to be careful with a few other fields that are evaluated
during the course of workflow execution, such as 'trigger_expr_id' or
'condition' on workflow transitions.


> The person to prepare such a merge could temporarily hack in Nan's
> introspection check to iterate over these methods at loading time to
> see if all have been covered.

Yup. Similarly, changing browse_record to default to an empty dict
context when no context is passed would make it pass that context
automatically to all workflow-called methods. If this does not trigger
too many false positives in other places, running the full test suite
(as runbot does) with such a server would validate that all relevant
methods have been indeed updated.


> The question is whether this 'last minute'(?) API change is fair
> towards 3rd party addon developers but AFAIK stranger things have
> been pulled off in this community ;-) In that respect it would be
> informative when a proper RC1 is planned.

This seems acceptable before releasing RC1, but we probably won't have
the resources to do it internally. If anyone from the community is ready
to do this (just changing the API of business methods called by
workflows to allow an optional context), we would however be happy to
review and merge it.
RC1 is planned very soon (if possible before the end of the year),
provided we are able to finish stabilizing the new web client and bring
down bugs and merge props to an acceptable level (something like 0
remaining Medium+ bugs, less than 40 pending R&D merge proposals, and 0
pending merge proposals from the community)
This post[1] from Fabien on the forum provides some more details on what
we're still working on.

[1] http://www.openerp.com/forum/post96294.html#p96294

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-server/context-in-workflows into lp:openobject-server

Olivier Dony (OpenERP)
In reply to this post by Stefan Rijnhart (Therp)
On 12/14/2011 02:28 PM, Stefan Rijnhart (Therp) wrote:
> The simplest is just to add the keyword argument. If you install all
> addons from openobject-addons/trunk (well, n10l_de breaks for me) you
> can get an idea of the scale of the impact. The query "SELECT
> wkf.osv, wkf_activity.action FROM wkf, wkf_activity WHERE wkf_id =
> wkf.id AND kind =  'function' AND action LIKE '%()%'" gives you some
> 80 methods to convert.

You also need to be careful with a few other fields that are evaluated
during the course of workflow execution, such as 'trigger_expr_id' or
'condition' on workflow transitions.


> The person to prepare such a merge could temporarily hack in Nan's
> introspection check to iterate over these methods at loading time to
> see if all have been covered.

Yup. Similarly, changing browse_record to default to an empty dict
context when no context is passed would make it pass that context
automatically to all workflow-called methods. If this does not trigger
too many false positives in other places, running the full test suite
(as runbot does) with such a server would validate that all relevant
methods have been indeed updated.


> The question is whether this 'last minute'(?) API change is fair
> towards 3rd party addon developers but AFAIK stranger things have
> been pulled off in this community ;-) In that respect it would be
> informative when a proper RC1 is planned.

This seems acceptable before releasing RC1, but we probably won't have
the resources to do it internally. If anyone from the community is ready
to do this (just changing the API of business methods called by
workflows to allow an optional context), we would however be happy to
review and merge it.
RC1 is planned very soon (if possible before the end of the year),
provided we are able to finish stabilizing the new web client and bring
down bugs and merge props to an acceptable level (something like 0
remaining Medium+ bugs, less than 40 pending R&D merge proposals, and 0
pending merge proposals from the community)
This post[1] from Fabien on the forum provides some more details on what
we're still working on.

[1] http://www.openerp.com/forum/post96294.html#p96294


PS: sorry if you receive this msg twice, I'm re-posting to the mp for
the record.

--
https://code.launchpad.net/~openerp-community/openobject-server/context-in-workflows/+merge/85518
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/context-in-workflows.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp