Merge lp:~therp-nl/openobject-server/ronald@therp.nl_fix_sequence_lp960201 into lp:openobject-server
- ronald@therp.nl_fix_sequence_lp960201
- Merge into trunk
Proposed by
Ronald Portier (Therp)
Status: | Merged |
---|---|
Merge reported by: | Olivier Dony (Odoo) |
Merged at revision: | not available |
Proposed branch: | lp:~therp-nl/openobject-server/ronald@therp.nl_fix_sequence_lp960201 |
Merge into: | lp:openobject-server |
Diff against target: |
213 lines (+108/-10) (has conflicts) 2 files modified
openerp/addons/base/ir/ir_sequence.py (+97/-9) openerp/addons/base/ir/ir_sequence_view.xml (+11/-1) Text conflict in openerp/addons/base/ir/ir_sequence.py |
To merge this branch: | bzr merge lp:~therp-nl/openobject-server/ronald@therp.nl_fix_sequence_lp960201 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
OpenERP Core Team | Pending | ||
Review via email: mp+136913@code.launchpad.net |
Commit message
Description of the change
Fixes lp960201
To post a comment you must log in.
Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote : | # |
Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote : | # |
Note: revision 4978 does not contain the fix for the creation of sequence, that part was applied in revision 4979
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'openerp/addons/base/ir/ir_sequence.py' | |||
2 | --- openerp/addons/base/ir/ir_sequence.py 2012-11-26 09:43:37 +0000 | |||
3 | +++ openerp/addons/base/ir/ir_sequence.py 2012-11-29 11:40:33 +0000 | |||
4 | @@ -22,9 +22,13 @@ | |||
5 | 22 | import logging | 22 | import logging |
6 | 23 | import time | 23 | import time |
7 | 24 | 24 | ||
8 | 25 | <<<<<<< TREE | ||
9 | 25 | from osv import osv, fields | 26 | from osv import osv, fields |
10 | 26 | from tools.translate import _ | 27 | from tools.translate import _ |
11 | 27 | 28 | ||
12 | 29 | ======= | ||
13 | 30 | from openerp.tools.translate import _ | ||
14 | 31 | >>>>>>> MERGE-SOURCE | ||
15 | 28 | import openerp | 32 | import openerp |
16 | 29 | 33 | ||
17 | 30 | _logger = logging.getLogger(__name__) | 34 | _logger = logging.getLogger(__name__) |
18 | @@ -40,7 +44,7 @@ | |||
19 | 40 | _sql_constraints = [ | 44 | _sql_constraints = [ |
20 | 41 | ('code_unique', 'unique(code)', '`code` must be unique.'), | 45 | ('code_unique', 'unique(code)', '`code` must be unique.'), |
21 | 42 | ] | 46 | ] |
23 | 43 | 47 | ||
24 | 44 | def _code_get(self, cr, uid, context=None): | 48 | def _code_get(self, cr, uid, context=None): |
25 | 45 | cr.execute('select code, name from ir_sequence_type') | 49 | cr.execute('select code, name from ir_sequence_type') |
26 | 46 | return cr.fetchall() | 50 | return cr.fetchall() |
27 | @@ -55,6 +59,32 @@ | |||
28 | 55 | """ | 59 | """ |
29 | 56 | _name = 'ir.sequence' | 60 | _name = 'ir.sequence' |
30 | 57 | _order = 'name' | 61 | _order = 'name' |
31 | 62 | |||
32 | 63 | def _get_number_next_actual( | ||
33 | 64 | self, cr, user, ids, field_name, arg, context=None): | ||
34 | 65 | '''Return number from ir_sequence row when no_gap implementation, | ||
35 | 66 | and number from postgres sequence when standard implementation.''' | ||
36 | 67 | res = dict.fromkeys(ids) | ||
37 | 68 | for seq_id in ids: | ||
38 | 69 | this_obj = self.browse(cr, user, seq_id, context=context) | ||
39 | 70 | if this_obj.implementation != 'standard': | ||
40 | 71 | res[seq_id] = this_obj.number_next | ||
41 | 72 | else: | ||
42 | 73 | # get number from postgres sequence. Cannot use | ||
43 | 74 | # currval, because that might give an error when | ||
44 | 75 | # not having used nextval before. | ||
45 | 76 | statement = ( | ||
46 | 77 | "SELECT last_value, increment_by, is_called" | ||
47 | 78 | " FROM ir_sequence_%03d" | ||
48 | 79 | % seq_id) | ||
49 | 80 | cr.execute(statement) | ||
50 | 81 | (last_value, increment_by, is_called) = cr.fetchone() | ||
51 | 82 | if is_called: | ||
52 | 83 | res[seq_id] = last_value + increment_by | ||
53 | 84 | else: | ||
54 | 85 | res[seq_id] = last_value | ||
55 | 86 | return res | ||
56 | 87 | |||
57 | 58 | _columns = { | 88 | _columns = { |
58 | 59 | 'name': openerp.osv.fields.char('Name', size=64, required=True), | 89 | 'name': openerp.osv.fields.char('Name', size=64, required=True), |
59 | 60 | 'code': openerp.osv.fields.selection(_code_get, 'Code', size=64), | 90 | 'code': openerp.osv.fields.selection(_code_get, 'Code', size=64), |
60 | @@ -67,11 +97,31 @@ | |||
61 | 67 | 'active': openerp.osv.fields.boolean('Active'), | 97 | 'active': openerp.osv.fields.boolean('Active'), |
62 | 68 | 'prefix': openerp.osv.fields.char('Prefix', size=64, help="Prefix value of the record for the sequence"), | 98 | 'prefix': openerp.osv.fields.char('Prefix', size=64, help="Prefix value of the record for the sequence"), |
63 | 69 | 'suffix': openerp.osv.fields.char('Suffix', size=64, help="Suffix value of the record for the sequence"), | 99 | 'suffix': openerp.osv.fields.char('Suffix', size=64, help="Suffix value of the record for the sequence"), |
65 | 70 | 'number_next': openerp.osv.fields.integer('Next Number', required=True, help="Next number of this sequence"), | 100 | 'number_next': openerp.osv.fields.integer( |
66 | 101 | 'Next Number', required=True, | ||
67 | 102 | help='Next number originally set or reset for this sequence'), | ||
68 | 103 | 'number_next_actual': openerp.osv.fields.function( | ||
69 | 104 | _get_number_next_actual, type='integer', required=True, | ||
70 | 105 | string='Actual Next Number', | ||
71 | 106 | help='Next number that will actually be used.' | ||
72 | 107 | ' Will be zero for new sequence.'), | ||
73 | 108 | 'prohibit_number_next_change': openerp.osv.fields.boolean( | ||
74 | 109 | 'Prohibit change of Next Number', | ||
75 | 110 | help='Changing next number can wreak havoc on sequences.' | ||
76 | 111 | ' Checking this option will prevent accidental changes.'), | ||
77 | 112 | 'prohibit_number_next_lower': openerp.osv.fields.boolean( | ||
78 | 113 | 'Prohibit lower value for Next Number', | ||
79 | 114 | help='When manually setting Next Number to a value lower than the' | ||
80 | 115 | ' the current value, there is a big risk for duplicate key errors' | ||
81 | 116 | ' when trying to add new sequences generated to the database.' | ||
82 | 117 | ' This is especially the case when your prefix or suffix is not' | ||
83 | 118 | ' changed at the same time, or has no dynamic part (formula) that' | ||
84 | 119 | ' will also change.'), | ||
85 | 71 | 'number_increment': openerp.osv.fields.integer('Increment Number', required=True, help="The next number of the sequence will be incremented by this number"), | 120 | 'number_increment': openerp.osv.fields.integer('Increment Number', required=True, help="The next number of the sequence will be incremented by this number"), |
86 | 72 | '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."), | 121 | '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."), |
87 | 73 | 'company_id': openerp.osv.fields.many2one('res.company', 'Company'), | 122 | 'company_id': openerp.osv.fields.many2one('res.company', 'Company'), |
88 | 74 | } | 123 | } |
89 | 124 | |||
90 | 75 | _defaults = { | 125 | _defaults = { |
91 | 76 | 'implementation': 'standard', | 126 | 'implementation': 'standard', |
92 | 77 | 'active': True, | 127 | 'active': True, |
93 | @@ -79,6 +129,8 @@ | |||
94 | 79 | 'number_increment': 1, | 129 | 'number_increment': 1, |
95 | 80 | 'number_next': 1, | 130 | 'number_next': 1, |
96 | 81 | 'padding' : 0, | 131 | 'padding' : 0, |
97 | 132 | 'prohibit_number_next_change': False, | ||
98 | 133 | 'prohibit_number_next_lower': False, | ||
99 | 82 | } | 134 | } |
100 | 83 | 135 | ||
101 | 84 | def init(self, cr): | 136 | def init(self, cr): |
102 | @@ -122,7 +174,7 @@ | |||
103 | 122 | # object depends on it. | 174 | # object depends on it. |
104 | 123 | cr.execute("DROP SEQUENCE IF EXISTS %s RESTRICT " % names) | 175 | cr.execute("DROP SEQUENCE IF EXISTS %s RESTRICT " % names) |
105 | 124 | 176 | ||
107 | 125 | def _alter_sequence(self, cr, id, number_increment, number_next): | 177 | def _alter_sequence(self, cr, id, number_increment, number_next=0): |
108 | 126 | """ Alter a PostreSQL sequence. | 178 | """ Alter a PostreSQL sequence. |
109 | 127 | 179 | ||
110 | 128 | There is no access rights check. | 180 | There is no access rights check. |
111 | @@ -130,10 +182,12 @@ | |||
112 | 130 | if number_increment == 0: | 182 | if number_increment == 0: |
113 | 131 | raise osv.except_osv(_('Warning!'),_("Increment number must not be zero.")) | 183 | raise osv.except_osv(_('Warning!'),_("Increment number must not be zero.")) |
114 | 132 | assert isinstance(id, (int, long)) | 184 | assert isinstance(id, (int, long)) |
119 | 133 | cr.execute(""" | 185 | statement = ("ALTER SEQUENCE ir_sequence_%03d INCREMENT BY %d" % |
120 | 134 | ALTER SEQUENCE ir_sequence_%03d INCREMENT BY %%s RESTART WITH %%s | 186 | (id, number_increment)) |
121 | 135 | """ % id, (number_increment, number_next)) | 187 | if number_next: |
122 | 136 | 188 | statement += " RESTART WITH %d" % (number_next, ) | |
123 | 189 | cr.execute(statement) | ||
124 | 190 | |||
125 | 137 | def create(self, cr, uid, values, context=None): | 191 | def create(self, cr, uid, values, context=None): |
126 | 138 | """ Create a sequence, in implementation == standard a fast gaps-allowed PostgreSQL sequence is used. | 192 | """ Create a sequence, in implementation == standard a fast gaps-allowed PostgreSQL sequence is used. |
127 | 139 | """ | 193 | """ |
128 | @@ -147,10 +201,37 @@ | |||
129 | 147 | super(ir_sequence, self).unlink(cr, uid, ids, context) | 201 | super(ir_sequence, self).unlink(cr, uid, ids, context) |
130 | 148 | self._drop_sequence(cr, ids) | 202 | self._drop_sequence(cr, ids) |
131 | 149 | return True | 203 | return True |
132 | 204 | |||
133 | 205 | def _pre_validate(self, cr, uid, ids, values, context=None): | ||
134 | 206 | '''If number_next changed, check wether change allowed. | ||
135 | 207 | If number is lower than actual next number, check wether this is allowed. | ||
136 | 208 | Raise error when invalid change requested.''' | ||
137 | 209 | if not 'number_next' in values: | ||
138 | 210 | return | ||
139 | 211 | nn_new = values['number_next'] | ||
140 | 212 | for this_obj in self.browse(cr, uid, ids, context=context): | ||
141 | 213 | # Only validate when changed. | ||
142 | 214 | if this_obj.number_next != nn_new: | ||
143 | 215 | prohibit_change = values.get( | ||
144 | 216 | 'prohibit_number_next_change', | ||
145 | 217 | this_obj.prohibit_number_next_change) | ||
146 | 218 | if prohibit_change: | ||
147 | 219 | raise openerp.osv.orm.except_orm( | ||
148 | 220 | _('ValidationError'), | ||
149 | 221 | _('Chance of Next Number has been prohibited')) | ||
150 | 222 | prohibit_lower = values.get( | ||
151 | 223 | 'prohibit_number_next_lower', | ||
152 | 224 | this_obj.prohibit_number_next_lower) | ||
153 | 225 | if prohibit_lower and (this_obj.number_next_actual > nn_new): | ||
154 | 226 | raise openerp.osv.orm.except_orm( | ||
155 | 227 | _('ValidationError'), | ||
156 | 228 | _('Setting Next Number to a value lower than the' | ||
157 | 229 | ' actual value has been prohibited')) | ||
158 | 150 | 230 | ||
159 | 151 | def write(self, cr, uid, ids, values, context=None): | 231 | def write(self, cr, uid, ids, values, context=None): |
160 | 152 | if not isinstance(ids, (list, tuple)): | 232 | if not isinstance(ids, (list, tuple)): |
161 | 153 | ids = [ids] | 233 | ids = [ids] |
162 | 234 | self._pre_validate(cr, uid, ids, values, context=context) | ||
163 | 154 | new_implementation = values.get('implementation') | 235 | new_implementation = values.get('implementation') |
164 | 155 | rows = self.read(cr, uid, ids, ['implementation', 'number_increment', 'number_next'], context) | 236 | rows = self.read(cr, uid, ids, ['implementation', 'number_increment', 'number_next'], context) |
165 | 156 | super(ir_sequence, self).write(cr, uid, ids, values, context) | 237 | super(ir_sequence, self).write(cr, uid, ids, values, context) |
166 | @@ -161,7 +242,15 @@ | |||
167 | 161 | n = values.get('number_next', row['number_next']) | 242 | n = values.get('number_next', row['number_next']) |
168 | 162 | if row['implementation'] == 'standard': | 243 | if row['implementation'] == 'standard': |
169 | 163 | if new_implementation in ('standard', None): | 244 | if new_implementation in ('standard', None): |
171 | 164 | self._alter_sequence(cr, row['id'], i, n) | 245 | # Implementation has NOT changed. |
172 | 246 | # Only change sequence if really requested. | ||
173 | 247 | if (('number_next' in values) | ||
174 | 248 | and (values['number_next'] != row['number_next'])): | ||
175 | 249 | self._alter_sequence(cr, row['id'], i, n) | ||
176 | 250 | else: | ||
177 | 251 | # Just in case only increment changed | ||
178 | 252 | if 'number_increment' in values: | ||
179 | 253 | self._alter_sequence(cr, row['id'], i) | ||
180 | 165 | else: | 254 | else: |
181 | 166 | self._drop_sequence(cr, row['id']) | 255 | self._drop_sequence(cr, row['id']) |
182 | 167 | else: | 256 | else: |
183 | @@ -169,7 +258,6 @@ | |||
184 | 169 | pass | 258 | pass |
185 | 170 | else: | 259 | else: |
186 | 171 | self._create_sequence(cr, row['id'], i, n) | 260 | self._create_sequence(cr, row['id'], i, n) |
187 | 172 | |||
188 | 173 | return True | 261 | return True |
189 | 174 | 262 | ||
190 | 175 | def _interpolate(self, s, d): | 263 | def _interpolate(self, s, d): |
191 | 176 | 264 | ||
192 | === modified file 'openerp/addons/base/ir/ir_sequence_view.xml' | |||
193 | --- openerp/addons/base/ir/ir_sequence_view.xml 2012-10-13 12:09:24 +0000 | |||
194 | +++ openerp/addons/base/ir/ir_sequence_view.xml 2012-11-29 11:40:33 +0000 | |||
195 | @@ -20,7 +20,17 @@ | |||
196 | 20 | <field name="suffix"/> | 20 | <field name="suffix"/> |
197 | 21 | <field name="padding"/> | 21 | <field name="padding"/> |
198 | 22 | <field name="number_increment"/> | 22 | <field name="number_increment"/> |
200 | 23 | <field name="number_next"/> | 23 | <!-- If number_next_actual is 0, we have a brand new record --> |
201 | 24 | <field | ||
202 | 25 | name="prohibit_number_next_change" | ||
203 | 26 | attrs="{'invisible':[('number_next_actual','=',0)]}" /> | ||
204 | 27 | <field name="prohibit_number_next_lower" | ||
205 | 28 | attrs="{'invisible':[('number_next_actual','=',0)]}" /> | ||
206 | 29 | <field name="number_next_actual" | ||
207 | 30 | attrs="{'invisible':[('number_next_actual','=',0)]}" /> | ||
208 | 31 | <field | ||
209 | 32 | name="number_next" | ||
210 | 33 | attrs="{'readonly':[('prohibit_number_next_change','=',True),('number_next_actual','>',0)]}" /> | ||
211 | 24 | <field name="implementation"/> | 34 | <field name="implementation"/> |
212 | 25 | </group> | 35 | </group> |
213 | 26 | <group col="3" string="Legend (for prefix, suffix)"> | 36 | <group col="3" string="Legend (for prefix, suffix)"> |
Hi Ronald,
Thanks a lot for the patch and for your patience, sorry it took so long to process it.
Globally I approve your fix, but some parts of it were not suitable for merging in a stable version, and we need a fix in stable versions too. In order to save you the trouble of updating your merge prop and rebasing it on 7.0, Cedric has done it for you in this merge proposal, based on yours: /code.launchpad .net/~openerp- dev/openobject- server/ 7.0-sequence- next-fix- csn/+merge/ 164363
https:/
It's now been merged in 7.0 at revision 4978 rev-id: <email address hidden> (credited to you), with an extra patch at revision 4979.
As a result I'll mark this merge proposal as merged, even if it's not technically true. The essence of the patch is indeed merged in 7.0 and will be automatically forward-ported to trunk soon.
Here is a summary of the changes we did: next_actual' is visible in all views. This required fixing the code to avoid crashing during creation but makes the UI simpler for end-users. This implied a small change to the field label and tooltip too. next_actual" field was visible. The user would only alter or decrease the sequence number if they really meant it. Also the "prohibit_ number_ next_lower" is a nice-to-have that cannot be made reliable with "standard" sequences: PostgreSQL-backed sequences are cross-transactions so you would be subject to a dirty read phenomenon.
- we changed the view so that only the 'number_
- we removed the new boolean flags you added (prohibit_*), both because they could not be added to a stable version and because they were technically not necessary to fix the issue. These flags were also not very useful now that only the "number_
- we changed the default value for `number_next` passed to _alter_sequence() to None, to distinguish "no value" from 0. Otherwise trying to set the value to 0 would be silently ignored. Technically the default PostgreSQL settings forbid starting sequences at 0-, but this is something than can be changed for each sequence, so someone might really want to use it.
Thanks again for this excellent contribution!