A problem was found on cascade activities not doing correctly the rollback:
Test case for GardenWorld (note this test case could not be reproduced if the is fixed, but is possible there are other chained documents that are completed in workflows creating the same issue):
Create a Sales Order:
Type = Prepay Order
Payment Rule = Cash
Create an order line with one Seeder product and complete the order
it must be left in "Waiting Payment" status
Create a Payment that pays the sales order
The user is notified with an error "Validation Error Invoice Paid" and the payment is left in "In Progress" status
but if you check the sales order related documents you can see that the related shipment and invoice documents where created - they must be rolled back
The cause is because every workflow activity is treated independently in a chain of documents, and the class MWFActivity:875 creates a Savepoint for every document.
So, the allocation throws an error, rollback to its own savepoint (reversing JUST the allocation) - and the rest of documents are completed and committed.
A possible patch can be:
But would be better to test carefully the impact on other cases.
About your comment in github:
don't see any changes here, just //throw new AdempiereException(e); ?
You're right, sorry, I added the code and for the tests I just commented it to register the effect.
My fault I committed the commented line.
Anyways I think is good to document it further, the following screenshot illustrates better the issue:
This is based on the test case depicted in the description.
when calling a document that creates and complete a new document the MWFActivity is called in cascade
every MWFActivity sets a savePoint
when one of the deep documents throw an exception (in this case the Allocation in third level here threw an exception) - then the transaction is rolled back just to the first save point - but all the other documents are committed
so, at the end you have a payment with error, and shipment/invoice created as if the payment was correctly applied
This is just one test case, but I think this can happen in any case where we call workflow activities in cascade (something common) and one of the deep documents throw an exception (exception can be thrown also in a ModelValidator because of a specific custom check).
The intention of the patch is simply to throw the exception back to the caller in cascade too.
Change here broke the test case
Trying to complete an invoice with no lines has similar effect - a workflow running (instead of terminated) with the requirement to go and abort the workflow before being able to continue.