Re: [Openerp-community-reviewer] [Merge] lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse
I've finally resubmitted this as part of the pull request at https://github.com/OCA/stock-logistics-warehouse/pull/17. Please reject the merge proposal here to make it clear it won't be processed on Launchpad. -- https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/223880 Your team Stock and Logistic Core Editors is requested to review the proposed merge of lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
[Openerp-community-reviewer] [Merge] lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse
The proposal to merge lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse has been updated. Status: Needs review = Work in progress For more details, see: https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/223880 -- https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/223880 Your team Stock and Logistic Core Editors is requested to review the proposed merge of lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse
Review: Resubmit The source code management for this project has been moved to https://github.com/OCA/stock-logistics-warehouse Could you resubmit this MP on the new site? -- https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/223880 Your team Stock and Logistic Core Editors is requested to review the proposed merge of lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse
Review: Needs Fixing Hello, I see some little mistake that I correct in : http://bazaar.launchpad.net/~acsone-openerp/stock-logistic-warehouse/7.0-inventory-hierarchical-fill2-lga/revision/40 http://bazaar.launchpad.net/~acsone-openerp/stock-logistic-warehouse/7.0-inventory-hierarchical-fill2-lga/revision/41 It seems I forgot to report them on this branch :(. The most important of them is the second one. The change is the following: l.699 ids_to_check = ids should be : ids_to_check = list(ids) -- https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/223880 Your team Stock and Logistic Core Editors is subscribed to branch lp:stock-logistic-warehouse. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse
laetitia-gangloff you're right, Loïc had to split the proposal in 2 branches to make the review easier but I forgot to push back the fixes he an you made to the modules in this branch. I'll fix it now. -- https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/223880 Your team Stock and Logistic Core Editors is subscribed to branch lp:stock-logistic-warehouse. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse
laetitia-gangloff,
Can you please explain this change in your branch? I don't see the point
because ids is already sure to be Iterable. Does it really have to be a list?
@@ -285,11 +285,11 @@
def write(self, cr, uid, ids, vals, context=None):
Refuse write if an inventory is being conducted
self._check_inventory(cr, uid, ids, context=context)
if not isinstance(ids, Iterable):
ids = [ids]
-ids_to_check = list(ids)
+ids_to_check = ids
# If changing the parent, no inventory must conducted there either
if vals.get('location_id'):
ids_to_check.append(vals['location_id'])
self._check_inventory(cr, uid, ids_to_check, context=context)
return super(StockLocation, self).write(cr, uid, ids, vals,
--
https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/223880
Your team Stock and Logistic Core Editors is subscribed to branch
lp:stock-logistic-warehouse.
--
Mailing list: https://launchpad.net/~openerp-community-reviewer
Post to : openerp-community-reviewer@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openerp-community-reviewer
More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse
It is to avoid a RuntimeError: maximum recursion depth exceeded when change location_id. ids was just assigned in ids_to_check so, when ids_to_check change, ids change too and the write is not done with right parameter, an we get the above error. With list(ids) we create a new list, so ids and ids_to_check are not same. -- https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/223880 Your team Stock and Logistic Core Editors is subscribed to branch lp:stock-logistic-warehouse. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse
laetitia-gangloff, right you are again. I propose to write it explicitly this way, is it OK for you? if not isinstance(ids, Iterable): ids_to_check = [ids] else: # Copy the date to avoid changing 'ids', it would trigger an infinite recursion ids_to_check = list(ids) -- https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/223880 Your team Stock and Logistic Core Editors is subscribed to branch lp:stock-logistic-warehouse. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse
I meant copy the datA, not copy the date of course... -- https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/223880 Your team Stock and Logistic Core Editors is subscribed to branch lp:stock-logistic-warehouse. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse
Your proposition is OK for me. Maybe, can you take test of this problem ? -- https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/223880 Your team Stock and Logistic Core Editors is subscribed to branch lp:stock-logistic-warehouse. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse
l. 509/l. 516 : raise osv.except_osv Is it not better to use orm.except_orm ? -- https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/223880 Your team Stock and Logistic Core Editors is subscribed to branch lp:stock-logistic-warehouse. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse
laetitia-gangloff: that part of the code was backported from trunk-wms at the time, and I wanted to ease the migration to v8. But eventually Odoo's Quentin Di Paoli changed his mind about that feature and removed it from odoo/master. But he still uses osv.except_osv all over the WMS code. So I have no idea what's best, opinions welcome. -- https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/223880 Your team Stock and Logistic Core Editors is subscribed to branch lp:stock-logistic-warehouse. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse
Review: Approve code, test, functional test Thank you for the explanation. I have no idea what's best. I retested this module and it seems now OK. -- https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/223880 Your team Stock and Logistic Core Editors is subscribed to branch lp:stock-logistic-warehouse. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse
Review: Abstain i'm a contributor -- https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/223880 Your team Stock and Logistic Core Editors is subscribed to branch lp:stock-logistic-warehouse. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse
Loïc and I are still working on this proposal. Good progress was made: we re-aligned the features with v8 in mind, and improved the code style and the views. Acsone joined in and we're still making tests, and i hope Loïc can make a new proposal in the coming days. -- https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/210630 Your team Stock and Logistic Core Editors is subscribed to branch lp:stock-logistic-warehouse. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
[Openerp-community-reviewer] [Merge] lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse
The proposal to merge lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse has been updated. Status: Needs review = Work in progress For more details, see: https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/210630 -- https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/210630 Your team Stock and Logistic Core Editors is subscribed to branch lp:stock-logistic-warehouse. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location into lp:stock-logistic-warehouse
Review: Needs Fixing code review, test
Hello,
Thanks for this contribution, the functionality is great and I'd love to see it
in OCA. And thanks for including tests.
A few minor changes are needed to bring the module to the coding standards
* use orm.Model / orm.TransientModel as base classes instead of osv.osv and
osv.osv_memory
* model column default values: in OpenERP 7.0, not need to use a lambda for a
constant value, just use the constant
Additionally:
* in __openerp__.py, I think you should use 'Warehouse Management' for the
category (i.e. the same one as the official 'stock' addon
* no space before '!' in english (I'd remove the exclamation marks in exception
messages altogether, that the UI job to display an icon)
* in confirm_uninventoried_location_wizard: you should add a 'if context is
None: context = {}' at the beginning.
* in confirm_uninventoried_locations: you are ignoring an osv_except exception.
Could you add a comment in the code explaining why this is safe? If this is
about the No product in this location warning raised in the reimplementation
of _fill_location_lines, then I think a check that we are ignoring the correct
exception should be added. You could for instance subclass except_osv and raise
+ ignore the subclass. Otherwise I think the code may hide things we do not
want to hide (or even fail because of an except_osv
--
https://code.launchpad.net/~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location/+merge/210630
Your team Stock and Logistic Core Editors is subscribed to branch
lp:stock-logistic-warehouse.
--
Mailing list: https://launchpad.net/~openerp-community-reviewer
Post to : openerp-community-reviewer@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openerp-community-reviewer
More help : https://help.launchpad.net/ListHelp
