Issue 10054

Title
Batch posting invoices
Priority
feature
Status
resolved
Superseder
Allow to skip user warning globaly (issue 7442)
Nosy list
albertca, coogor, dave, edbo, pokoli, reviewbot, roundup-bot, yangoon
Assigned to
ced
Keywords
review

Created on 2021-02-02.16:08:11 by ced, last changed 1 month ago by roundup-bot.

Messages

New changeset ed5412a9906e by Cédric Krier in branch 'default':
Pass period records to super call of close
https://hg.tryton.org/tryton-env/rev/ed5412a9906e
New changeset 7e43bf63a541 by Cédric Krier in branch 'default':
Pass period records to super call of close
https://hg.tryton.org/modules/account_invoice/rev/7e43bf63a541
New changeset 26f94e7319c6 by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/tryton-env/rev/26f94e7319c6
New changeset 199a73cef7ef by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/modules/sale_payment/rev/199a73cef7ef
New changeset e3ffe9b56762 by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/modules/sale/rev/e3ffe9b56762
New changeset 18105ede67d2 by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/modules/purchase/rev/18105ede67d2
New changeset 326ccecb19dc by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/modules/commission_waiting/rev/326ccecb19dc
New changeset 08242d36b1f7 by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/modules/commission/rev/08242d36b1f7
New changeset af0e374fc258 by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/modules/account_invoice_stock/rev/af0e374fc258
New changeset c7c5d446944b by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/modules/account_invoice_history/rev/c7c5d446944b
New changeset 5d6db25a9348 by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/modules/account_invoice/rev/5d6db25a9348
New changeset 814964711e53 by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/modules/account_fr_chorus/rev/814964711e53
New changeset 5b57f4f7d177 by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/modules/account_deposit/rev/5b57f4f7d177
Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2021-02-15.11:40:48

I would rather prefer something how it's done with the sale module. Confirming a sale just adds it to a queue and even I don't have any workers it just works. So no (almost) dead code to maintain.

As far as you do not modify anything from the standard code invoices will be posted interactively (no queue usage).

If you want to use the queue you need to call the new method from the process responsible of processing invoices.

Tryton by default does not have any process to post invoices it's up to customization to post invoices in a bunch. So this customization should take care of adding invoices to queue if required.

