journal invoice_sequence_id must exist for invoices

[Ferdinand]

branch-server 1902
branch-addons official 2469
branch-addons chricar_5.0_price_unit 2507
trunk-extra-addons 3977

pls see FIXME comments below

account/invoice.py
    def action_number(self, cr, uid, ids, *args):
        cr.execute('SELECT id, type, number, move_id, reference ' \
                'FROM account_invoice ' \
                'WHERE id IN ('+','.join(map(str,ids))+')')
        obj_inv = self.browse(cr, uid, ids)[0]
        for (id, invtype, number, move_id, reference) in cr.fetchall():
            if not number:
                if obj_inv.journal_id.invoice_sequence_id:
                    # FIXME - I think iobj_inv.journal_id.invoice_sequence_id must exists for invoices etc - else should never happen
                    sid = obj_inv.journal_id.invoice_sequence_id.id
                    number = self.pool.get('ir.sequence').get_id(cr, uid, sid, 'id=%s', {'fiscalyear_id': obj_inv.period_id.fiscalyear_id.id})
                else:
                    # FIXME - this gives random results im more than ONE resource matches.
                    number = self.pool.get('ir.sequence').get(cr, uid,
                            'account.invoice.' + invtype)

I suggest to raise an error instead

[Ferdinand (office-chricar)]

BTW - we do not understand why we need to iterate cr.fetchall - it can only be the one which is fetched by obj_inv - or do I miss something?
It can always be only one resource

[Ferdinand (office-chricar)]

