IDEMPIERE-6120: Bugged SQL in DunningRunCreate

[lHeidbreder]

https://idempiere.atlassian.net/browse/IDEMPIERE-6120

Pull Request Checklist

• My code follows the code guidelines of this project
• My code follows the best practices of this project
• I have performed a self-review of my own code
• My code is easy to understand and review.
• I have checked my code and corrected any misspellings
• [-] In hard-to-understand areas, I have commented my code.
• My changes generate no new warnings

Tests

• I have tested the direct scenario that my code is solving
• I checked all collaterals that can be affected by my changes, and tested other potential affected scenarios
• New and existing unit tests pass locally with my changes
• I have added unit tests that prove my fix is effective or that my feature works

Documentation

• I have made corresponding changes to the documentation as follows:
• • New feature (non-breaking change which adds functionality): I have created the New Feature page in the project wiki explaining the functionality and how to use it. If relevant, I have committed sample data to the core seed to have usable examples in GardenWorld.
• • Breaking change (fix or feature that would cause existing functionality to not work as expected): I have documented the change in a clear way that everyone in the community can understand the impact of the change.
• • Improvement (improves and existing functionality): This documentation is needed if the improvement changes the way the user interacts with the system or the outcome of a process/task changes. If it is just, for instance, a performance improvement, documentation might not be needed.
• The changed/added documentation is in the project wiki (not privately-hosted pdf files or links pointing to a company website) and is complete and self-explanatory.

[CarlosRuiz-globalqss]

why removing here the condition C_DunningRunLine.Processed <> 'N' ?

On the other hand, checking the compiere code, there is another fix on the line 199, it must be changed to:
+ "WHERE drl.Processed='Y' AND dr2.C_DunningRun_ID=? AND drl.C_Invoice_ID=?"; // ##1 ##2

[lHeidbreder]

You will find Processed<>'N' two lines earlier in WHERE cd.Processed <> 'N', so I didn't remove it. cd refers to DunningRunLine, simply because it's the query's lynchpin and it was first.
If you'd like, I'd gladly give them more expressive names. That does sound like a smart idea.

The other change I'll also gladly add while I'm at it.

[lHeidbreder]

Also while renaming I just noticed that joining C_DunningRun seems to be unnecessary currently - however maybe it could use safeguards like checks for C_DunningRun.AD_Client_ID. What are your thoughts on that?

[CarlosRuiz-globalqss]

I think your conversion is wrong, reviewing the equivalent SQL must look like this:

AND i.C_Invoice_ID IN (SELECT drl.C_Invoice_ID 
  FROM C_DunningRunLine drl
	JOIN C_DunningRunEntry dre ON (dre.C_DunningRunEntry_ID=drl.C_DunningRunEntry_ID)
	JOIN C_DunningRunEntry dre2 ON (dre.C_DunningRun_ID=dre2.C_DunningRun_ID)
  WHERE dre2.C_DunningLevel_ID = ${c_dunninglevel_id} AND drl.Processed <> 'N'
)

There is no C_DunningRun on the initial query, but there is a second C_DunningRunEntry

[lHeidbreder]

I agree that I'm not converting the original completely, as I'm trying to fix a bug in the original that stems from using C_DunningRunEntry twice.

As described here: https://idempiere.atlassian.net/browse/IDEMPIERE-6120
there is a bug in the old query when you create a dunning run using all levels of a sequential dunning strategy. So unless I'm mistaken in calling this behavior a bug, I don't think my new query is actually wrong.

Obviously I don't know of all use cases but I don't see any where this would be correct

[CarlosRuiz-globalqss]

I think the double C_DunningRunEntry has a reason, but I don't have data to test it, and Dunning has different modes of setting this up.

So, unless there is a failing case that can be tested, and that doesn't affect the working cases, I would suggest that we keep the query as is (or at least converting it in a way that is compatible).

Would be worthy to set up unit tests with the dunning cases, sequential and non-sequential.

[lHeidbreder]

I may be able to extract some test data but I can't write unit tests, as the test fragment does not compile for me.

I am certain though that the current query does not work as intended.

[CarlosRuiz-globalqss]

if you can provide a 2pack with the configuration - or explain in the ticket steps to configure in GardenWorld

[mschnbeck]

As the part in question is used only when doing sequential dunning, it's sure that it won't affect other dunning methods. Sequential dunning with more than two level was wrong with the current implementation because as soon as there was one invoice with level three or four all invoices which where dunned once then would immediately dunned with level three or four. The decision which level has to be used is not done according to the invoice in question but looking at all invoices ever dunned.

[CarlosRuiz-globalqss]

please describe the failing test case so we can follow the need of the fix