[ADD] account_sequence: Reintroduce `ir.sequence` in account_journal to fix concurrency issues #104345

clbr-odoo · 2022-10-27T14:29:38Z

This module allows to bind sequence_id to selected journals. Account_move names will then be computed based on ir.sequence and no longer on sequence.mixin. When sequence is set to no_gap implementation, the behavior will be the same as before however standard implementation of sequences will allow to solve concurrency issues that happened in some cases with mixins.

Task: 3006843

…to fix concurrency issues This module allows to bind `sequence_id` to selected journals. Account_move names will then be computed based on `ir.sequence` and no longer on `sequence.mixin`. This is necessary in somes corner cases where `sequence.mixin` creates concurrency issues. Task: 3006843

robodoo · 2022-10-27T14:29:45Z

clbr-odoo · 2022-10-27T14:33:35Z

@william-andre @qdp-odoo

william-andre

Looking good

william-andre · 2022-10-27T14:48:31Z

addons/account_sequence/models/account_move.py

+        moves_with_ir_sequence = self.filtered('journal_id.sequence_id')
+        for move in moves_with_ir_sequence:
+            seq = move._get_journal_sequence()
+            rec = self.env['account.move'].search([('journal_id.sequence_id', '=', seq.id)], order='name desc', limit=1)


Ordering with name desc will not work depending on the padding

In [1]: "0001" < "0002" # expect True Out[1]: True In [2]: "10" < "2" # expect False Out[2]: True

william-andre · 2022-10-27T14:49:40Z

addons/account_sequence/models/account_move.py

+    @api.depends('state', 'journal_id', 'date')
+    def _compute_name(self):
+        # OVERRIDE
+        moves_with_ir_sequence = self.filtered('journal_id.sequence_id')
+        for move in moves_with_ir_sequence:
+            if not move.posted_before and not move.state == 'posted':
+                seq = move._get_journal_sequence()
+                move.name = seq.get_next_char(seq.number_next_actual)
+        super(AccountMove, self - moves_with_ir_sequence)._compute_name()


Should we compute the name when it is not posted?
I'm not sure how it can work with ir.sequence because you can't know when you have to consume a new number

I tried to keep current behavior as much as possible and keep the feature.
That's why I'm never consuming number in _compute_name(), only when _post()ing for the first time.

william-andre · 2022-10-27T14:52:09Z

addons/account_sequence/models/account_move.py

+            if not move.posted_before and (move.name == '/' or move.name == next_name):
+                # We only compute new name if user didn't manually change the name
+                move.name = seq.with_context(ir_sequence_date=move.date).next_by_id()


Shouldn't we also force sequence_number and sequence_prefix from here, so that the gap detection mechanism works too?

If i'm correct, sequence_number and sequence_prefix are computed from name so holes detection does work.
See here

But it doesn't work for crazy sequences that don't match the prefix, which can be the reason why someone uses ir.sequence

william-andre · 2022-10-27T14:54:46Z

addons/account_sequence/tests/test_ir_sequence_concurrency.py

+    def test_sequence_concurrency_payments(self):
+        """Posting concurrent payments should not raise errors
+        with sequence's standard implementation"""
+        env0, env1, env2 = self.data['envs']
+
+        # start the transactions here on cr1 to simulate concurrency with cr2
+        env1.cr.execute('SELECT 1')
+
+        # Post new payment in cr2
+        payment1 = env2['account.payment'].browse(self.data['payment_ids'][1])
+        payment1.action_post()
+        env2.cr.commit()
+
+        # Post another payment in cr1
+        payment2 = env1['account.payment'].browse(self.data['payment_ids'][2])
+        payment2.action_post()
+        env1.cr.commit()
+
+        # Check values
+        payments = env0['account.payment'].browse(self.data['payment_ids'])
+        # Everything should be posted
+        self.assertNotEqual(payments.mapped('name'), ['/', '/', '/'])


I think that the point of the concurrency with the payments was to

post an invoice in journal I in transaction T1

post a payment in journal P in transaction T2

post a payment in journal P in transaction T1

post an invoice in journal I in transaction T2

