Batch posting invoices
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 #7442 (closed).
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.
Activity
assigned to @ced
added accounting type::performance + 1 deleted label
review333811002 updated at https://codereview.tryton.org/333811002/#ps353271002
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)?
@yangoon please do not remove all information from the header. This is not respectful of the work of others!
- 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.
@yangoon please do not remove all information from the header. This is not respectful of the work of others!
Could you please explain expand? The title of the issue is completely intact and I answered from the web interface (answering per mail used to not work for me).
@yangoon incredible you continue to modify the header.
review333811002 updated at https://codereview.tryton.org/333811002/#ps359091002
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.
@yangoon incredible you continue to modify the header.
I see. This is due to roundup re-setting to inital values when someone other changed meanwhile. No need to shout.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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
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.
:
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?
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.
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).
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_numberininvoice.pyof the account_invoice module I find the comment# Do not need to lock the table # because sequence.get_id is sequentialSo it this not making things too complex? and can the number added later when the invoice is gotten from the queue and actually posted?
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)
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/5b57f4f7d177New 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/814964711e53New changeset 5d6db25a9348 by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/modules/account_invoice/rev/5d6db25a9348New 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/c7c5d446944bNew 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/af0e374fc258New changeset 08242d36b1f7 by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/modules/commission/rev/08242d36b1f7New changeset 326ccecb19dc by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/modules/commission_waiting/rev/326ccecb19dcNew changeset 18105ede67d2 by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/modules/purchase/rev/18105ede67d2New changeset e3ffe9b56762 by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/modules/sale/rev/e3ffe9b56762New changeset 199a73cef7ef by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/modules/sale_payment/rev/199a73cef7efNew changeset 26f94e7319c6 by Cédric Krier in branch 'default':
Add method to post invoices by batch
https://hg.tryton.org/tryton-env/rev/26f94e7319c6
