Merge lp:~openerp-community/openobject-server/contact-id-solution-for-v7-contact-management into lp:openobject-server/7.0

Proposed by Raphaël Valyi - http://www.akretion.com on 2013-04-17
Status: Needs review
Proposed branch: lp:~openerp-community/openobject-server/contact-id-solution-for-v7-contact-management
Merge into: lp:openobject-server/7.0
Diff against target: 205 lines (+157/-0) (has conflicts)
2 files modified
openerp/addons/base/res/res_partner.py (+152/-0)
openerp/addons/base/res/res_security.xml (+5/-0)
Text conflict in openerp/addons/base/res/res_partner.py
To merge this branch: bzr merge lp:~openerp-community/openobject-server/contact-id-solution-for-v7-contact-management
Reviewer Review Type Date Requested Status
Olivier Dony (Odoo) 2013-04-17 Disapprove on 2013-04-17
Review via email: mp+159316@code.launchpad.net

Description of the change

See commit messages and bug comments please.

To post a comment you must log in.
Olivier Dony (Odoo) (odo-openerp) wrote :
Download full text (8.5 KiB)

On a functional point of view
-----------------------------
Frustrations aside, I understand that you want to restore the semantics of the old 6.1 partner_id field because you're afraid of possible impacts on modules where this semantic change was not properly considered during the migration. So you want to hide the new model to them or delay it until the next version. However hiding the new model introduces dangerous inconsistencies and could be worse than really fixing the affected modules.

The rationale for the new model is presented here: http://bit.ly/17eMp8F (and will be updated with more technical details)

The change of semantics of the "partner_id" field is the *main part* of it. In the 7.0 model the partner_id field is the main and only reference to the person and business to which a document is related, and it carries the full information: contact + company. It is essential to realize this, and as important to deal with it properly when migrating modules to 7.0 as to deal with the removal of 'res.partner.address'.
Not many third-party modules have been migrated to v7.0 yet, and even in those that were, dealing with this fact properly is better and more future-proof than trying to partially hide the new model and delay the proper migration of the code to the next version.

By hiding the new model partially you instead create more (hidden) inconsistencies:

1. You mix different semantics in the models.
On some documents the "partner_id" field will carry the contact information, while on others it will only carry the company info and a second field has to be accessed for the contact. The only case where the "partner_id" really must point to the company information exclusively is the Journal Entries/Items, and this must be managed transparently by the core accounting module. Everywhere else in 7.0 the "partner_id" field should refer to the real "contact + company" pair, and there is no loss of information.
Yes there may be cases where reports need to aggregate data by company rather than contact, and this can be done easily as part of the migration: either join the right table in SQL directly or (for better performance) denormalize the company reference as an extra column in the documents directly. In the latter case the extra column must *not* be exposed and alterable by the user, it is an implementation detail of the reporting and must be automatically maintained by the system.
This is what we propose to do in the account_report_company module for invoices: an extra denormalized field "partner_commercial_id" is added to invoices in order to provide per-company Invoice reports without performance penalty. This field *must not* replace the main "partner_id" reference, it is a technical detail and not the main reference! It is *not* a simple name swap! It must *not* be alterable by the user!

2. You now have to manually deal with the propagation of the contact information.
As you've seen you now have to manually deal with contact info propagation, otherwise this information is simply lost. By hiding the new model on several models you will now force any custom module that alters the main workflows to deal with the contact propagation m...

Read more...

review: Disapprove
4945. By Sébastien BEAU - http://www.akretion.com on 2013-04-17

[IMP] add the possibility to group_by on contact

4946. By Sébastien BEAU - http://www.akretion.com on 2013-04-17

[IMP] mixin should also add the contact_id

4947. By Sébastien BEAU - http://www.akretion.com on 2013-04-18

[FIX] Removing the contact if it's used is not posible anymore

4948. By Raphaël Valyi - http://www.akretion.com on 2013-04-18

[FIX] fixed syntax error from previous commit

4949. By Raphaël Valyi - http://www.akretion.com on 2013-04-21

[IMP] splitted contact mixin into res.contact.mixin inherit res.contact.mixin.methods. Object inheriting from res.contact.mixin works the same, but now, in objects like project.project that inheritS from an object having the mixin can now inherit from just res.contact.mixin.methods so they get the right mixin methods, but without getting a redundant contact_id table. Seems to work great.

Unmerged revisions

4949. By Raphaël Valyi - http://www.akretion.com on 2013-04-21

[IMP] splitted contact mixin into res.contact.mixin inherit res.contact.mixin.methods. Object inheriting from res.contact.mixin works the same, but now, in objects like project.project that inheritS from an object having the mixin can now inherit from just res.contact.mixin.methods so they get the right mixin methods, but without getting a redundant contact_id table. Seems to work great.

4948. By Raphaël Valyi - http://www.akretion.com on 2013-04-18

[FIX] fixed syntax error from previous commit

4947. By Sébastien BEAU - http://www.akretion.com on 2013-04-18

[FIX] Removing the contact if it's used is not posible anymore

4946. By Sébastien BEAU - http://www.akretion.com on 2013-04-17

