Merge lp://staging/~arthru/prestashoperpconnect/import-product-combinations into lp://staging/prestashoperpconnect

Proposed by arthru
Status: Merged
Merged at revision: 286
Proposed branch: lp://staging/~arthru/prestashoperpconnect/import-product-combinations
Merge into: lp://staging/prestashoperpconnect
Diff against target: 668 lines (+525/-17)
8 files modified
prestashoperpconnect/__init__.py (+1/-0)
prestashoperpconnect/__openerp__.py (+4/-0)
prestashoperpconnect/product.py (+47/-12)
prestashoperpconnect/product_combination.py (+397/-0)
prestashoperpconnect/security/ir.model.access.csv (+3/-1)
prestashoperpconnect/unit/binder.py (+3/-0)
prestashoperpconnect/unit/import_synchronizer.py (+56/-0)
prestashoperpconnect/unit/mapper.py (+14/-4)
To merge this branch: bzr merge lp://staging/~arthru/prestashoperpconnect/import-product-combinations
Reviewer Review Type Date Requested Status
Guewen Baconnier @ Camptocamp code review Pending
arthru Pending
Review via email: mp+193021@code.staging.launchpad.net

This proposal supersedes a proposal from 2013-10-25.

Description of the change

This permits to import a product by combination in prestashop

To post a comment you must log in.
Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote : Posted in a previous version of this proposal

Good piece of work Arthur,

Some comments:

--

What is a "product combination". At least a few words (in the docstring of the Python module product_combination.py) to explain this concept could be useful.

--

l.44-47, l.217ff. l.226ff (and other places, possibly on methods search, browse, read, create, unlink)

    model = self.session.pool.get('product.product')
    product_ids = model.search(self.session.cr, self.session.uid, [
        ('default_code', '=', code)
    ])

A better idiom is:

    product_ids = self.session.search('product.product', [('default_code', '=', code)])

(side note on the alignment: "Arguments on first line forbidden when not using vertical alignment", source http://www.python.org/dev/peps/pep-0008/#indentation)

--

l.134
"unidecode" is to add in the "external_dependencies" in __openerp__.py

--

l.212 (and each time you use the unwrap keyword argument of to_openerp())

Please use the keyword when calling a keyword argument, so instead of

    attribute_id = option_binder.to_openerp(
        option_value['id_attribute_group'], True)

Use

    attribute_id = option_binder.to_openerp(
        option_value['id_attribute_group'], unwrap=True)

--
l.264, l.302, l.453

Instead of:

  type(main_product[attribute]) is list

Use:

  isinstance(main_product[attribute], list)

--
l.306-321:
suggestion (as you want): externalize the part which get the option value in another ConnectorUnit, allowing to customize the way the options values are get.

--
l.599
    if len(combinations) == 0:
=>
    if not combinations:

--

Thanks!

review: Needs Fixing (code review)
Revision history for this message
arthru (arthru) wrote : Posted in a previous version of this proposal

I made most of the changes. Thanks a lot for these advice, I'll try to apply them in the code that already exist !

review: Needs Resubmitting
283. By arthru

Fix the dependency on unidecode

284. By arthru

Fix a search call

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.