This would lead to a deadlock, leaving the connection hanging if not detected by PostgreSQL.
At least it is the use case I remember from the original feedback.

Ok, I will test that too.

william-andre · 2022-10-27T15:00:25Z

addons/account_sequence/tests/test_ir_sequence_concurrency.py

+from odoo.addons.account.tests.test_sequence_mixin import TestSequenceMixinConcurrency
+
+
+@tagged('post_install', '-at_install', 'test_ir_sequence_concurrency')
+class TestIrSequenceConcurrency(TestSequenceMixinConcurrency):


Importing and inheriting this class will lead to running the tests 3 times

first time when running the tests of account, where the class is defined

second time when importing it in this file, since the test suite will detect the name TestSequenceMixinConcurrency in a test file and run it

third time when running all the tests of TestIrSequenceConcurrency since it inherited the tests of the parent class

In order to avoid that, split the setup and the tests of TestSequenceMixinConcurrency in 2 different classes and inherit only the setup from here

william-andre · 2022-10-27T15:01:31Z

addons/account_sequence/models/account_move.py

+            seq = move._get_journal_sequence()
+            next_name = seq.get_next_char(seq.number_next_actual)
+            if not move.posted_before and (move.name == '/' or move.name == next_name):
+                # We only compute new name if user didn't manually change the name


Not sure I understand how one can edit manually the number when using ir.sequence
It should even be readonly in the view I think

Basically when the user edited the name it's kept like that on posting (same behavior as mixins), the ir_sequence won't be touched (so yes you can manually create holes, but it will warn you, just like with current mixin).

But then it will give you the same number twice?

william-andre · 2022-10-27T15:07:08Z

addons/account_sequence/models/account_journal.py

+    sequence_id = fields.Many2one('ir.sequence', string='Entry Sequence',
+                                  help='This field contains the information related to the numbering of the journal entries of this journal.',
+                                  copy=False, check_company=True)
+    refund_sequence_id = fields.Many2one('ir.sequence', string='Credit Note Entry Sequence',
+                                         help='This field contains the information related to the numbering of the credit note entries of this journal.',
+                                         copy=False, check_company=True)


This field contains...

A bit too technical for a tooltip

Regarding the spacing, just a personal preference:

Suggested change

sequence_id = fields.Many2one('ir.sequence', string='Entry Sequence',

help='This field contains the information related to the numbering of the journal entries of this journal.',

copy=False, check_company=True)

refund_sequence_id = fields.Many2one('ir.sequence', string='Credit Note Entry Sequence',

help='This field contains the information related to the numbering of the credit note entries of this journal.',

copy=False, check_company=True)

sequence_id = fields.Many2one(

comodel_name='ir.sequence',

string='Entry Sequence',

copy=False,

check_company=True,

help="This field contains the information related to the numbering of the journal entries of this journal.",

)

refund_sequence_id = fields.Many2one(

comodel_name='ir.sequence',

string='Credit Note Entry Sequence',

copy=False,

check_company=True,

help="This field contains the information related to the numbering of the credit note entries of this journal.",

)

william-andre · 2022-10-27T15:08:22Z

addons/account_sequence/__init__.py

@@ -0,0 +1,3 @@
+# -*- coding: utf-8 -*-


This is always the case since python3, it is generally preferred not to add it

william-andre · 2022-10-27T15:09:19Z

addons/account_sequence/models/account_move.py

+    @api.depends('journal_id', 'date')
+    def _compute_highest_name(self):
+        # OVERRIDE
+        moves_with_ir_sequence = self.filtered('journal_id.sequence_id')
+        for move in moves_with_ir_sequence:
+            seq = move._get_journal_sequence()
+            rec = self.env['account.move'].search([('journal_id.sequence_id', '=', seq.id)], order='name desc', limit=1)
+            move.highest_name = rec.name or False
+        super(AccountMove, self - moves_with_ir_sequence)._compute_highest_name()


Why do you need the highest_name?

To not break that warning.

william-andre · 2022-10-27T15:09:52Z

addons/account_sequence/models/account_move.py

