[Openerp-community] [Merge] lp:~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss into lp:~openerp-community/server-env-tools/6.1-mass_editing
Stefan Rijnhart (Therp) has proposed merging
lp:~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss into
lp:~openerp-community/server-env-tools/6.1-mass_editing.
Requested reviews:
Maxime Chambreuil (http://www.savoirfairelinux.com) (max3903)
For more details, see:
https://code.launchpad.net/~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss/+merge/201321
--
https://code.launchpad.net/~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss/+merge/201321
Your team OpenERP Community is subscribed to branch
lp:~openerp-community/server-env-tools/6.1-mass_editing.
=== modified file 'mass_editing/wizard/mass_editing_wizard.py'
--- mass_editing/wizard/mass_editing_wizard.py 2013-08-20 03:54:59 +
+++ mass_editing/wizard/mass_editing_wizard.py 2014-01-12 19:53:19 +
@@ -107,7 +107,10 @@
elif val == 'remove':
dict.update({split_key: False})
elif val == 'remove_m2m':
-dict.update({split_key: [(5,0,[])]})
+m2m_list = []
+for m2m_id in vals.get(split_key, False)[0][2]:
+m2m_list.append((3, m2m_id))
+dict.update({split_key: m2m_list})
elif val == 'add':
m2m_list = []
for m2m_id in vals.get(split_key, False)[0][2]:
___
Mailing list: https://launchpad.net/~openerp-community
Post to : openerp-community@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openerp-community
More help : https://help.launchpad.net/ListHelp
[Openerp-community] [Merge] lp:~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss into lp:~openerp-community/server-env-tools/6.1-mass_editing
The proposal to merge lp:~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss into lp:~openerp-community/server-env-tools/6.1-mass_editing has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss/+merge/201321 -- https://code.launchpad.net/~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss/+merge/201321 Your team OpenERP Community is subscribed to branch lp:~openerp-community/server-env-tools/6.1-mass_editing. ___ Mailing list: https://launchpad.net/~openerp-community Post to : openerp-community@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community] [Merge] lp:~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss into lp:~openerp-community/server-env-tools/6.1-mass_editing
Review: Needs Fixing vals.get(split_key, False)[0][2] When split_key is not in vals, we'll have a problem :-) -- https://code.launchpad.net/~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss/+merge/201321 Your team OpenERP Community is subscribed to branch lp:~openerp-community/server-env-tools/6.1-mass_editing. ___ Mailing list: https://launchpad.net/~openerp-community Post to : openerp-community@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community] [Merge] lp:~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss into lp:~openerp-community/server-env-tools/6.1-mass_editing
Review: Needs Fixing
Instead of dict.update({split_key: m2m_list})
You can simply write
dict['split_key'] = m2m_list
It might be a bit more readable
--
https://code.launchpad.net/~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss/+merge/201321
Your team OpenERP Community is subscribed to branch
lp:~openerp-community/server-env-tools/6.1-mass_editing.
___
Mailing list: https://launchpad.net/~openerp-community
Post to : openerp-community@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openerp-community
More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community] [Merge] lp:~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss into lp:~openerp-community/server-env-tools/6.1-mass_editing
Il 17/03/2014 17:38, Yannick Vaucher @ Camptocamp ha scritto:
Review: Needs Fixing
Instead of dict.update({split_key: m2m_list})
You can simply write
dict['split_key'] = m2m_list
It might be a bit more readable
If you use dict['split_key'], are you sure 'split_key' key exists in
dict? If you use update, python create the key for you if not exist or
update it if exists yet.
___
Mailing list: https://launchpad.net/~openerp-community
Post to : openerp-community@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openerp-community
More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community] [Merge] lp:~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss into lp:~openerp-community/server-env-tools/6.1-mass_editing
Thanks for the comments. Now using vals[model_field][0][2] or [], taking Francesco's response on the mailing list into account. Changing some variable names after complaints by pylint. -- https://code.launchpad.net/~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss/+merge/201321 Your team OpenERP Community is subscribed to branch lp:~openerp-community/server-env-tools/6.1-mass_editing. ___ Mailing list: https://launchpad.net/~openerp-community Post to : openerp-community@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community] [Merge] lp:~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss into lp:~openerp-community/server-env-tools/6.1-mass_editing
Review: Needs Fixing code review @Stefan: I thin you missed the point of vals[model_field]: There is a chance of a KeyError if model_field not in val and a chance of index error if len(vals[model_field]) < 1 or len(vals[model_field][0]) < 3 -- https://code.launchpad.net/~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss/+merge/201321 Your team OpenERP Community is subscribed to branch lp:~openerp-community/server-env-tools/6.1-mass_editing. ___ Mailing list: https://launchpad.net/~openerp-community Post to : openerp-community@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community] [Merge] lp:~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss into lp:~openerp-community/server-env-tools/6.1-mass_editing
@Sandy, This code relies on the constructed view which guarantees that when selection_fieldname occurs in vals, then so does 'fieldname'. In the case of m2m, it's value is guaranteed to be in the form [(6, 0, list)], so accessing the list through vals[model_field][0][2] should be fine. -- https://code.launchpad.net/~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss/+merge/201321 Your team OpenERP Community is subscribed to branch lp:~openerp-community/server-env-tools/6.1-mass_editing. ___ Mailing list: https://launchpad.net/~openerp-community Post to : openerp-community@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community] [Merge] lp:~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss into lp:~openerp-community/server-env-tools/6.1-mass_editing
Review: Approve code review, no test OK, sorry about the misunderstanding. Nevertheless, I would prefer to see some assertions, gets and tries as this function looks likely to throw an IndexError. But I see you've take them into account (removal of redundant checks, values checking, etc) -- https://code.launchpad.net/~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss/+merge/201321 Your team OpenERP Community is subscribed to branch lp:~openerp-community/server-env-tools/6.1-mass_editing. ___ Mailing list: https://launchpad.net/~openerp-community Post to : openerp-community@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community] [Merge] lp:~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss into lp:~openerp-community/server-env-tools/6.1-mass_editing
Review: Approve -- https://code.launchpad.net/~therp-nl/server-env-tools/6.1-mass_editing-fix_dataloss/+merge/201321 Your team OpenERP Community is subscribed to branch lp:~openerp-community/server-env-tools/6.1-mass_editing. ___ Mailing list: https://launchpad.net/~openerp-community Post to : openerp-community@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community More help : https://help.launchpad.net/ListHelp
