Merge lp:~akretion-team/openerp-product-attributes/openerp-product-attributes_limit_database_column_name into lp:~product-core-editors/openerp-product-attributes/7.0

[David BEAL (ak)]

[IMP] add a function to define allowed chars for database column name + cleaning code .py alignement

[Benoit Guillot - http://www.akretion.com (benoit-guillot-z)]

LGTM,

code review, no test

review: Approve

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

Hi,

The next time, can you avoid to mix a cleaning and a fix?
Because we can't know where is your change and what it does without parsing and searching through all the diffs.

Can I propose a better name for the function "set_column_name"? "safe_column_name" maybe?

Thanks!

review: Needs Fixing (code review)

[David BEAL (ak) (davidbeal)]

Ok

Changes are done

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

Thanks for the change!
Why not

re.sub(r'\W', '', string)

Instead of

filter((lambda x: re.search('[0-9a-z_]', x)), string)

?

[David BEAL (ak) (davidbeal)]

attribute_code in magento doesn't support upper case

If you want to share attributes with openerp, magento and other plateforms you need to apply common rules with the more restrictif system

[David BEAL (ak) (davidbeal)]

Hi

Is it OK for everybody ?

Thanks for your response

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

You apply lower() on the string just before the filter, so anyway you can't
have any uppercase chars, that's why I put \W. However, your regexp is more
explicit.

My remark was more on the usage of filter() which is inefficient vs
re.sub().

Thanks for the changes. Approve (I don't remember how to change my review
state from mail, I will change it afterwards from launchpad)
Le 2 oct. 2013 11:35, "David BEAL" <email address hidden> a écrit :

> attribute_code in magento doesn't support upper case
>
> If you want to share attributes with openerp, magento and other plateforms
> you need to apply common rules with the more restrictif system
> --
>
> https://code.launchpad.net/~akretion-team/openerp-product-attributes/openerp-product-attributes_limit_database_column_name/+merge/188014
> You are reviewing the proposed merge of
> lp:~akretion-team/openerp-product-attributes/openerp-product-attributes_limit_database_column_name
> into lp:openerp-product-attributes.
>

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

review: Approve (code review)

[Quentin THEURET @Amaris (qtheuret)]

LGTM

review: Approve (code review)

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

The merge gives a conflict. Can you rebase on the latest revision please?

review: Needs Resubmitting

[lp:~akretion-team/openerp-product-attributes/openerp-product-attributes_limit_database_column_name]

219. By David BEAL (ak) on 2013-11-13

[MERGE] merge again from main branch

220. By David BEAL (ak) on 2013-11-13

[FIX] extract 'sep' unused variable

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

LGTM

review: Approve (code review)

=== modified file 'base_custom_attributes/custom_attributes.py'
--- base_custom_attributes/custom_attributes.py	2013-11-12 08:30:11 +0000
+++ base_custom_attributes/custom_attributes.py	2013-11-13 08:38:32 +0000
@@ -1,9 +1,9 @@
 # -*- encoding: utf-8 -*-
 ###############################################################################
 #                                                                             #
-#   base_attribute.attributes for OpenERP                                        #
-#   Copyright (C) 2011 Akretion Benoît GUILLOT <[e-mail]>  #
-#   Copyright (C) 2013 Akretion Raphaël VALYI <[e-mail]>    #
+#   base_attribute.attributes for OpenERP                                     #
+#   Copyright (C) 2011 Akretion Benoît GUILLOT <[e-mail]>
+#   Copyright (C) 2013 Akretion Raphaël VALYI <[e-mail]>
 #                                                                             #
 #   This program is free software: you can redistribute it and/or modify      #
 #   it under the terms of the GNU Affero General Public License as            #
@@ -26,6 +26,15 @@
 from openerp.tools.translate import _
 from lxml import etree
 from unidecode import unidecode # Debian package python-unidecode
+import re
+
+
+def safe_column_name(string):
+    """This function prevent portability problem in database column name
+    with other DBMS system
+    Use case : if you synchronise attributes with other applications """
+    string = unidecode(string.replace(' ', '_').lower())
+    return re.sub(r'[^0-9a-z_]','', string)
 
 
 class attribute_option(orm.Model):
@@ -40,15 +49,28 @@
         return [(r['model'], r['name']) for r in res]
 
     _columns = {
-        'name': fields.char('Name', size=128, translate=True, required=True),
-        'value_ref': fields.reference('Reference', selection=_get_model_list, size=128),
-        'attribute_id': fields.many2one('attribute.attribute', 'Product Attribute', required=True),
+        'name': fields.char(
+            'Name',
+            size=128,
+            translate=True,
+            required=True),
+        'value_ref': fields.reference(
+            'Reference',
+            selection=_get_model_list,
+            size=128),
+        'attribute_id': fields.many2one(
+            'attribute.attribute',
+            'Product Attribute',
+            required=True),
         'sequence': fields.integer('Sequence'),
     }
 
     def name_change(self, cr, uid, ids, name, relation_model_id, context=None):
         if relation_model_id:
-            warning = {'title': _('Error!'), 'message': _("Use the 'Load Options' button instead to select appropriate model references.")}
+            warning = {'title': _('Error!'),
+                       'message': _("Use the 'Load Options' button "
+                                    "instead to select appropriate "
+                                    "model references'")}
             return {"value": {"name": False}, "warning": warning}
         else:
             return True
@@ -59,11 +81,15 @@
     _rec_name = 'attribute_id'
 
     _columns = {
-        'attribute_id': fields.many2one('attribute.attribute', 'Attribute', required=True),
+        'attribute_id': fields.many2one(
+            'attribute.attribute',
+            'Product Attribute',
+            required=True),
     }
 
     _defaults = {
-        'attribute_id': lambda self, cr, uid, context: context.get('attribute_id', False)
+        'attribute_id': lambda self, cr, uid, context:
+                            context.get('attribute_id',False)
     }
 
     def validate(self, cr, uid, ids, context=None):
@@ -86,12 +112,16 @@
         res = super(attribute_option_wizard, self).create(cr, uid, vals, context)
         return res
 
-    def fields_view_get(self, cr, uid, view_id=None, view_type='form', context=None, toolbar=False, submenu=False):
-        res = super(attribute_option_wizard, self).fields_view_get(cr, uid, view_id, view_type, context, toolbar, submenu)
+    def fields_view_get(self, cr, uid, view_id=None, view_type='form',
+                        context=None, toolbar=False, submenu=False):
+        res = super(attribute_option_wizard, self).fields_view_get(
+            cr, uid, view_id, view_type, context, toolbar, submenu)
         if view_type == 'form' and context and context.get("attribute_id"):
             attr_obj = self.pool.get("attribute.attribute")
-            model_id = attr_obj.read(cr, uid, [context.get("attribute_id")], ['relation_model_id'])[0]['relation_model_id'][0]
-            relation = self.pool.get("ir.model").read(cr, uid, [model_id], ["model"])[0]["model"]
+            model_id = attr_obj.read(cr, uid, [context.get("attribute_id")],
+                            ['relation_model_id'])[0]['relation_model_id'][0]
+            relation = self.pool.get("ir.model").read(cr, uid, [model_id],
+                                                      ["model"])[0]["model"]
             res['fields'].update({'option_ids': {
                             'domain': [],
                             'string': "Options",
@@ -118,11 +148,10 @@
         kwargs = {'name': "%s" % attribute.name}
         if attribute.ttype in ['many2many', 'text']:
             parent = etree.SubElement(parent, 'group', colspan="2", col="4")
-            sep = etree.SubElement(parent,
+            etree.SubElement(parent,
                                    'separator',
                                     string="%s" % attribute.field_description,
-                                    colspan="4")  
-        
+                                    colspan="4")
             kwargs['nolabel'] = "1"
         if attribute.ttype in ['many2one', 'many2many']:
             if attribute.relation_model_id:
@@ -142,23 +171,30 @@
         kwargs['required'] = str(attribute.required or
                                  attribute.required_on_views)
         field = etree.SubElement(parent, 'field', **kwargs)
-        orm.setup_modifiers(field, self.fields_get(cr, uid, attribute.name, context))
+        orm.setup_modifiers(field, self.fields_get(cr, uid, attribute.name,
+                                                   context))
         return parent
 
-    def _build_attributes_notebook(self, cr, uid, attribute_group_ids, context=None):
-        notebook = etree.Element('notebook', name="attributes_notebook", colspan="4")
+    def _build_attributes_notebook(self, cr, uid, attribute_group_ids,
+                                   context=None):
+        notebook = etree.Element('notebook', name="attributes_notebook",
+                                 colspan="4")
         toupdate_fields = []
         grp_obj = self.pool.get('attribute.group')
-        for group in grp_obj.browse(cr, uid, attribute_group_ids, context=context):
-            page = etree.SubElement(notebook, 'page', string=group.name.capitalize())
+        for group in grp_obj.browse(cr, uid, attribute_group_ids,
+                                    context=context):
+            page = etree.SubElement(notebook, 'page',
+                                    string=group.name.capitalize())
             for attribute in group.attribute_ids:
                 if attribute.name not in toupdate_fields:
                     toupdate_fields.append(attribute.name)
-                    self._build_attribute_field(cr, uid, page, attribute, context=context)
+                    self._build_attribute_field(cr, uid, page, attribute,
+                                                context=context)
         return notebook, toupdate_fields
 
-    def relation_model_id_change(self, cr, uid, ids, relation_model_id, option_ids, context=None):
-        "removed selected options as they would be inconsistent" 
+    def relation_model_id_change(self, cr, uid, ids, relation_model_id,
+                                 option_ids, context=None):
+        "removed selected options as they would be inconsistent"
         return {'value': {'option_ids': [(2, i[1]) for i in option_ids]}}
 
     def button_add_options(self, cr, uid, ids, context=None):
@@ -173,24 +209,36 @@
         }
 
     _columns = {
-        'field_id': fields.many2one('ir.model.fields', 'Ir Model Fields', required=True, ondelete="cascade"),
-        'attribute_type': fields.selection([('char','Char'),
-                                            ('text','Text'),
-                                            ('select','Select'),
-                                            ('multiselect','Multiselect'),
-                                            ('boolean','Boolean'),
-                                            ('integer','Integer'),
-                                            ('date','Date'),
-                                            ('datetime','Datetime'),
-                                            ('binary','Binary'),
-                                            ('float','Float')],
-                                           'Type', required=True),
-        'serialized': fields.boolean('Field serialized',
-                                     help="If serialized, the field will be stocked in the serialized field: "
-                                     "attribute_custom_tmpl or attribute_custom_variant depending on the field based_on"),
-        'option_ids': fields.one2many('attribute.option', 'attribute_id', 'Attribute Options'),
+        'field_id': fields.many2one(
+            'ir.model.fields',
+            'Ir Model Fields',
+            required=True,
+            ondelete="cascade"),
+        'attribute_type': fields.selection([
+            ('char', 'Char'),
+            ('text', 'Text'),
+            ('select', 'Select'),
+            ('multiselect', 'Multiselect'),
+            ('boolean', 'Boolean'),
+            ('integer', 'Integer'),
+            ('date', 'Date'),
+            ('datetime', 'Datetime'),
+            ('binary', 'Binary'),
+            ('float', 'Float')],
+            'Type', required=True),
+        'serialized': fields.boolean(
+            'Field serialized',
+            help="If serialized, the field will be stocked in the serialized "
+                 "field: attribute_custom_tmpl or attribute_custom_variant "
+                 "depending on the field based_on"),
+        'option_ids': fields.one2many(
+            'attribute.option',
+            'attribute_id',
+            'Attribute Options'),
         'create_date': fields.datetime('Created date', readonly=True),
-        'relation_model_id': fields.many2one('ir.model', 'Model'),
+        'relation_model_id': fields.many2one(
+            'ir.model',
+            'Model'),
         'required_on_views': fields.boolean(
             'Required (on views)',
             help="If activated, the attribute will be mandatory on the views, "
@@ -199,11 +247,10 @@
 
     def create(self, cr, uid, vals, context=None):
         if vals.get('relation_model_id'):
-            relation = self.pool.get('ir.model').read(cr, uid,
-            [vals.get('relation_model_id')], ['model'])[0]['model']
+            relation = self.pool.get('ir.model').read(
+                cr, uid, [vals.get('relation_model_id')], ['model'])[0]['model']
         else:
             relation = 'attribute.option'
-
         if vals['attribute_type'] == 'select':
             vals['ttype'] = 'many2one'
             vals['relation'] = relation
@@ -216,27 +263,31 @@
 
         if vals.get('serialized'):
             field_obj = self.pool.get('ir.model.fields')
-            serialized_ids = field_obj.search(cr, uid,
-            [('ttype', '=', 'serialized'), ('model_id', '=', vals['model_id']),
-            ('name', '=', 'x_custom_json_attrs')], context=context)
+            serialized_ids = field_obj.search(cr, uid, [
+                ('ttype', '=', 'serialized'),
+                ('model_id', '=', vals['model_id']),
+                ('name', '=', 'x_custom_json_attrs')],
+                context=context)
             if serialized_ids:
                 vals['serialization_field_id'] = serialized_ids[0]
             else:
                 f_vals = {
                     'name': u'x_custom_json_attrs',
-                    'field_description': u'Serialized JSON Attributes', 
+                    'field_description': u'Serialized JSON Attributes',
                     'ttype': 'serialized',
                     'model_id': vals['model_id'],
                 }
-                vals['serialization_field_id'] = field_obj.create(cr, uid, f_vals, {'manual': True})
+                vals['serialization_field_id'] = field_obj.create(
+                    cr, uid, f_vals, {'manual': True})
         vals['state'] = 'manual'
         return super(attribute_attribute, self).create(cr, uid, vals, context)
 
-    def onchange_field_description(self, cr, uid, ids, field_description, name, create_date, context=None):
+    def onchange_field_description(self, cr, uid, ids, field_description,
+                                   name, create_date, context=None):
         name = name or u'x_'
         if field_description and not create_date:
-            name = unidecode(u'x_%s' % field_description.replace(' ', '_').lower())
-        return  {'value' : {'name' : name}}
+            name = unicode('x_' + safe_column_name(field_description))
+        return {'value': {'name': name}}
 
     def onchange_name(self, cr, uid, ids, name, context=None):
         res = {}
@@ -244,15 +295,15 @@
             name = u'x_%s' % name
         else:
             name = u'%s' % name
-        res = {'value' : {'name' : unidecode(name)}}
+        res = {'value': {'name': unidecode(name)}}
 
         #FILTER ON MODEL
-        model_domain = []
         model_name = context.get('force_model')
         if not model_name:
             model_id = context.get('default_model_id')
             if model_id:
-                model = self.pool['ir.model'].browse(cr, uid, model_id, context=context)
+                model = self.pool['ir.model'].browse(cr, uid, model_id,
+                                                     context=context)
                 model_name = model.model
         if model_name:
             model_obj = self.pool[model_name]
@@ -264,8 +315,8 @@
     def _get_default_model(self, cr, uid, context=None):
         if context and context.get('force_model'):
             model_id = self.pool['ir.model'].search(cr, uid, [
-                    ['model', '=', context['force_model']]
-                    ], context=context)
+                                        ('model', '=', context['force_model'])
+                                                             ], context=context)
             if model_id:
                 return model_id[0]
         return None
@@ -276,29 +327,42 @@
 
 
 class attribute_group(orm.Model):
-    _name= "attribute.group"
+    _name = "attribute.group"
     _description = "Attribute Group"
-    _order="sequence"
+    _order ="sequence"
 
     _columns = {
-        'name': fields.char('Name', size=128, required=True, translate=True),
+        'name': fields.char(
+            'Name',
+            size=128,
+            required=True,
+            translate=True),
         'sequence': fields.integer('Sequence'),
-        'attribute_set_id': fields.many2one('attribute.set', 'Attribute Set'),
-        'attribute_ids': fields.one2many('attribute.location', 'attribute_group_id', 'Attributes'),
-        'model_id': fields.many2one('ir.model', 'Model', required=True),
+        'attribute_set_id': fields.many2one(
+            'attribute.set',
+            'Attribute Set'),
+        'attribute_ids': fields.one2many(
+            'attribute.location',
+            'attribute_group_id',
+            'Attributes'),
+        'model_id': fields.many2one(
+            'ir.model',
+            'Model',
+            required=True),
     }
 
     def create(self, cr, uid, vals, context=None):
-        for attribute in vals.get('attribute_ids', []):
-            if vals.get('attribute_set_id') and attribute[2] and not attribute[2].get('attribute_set_id'):
+        for attribute in vals['attribute_ids']:
+            if vals.get('attribute_set_id') and attribute[2] and \
+                not attribute[2].get('attribute_set_id'):
                 attribute[2]['attribute_set_id'] = vals['attribute_set_id']
         return super(attribute_group, self).create(cr, uid, vals, context)
 
     def _get_default_model(self, cr, uid, context=None):
         if context and context.get('force_model'):
-            model_id = self.pool['ir.model'].search(cr, uid, [
-                    ['model', '=', context['force_model']]
-                    ], context=context)
+            model_id = self.pool['ir.model'].search(
+                cr, uid, [['model', '=', context['force_model']]],
+                context=context)
             if model_id:
                 return model_id[0]
         return None
@@ -312,16 +376,26 @@
     _name = "attribute.set"
     _description = "Attribute Set"
     _columns = {
-        'name': fields.char('Name', size=128, required=True, translate=True),
-        'attribute_group_ids': fields.one2many('attribute.group', 'attribute_set_id', 'Attribute Groups'),
-        'model_id': fields.many2one('ir.model', 'Model', required=True),
+        'name': fields.char(
+            'Name',
+            size=128,
+            required=True,
+            translate=True),
+        'attribute_group_ids': fields.one2many(
+            'attribute.group',
+            'attribute_set_id',
+            'Attribute Groups'),
+        'model_id': fields.many2one(
+            'ir.model',
+            'Model',
+            required=True),
         }
 
     def _get_default_model(self, cr, uid, context=None):
         if context and context.get('force_model'):
-            model_id = self.pool['ir.model'].search(cr, uid, [
-                    ['model', '=', context['force_model']]
-                    ], context=context)
+            model_id = self.pool['ir.model'].search(
+                cr, uid, [['model', '=', context['force_model']]],
+                context=context)
             if model_id:
                 return model_id[0]
         return None
@@ -330,22 +404,37 @@
         'model_id': _get_default_model
     }
 
+
 class attribute_location(orm.Model):
     _name = "attribute.location"
     _description = "Attribute Location"
     _order="sequence"
     _inherits = {'attribute.attribute': 'attribute_id'}
 
-
     def _get_attribute_loc_from_group(self, cr, uid, ids, context=None):
-        return self.pool.get('attribute.location').search(cr, uid, [('attribute_group_id', 'in', ids)], context=context)
+        return self.pool.get('attribute.location').search(
+            cr, uid, [('attribute_group_id', 'in', ids)], context=context)
 
     _columns = {
-        'attribute_id': fields.many2one('attribute.attribute', 'Attribute', required=True, ondelete="cascade"),
-        'attribute_set_id': fields.related('attribute_group_id', 'attribute_set_id', type='many2one', relation='attribute.set', string='Attribute Set', readonly=True,
-store={
-            'attribute.group': (_get_attribute_loc_from_group, ['attribute_set_id'], 10),
-        }),
-        'attribute_group_id': fields.many2one('attribute.group', 'Attribute Group', required=True),
+        'attribute_id': fields.many2one(
+            'attribute.attribute',
+            'Product Attribute',
+            required=True,
+            ondelete="cascade"),
+        'attribute_set_id': fields.related(
+            'attribute_group_id',
+            'attribute_set_id',
+            type='many2one',
+            relation='attribute.set',
+            string='Attribute Set',
+            readonly=True,
+            store={
+                'attribute.group': (_get_attribute_loc_from_group,
+                                    ['attribute_set_id'], 10),
+            }),
+        'attribute_group_id': fields.many2one(
+            'attribute.group',
+            'Attribute Group',
+            required=True),
         'sequence': fields.integer('Sequence'),
         }

=== modified file 'base_custom_attributes/ir_model.py'
--- base_custom_attributes/ir_model.py	2012-08-21 14:10:21 +0000
+++ base_custom_attributes/ir_model.py	2013-11-13 08:38:32 +0000
@@ -1,8 +1,8 @@
 # -*- encoding: utf-8 -*-
 ###############################################################################
 #                                                                             #
-#   product_custom_attributes for OpenERP                                      #
-#   Copyright (C) 2011 Akretion Benoît GUILLOT <[e-mail]>  #
+#   product_custom_attributes for OpenERP
+#   Copyright (C) 2011 Akretion Benoît GUILLOT <[e-mail]>
 #                                                                             #
 #   This program is free software: you can redistribute it and/or modify      #
 #   it under the terms of the GNU Affero General Public License as            #
@@ -27,8 +27,13 @@
 
     _inherit = "ir.model.fields"
     _columns = {
-        'field_description': fields.char('Field Label', required=True, size=256, translate=True),
+        'field_description': fields.char(
+            'Field Label',
+            required=True,
+            size=256,
+            translate=True),
     }
     _sql_constraints = [
-        ('name_model_uniq', 'unique (name, model_id)', 'The name of the field has to be uniq for a given model !'),
+        ('name_model_uniq', 'unique (name, model_id)',
+            'The name of the field has to be uniq for a given model !'),
     ]