[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