Merge into 7.0 : contact-id-solution-for-v7-contact-management : Code : Odoo Server (MOVED TO GITHUB)

Status:

Needs review

Proposed branch:

lp://staging/~openerp-community/openobject-server/contact-id-solution-for-v7-contact-management

Merge into:

lp://staging/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://staging/~openerp-community/openobject-server/contact-id-solution-for-v7-contact-management

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Olivier Dony (Odoo)		2013-04-17	Disapprove on 2013-04-17
Review via email: mp+159316@code.staging.launchpad.net

Description of the change

See commit messages and bug comments please.

Revision history for this message

Olivier Dony (Odoo) (odo-openerp) wrote on 2013-04-17:

#

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...

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 manually. Including PDF reports, etc.

3. You modify business flows in invalid ways and allow users to make dangerous mistakes.
By adding an extra contact_id field that can (apparently purposefully) be desynchronized from the partner_id you are creating an easy way for users to create invalid documents that the system does not expect. In the 7.0 model there must be only *one reference* per business partner on each document. If there are multiple references that can be altered independently they may refer to different companies and the business flows must deal with it properly.
For example Sales Orders allow 3 different partners to be specified for an order: ordering partner, invoicing partner and shipping partner. These 3 references may point to different companies and the business flows deal with that accordingly.
By adding an extra "contact_id" reference that can be modified to point to a 4th company you are allowing users to create invalid documents that the system does not expect, and the same is true for the 2nd field on any model where there was only one. In order to be consistent you would need to prevent users from modifying these extra fields completely. And when the original model had several distinct partner references (e.g. Sales Orders), you'd have to duplicate the 'contact_id' logic for *each* of them, hence 6 fields total on SO.

4. You have inconsistent/misleading data in the database and UI.
Due to the unified 7.0 model for partners and contacts, res.partner records that are simple contacts and not "commercial entities" still have field values in the database for all commercial fields: pricelists, payment terms, related G/L accounts, etc. You cannot ignore this fact: not only will the fields still be visible and editable on the contacts while having (hopefully) no effect, but the data will also be incorrect in the database. This is dangerous because both the users and the business logic may expect these fields to carry valid information, while they simply have no sense. This is the reason why the solution we propose on bug 1160365 hides the commercial fields from the UI and forces them to behave like "related fields" that are automatically synchronized with the values set on their parent commercial entity.

These consequences must be properly dealt with otherwise the solution is worse than what it tries to cure. Yes the solution we propose is not perfect yet, but it tries hard to prevent any inconsistency, and it only requires module maintainers to be properly aware of the 7.0 model when migrating their modules. This seems like a normal requirement, even though we clearly need to provide more technical details on this model change and how to migrate to it.
But dealing with the new model does *not* take a lot of work and does *not* require years of stabilization as has been claimed. The technical changes required can be described in a short checklist that will not take days to verify on any module and will ensure that they're 100% correct now and in the future.

It seems clear that your solution does not *replace* a proper fix of the remaining issues in 7.0, but I imagine some could still consider applying it as a module on top of the official 7.0 branch, for those who cannot properly deal with the migration to the 7.0 model now.

BTW, maybe someone who is actually dealing with migrating 6.1 modules to 7.0 could do the exercise of checking their code and listing the number of changes that are really needed in order to migrate to the new model fully? This would help put numbers on the effort that is really required, and the chance of seeing problems after the migration.

Applying your solution and dealing with the consequences highlighted above simply in order to delay the proper migration of modules to the new model seems like a very risky choice, so people considering it might want to properly analyze/measure the cost of properly migrating their module immediately.

On a technical level
--------------------
Some preliminary remarks, ignoring the functional issues:
- Shouldn't the mixin class be responsible for adding the extra fields that it depends on? What is the rationale for not doing so anymore?
- Shouldn't you mixin support cases where there are multiple partner fields on a document, in which case the extra contact field logically needs to be duplicated for each? (see functional point 3. above)
- for search views, shouldn't the mixin also deal with group_by filters, to make the contact information available?
- l.25: why override read()? If the write() logic is right it should be unnecessary. I imagine this is to deal with cases where the value is missing, e.g. for existing records, but shouldn't this be better handled by some sort of  migration script? In addition, it does not deal with the case where fields=None means "all fields in self._columns".
- l.77-78: calling fields_get() can be expensive, so you should avoid making 2 calls if one call can provide both results, and also avoid doing it if the result is discarded (e.g. if the fields are not in the view). Note that the content of "partner_descr" should also be already present in the result obtained from the super() call in this case.
- l.90: setting the @groups property on the contact_id field does not seem correct, even if that happens too late to have any effect in the UI. Perhaps you meant it for search views instead?
- l.109: the recursive implementation could be replaced by a simpler and shorter iterative version

review: Disapprove

Reply

lp://staging/~openerp-community/openobject-server/contact-id-solution-for-v7-contact-management updated on 2013-04-21

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

Odoo Server (MOVED TO GITHUB)

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

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers