Merge lp:~numerigraphe-team/ocb-addons/7.0-bug-1079548-sgo into lp:ocb-addons

[Lionel Sausin - Initiatives/Numérigraphe]

This fixes a bug where the MRP scheduler fails when messages are too long to be stored in the database (particularly if the locale strings are verbose).
The length constraint is removed altogether as proposed by Guewen Baconnier.

This was approved already but I'm resubmitting because the migration team confirms they WILL truncate the data if we allow larger texts than trunk[1].
So it's trivial only because it's a message log.
I'll leave it up to you to decide among consenting adults as humorously suggested by Raphael.

[1] Complete reply from the migration team:
"""
If you have used 'custom' text fields, then migration process will not check the size of the data.

But if you have increased the size of OpenERP 'standard' fields by patching in the source version code, then migration process will truncate the data to the original shorter size (according to standard code of target version).

However, if your patch can be bundled as a separate custom module, you have the option to send the module to OpenERP so that your changes are considered. In that case, data will not be truncated. However this is a separate service; you may discuss this with your OpenERP account manager or the sales team for more info.
"""

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

The size argument could be entirely removed, it is no longer required since the version 7.0.
It should be used when there is a reason to restrict the length of a field (e.g. ean13), there is no reason to do so here (varchar and text are the same thing under the hood, so the limitation won't bring better performance [0]).

For the record, here is the MP on the OpenERP's branch:
https://code.launchpad.net/~openerp-dev/openobject-addons/trunk-bug-1079548-sgo/+merge/137482

Thanks

[0] http://stackoverflow.com/questions/4848964/postgresql-difference-between-text-and-varchar-character-varying

review: Needs Fixing

[Stefan Rijnhart (Opener) (stefan-opener)]

Hi Lionel,

thanks for joining the OCB initiative! In general, you can attribute the original author using the --author <email-of-author>

Usually, bzr log -r -1 <original branch> will tell you the author's email address.

This way, the original author is immediately clear in the bzr history and also on Launchpad.

Would you consider recommitting this with the correct author?

review: Needs Fixing

[Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives)]

Stefan,
I'm usually careful to quote authors when chery-pick patches because bzr loses them, but I was not aware that it was useful when merging too. I'll sure fix this.
BTW I initially only made a merge commit to add the missing "--fixes" from the original branch.

review: Needs Resubmitting

[Stefan Rijnhart (Opener) (stefan-opener)]

Thanks for the changes so far! Did you make the change from char to text on purpose? This affects the display widget in the client, but I'm not sure if this is good or bad.

review: Needs Information

[Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives)]

 > Thanks for the changes so far! Did you make the change from char to
text on purpose? This affects the display widget in the client, but I'm
not sure if this is good or bad.
Uhh, I didn't think about that, sorry. I supposed size was mandatory in
char() - or is bug #1088379 fixed ?

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

> > Thanks for the changes so far! Did you make the change from char to
> text on purpose? This affects the display widget in the client, but I'm
> not sure if this is good or bad.
> Uhh, I didn't think about that, sorry. I supposed size was mandatory in
> char() - or is bug #1088379 fixed ?

Sorry I was maybe not clear in my comment. My idea was indeed just to remove the size argument, but to keep a char field.

The bug lp:1088379 concerns only the custom fields. Char fields in _columns are not concerned by these bug.

[Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives)]

OK I've reverted to char().

[Stefan Rijnhart (Opener) (stefan-opener)]

Thanks!

review: Approve

[Pedro Manuel Baeza (pedro.baeza)]

LGTM

Regards

review: Approve (code review)

[Holger Brunn (Therp) (hbrunn)]

review: Approve (code review)

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

are we all aware that this MP is not going to be accepted in the official stable branch, because it changes the database schema definition?

I personally have nothing against merging in OCB.

review: Needs Information (code review, no test)

[Leonardo Pistone (lepistone)]

Good point Alexandre.

But unless someone can come up with a practical case where this specific example of schema change could be a problem (I don't), I approve.

review: Approve (code review)

[Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives)]

I'll ask the maintenance team if there is a risk that text fields get truncated during migration.

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

OCB is for consenting adults, it's a very trivial schema change.

review: Approve

[Stefan Rijnhart (Opener) (stefan-opener)]

Let's hope the change gets adopted in upstream 8.0. At that point, the truncation will only happen when reverting back from OCB 7.0 to OpenERP 7.0. At least in this particular case the impact of the truncation will be nil.

The custom module approach does not work for people who dp "--upgrade all" occasionally (such as OCB users), as the inherited module gets upgraded first, thereby resetting the column width and truncating the data.

If this change does not get adopted in OpenERP 8.0 but will be ported to OCB 8.0, this will cause truncation when upgrading with OPW and interestingly, also with OpenUpgrade. The latter can be prevented by merging in OCB 8.0 addons before running the upgrade. Maybe OPW-users should ask if the same can be done for their migrations.

In all cases, this change is still an improvement even if a migration causes a mild regression for existing records.

review: Approve

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

Thanks Lionel

review: Approve

[Yannick Vaucher @ Camptocamp (yvaucher-c2c)]

LGTM Thanks

review: Approve

[Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives)]

Latest news from the migration team: all is fine after all:
"I made a small test and I found that, you do not need to send your custom addons for migration because migration process will not truncate the data , but when you create a new recorded with interface according increased size, at that time, data will be truncated according to our 'standard' code of (version7.0)."

review: Approve (no truncation problem after all)

[Holger Brunn (Therp) (hbrunn)]

review: Approve (code review)

[Holger Brunn (Therp) (hbrunn)]

We have a lot of approvals but still also a lot of pending review requests. Given that 5 approvals already is unusually many, I'll approve this one.

[Leonardo Pistone (lepistone)]

Thanks Holger.

It's actually more then 5: the "pending votes" are from people like me that approved the old version of the proposal (i'm approving again here). The new proposal looks identical, what happened?

review: Approve

[Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives)]

Well sorry for the noise, but there was a moment of uncertainty after
you approved, when the migration team announced they would truncate the
data during migration to v8.
But they won't after all, so this change is really no big deal.

=== modified file 'procurement/procurement.py'
--- procurement/procurement.py	2013-11-12 15:17:47 +0000
+++ procurement/procurement.py	2014-03-18 11:02:25 +0000
@@ -104,7 +104,7 @@
             readonly=True, required=True, help="If you encode manually a Procurement, you probably want to use" \
             " a make to order method."),
         'note': fields.text('Note'),
-        'message': fields.char('Latest error', size=124, help="Exception occurred while computing procurement orders."),
+        'message': fields.char('Latest error', help="Exception occurred while computing procurement orders."),
         'state': fields.selection([
             ('draft','Draft'),
             ('cancel','Cancelled'),