+    name = fields.Char(compute='_compute_name')
+    highest_name = fields.Char(compute='_compute_highest_name')


This is useless as it is not changing anything from the original declaration

celm1990 · 2022-10-27T15:49:14Z

Only curiosity, What happen with latam modules(l10n_*) where use 1 only journal but many latam.documents, this module is not compatible?

https://github.com/odoo/odoo/blob/16.0/addons/l10n_latam_invoice_document/models/account_move.py#L127

moylop260 · 2022-10-27T16:31:23Z

Related to #91873

unittests related:

13.0 - [REF] account: Adding unittests for concurrency issues in account_move sequences #91614 🟢
14.0 - [REF] account: Adding unittests for concurrency issues in account_move sequences #91525 🔴

fpodoo · 2022-10-27T21:43:32Z

That seems over-engineered to me. Two mechanisms just set a sequence. People have to choose between:

one that creates concurrency issues, or
one with all the issues we had before the mixin: if you import PG sequence is not updated, if you resequence/rename invoices you might not create new invoices anymore (name conflict), holes in sequences...

As people won't understand both approaches, they will still have concurrency without understanding what to do.

Instead I proposed an alternative to fix the root cause of the issue (concurrency due to lock on preceding invoice), based on unique constraints properties:
#55930 (comment)

Why did we dismiss this approach? (It's not an issue for latam, just do the constraint on out invoice for latam)

gdgellatly · 2022-10-28T01:01:49Z

@fpodoo The current implementation causes more issues than just concurrency.

Take for example, change the type of a document, say Credit Note to Invoice and like magic all your Invoices now take credit note sequences. Don't notice for an hour, or a day or a week, you can't fix new ones because at least in nearly every tax jurisdiction I've ever known, invoices must have a unique identifier that does not change once they are in the hands of an external party (primarily to prevent VAT fraud). Same for changing journals. At scale, with lots of users this whole move sequencing thing is a bit of a mess tbh.

fpodoo · 2022-10-28T06:31:07Z

change the type of a document, say Credit Note to Invoice

What do you mean? That's not even a real use case as Odoo does not allow to change the type of a document through the interface. (e.g. you probably messed up with SQL directly, but whatever the implementation, if you break consistency of data through SQL, you'll have issues)

ir.sequence have real issues that you can break from UI:

resequence wizard will fail
cancel and delete last invoice before sending to customer --> hole in numbers
too much concurrency --> hole in invoice numbers
import old invoices --> creation will fail as numbers already exists (pg sequence not sync with imported numbers)
edit invoices number --> no more match with postgresql sequence

compared to that, the drawbacks of the mixin are very limited (the only issue I see is when you change prefix / numbering which is a weird use case)

gdgellatly · 2022-10-28T09:50:48Z

change the type of a document, say Credit Note to Invoice

What do you mean? That's not even a real use case as Odoo does not allow to change the type of a document through the interface. (e.g. you probably messed up with SQL directly, but whatever the implementation, if you break consistency of data through SQL, you'll have issues)

Lol, such a basic function I assume it was part of core. Absolutely necessary for emailed in invoices going direct to journal.

Lol again, I checked, it is part of odoo account module

<record model="ir.actions.server" id="action_move_switch_invoice_to_credit_note">
            <field name="name">Switch into refund/credit note</field>
            <field name="model_id" ref="account.model_account_move"/>
            <field name="groups_id" eval="[(4, ref('account.group_account_invoice'))]"/>
            <field name="binding_model_id" ref="account.model_account_move" />
            <field name="state">code</field>
            <field name="binding_view_types">form</field>
            <field name="code">
if records:
    action = records.action_switch_invoice_into_refund_credit_note()
            </field>
        </record>

But in any case, the other one we face which sucks even harder is users accidentally overtyping the invoice number with vendor invoice number. But actually that doesn't make such a mess as you never send supplier invoices.

ir.sequence have real issues that you can break from UI:

resequence wizard will fail
non issue as no need to resequence.

cancel and delete last invoice before sending to customer --> hole in numbers
non issue in many jurisdictions incl NZ, Oz, UK