As always, if the queue is not enabled the invoices will be posted at the end of the transaction (like it's done for sales)

Author: [hidden] (edbo)
Date: 2021-02-06.12:43:30

I would rather prefer something how it's done with the sale module. Confirming a sale just adds it to a queue and even I don't have any workers it just works. So no (almost) dead code to maintain.

I also understand the problem of the invoice having a number, but no moves. Looking at the review the invoice already gets a number and is then pushed into the queue. When the queue is large it will take a while to get the invoice posted. Also it is possible to retract the invoice and delete it. So the number is discarded which leads to gaps in the numbering (in some countries not allowed?). To prevent this, don't allow to delete an invoice but cancel it.

Because a queue can have multiple workers (concurrent design), it's hard to get given numbers right because you have to have one source of truth. Maybe a special function is needed for this. But when I look into the set_number in invoice.py of the account_invoice module I find the comment

# Do not need to lock the table
# because sequence.get_id is sequential

So it this not making things too complex? and can the number added later when the invoice is gotten from the queue and actually posted?

Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2021-02-04.11:19:07

It's really anoying when somebody claims for design problems but can not provide an example to fix it. If it's so obvious why you do not take the effort to test and provide the example?

For me, if you are unable to provide a working example it's because there is no design issue there and all the discussion has become useless.

And yes, I remember my proposal and I think it's still valid for the users who need (but not for everyone).

Author: [hidden] (yangoon) Tryton translator
Date: 2021-02-04.11:09:43

For me it seems you did not test the feature and you are giving arguments based on a personal interpretation of the text.

What is your interpretation of the text?

Could you please test it and provide and scenario where an invoice is posted but not accounted?

Do you still remember your proposal in msg64230 that users of this feature should just create a separate domain tab for this case? Do you still remember your own input in msg64244:

The only missing part will be to create the move and the invoice report cache which will be done by the workers.
And if this fails it will be done manually by the user.

This issue has turned into an example of dogmatism. It seems to me that you are not really interested in looking at the obvious design problems. I will not spend further time in responding to those continually put up smoke screens.

Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2021-02-04.09:33:07

:

Could you please provide an scenario (with the steps to reproduce) where an invoice is posted and not accounted?
Citing the relevant bits from the feature request (msg64218):

The post button will still be available as long as the invoice has no move. This will allow user to manually force the posting after having a number to find the source of a problem. And a check when closing a period will be added to ensure that all numbered invoices on the period date have a move.

For me it seems you did not test the feature and you are giving arguments based on a personal interpretation of the text.

Could you please test it and provide and scenario where an invoice is posted but not accounted?

Author: [hidden] (yangoon) Tryton translator
Date: 2021-02-04.09:20:15

Could you please provide an scenario (with the steps to reproduce) where an invoice is posted and not accounted?

Citing the relevant bits from the feature request (msg64218):

The post button will still be available as long as the invoice has no move. This will allow user to manually force the posting after having a number to find the source of a problem. And a check when closing a period will be added to ensure that all numbered invoices on the period date have a move.

Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2021-02-03.22:00:28

Could you please provide an scenario (with the steps to reproduce) where an invoice is posted and not accounted?

Author: [hidden] (yangoon) Tryton translator
Date: 2021-02-03.20:17:33

The state of the invoice is posted because it has a number. This is the
design with and without the new implementation.

I hope this is a misunderstanding of the current design. If not then the design is seriously flawed as pointed out by @coogor. It is for sure not legally correct in Germany, and I assume as well at least in the EU. The posting of an invoice is inherently coupled with the posting of the accounting moves in an unmutable way. The according legal requirements can be found in the GOBD

Author: [hidden] (coogor)
Date: 2021-02-03.17:43:44

The state of the invoice is posted because it has a number. This is the design with and without the new implementation.

then the old design has already a problem. An invoice with a number is nice, but as long as it is not issued and posted to accounting it is not more than a piece of paper

Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2021-02-03.14:52:11

This is not the place to argue about how we are organized and how an issue is accepted or what does being (or not) in the nosy list mean. Furthermore, this does not add any significant value to the issue. Just noise which there is some people that do not want to see.

Having said that I will only aswer the parts relevants to the issue:

The new unusual use of states (i.e. withdrawing the tradition that states
describe the state of the object and not the state of the last user action)
is not a design question? What is then a design question?

The state of the invoice is posted because it has a number. This is the design with and without the new implementation.

Nothing else to add from my side.

Author: [hidden] (yangoon) Tryton translator
Date: 2021-02-03.14:40:12

Sergi Almacellas Abellana sergi@koolpi.com added the comment:

There is nothing more to say from my side, feel free to open a discuss
topic to gather more feedback about this feature. I will be happy to
answer the doubts there also.

I am not the proponent of this feature request. Usually proponents of
feature requests are pointed to discuss.tryton.org. Rules should be the
same for all contributors.

The forum is for design and ideas discussion.
The proponent of this feature proposed an implementation (not design) which
also sounds good to me.

The new unusual use of states (i.e. withdrawing the tradition that states
describe the state of the object and not the state of the last user action)
is not a design question? What is then a design question?

This is two core developers that approve this implementation.

Until now there is even not a LGTM on the codereview.

There are two other core developers on the nosy list which
did not exposed any concern, so I understand that they also like the design
and want to follow the implementation of the issue.

Being on nosy is a sign of consent? The original proponent is no more on nosy,
is he still interested in the feature?

If this is not the case I will expect them to read this message an explain their concerns.

An explicit message is a valid input, agreed. But the fact that someone doesn't put in a message, is neither a consent nor a contradiction, it is just: he didn' t put a message.

You are the one that have concerns about the current implementation
You are the one proposing to add another state (which we do not think it is
required and already explained why). You are the one that want feedback for a
wider audience. So its you who should take any action.

Please do not misrepresent the facts. I am still not the proponent.

Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2021-02-03.12:59:53

There is nothing more to say from my side, feel free to open a discuss topic
to gather more feedback about this feature. I will be happy to answer the
doubts there also.

I am not the proponent of this feature request. Usually proponents of feature requests are pointed to discuss.tryton.org. Rules should be the same for all contributors.

The forum is for design and ideas discussion.
The proponent of this feature proposed an implementation (not design) which also sounds good to me.
This is two core developers that approve this implementation.
There are two other core developers on the nosy list which did not exposed any concern, so I understand that they also like the design and want to follow the implementation of the issue. If this is not the case I will expect them to read this message an explain their concerns.

You are the one that have concerns about the current implementation
You are the one proposing to add another state (which we do not think it is required and already explained why).
You are the one that want feedback for a wider audience.
So its you who should take any action.

Feel free to open a discussion (if you want a others to join the talk) or comment here to explain your concerns but please consider what we already talked and that we follow KISS principle, so if there is no need for any extra state nor any extra action we will not add it.

Of course you can always have a customization module to add the custom functionalities you require.

Author: [hidden] (yangoon) Tryton translator
Date: 2021-02-03.12:26:08

Sergi Almacellas Abellana sergi@koolpi.com added the comment:

Please note that batch posting is only here to have a common solution for it
but it's not enabled by default and thats the main reason to not add a new
state.

That doesn't answer the question that the method can leave behind a broken state.

I do not think we should mantaing a module for just adding a domain. The
reasoning is clear: ""The post_batch function is not called by default, so
anyone calling this domain will need to develop it's own solution", this
solution should include the required domain if requestd.

The additional module is not about just adding a domain tab but should include a new correct state and its handling.

There is nothing more to say from my side, feel free to open a discuss topic
to gather more feedback about this feature. I will be happy to answer the
doubts there also.

I am not the proponent of this feature request. Usually proponents of feature requests are pointed to discuss.tryton.org. Rules should be the same for all contributors.

Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2021-02-03.11:34:10

Please note that batch posting is only here to have a common solution for it but it's not enabled by default and thats the main reason to not add a new state.

I do not think we should mantaing a module for just adding a domain. The reasoning is clear: ""The post_batch function is not called by default, so anyone calling this domain will need to develop it's own solution", this solution should include the required domain if requestd.

There is nothing more to say from my side, feel free to open a discuss topic to gather more feedback about this feature. I will be happy to answer the doubts there also.

Author: [hidden] (yangoon) Tryton translator
Date: 2021-02-03.11:18:20

Sergi Almacellas Abellana sergi@koolpi.com added the comment:

This is only the case for when using the batch processing, if you use the
normal post button the behaviour will be exactly the same as the queue won't
be used. (so the invoices will be fully posted)

IIUC we already agreed on the right solution for problem when using batch
posting:

I think we can asume anyone using batch posting will have a custom domain
action to show posted invoices without move.
As this does not make sense for manual posting I do not think it makes sense
to have it by default.

When a method allows per se to leave behind objects that are not really in the state they are declared to be I would consider it broken by design.

I think to not infringe the integrity of states we should consider to:

  • Introduce an additional state like 'processing'
  • Introduce the aforementioned domain tab
  • Put this all in an addon module if it seems to make no sense by default (I am all for it).

Finally I still advocate to put those topics on the public list (discuss.tryton.org) such as is done and required for other features. I think it is important to get feedback from the broader community about their expectations how states should be handled throughout the application.

Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2021-02-03.10:17:23

This is only the case for when using the batch processing, if you use the normal post button the behaviour will be exactly the same as the queue won't be used. (so the invoices will be fully posted)

IIUC we already agreed on the right solution for problem when using batch posting:

I think we can asume anyone using batch posting will have a custom domain action to show posted invoices without move.
As this does not make sense for manual posting I do not think it makes sense to have it by default.

Author: [hidden] (yangoon) Tryton translator
Date: 2021-02-03.10:11:20

Yes, post_batch is decorated with the posted translation which will update
the state to posted and give it a number.

The only missing part will be to create the move and the invoice report cache
which will be done by the workers. And if this fails it will be done manually
by the user.

So we can have invoices in state 'posted' that are not really posted, they are just numbered and marked as posted. Until now we could expect that an invoice was really posted into the accounting when it has state 'posted'. With the current proposal this state turns into something like 'expected to be posted' and 'not sure if really posted'.

AFAIS we use in Tryton the states to reflect the result of an action. This proposal breaks with the tradition and uses states to just reflect that an action was intended. I tend to see that as a major regression.

Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2021-02-03.09:04:28

The batch process numbers and posts them on the same transaction. We only delegate the generation of the moves to
the workers.
Once the invoice is posted it can be only cancelled (if the company allows it) or paid. So it's not possible to draft nor delete it. So we guarantee that there are no gaps on the sequence.
Nothing change with the proposal.

Sure? When post_batch succeeds and then delegates to the workers there can according to my understanding be invoices left, that are numbered but in state draft (if they don't succeed on the workers).

Yes, post_batch is decorated with the posted translation which will update the state to posted and give it a number.

The only missing part will be to create the move and the invoice report cache which will be done by the workers.
And if this fails it will be done manually by the user.

Author: [hidden] (yangoon) Tryton translator
Date: 2021-02-02.19:30:20

The batch process numbers and posts them on the same transaction. We only delegate the generation of the moves to
the workers.
Once the invoice is posted it can be only cancelled (if the company allows it) or paid. So it's not possible to draft nor delete it. So we guarantee that there are no gaps on the sequence.
Nothing change with the proposal.

Sure? When post_batch succeeds and then delegates to the workers there can according to my understanding be invoices left, that are numbered but in state draft (if they don't succeed on the workers).

I think we can asume anyone using batch posting will have a custom domain action to show posted invoices without move.
As this does not make sense for manual posting I do not think it makes sense to have it by default.

Fair enough. I probably misunderstood a little bit the initial proposal. After re-reading it seems that there shall nothing be changed on the current behavior of the post button.

Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2021-02-02.18:40:20
  • When separating numbering and posting into different transactions, it could be that we have already numbered invoices while some of them couldn't be valid. Perhaps they are invalidated later. I would like to hear if it could be a problem for other legislatures to have evtl. gaps in the invoice numbering. Currently the workflow allows to re-edit completely an invoice resp. delete it in state draft. AFAIK it is not allowed for Germany to have gaps in invoice sequences.
    The batch process numbers and posts them on the same transaction. We only delegate the generation of the moves to the workers.
    Once the invoice is posted it can be only cancelled (if the company allows it) or paid. So it's not possible to draft nor delete it. So we guarantee that there are no gaps on the sequence.
    Nothing change with the proposal.
  • A common problem with workers is that the users do not get feedback about failed transactions. Is it correct that a user will have to check manually if all selected invoices were posted (until now he gets the feedback directly when posting the invoice)?

I think we can asume anyone using batch posting will have a custom domain action to show posted invoices without move.
As this does not make sense for manual posting I do not think it makes sense to have it by default.

Author: [hidden] (yangoon) Tryton translator
Date: 2021-02-02.18:27:42

First I think it would be better to do the planing of such a comprehensive feature on discuss.tryton.org.

Some questions on which I would like to have the input of many people:

  • When separating numbering and posting into different transactions, it could be that we have already numbered invoices while some of them couldn't be valid. Perhaps they are invalidated later. I would like to hear if it could be a problem for other legislatures to have evtl. gaps in the invoice numbering. Currently the workflow allows to re-edit completely an invoice resp. delete it in state draft. AFAIK it is not allowed for Germany to have gaps in invoice sequences.
  • A common problem with workers is that the users do not get feedback about failed transactions. Is it correct that a user will have to check manually if all selected invoices were posted (until now he gets the feedback directly when posting the invoice)?
Author: [hidden] (dave) Tryton committer
Date: 2021-02-02.16:21:18

Would it be possible to call it something like post_batch instead?

Bunch is quite informal (unless talking about specific groups of things such as flowers) and so I don't think it is appropriate to use it here.

Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2021-02-02.16:08:11

As explained in the Jérémy Mousset's presentation posting a lot of invoice must be serialized which limits the speed. Also we see that the problem is because of the strict sequence used to number the invoice. So the solution is to separate into different transactions the numbering and the remaining operations.
Also batch posting is not really needed from the User Interface, it happens when automating processed with scheduled job for example.
So I propose to move in a private method the code of post and to add a new workflow method post_bunch which just set the number (and store to today the missing dates like invoice_date and payment_term_date) and after that it calls _post from the __queue__.
The post button will still be available as long as the invoice has no move. This will allow user to manually force the posting after having a number to find the source of a problem. And a check when closing a period will be added to ensure that all numbered invoices on the period date have a move.
The post_bunch will skip the warnings using issue7442.

Side Note:
* I checked if moving at the end of the transaction the usage of sequence strict but it does not solve really the problem because REPEATABLE READ prevents other transactions started after to use the sequence even if the first one has been committed.
* I think the proposal to use a separate transaction using a two-phase commit not really straight forward because both transactions could create a dead lock situation.

History
Date User Action Args
2021-03-17 16:52:24cedlinkissue8462 superseder
2021-02-27 18:39:05roundup-botsetmessages: + msg65006
2021-02-27 18:39:00roundup-botsetmessages: + msg65005
2021-02-27 15:25:13roundup-botsetmessages: + msg64927
2021-02-27 15:25:09roundup-botsetmessages: + msg64926
2021-02-27 15:25:02roundup-botsetmessages: + msg64925
2021-02-27 15:24:51roundup-botsetmessages: + msg64924
2021-02-27 15:24:45roundup-botsetmessages: + msg64923
2021-02-27 15:24:35roundup-botsetmessages: + msg64922
2021-02-27 15:24:27roundup-botsetmessages: + msg64921

Showing 10 items. Show all history (warning: this could be VERY long)