Additional remarks
Error should only occure if
number = self.pool.get('ir.sequence').get(cr, uid,'account.invoice.' + invtype
returns multiple rows.

[Ferdinand (office-chricar)]

patch
raises error if multiple or no sequences are found

BUT - after correction of the error (definiont of invoice sequence) - the validate button does not work any more = no invoice number is generated once an error is thrown.
(have no time now to investigat this)

[Ferdinand (office-chricar)]

Problem:
before the invoice number generation the account move and move line records are already written
if the invoice number generation fails - this (and may be something else) must be reverted as appartenly this is not transaction done in ONE transaction.

[Ferdinand (office-chricar)]

the problem is here - IMHO it should be considered (mandatory) that all functions of an action are in ONE transaction
account_invoice_workflow.xml

        <record id="act_open" model="workflow.activity">
            <field name="wkf_id" ref="wkf"/>
            <field name="name">open</field>
            <field name="action">action_date_assign()
action_move_create()
action_number()
write({'state':'open'})</field>
            <field name="kind">function</field>
        </record>

[Borja López Soilán (NeoPolus) (borjals)]

Ferdinand, the problem with the account move and move line records being already written, is caused by this bug: https://bugs.launchpad.net/openobject-server/+bug/462285 ("
ir.sequence is commiting to database on get")

That one is a nasty bug that causes the transactions to commit each time a sequence number is get!

On the workflow action, action_date_assign(), action_move_create() and action_number() are being called, but the transaction is being commited several times:
  - action_move_create() calls the post method of the account.move object ("self.pool.get('account.move').post(cr, uid, [move_id])") And guess what?, the post method gets the sequence number for the move ("new_name = self.pool.get('ir.sequence').get_id(cr, uid, journal.sequence_id.id, context=c)"), and thus ('thanks' to the ir.sequence bug) it commits everything done so far! (even worse: it commits for each invoice)
  - action_number() also calls the ir.sequence get_id/get, so it commits again (once for each invoice).

So that means, as long as the #462285 bug is not solved, there is no way to prevent this kind of problems. If you raise any exception on a workflow activity... chaos happens!: the workflow does advance even though the invoice remains in draft state, the account moves might be generated, or might not, or might be generated for some but not all the invoices, the invoices sequence may advance (thus you will have gaps), etc...

[Ferdinand (office-chricar)]

thanks for pointing out

"That one is a nasty bug that causes the transactions to commit each time a sequence number is get!"
this obviously breaks any transaction logic - indeed very nasty

probably the was ir_sequence get_id is programmed could be improved

IMHO the "select for .... update" is not necessary
the "update" statement locks the row - this should be sufficient.

I do not see the need of the commit - except for allowing a very high number of concurrent transaction - the high price for this is "no transaction".
may be we need blocking and not blocking ir_sequences, but for all accounting purpose (most documents created by OpenERP) gapless number series must have highest priority to pass audits.

BTW the provided patch
* will at least avoid wrong invoice numbers
* I could live with with the fact that moves are created - in case of failure these must be canceled - which is some lines of code and should not happen often.

I realized that the workflow advances - but I could not figure out where this is info stored.
If the patch raises an exception further validations after correcting the former error do nothing, although the account_invoice does not show any changes. (or at least I didn't see them) . Would be great if you can point this out

[Jan Verlaan (jan-verlaan)]

Hi Ferdinand,

why solving this bug with a patch that camouflage the original bug without solving it? (Removing transactions done)
The original bug https://bugs.launchpad.net/openobject-server/+bug/462285 is creating more problems, that's the one that should be solved.
All financial transactions should be transactional. If the last one belonging to a transactions fails, all previous shouldn't be committed.

E.g. in statements sessions where you can create bank statements you will see the same problem now. When half way a bank statement a transaction errors, the half of the statement is done while the other half hasn't be processed. A normal user doesn't now how to proceed further from here to get the rest of the lines processed.

Regarding the invoice sequence without gaps, as far my understanding this is not needed anymore while using automated systems.
What could be needed is that a upwards sequence should be there, but you may have a gap in it. For instance other ERP's allow sequences assigned per 10 to a specific user that will be consumed first before getting the next 10 numbers. This is used in high transactional environments to avoid the sequence-table locking. Here gaps a created automatically.

Resume: bug solving should be done a the place of the bug, not by solving the habit of the dis-functionality.

Just my 2 ct.

[Ferdinand (office-chricar)]

Jan sorry you didn't get me.

the patch solves the problem if no invoice sequence is entered for the journal and more than ONE sequence is defined for the invoice type. this will return a rendom sequence which is used
hence no
 obj_inv.journal_id.invoice_sequence_id
is returned and
 number = self.pool.get('ir.sequence').get(cr, uid,
                            'account.invoice.' + invtype)
returns random data

Example
* Main Invoice
* Invoice 2009
* Invoice 2010
all have code "account.invoice.in_invoice"

it does not try to solve the transaction problem (which I wasn't aware of) and will be neccessary also if the transaction problem is solved.

[Jan Verlaan (jan-verlaan)]

Excuse moi, did read to fast I guess. Now I get you :-)

[Borja López Soilán (NeoPolus) (borjals)]

Ferdinand: "IMHO the "select for .... update" is not necessary
the "update" statement locks the row - this should be sufficient."

The "SELECT ... FOR UPDATE" its necessary, as OpenERP uses "read commited" as the default transaction level.

Without "FOR UPDATE" this may happen:

        Transaction 1 | Transaction 2
reads sequence number (1000) |
... | reads sequence number (1000)
writes number+1 (1001) |
... | writes number+1 (1001)
commits |
                                                           | commits

Final sequence number = 1001

With "FOR UPDATE" you get the expected behaviour:

        Transaction 1 | Transaction 2
reads sequence number (1000) |
... | tries to read sequence number, gets locked (waiting for the first transaction)
writes number+1 (1001) |
... |
commits |
                                                           | the read of the sequence number ends (1001)
                                                           | writes number+1 (1002)
                                                           | commits

Final sequence number = 1002

[Ferdinand (office-chricar)]

I forgot to mention
for my proposition one has to store the last number
update set last_number= last_number+1 ....
locks the table

[JMA(Open ERP) (jma-openerp)]

Hello Ferdinand,

I would request you to check with the latest trunk changes that have taken place.

Now things are handled in a different manner and they no more loop.

Have checked it with no sequences and more than one sequence and it works.

Would you please review this?

Thank you.

[Jay Vora (Serpent Consulting Services) (jayvora)]

Hello,

Corrections have taken place in trunk by better approach.
Thanks.