Implementation of triggers

[udono, 2010-05-18 06:08:04]

Have not test it, but looks good. Comments and questions added.

http://codereview.appspot.com/1253042/diff/1/2
File doc/ref/models/models.rst (right):

http://codereview.appspot.com/1253042/diff/1/2#newcode209
doc/ref/models/models.rst:209: ``on_create`` is set and ``condition`` is true.
The text doesn't fit to the signature. Maybe better first explain ``id``, and
then come to the relation to ir.triggger?

Missing doc for ir.trigger.

http://codereview.appspot.com/1253042/diff/1/2#newcode227
doc/ref/models/models.rst:227: Return eligible record ids for write actions by
triggers. This dictionary
Better to explain what an 'eligible' record for a trigger is.
And the signature sounds like 'eligible recorts are written'. Isn't the naming
'get_eligibles' not better?

http://codereview.appspot.com/1253042/diff/1/8
File trytond/model/modelstorage.py (right):

http://codereview.appspot.com/1253042/diff/1/8#newcode97
trytond/model/modelstorage.py:97: :param id: the created id
the id for the create triggers?

http://codereview.appspot.com/1253042/diff/1/8#newcode111
trytond/model/modelstorage.py:111: def read(self, cursor, user, ids,
fields_names=None, context=None):
Why not having read triggers? I think of synchronizing with LDAP. Or is it a
misleading aproach?

http://codereview.appspot.com/1253042/diff/1/8#newcode167
trytond/model/modelstorage.py:167: def trigger_write_eligibles(self, cursor,
user, ids, context=None):
Don't know if the naming is good? actually when using this method, I 'get'
something, and not 'write' it somewhere.
What about trigger_get_write_eligibles? or just trigger_get_eligibles

http://codereview.appspot.com/1253042/diff/1/8#newcode173
trytond/model/modelstorage.py:173: :param ids: a list of ids or an id
a list of ids or an id for the write triggers?

http://codereview.appspot.com/1253042/diff/1/8#newcode257
trytond/model/modelstorage.py:257: :param ids: the deleted ids
The ids for delete triggers?

[ced, 2010-05-18 06:35:15]

http://codereview.appspot.com/1253042/diff/1/2
File doc/ref/models/models.rst (right):

http://codereview.appspot.com/1253042/diff/1/2#newcode227
doc/ref/models/models.rst:227: Return eligible record ids for write actions by
triggers. This dictionary
On 2010/05/18 06:08:05, udono wrote:
> Better to explain what an 'eligible' record for a trigger is.
> And the signature sounds like 'eligible recorts are written'. Isn't the naming
> 'get_eligibles' not better?

param doc is in docstring. Here it is a explanation of the method.

http://codereview.appspot.com/1253042/diff/1/8
File trytond/model/modelstorage.py (right):

http://codereview.appspot.com/1253042/diff/1/8#newcode111
trytond/model/modelstorage.py:111: def read(self, cursor, user, ids,
fields_names=None, context=None):
On 2010/05/18 06:08:05, udono wrote:
> Why not having read triggers? I think of synchronizing with LDAP. Or is it a
> misleading aproach?
Read doesn't change anything.

[yangoon1, 2010-05-18 14:56:18]

http://codereview.appspot.com/1253042/diff/6001/7004
File trytond/ir/trigger.py (right):

http://codereview.appspot.com/1253042/diff/6001/7004#newcode17
trytond/ir/trigger.py:17: on_time = fields.Boolean('On Time', select=1, states={
Just a general remark at first glance:
Generally it is better to have some help texts for the usage, not only error
messages after wrong usage.

[ced, 2010-05-18 18:17:44]

http://codereview.appspot.com/1253042/diff/6001/7004
File trytond/ir/trigger.py (right):

http://codereview.appspot.com/1253042/diff/6001/7004#newcode17
trytond/ir/trigger.py:17: on_time = fields.Boolean('On Time', select=1, states={
On 2010/05/18 14:56:19, yangoon wrote:
> Just a general remark at first glance:
> Generally it is better to have some help texts for the usage, not only error
> messages after wrong usage.

Here there will be no error message for wrong usage.
And I find field string self-explained.

[yangoon1, 2010-05-18 18:41:37]

http://codereview.appspot.com/1253042/diff/6001/7004
File trytond/ir/trigger.py (right):

http://codereview.appspot.com/1253042/diff/6001/7004#newcode17
trytond/ir/trigger.py:17: on_time = fields.Boolean('On Time', select=1, states={
On 2010/05/18 18:17:45, ced wrote:
> On 2010/05/18 14:56:19, yangoon wrote:
> > Just a general remark
...
> Here there will be no error message for wrong usage.
> And I find field string self-explained.

A general remark applies obviously not only to a specific field.
It is preferable that a user won't run into
'"On Time" and others are mutually exclusive!'
resp.
'Condition must be a python expression!'
explaining him the use of those fields directly.

[ced, 2010-05-18 18:55:34]

http://codereview.appspot.com/1253042/diff/6001/7004
File trytond/ir/trigger.py (right):

http://codereview.appspot.com/1253042/diff/6001/7004#newcode17
trytond/ir/trigger.py:17: on_time = fields.Boolean('On Time', select=1, states={
On 2010/05/18 18:41:37, yangoon wrote:
> On 2010/05/18 18:17:45, ced wrote:
> > On 2010/05/18 14:56:19, yangoon wrote:
> > > Just a general remark
> ...
> > Here there will be no error message for wrong usage.
> > And I find field string self-explained.
> 
> A general remark applies obviously not only to a specific field.
> It is preferable that a user won't run into
> '"On Time" and others are mutually exclusive!'
> resp.
> 'Condition must be a python expression!'
> explaining him the use of those fields directly.
> 
This will not happen if you use the client.

[udono, 2010-05-23 14:32:24]

http://codereview.appspot.com/1253042/diff/19001/20009
File trytond/res/user.py (right):

http://codereview.appspot.com/1253042/diff/19001/20009#newcode86
trytond/res/user.py:86: def init(self, cursor, module_name):
Is this related to implementation of triggers?

[ced, 2010-05-24 06:51:41]

http://codereview.appspot.com/1253042/diff/19001/20009
File trytond/res/user.py (right):

http://codereview.appspot.com/1253042/diff/19001/20009#newcode86
trytond/res/user.py:86: def init(self, cursor, module_name):
On 2010/05/23 14:32:24, udono wrote:
> Is this related to implementation of triggers?

This is required for migration. But I will surely commit in separate.