Merge lp:~therp-nl/therp-backports/openerp-server-6.1-lp960201_fix_sequence into lp:therp-backports/server-6.1

Proposed by Stefan Rijnhart (Opener) on 2012-11-29
Status: Merged
Merged at revision: 4300
Proposed branch: lp:~therp-nl/therp-backports/openerp-server-6.1-lp960201_fix_sequence
Merge into: lp:therp-backports/server-6.1
Diff against target: 205 lines (+105/-10)
2 files modified
openerp/addons/base/ir/ir.xml (+11/-1)
openerp/addons/base/ir/ir_sequence.py (+94/-9)
To merge this branch: bzr merge lp:~therp-nl/therp-backports/openerp-server-6.1-lp960201_fix_sequence
Reviewer Review Type Date Requested Status
Stefan Rijnhart (Opener) (community) Approve on 2013-04-05
Holger Brunn (Therp) Approve on 2013-04-05
Cristian Salamea (community) Needs Information on 2013-04-04
Review via email: mp+136993@code.launchpad.net
To post a comment you must log in.
Cristian Salamea (ovnicraft) wrote :

Hello, there is any news about the status of this merge ?
I am testing and works ok by now, no errors and gives the correct feedback.
I want to know our opinion.

Regards,

review: Needs Information

Yes, review from me and Ronald's other colleagues is long overdue. Note that this proposal is on the therp-backports of OpenERP, not on the official branches. That proposal is here: https://code.launchpad.net/~therp-nl/openobject-server/ronald@therp.nl_fix_sequence_lp960201/+merge/136913. It has been neglected there as well, by the OpenERP developers. If you have tested the code succesfully, I would appreciate it if you submitted your approval there, using 'test' as a review type.

I'll take the opportunity to review now as well. Ronald, is there a technical reason for the whole 'prohibit' scheme, or do you really want to save junior administrators who strayed into this dark corner of the configuration from accidentally changing the next number? I think it clutters up this otherwise really good patch and I would prefer it if you removed it.

review: Needs Fixing

Stefan,

The prohibit "scheme" is not there for a technical reason, but to prevent users from shooting themselves in their foot.

There are already to many places in OpenERP were users are led to dangerous mistakes and errors, no need to add to them.

Ronald

Holger Brunn (Therp) (hbrunn) wrote :

Making it more complicated for users to screw up is a good thing in my opinion. And I like your solution.

review: Approve

