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

Proposed by Lionel Sausin - Initiatives/Numérigraphe
Status: Superseded
Proposed branch: lp:~numerigraphe-team/ocb-addons/7.0-bug-1079548-sgo
Merge into: lp:ocb-addons
Diff against target: 12 lines (+1/-1)
1 file modified
procurement/procurement.py (+1/-1)
To merge this branch: bzr merge lp:~numerigraphe-team/ocb-addons/7.0-bug-1079548-sgo
Reviewer Review Type Date Requested Status
Raphaël Valyi - http://www.akretion.com Approve
Leonardo Pistone code review Approve
Alexandre Fayolle - camptocamp code review, no test Needs Information
Holger Brunn (Therp) code review Approve
Pedro Manuel Baeza code review Approve
Stefan Rijnhart (Opener) Approve
Guewen Baconnier @ Camptocamp Pending
Lionel Sausin - Initiatives/Numérigraphe Pending
Review via email: mp+209917@code.launchpad.net

This proposal supersedes a proposal from 2014-03-07.

This proposal has been superseded by a proposal from 2014-03-18.

Description of the change

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.

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

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
Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

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
Revision history for this message
Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives) wrote :

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

Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

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

Revision history for this message
Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives) wrote :

OK I've reverted to char().

Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

Thanks!

review: Approve
Revision history for this message
Pedro Manuel Baeza (pedro.baeza) wrote :

LGTM

Regards

review: Approve (code review)
Revision history for this message
Holger Brunn (Therp) (hbrunn) :
review: Approve (code review)
Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

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)
Revision history for this message
Leonardo Pistone (lepistone) wrote :

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)
Revision history for this message
Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives) wrote :

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

Revision history for this message
Raphaël Valyi - http://www.akretion.com (rvalyi) wrote :

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'procurement/procurement.py'
2--- procurement/procurement.py 2013-11-12 15:17:47 +0000
3+++ procurement/procurement.py 2014-03-07 17:38:30 +0000
4@@ -104,7 +104,7 @@
5 readonly=True, required=True, help="If you encode manually a Procurement, you probably want to use" \
6 " a make to order method."),
7 'note': fields.text('Note'),
8- 'message': fields.char('Latest error', size=124, help="Exception occurred while computing procurement orders."),
9+ 'message': fields.char('Latest error', help="Exception occurred while computing procurement orders."),
10 'state': fields.selection([
11 ('draft','Draft'),
12 ('cancel','Cancelled'),