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

[Stefan Rijnhart (Opener)]

[Cristian Salamea (ovnicraft)]

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

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

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/[e-mail]_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

[Ronald Portier (Therp) (rportier1962)]

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)]

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

review: Approve

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

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

=== 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 @@
                         <field name="suffix"/>
                         <field name="padding"/>
                         <field name="number_increment"/>
-                        <field name="number_next"/>
+                            <!-- If number_next_actual is 0, we have a brand new record -->
+                            <field
+                                name="prohibit_number_next_change"
+                                attrs="{'invisible':[('number_next_actual','=',0)]}" />
+                            <field name="prohibit_number_next_lower"
+                                attrs="{'invisible':[('number_next_actual','=',0)]}" />
+                            <field name="number_next_actual"
+                                attrs="{'invisible':[('number_next_actual','=',0)]}" /> 
+                            <field 
+                                name="number_next"
+                                attrs="{'readonly':[('prohibit_number_next_change','=',True),('number_next_actual','&gt;',0)]}" />
                         <field name="implementation"/>
                         <separator colspan="4" string="Legend (for prefix, suffix)"/>
                         <group col="8" colspan="4">
 
=== 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 @@
 import logging
 import time
 
+from openerp.tools.translate import _
 import openerp
 
 _logger = logging.getLogger(__name__)
@@ -37,7 +38,7 @@
     _sql_constraints = [
         ('code_unique', 'unique(code)', '`code` must be unique.'),
     ]
-
+    
 def _code_get(self, cr, uid, context=None):
     cr.execute('select code, name from ir_sequence_type')
     return cr.fetchall()