[IMP] mixin should also add the contact_id

4945. By Sébastien BEAU - http://www.akretion.com on 2013-04-17

[IMP] add the possibility to group_by on contact

4944. By Raphaël Valyi - http://www.akretion.com on 2013-04-17

[FIX] fixed reading single record, added advanced contact group to see both contact_id and partner_id, renamed a few methods to fake political correctness

4943. By Raphaël Valyi - http://www.akretion.com on 2013-04-17

[FIX] added mixin to be able to easily 'decorate' a v7 object having a partner_id field with a new contact_id field; this is what I consider a sane alternative to the other solution based on data duplication and semantic permutation of company and contact compared to v6.1

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'openerp/addons/base/res/res_partner.py'
2--- openerp/addons/base/res/res_partner.py 2013-04-19 16:47:28 +0000
3+++ openerp/addons/base/res/res_partner.py 2013-04-21 22:41:25 +0000
4@@ -30,7 +30,12 @@
5 from openerp import pooler, tools
6 from openerp.osv import osv, fields
7 from openerp.tools.translate import _
8+<<<<<<< TREE
9 from openerp.tools.yaml_import import is_comment
10+=======
11+from openerp.osv.orm import setup_modifiers
12+from lxml import etree
13+>>>>>>> MERGE-SOURCE
14
15 class format_address(object):
16 def fields_view_get_address(self, cr, uid, arch, context={}):
17@@ -164,10 +169,136 @@
18 ADDRESS_FIELDS = ('street', 'street2', 'zip', 'city', 'state_id', 'country_id')
19 POSTAL_ADDRESS_FIELDS = ADDRESS_FIELDS # deprecated, to remove after 7.0
20
21+
22+class contact_mixin_methods(osv.AbstractModel):
23+ _name = 'res.contact.mixin.methods'
24+
25+ def create(self, cr, uid, vals, context=None):
26+ vals = self.write_contact(cr, uid, [], vals, context)
27+ return super(contact_mixin_methods, self).create(cr, uid, vals, context)
28+
29+ def read(self, cr, user, ids, fields=None, context=None,
30+ load='_classic_read'):
31+ res = super(contact_mixin_methods, self).read(cr, user, ids, fields, context,
32+ load)
33+ if fields and 'contact_id' in fields:
34+ if isinstance(res, list):
35+ for rec in res:
36+ if not rec.get('contact_id') and rec.get('partner_id'):
37+ rec['contact_id'] = rec['partner_id']
38+ else:
39+ if not res.get('contact_id') and res.get('partner_id'):
40+ res['contact_id'] = res['partner_id']
41+ return res
42+
43+ def write(self, cr, uid, ids, vals, context=None):
44+ vals = self.write_contact(cr, uid, ids, vals, context)
45+ return super(contact_mixin_methods, self).write(cr, uid, ids, vals, context)
46+
47+ def write_contact(self, cr, uid, ids, vals, context=None):
48+ partner_obj = self.pool.get('res.partner')
49+ if isinstance(vals.get('partner_id'), (list, tuple)):
50+ partner_id = vals.get('partner_id')[0]
51+ else:
52+ partner_id = vals.get('partner_id')
53+ if partner_id:
54+ if not vals.get('contact_id'):
55+ vals['contact_id'] = partner_id
56+ vals['partner_id'] = partner_obj.read(cr, uid, [partner_id],
57+ ['commercial_entity_id']
58+ )[0]['commercial_entity_id'][0]
59+ return vals
60+
61+ def onchange_contact_mixin(self, cr, uid, ids, contact_id, partner_id,
62+ context=None):
63+ res = {}
64+ if contact_id:
65+ commercial_entity_id = self.pool.get('res.partner').read(cr, uid,
66+ [contact_id], ['commercial_entity_id'])[0]['commercial_entity_id']
67+ res = {'value': {'partner_id': commercial_entity_id}}
68+ return res
69+
70+ def fields_view_get(self, cr, uid, view_id=None, view_type='form',
71+ context=None, toolbar=False,
72+ submenu=False):
73+ res = super(contact_mixin_methods, self).fields_view_get(cr, uid, view_id,
74+ view_type,
75+ context,
76+ toolbar,
77+ submenu)
78+ if view_type in ('form', 'tree', 'search'):
79+ eview = etree.fromstring(res['arch'])
80+ partner_fields = eview.xpath("//field[@name='partner_id']")
81+ partner_descr = self.fields_get(cr, uid, ['partner_id'], context)
82+ contact_descr = self.fields_get(cr, uid, ['contact_id'], context)
83+ for partner in partner_fields:
84+ kw = {
85+ 'name': 'contact_id',
86+ 'options': "{'always_reload': True}",
87+ 'context': partner.attrib.get('context', ''),
88+ 'domain': partner.attrib.get('domain', ''),
89+ 'modifiers': partner.attrib.get('modifiers', ''),
90+ }
91+ if view_type in ('form', 'tree'):
92+ kw['on_change'] = \
93+ "onchange_contact_mixin(contact_id, partner_id, context)"
94+ kw['groups'] = "base.group_contact_advanced"
95+ if not self.user_has_groups(cr, uid, "base.group_contact_advanced", context):
96+ partner.set('invisible', '1')
97+ setup_modifiers(partner, partner_descr, context, view_type == 'tree')
98+ kw['string'] = partner.attrib.get('string', partner_descr['partner_id']['string'])
99+ contact = etree.Element('field', **kw)
100+ p_index = partner.getparent().index(partner)
101+ partner.getparent().insert(p_index, contact)
102+ res['fields'].update(contact_descr)
103+ if view_type == 'search':
104+ group_by_fields = eview.xpath("//filter[@context=\"{'group_by':'partner_id'}\"]")
105+ for group_by_field in group_by_fields:
106+ kw = {
107+ 'string': 'Contact',
108+ 'icon': "terp-partner",
109+ 'context': "{'group_by':'contact_id'}",
110+ 'domain': "[]",
111+ }
112+ g_contact = etree.Element('filter', **kw)
113+ g_index = group_by_field.getparent().index(group_by_field)
114+ group_by_field.getparent().insert(g_index, g_contact)
115+ res['arch'] = etree.tostring(eview, encoding="utf-8")
116+ return res
117+
118+class contact_mixin(osv.AbstractModel):
119+ _name = 'res.contact.mixin'
120+ _inherit = 'res.contact.mixin.methods'
121+ _columns = {
122+ 'contact_id': fields.many2one('res.partner', 'Contact'),
123+ }
124+
125 class res_partner(osv.osv, format_address):
126 _description = 'Partner'
127 _name = "res.partner"
128
129+ def unlink(self, cr, uid, ids, context=None):
130+ errors = {}
131+ for model_name, model_obj in self.pool.models.iteritems():
132+ if hasattr(model_obj, '_contact_mixin') and model_name != 'res.contact.mixin':
133+ domain = [
134+ '|',
135+ ['contact_id', 'in', ids],
136+ ['partner_id', 'in', ids],
137+ ]
138+ res_ids = model_obj.search(cr, uid, domain, context=context)
139+ if res_ids:
140+ res = model_obj.browse(cr, uid, res_ids, context=context)
141+ errors[_(model_obj._description)] = res.pop(0)[model_obj._rec_name] or ""
142+ for obj in res:
143+ errors[_(model_obj._description)] += "; %s"%(obj[model_obj._rec_name] or "")
144+ if errors:
145+ raise osv.except_osv(_('User Error'),
146+ _("Cannot delete contact %s because they are used in the following records:\n"
147+ "%s") % (ids, "\n".join(["%s : %s"%(key, errors[key]) for key in errors])))
148+ return super(res_partner, self).unlink(cr, uid, ids, context=context)
149+
150+
151 def _address_display(self, cr, uid, ids, name, args, context=None):
152 res = {}
153 for partner in self.browse(cr, uid, ids, context=context):
154@@ -195,6 +326,7 @@
155 result[obj.id] = obj.image != False
156 return result
157
158+<<<<<<< TREE
159 def _commercial_partner_compute(self, cr, uid, ids, name, args, context=None):
160 """ Returns the partner that is considered the commercial
161 entity of this partner. The commercial entity holds the master data
162@@ -210,8 +342,28 @@
163 # indirection to avoid passing a copy of the overridable method when declaring the function field
164 _commercial_partner_id = lambda self, *args, **kwargs: self._commercial_partner_compute(*args, **kwargs)
165
166+=======
167+ def _commercial_entity_id(self, cr, uid, ids, field_names, arg,
168+ context=None):
169+ res = {}
170+ for partner in self.browse(cr, uid, ids, context=context):
171+ if partner.is_company:
172+ res[partner.id] = partner.id
173+ elif partner.parent_id:
174+ commercial_entity_id = self.read(cr, uid,
175+ [partner.parent_id.id],
176+ ['commercial_entity_id']
177+ )[0]['commercial_entity_id']
178+ res[partner.id] = commercial_entity_id
179+ else:
180+ res[partner.id] = partner.id
181+ return res
182+
183+>>>>>>> MERGE-SOURCE
184 _order = "name"
185 _columns = {
186+ 'commercial_entity_id': fields.function(_commercial_entity_id,
187+ string='Commercial Entity', type='many2one', relation='res.partner'),
188 'name': fields.char('Name', size=128, required=True, select=True),
189 'date': fields.date('Date', select=1),
190 'title': fields.many2one('res.partner.title', 'Title'),
191
192=== modified file 'openerp/addons/base/res/res_security.xml'
193--- openerp/addons/base/res/res_security.xml 2012-09-26 07:03:39 +0000
194+++ openerp/addons/base/res/res_security.xml 2013-04-21 22:41:25 +0000
195@@ -30,5 +30,10 @@
196 <field name="domain_force">[('company_ids','child_of',[user.company_id.id])]</field>
197 </record>
198
199+ <record id="group_contact_advanced" model="res.groups">
200+ <field name="name">Advanced Contacts Management</field>
201+ <field name="implied_ids" eval="[(4, ref('base.group_user'))]"/>
202+ </record>
203+
204 </data>
205 </openerp>