too much concurrency --> hole in invoice numbers
non issue in many jurisdictions incl NZ, Oz, UK

import old invoices --> creation will fail as numbers already exists (pg sequence not sync with imported numbers)
at a guess I've imported 100k invoices over the years - never realized this was an issue.

edit invoices number --> no more match with postgresql sequence
again non issue, once issued, the invoice number is the number.

We always use Standard instead of no gap sequences. Weirdly we find we get far fewer gaps using standard.

compared to that, the drawbacks of the mixin are very limited (the only issue I see is when you change prefix / numbering which is a weird use case)

william-andre · 2022-10-31T10:53:19Z

Replaced by completely different approach

[FIX] account: unique constraint in sequence mixin #104606

C3POdoo requested review from a team and Levizar and removed request for a team October 27, 2022 14:30

C3POdoo added the RD research & development, internal work label Oct 27, 2022

william-andre reviewed Oct 27, 2022

View reviewed changes

moylop260 mentioned this pull request Oct 27, 2022

[BUG] account: Concurrency errors increased considerably in account.move for Odoo>=v14.0 #91873

Closed

clbr-odoo closed this Oct 31, 2022

rafaelbn mentioned this pull request Nov 7, 2022

La inconsistencia de las secuencias de facturas a partir de v14 (¿es o será legal en España?) OCA/l10n-spain#2246

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ADD] account_sequence: Reintroduce `ir.sequence` in account_journal to fix concurrency issues #104345

[ADD] account_sequence: Reintroduce `ir.sequence` in account_journal to fix concurrency issues #104345

clbr-odoo commented Oct 27, 2022

robodoo commented Oct 27, 2022 •

edited

Loading

clbr-odoo commented Oct 27, 2022

william-andre left a comment

william-andre Oct 27, 2022

william-andre Oct 27, 2022

clbr-odoo Oct 27, 2022 •

edited

Loading

william-andre Oct 27, 2022

clbr-odoo Oct 27, 2022

william-andre Oct 28, 2022

william-andre Oct 27, 2022

clbr-odoo Oct 27, 2022

william-andre Oct 27, 2022

william-andre Oct 27, 2022

clbr-odoo Oct 27, 2022

william-andre Oct 28, 2022

william-andre Oct 27, 2022

william-andre Oct 27, 2022

william-andre Oct 27, 2022

clbr-odoo Oct 27, 2022

william-andre Oct 27, 2022

celm1990 commented Oct 27, 2022

moylop260 commented Oct 27, 2022

fpodoo commented Oct 27, 2022 •

edited

Loading

gdgellatly commented Oct 28, 2022

fpodoo commented Oct 28, 2022 •

edited

Loading

gdgellatly commented Oct 28, 2022

william-andre commented Oct 31, 2022

		name = fields.Char(compute='_compute_name')
		highest_name = fields.Char(compute='_compute_highest_name')

[ADD] account_sequence: Reintroduce ir.sequence in account_journal to fix concurrency issues #104345

[ADD] account_sequence: Reintroduce ir.sequence in account_journal to fix concurrency issues #104345

Conversation

clbr-odoo commented Oct 27, 2022

robodoo commented Oct 27, 2022 • edited Loading

clbr-odoo commented Oct 27, 2022

william-andre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clbr-odoo Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

celm1990 commented Oct 27, 2022

moylop260 commented Oct 27, 2022

fpodoo commented Oct 27, 2022 • edited Loading

gdgellatly commented Oct 28, 2022

fpodoo commented Oct 28, 2022 • edited Loading

gdgellatly commented Oct 28, 2022

william-andre commented Oct 31, 2022

[ADD] account_sequence: Reintroduce `ir.sequence` in account_journal to fix concurrency issues #104345

[ADD] account_sequence: Reintroduce `ir.sequence` in account_journal to fix concurrency issues #104345

robodoo commented Oct 27, 2022 •

edited

Loading

clbr-odoo Oct 27, 2022 •

edited

Loading

fpodoo commented Oct 27, 2022 •

edited

Loading

fpodoo commented Oct 28, 2022 •

edited

Loading