@@ -52,6 +53,32 @@
     """
     _name = 'ir.sequence'
     _order = 'name'
+    
+    def _get_number_next_actual(
+            self, cr, user, ids, field_name, arg, context=None):
+        '''Return number from ir_sequence row when no_gap implementation,
+        and number from postgres sequence when standard implementation.'''
+        res = dict.fromkeys(ids)
+        for seq_id in ids:
+            this_obj = self.browse(cr, user, seq_id, context=context)
+            if  this_obj.implementation != 'standard':
+                res[seq_id] = this_obj.number_next
+            else:
+                # get number from postgres sequence. Cannot use
+                # currval, because that might give an error when
+                # not having used nextval before.
+                statement = (
+                    "SELECT last_value, increment_by, is_called"
+                    " FROM ir_sequence_%03d"
+                    % seq_id)
+                cr.execute(statement)
+                (last_value, increment_by, is_called) = cr.fetchone()
+                if  is_called:
+                    res[seq_id] = last_value + increment_by
+                else:
+                    res[seq_id] = last_value
+        return res
+    
     _columns = {
         'name': openerp.osv.fields.char('Name', size=64, required=True),
         'code': openerp.osv.fields.selection(_code_get, 'Code', size=64),
@@ -64,11 +91,31 @@
         'active': openerp.osv.fields.boolean('Active'),
         'prefix': openerp.osv.fields.char('Prefix', size=64, help="Prefix value of the record for the sequence"),
         'suffix': openerp.osv.fields.char('Suffix', size=64, help="Suffix value of the record for the sequence"),
-        'number_next': openerp.osv.fields.integer('Next Number', required=True, help="Next number of this sequence"),
+        'number_next': openerp.osv.fields.integer(
+            'Next Number', required=True,
+            help='Next number originally set or reset for this sequence'),
+        'number_next_actual': openerp.osv.fields.function(
+            _get_number_next_actual, type='integer', required=True,
+            string='Actual Next Number',
+            help='Next number that will actually be used.'
+            ' Will be zero for new sequence.'),
+        'prohibit_number_next_change': openerp.osv.fields.boolean(
+            'Prohibit change of Next Number',
+            help='Changing next number can wreak havoc on sequences.'
+            ' Checking this option will prevent accidental changes.'),
+        'prohibit_number_next_lower': openerp.osv.fields.boolean(
+            'Prohibit lower value for Next Number',
+            help='When manually setting Next Number to a value lower than the'
+            ' the current value, there is a big risk for duplicate key errors'
+            ' when trying to add new sequences generated to the database.'
+            ' This is especially the case when your prefix or suffix is not'
+            ' changed at the same time, or has no dynamic part (formula) that'
+            ' will also change.'),
         'number_increment': openerp.osv.fields.integer('Increment Number', required=True, help="The next number of the sequence will be incremented by this number"),
         '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."),
         'company_id': openerp.osv.fields.many2one('res.company', 'Company'),
     }
+    
     _defaults = {
         'implementation': 'standard',
         'active': True,
@@ -76,6 +123,8 @@
         'number_increment': 1,
         'number_next': 1,
         'padding' : 0,
+        'prohibit_number_next_change': False,
+        'prohibit_number_next_lower': False,
     }
 
     def init(self, cr):
@@ -117,16 +166,18 @@
         # object depends on it.
         cr.execute("DROP SEQUENCE IF EXISTS %s RESTRICT " % names)
 
-    def _alter_sequence(self, cr, id, number_increment, number_next):
+    def _alter_sequence(self, cr, id, number_increment, number_next=0):
         """ Alter a PostreSQL sequence.
 
         There is no access rights check.
         """
         assert isinstance(id, (int, long))
-        cr.execute("""
-            ALTER SEQUENCE ir_sequence_%03d INCREMENT BY %%s RESTART WITH %%s
-            """ % id, (number_increment, number_next))
-
+        statement = ("ALTER SEQUENCE ir_sequence_%03d INCREMENT BY %d" %
+            (id, number_increment))
+        if  number_next:
+            statement += " RESTART WITH %d" % (number_next, )
+        cr.execute(statement)
+        
     def create(self, cr, uid, values, context=None):
         """ Create a sequence, in implementation == standard a fast gaps-allowed PostgreSQL sequence is used.
         """
@@ -140,10 +191,37 @@
         super(ir_sequence, self).unlink(cr, uid, ids, context)
         self._drop_sequence(cr, ids)
         return True
+    
+    def _pre_validate(self, cr, uid, ids, values, context=None):
+        '''If number_next changed, check wether change allowed.
+If number is lower than actual next number, check wether this is allowed.
+Raise error when invalid change requested.'''
+        if  not 'number_next' in values:
+            return 
+        nn_new = values['number_next']
+        for this_obj in self.browse(cr, uid, ids, context=context):
+            # Only validate when changed.
+            if this_obj.number_next != nn_new:
+                prohibit_change = values.get(
+                    'prohibit_number_next_change',
+                    this_obj.prohibit_number_next_change)
+                if  prohibit_change:
+                    raise openerp.osv.orm.except_orm(
+                        _('ValidationError'),
+                        _('Chance of Next Number has been prohibited'))
+                prohibit_lower = values.get(
+                    'prohibit_number_next_lower',
+                    this_obj.prohibit_number_next_lower)
+                if  prohibit_lower and (this_obj.number_next_actual > nn_new):
+                    raise openerp.osv.orm.except_orm(
+                        _('ValidationError'),
+                        _('Setting Next Number to a value lower than the'
+                        ' actual value has been prohibited'))
 
     def write(self, cr, uid, ids, values, context=None):
         if not isinstance(ids, (list, tuple)):
             ids = [ids]
+        self._pre_validate(cr, uid, ids, values, context=context)
         new_implementation = values.get('implementation')
         rows = self.read(cr, uid, ids, ['implementation', 'number_increment', 'number_next'], context)
         super(ir_sequence, self).write(cr, uid, ids, values, context)
@@ -154,7 +232,15 @@
             n = values.get('number_next', row['number_next'])
             if row['implementation'] == 'standard':
                 if new_implementation in ('standard', None):
-                    self._alter_sequence(cr, row['id'], i, n)
+                    # Implementation has NOT changed.
+                    # Only change sequence if really requested.
+                    if (('number_next' in values)
+                    and (values['number_next'] != row['number_next'])):
+                        self._alter_sequence(cr, row['id'], i, n)
+                    else:
+                        # Just in case only increment changed
+                        if  'number_increment' in values:
+                            self._alter_sequence(cr, row['id'], i)
                 else:
                     self._drop_sequence(cr, row['id'])
             else:
@@ -162,7 +248,6 @@
                     pass
                 else:
                     self._create_sequence(cr, row['id'], i, n)
-
         return True
 
     def _interpolate(self, s, d):