I gladly bow to the majority. We need to remember this one if Therp branches get promoted to OCB/6.1 with stable/unstable split.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'openerp/addons/base/ir/ir.xml'
--- openerp/addons/base/ir/ir.xml 2012-05-16 12:38:46 +0000
+++ openerp/addons/base/ir/ir.xml 2012-11-29 17:02:22 +0000
@@ -151,7 +151,17 @@
151 <field name="suffix"/>151 <field name="suffix"/>
152 <field name="padding"/>152 <field name="padding"/>
153 <field name="number_increment"/>153 <field name="number_increment"/>
154 <field name="number_next"/>154 <!-- If number_next_actual is 0, we have a brand new record -->
155 <field
156 name="prohibit_number_next_change"
157 attrs="{'invisible':[('number_next_actual','=',0)]}" />
158 <field name="prohibit_number_next_lower"
159 attrs="{'invisible':[('number_next_actual','=',0)]}" />
160 <field name="number_next_actual"
161 attrs="{'invisible':[('number_next_actual','=',0)]}" />
162 <field
163 name="number_next"
164 attrs="{'readonly':[('prohibit_number_next_change','=',True),('number_next_actual','&gt;',0)]}" />
155 <field name="implementation"/>165 <field name="implementation"/>
156 <separator colspan="4" string="Legend (for prefix, suffix)"/>166 <separator colspan="4" string="Legend (for prefix, suffix)"/>
157 <group col="8" colspan="4">167 <group col="8" colspan="4">
158168
=== modified file 'openerp/addons/base/ir/ir_sequence.py'
--- openerp/addons/base/ir/ir_sequence.py 2012-01-24 11:47:30 +0000
+++ openerp/addons/base/ir/ir_sequence.py 2012-11-29 17:02:22 +0000
@@ -22,6 +22,7 @@
22import logging22import logging
23import time23import time
2424
25from openerp.tools.translate import _
25import openerp26import openerp
2627
27_logger = logging.getLogger(__name__)28_logger = logging.getLogger(__name__)
@@ -37,7 +38,7 @@
37 _sql_constraints = [38 _sql_constraints = [
38 ('code_unique', 'unique(code)', '`code` must be unique.'),39 ('code_unique', 'unique(code)', '`code` must be unique.'),
39 ]40 ]
4041
41def _code_get(self, cr, uid, context=None):42def _code_get(self, cr, uid, context=None):
42 cr.execute('select code, name from ir_sequence_type')43 cr.execute('select code, name from ir_sequence_type')
43 return cr.fetchall()44 return cr.fetchall()
@@ -52,6 +53,32 @@
52 """53 """
53 _name = 'ir.sequence'54 _name = 'ir.sequence'
54 _order = 'name'55 _order = 'name'
56
57 def _get_number_next_actual(
58 self, cr, user, ids, field_name, arg, context=None):
59 '''Return number from ir_sequence row when no_gap implementation,
60 and number from postgres sequence when standard implementation.'''
61 res = dict.fromkeys(ids)
62 for seq_id in ids:
63 this_obj = self.browse(cr, user, seq_id, context=context)
64 if this_obj.implementation != 'standard':
65 res[seq_id] = this_obj.number_next
66 else:
67 # get number from postgres sequence. Cannot use
68 # currval, because that might give an error when
69 # not having used nextval before.
70 statement = (
71 "SELECT last_value, increment_by, is_called"
72 " FROM ir_sequence_%03d"
73 % seq_id)
74 cr.execute(statement)
75 (last_value, increment_by, is_called) = cr.fetchone()
76 if is_called:
77 res[seq_id] = last_value + increment_by
78 else:
79 res[seq_id] = last_value
80 return res
81
55 _columns = {82 _columns = {
56 'name': openerp.osv.fields.char('Name', size=64, required=True),83 'name': openerp.osv.fields.char('Name', size=64, required=True),
57 'code': openerp.osv.fields.selection(_code_get, 'Code', size=64),84 'code': openerp.osv.fields.selection(_code_get, 'Code', size=64),
@@ -64,11 +91,31 @@
64 'active': openerp.osv.fields.boolean('Active'),91 'active': openerp.osv.fields.boolean('Active'),
65 'prefix': openerp.osv.fields.char('Prefix', size=64, help="Prefix value of the record for the sequence"),92 'prefix': openerp.osv.fields.char('Prefix', size=64, help="Prefix value of the record for the sequence"),
66 'suffix': openerp.osv.fields.char('Suffix', size=64, help="Suffix value of the record for the sequence"),93 'suffix': openerp.osv.fields.char('Suffix', size=64, help="Suffix value of the record for the sequence"),
67 'number_next': openerp.osv.fields.integer('Next Number', required=True, help="Next number of this sequence"),94 'number_next': openerp.osv.fields.integer(
95 'Next Number', required=True,
96 help='Next number originally set or reset for this sequence'),
97 'number_next_actual': openerp.osv.fields.function(
98 _get_number_next_actual, type='integer', required=True,
99 string='Actual Next Number',
100 help='Next number that will actually be used.'
101 ' Will be zero for new sequence.'),
102 'prohibit_number_next_change': openerp.osv.fields.boolean(
103 'Prohibit change of Next Number',
104 help='Changing next number can wreak havoc on sequences.'
105 ' Checking this option will prevent accidental changes.'),
106 'prohibit_number_next_lower': openerp.osv.fields.boolean(
107 'Prohibit lower value for Next Number',
108 help='When manually setting Next Number to a value lower than the'
109 ' the current value, there is a big risk for duplicate key errors'
110 ' when trying to add new sequences generated to the database.'
111 ' This is especially the case when your prefix or suffix is not'
112 ' changed at the same time, or has no dynamic part (formula) that'
113 ' will also change.'),
68 'number_increment': openerp.osv.fields.integer('Increment Number', required=True, help="The next number of the sequence will be incremented by this number"),114 'number_increment': openerp.osv.fields.integer('Increment Number', required=True, help="The next number of the sequence will be incremented by this number"),
69 'padding' : openerp.osv.fields.integer('Number Padding', required=True, help="OpenERP will automatically adds some '0' on the left of the 'Next Number' to get the required padding size."),115 'padding' : openerp.osv.fields.integer('Number Padding', required=True, help="OpenERP will automatically adds some '0' on the left of the 'Next Number' to get the required padding size."),
70 'company_id': openerp.osv.fields.many2one('res.company', 'Company'),116 'company_id': openerp.osv.fields.many2one('res.company', 'Company'),
71 }117 }
118
72 _defaults = {119 _defaults = {
73 'implementation': 'standard',120 'implementation': 'standard',
74 'active': True,121 'active': True,
@@ -76,6 +123,8 @@
76 'number_increment': 1,123 'number_increment': 1,
77 'number_next': 1,124 'number_next': 1,
78 'padding' : 0,125 'padding' : 0,
126 'prohibit_number_next_change': False,
127 'prohibit_number_next_lower': False,
79 }128 }
80129
81 def init(self, cr):130 def init(self, cr):
@@ -117,16 +166,18 @@
117 # object depends on it.166 # object depends on it.
118 cr.execute("DROP SEQUENCE IF EXISTS %s RESTRICT " % names)167 cr.execute("DROP SEQUENCE IF EXISTS %s RESTRICT " % names)
119168
120 def _alter_sequence(self, cr, id, number_increment, number_next):169 def _alter_sequence(self, cr, id, number_increment, number_next=0):
121 """ Alter a PostreSQL sequence.170 """ Alter a PostreSQL sequence.
122171
123 There is no access rights check.172 There is no access rights check.
124 """173 """
125 assert isinstance(id, (int, long))174 assert isinstance(id, (int, long))
126 cr.execute("""175 statement = ("ALTER SEQUENCE ir_sequence_%03d INCREMENT BY %d" %
127 ALTER SEQUENCE ir_sequence_%03d INCREMENT BY %%s RESTART WITH %%s176 (id, number_increment))
128 """ % id, (number_increment, number_next))177 if number_next:
129178 statement += " RESTART WITH %d" % (number_next, )
179 cr.execute(statement)
180
130 def create(self, cr, uid, values, context=None):181 def create(self, cr, uid, values, context=None):
131 """ Create a sequence, in implementation == standard a fast gaps-allowed PostgreSQL sequence is used.182 """ Create a sequence, in implementation == standard a fast gaps-allowed PostgreSQL sequence is used.
132 """183 """
@@ -140,10 +191,37 @@
140 super(ir_sequence, self).unlink(cr, uid, ids, context)191 super(ir_sequence, self).unlink(cr, uid, ids, context)
141 self._drop_sequence(cr, ids)192 self._drop_sequence(cr, ids)
142 return True193 return True
194
195 def _pre_validate(self, cr, uid, ids, values, context=None):
196 '''If number_next changed, check wether change allowed.
197If number is lower than actual next number, check wether this is allowed.
198Raise error when invalid change requested.'''
199 if not 'number_next' in values:
200 return
201 nn_new = values['number_next']
202 for this_obj in self.browse(cr, uid, ids, context=context):
203 # Only validate when changed.
204 if this_obj.number_next != nn_new:
205 prohibit_change = values.get(
206 'prohibit_number_next_change',
207 this_obj.prohibit_number_next_change)
208 if prohibit_change:
209 raise openerp.osv.orm.except_orm(
210 _('ValidationError'),
211 _('Chance of Next Number has been prohibited'))
212 prohibit_lower = values.get(
213 'prohibit_number_next_lower',
214 this_obj.prohibit_number_next_lower)
215 if prohibit_lower and (this_obj.number_next_actual > nn_new):
216 raise openerp.osv.orm.except_orm(
217 _('ValidationError'),
218 _('Setting Next Number to a value lower than the'
219 ' actual value has been prohibited'))
143220
144 def write(self, cr, uid, ids, values, context=None):221 def write(self, cr, uid, ids, values, context=None):
145 if not isinstance(ids, (list, tuple)):222 if not isinstance(ids, (list, tuple)):
146 ids = [ids]223 ids = [ids]
224 self._pre_validate(cr, uid, ids, values, context=context)
147 new_implementation = values.get('implementation')225 new_implementation = values.get('implementation')
148 rows = self.read(cr, uid, ids, ['implementation', 'number_increment', 'number_next'], context)226 rows = self.read(cr, uid, ids, ['implementation', 'number_increment', 'number_next'], context)
149 super(ir_sequence, self).write(cr, uid, ids, values, context)227 super(ir_sequence, self).write(cr, uid, ids, values, context)
@@ -154,7 +232,15 @@
154 n = values.get('number_next', row['number_next'])232 n = values.get('number_next', row['number_next'])
155 if row['implementation'] == 'standard':233 if row['implementation'] == 'standard':
156 if new_implementation in ('standard', None):234 if new_implementation in ('standard', None):
157 self._alter_sequence(cr, row['id'], i, n)235 # Implementation has NOT changed.
236 # Only change sequence if really requested.
237 if (('number_next' in values)
238 and (values['number_next'] != row['number_next'])):
239 self._alter_sequence(cr, row['id'], i, n)
240 else:
241 # Just in case only increment changed
242 if 'number_increment' in values:
243 self._alter_sequence(cr, row['id'], i)
158 else:244 else:
159 self._drop_sequence(cr, row['id'])245 self._drop_sequence(cr, row['id'])
160 else:246 else:
@@ -162,7 +248,6 @@
162 pass248 pass
163 else:249 else:
164 self._create_sequence(cr, row['id'], i, n)250 self._create_sequence(cr, row['id'], i, n)
165
166 return True251 return True
167252
168 def _interpolate(self, s, d):253 def _interpolate(self, s, d):

Subscribers

People subscribed via source and target branches

to all changes: