Issue 91043: Work - request relation, m2m implementation (Closed)
Patch Set 1
Total comments: 14
Patch Set 2 : Modifified wrt to comments
Total comments: 6
Patch Set 3 : Use IN_MAX for computing requests field. Removed needless to_delete.MessagesTotal messages: 16
http://codereview.appspot.com/91043/diff/1/2 File work.py (right): http://codereview.appspot.com/91043/diff/1/2#newcode9 Line 9: import itertools Not used http://codereview.appspot.com/91043/diff/1/2#newcode108 Line 108: _, work_id = req_ref.reference.split(',') work_id could be empty http://codereview.appspot.com/91043/diff/1/2#newcode179 Line 179: current = dict((req.id, req) for req in work.requests) currents http://codereview.appspot.com/91043/diff/1/2#newcode186 Line 186: if operator == 'set': I think it is better to make the operation directly like in model/fields/many2many.py http://codereview.appspot.com/91043/diff/1/2#newcode202 Line 202: print to_unlink print statement http://codereview.appspot.com/91043/diff/1/2#newcode210 Line 210: print req_ref_ids print statement
Sign in to reply to this message.
http://codereview.appspot.com/91043/diff/1/2 File work.py (right): http://codereview.appspot.com/91043/diff/1/2#newcode9 Line 9: import itertools On 2009/07/03 10:44:44, ced wrote: > Not used Done. http://codereview.appspot.com/91043/diff/1/2#newcode108 Line 108: _, work_id = req_ref.reference.split(',') On 2009/07/03 10:44:44, ced wrote: > work_id could be empty Why, isn't a reference always "model,id" ? http://codereview.appspot.com/91043/diff/1/2#newcode179 Line 179: current = dict((req.id, req) for req in work.requests) On 2009/07/03 10:44:44, ced wrote: > currents Done. http://codereview.appspot.com/91043/diff/1/2#newcode186 Line 186: if operator == 'set': On 2009/07/03 10:44:44, ced wrote: > I think it is better to make the operation directly like in > model/fields/many2many.py Done. http://codereview.appspot.com/91043/diff/1/2#newcode202 Line 202: print to_unlink On 2009/07/03 10:44:44, ced wrote: > print statement Done. http://codereview.appspot.com/91043/diff/1/2#newcode210 Line 210: print req_ref_ids On 2009/07/03 10:44:44, ced wrote: > print statement Done.
Sign in to reply to this message.
http://codereview.appspot.com/91043/diff/1/2 File work.py (right): http://codereview.appspot.com/91043/diff/1/2#newcode108 Line 108: _, work_id = req_ref.reference.split(',') On 2009/07/03 14:51:47, Bertrand Chenal wrote: > On 2009/07/03 10:44:44, ced wrote: > > work_id could be empty > > Why, isn't a reference always "model,id" ? Tryton allow to have empty id.
Sign in to reply to this message.
http://codereview.appspot.com/91043/diff/1/2 File work.py (right): http://codereview.appspot.com/91043/diff/1/2#newcode108 Line 108: _, work_id = req_ref.reference.split(',') But the search clause will ensure that there is an id. So it is ok
Sign in to reply to this message.
http://codereview.appspot.com/91043/diff/6/1004 File work.py (right): http://codereview.appspot.com/91043/diff/6/1004#newcode65 Line 65: string='Requests', fnct_inv='set_function_fields', I think it is better to use a widget one2many in views. http://codereview.appspot.com/91043/diff/6/1004#newcode102 Line 102: ('reference', 'in', ['project.work,%s' % i for i in ids]), You should create a loop with cursor.IN_MAX for long ids list http://codereview.appspot.com/91043/diff/6/1004#newcode185 Line 185: if operator == 'set': I think it should be better to have the same behavior then the many2many.py. So call the method directly instead of creating list. http://codereview.appspot.com/91043/diff/6/1004#newcode650 Line 650: Work() Remove the space in an other commit.
Sign in to reply to this message.
http://codereview.appspot.com/91043/diff/6/1004 File work.py (right): http://codereview.appspot.com/91043/diff/6/1004#newcode185 Line 185: if operator == 'set': On 2009/07/03 17:21:43, ced wrote: > I think it should be better to have the same behavior then the many2many.py. So > call the method directly instead of creating list. Why? here it's the same behavior (one write/delete per item in value) except when there is several ids in the second member of the item, in this case instead of calling the same action one by one, the action is called on the list.
Sign in to reply to this message.
http://codereview.appspot.com/91043/diff/6/1004 File work.py (right): http://codereview.appspot.com/91043/diff/6/1004#newcode185 Line 185: if operator == 'set': Exactly, we must have the same behavior.
Sign in to reply to this message.
On 2009/07/06 09:02:11, ced wrote: > http://codereview.appspot.com/91043/diff/6/1004 > File work.py (right): > > http://codereview.appspot.com/91043/diff/6/1004#newcode185 > Line 185: if operator == 'set': > Exactly, we must have the same behavior. How delete([1,2,3]) can be different from delete(1); delete(2); delete(3) ?
Sign in to reply to this message.
On 2009/07/06 09:03:58, Bertrand Chenal wrote: > How delete([1,2,3]) can be different from delete(1); delete(2); delete(3) ? If there is an ondelete=CASCADE that delete 2 or 3 when deleting 1
Sign in to reply to this message.
On 2009/07/06 09:15:39, ced wrote: > On 2009/07/06 09:03:58, Bertrand Chenal wrote: > > How delete([1,2,3]) can be different from delete(1); delete(2); delete(3) ? > > If there is an ondelete=CASCADE that delete 2 or 3 when deleting 1 It will be ok with one delete of a list and it may create an exception with several delete (depending of the order, sometimes it will create an exception sometimes not)
Sign in to reply to this message.
On 2009/07/06 09:30:46, Bertrand Chenal wrote: > On 2009/07/06 09:15:39, ced wrote: > > On 2009/07/06 09:03:58, Bertrand Chenal wrote: > > > How delete([1,2,3]) can be different from delete(1); delete(2); delete(3) ? > > > > If there is an ondelete=CASCADE that delete 2 or 3 when deleting 1 > > It will be ok with one delete of a list and it may create an exception with > several delete (depending of the order, sometimes it will create an exception > sometimes not) That is why we must have the same behavior!
Sign in to reply to this message.
On 2009/07/06 09:46:37, ced wrote: > > > > It will be ok with one delete of a list and it may create an exception with > > several delete (depending of the order, sometimes it will create an exception > > sometimes not) > > That is why we must have the same behavior! So it's important to mimic a default behavior that raise exceptions in a non-deterministic way, I don't see how it can be useful.
Sign in to reply to this message.
On 2009/07/06 09:51:24, Bertrand Chenal wrote: > On 2009/07/06 09:46:37, ced wrote: > > > > > > > It will be ok with one delete of a list and it may create an exception with > > > several delete (depending of the order, sometimes it will create an > exception > > > sometimes not) > > > > That is why we must have the same behavior! > > So it's important to mimic a default behavior that raise exceptions in a > non-deterministic way, I don't see how it can be useful. It is not non-deterministic and it is important to have the same behavior.
Sign in to reply to this message.
|
