Issue 427811003: trytond: Support MultiSelection in MultiValue field (Closed)
DescriptionPatch Set 1 #
Total comments: 1
Patch Set 2 : #
Total comments: 7
Patch Set 3 : Add support to MultiSelection in MultiValue field. #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 4
Patch Set 8 : #
Total comments: 1
Patch Set 9 : #
Total comments: 11
Patch Set 10 : Fix format #Patch Set 11 : Simplify and normalize tests #
MessagesTotal messages: 34
patch is not applicable URL: https://codereview.tryton.org/427811003
Sign in to reply to this message.
https://codereview.tryton.org/427811003/diff/427801004/trytond/trytond/model/... File trytond/trytond/model/multivalue.py (right): https://codereview.tryton.org/427811003/diff/427801004/trytond/trytond/model/... trytond/trytond/model/multivalue.py:71: and Value._fields[name]._type != 'multiselection'): I think I would prefer to test that it is a list of instances.
Sign in to reply to this message.
We need a test to verify the behavior.
Sign in to reply to this message.
patch is not applicable URL: https://codereview.tryton.org/427811003
Sign in to reply to this message.
On 2022/09/26 15:32:38, reviewbot wrote: > patch is not applicable > URL: https://codereview.tryton.org/427811003 You must upload the patch from trytond directory.
Sign in to reply to this message.
https://codereview.tryton.org/427811003/diff/419851005/trytond/trytond/model/... File trytond/trytond/model/multivalue.py (right): https://codereview.tryton.org/427811003/diff/419851005/trytond/trytond/model/... trytond/trytond/model/multivalue.py:72: return [r.id for r in value] To avoid double loop maybe it is better to have: return [r.id if isinstance(r, Mode) else r for r in value] https://codereview.tryton.org/427811003/diff/419851005/trytond/trytond/model/... trytond/trytond/model/multivalue.py:72: return [r.id for r in value] I think it will be better to return a tuple (not directly linked to the issue but as this part is changed... https://codereview.tryton.org/427811003/diff/419851005/trytond/trytond/tests/... File trytond/trytond/tests/multivalue.py (right): https://codereview.tryton.org/427811003/diff/419851005/trytond/trytond/tests/... trytond/trytond/tests/multivalue.py:24: if field in {'value', 'value_default', 'value_model', better to break line at opening { https://codereview.tryton.org/427811003/diff/419851005/trytond/trytond/tests/... File trytond/trytond/tests/test_multivalue.py (right): https://codereview.tryton.org/427811003/diff/419851005/trytond/trytond/tests/... trytond/trytond/tests/test_multivalue.py:35: ("foo",)) I think it is better to have a test for each kind of field https://codereview.tryton.org/427811003/diff/419851005/trytond/trytond/tests/... trytond/trytond/tests/test_multivalue.py:104: self.assertEqual(value.value_multiselection, ('bar', 'foo')) idem better to have a test for each type of field https://codereview.tryton.org/427811003/diff/419851005/trytond/trytond/tests/... trytond/trytond/tests/test_multivalue.py:166: ("bar", "foo")) idem https://codereview.tryton.org/427811003/diff/419851005/trytond/trytond/tests/... trytond/trytond/tests/test_multivalue.py:186: self.assertEqual(read['value_multiselection'], ("bar", "foo")) idem
Sign in to reply to this message.
https://codereview.tryton.org/427811003/diff/418131003/trytond/tests/multival... trytond/tests/multivalue.py:17: continuation line under-indented for hanging indent https://codereview.tryton.org/427811003/diff/418131003/trytond/tests/multival... trytond/tests/multivalue.py:54: continuation line under-indented for hanging indent URL: https://codereview.tryton.org/427811003
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/427811003
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/427811003
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/427811003
Sign in to reply to this message.
https://codereview.tryton.org/427811003/diff/423941003/trytond/tests/test_mul... File trytond/tests/test_multivalue.py (right): https://codereview.tryton.org/427811003/diff/423941003/trytond/tests/test_mul... trytond/tests/test_multivalue.py:175: None) For me this test should have the code of test_multivalue_getter_many2one and test_multivalue_getter_many2one should be removed. Also I would put next after the similar test test_get_multivalue_* https://codereview.tryton.org/427811003/diff/423941003/trytond/tests/test_mul... trytond/tests/test_multivalue.py:192: ("foo",)) idem there is no point to test twice the default value behavior. https://codereview.tryton.org/427811003/diff/423941003/trytond/tests/test_mul... trytond/tests/test_multivalue.py:228: def test_multivalue_setter_many2one(self): For me setter should be tested only once with the simple field because there is nothing special per type of field. https://codereview.tryton.org/427811003/diff/423941003/trytond/tests/test_mul... trytond/tests/test_multivalue.py:255: def test_multivalue_getter_many2one(self): There should be a also a getter test for simple fields and one for reference field.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/427811003
Sign in to reply to this message.
https://codereview.tryton.org/427811003/diff/447301003/trytond/tests/test_mul... File trytond/tests/test_multivalue.py (right): https://codereview.tryton.org/427811003/diff/447301003/trytond/tests/test_mul... trytond/tests/test_multivalue.py:278: '%s,%s' % (ReferenceModel.__name__, str(test_reference.id))) Could put each kind of new test following the same kind of existing tests so they are grouped by kind of test.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/427811003
Sign in to reply to this message.
https://codereview.tryton.org/427811003/diff/423981003/trytond/tests/multival... File trytond/tests/multivalue.py (right): https://codereview.tryton.org/427811003/diff/423981003/trytond/tests/multival... trytond/tests/multivalue.py:15: ('foo', "Foo"), wrong indentation https://codereview.tryton.org/427811003/diff/423981003/trytond/tests/multival... trytond/tests/multivalue.py:20: (None, ''), wrong indentation https://codereview.tryton.org/427811003/diff/423981003/trytond/tests/multival... trytond/tests/multivalue.py:35: } wrong indentation https://codereview.tryton.org/427811003/diff/423981003/trytond/tests/test_mul... File trytond/tests/test_multivalue.py (right): https://codereview.tryton.org/427811003/diff/423981003/trytond/tests/test_mul... trytond/tests/test_multivalue.py:216: record.set_multivalue('value_multiselection', ("foo", "bar"), break line at opening ( https://codereview.tryton.org/427811003/diff/423981003/trytond/tests/test_mul... trytond/tests/test_multivalue.py:234: record.set_multivalue('value_reference', test_reference, idem https://codereview.tryton.org/427811003/diff/423981003/trytond/tests/test_mul... trytond/tests/test_multivalue.py:277: self.assertEqual(read['value_reference'], break line at opening (
Sign in to reply to this message.
https://codereview.tryton.org/427811003/diff/423981003/trytond/tests/multival... File trytond/tests/multivalue.py (right): https://codereview.tryton.org/427811003/diff/423981003/trytond/tests/multival... trytond/tests/multivalue.py:15: ('foo', "Foo"), On 2022/10/06 06:48:06, ced wrote: > wrong indentation I don't know what the correct identation would be. I think I have done it according to other examples I have already seen. https://codereview.tryton.org/427811003/diff/423981003/trytond/tests/multival... trytond/tests/multivalue.py:20: (None, ''), On 2022/10/06 06:48:06, ced wrote: > wrong indentation I don't know what the correct identation would be. I think I have done it according to other examples I have already seen. https://codereview.tryton.org/427811003/diff/423981003/trytond/tests/multival... trytond/tests/multivalue.py:35: } On 2022/10/06 06:48:06, ced wrote: > wrong indentation I don't know what the correct identation would be.
Sign in to reply to this message.
https://codereview.tryton.org/427811003/diff/423981003/trytond/tests/multival... File trytond/tests/multivalue.py (right): https://codereview.tryton.org/427811003/diff/423981003/trytond/tests/multival... trytond/tests/multivalue.py:15: ('foo', "Foo"), On 2022/10/06 07:12:41, tiyujopite wrote: > On 2022/10/06 06:48:06, ced wrote: > > wrong indentation > > I don't know what the correct identation would be. I think I have done it > according to other examples I have already seen. 4 spaces per opened parenthesis or braket.
Sign in to reply to this message.
https://codereview.tryton.org/427811003/diff/423981003/trytond/tests/multival... File trytond/tests/multivalue.py (right): https://codereview.tryton.org/427811003/diff/423981003/trytond/tests/multival... trytond/tests/multivalue.py:15: ('foo', "Foo"), On 2022/10/06 08:02:23, ced wrote: > On 2022/10/06 07:12:41, tiyujopite wrote: > > On 2022/10/06 06:48:06, ced wrote: > > > wrong indentation > > > > I don't know what the correct identation would be. I think I have done it > > according to other examples I have already seen. > > 4 spaces per opened parenthesis or braket. See https://www.tryton.org/develop/guidelines/code#python
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/427811003
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/427811003
Sign in to reply to this message.
New changeset bd9ad6eedf5c by José Antonio Díaz in branch 'default': Support MultiSelection in MultiValue field https://hg.tryton.org/trytond/rev/bd9ad6eedf5c
Sign in to reply to this message.
New changeset bac74c2ea3cd by José Antonio Díaz in branch '6.4': Support MultiSelection in MultiValue field https://hg.tryton.org/trytond/rev/bac74c2ea3cd New changeset 9444c2eb9be8 by José Antonio Díaz in branch '6.2': Support MultiSelection in MultiValue field https://hg.tryton.org/trytond/rev/9444c2eb9be8 New changeset ce982fabd69a by José Antonio Díaz in branch '6.0': Support MultiSelection in MultiValue field https://hg.tryton.org/trytond/rev/ce982fabd69a
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
