Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IDEMPIERE-6120: Bugged SQL in DunningRunCreate #2329

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

IDEMPIERE-6120: Bugged SQL in DunningRunCreate #2329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions org.adempiere.base.process/src/org/compiere/process/DunningRunCreate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,12 +181,13 @@ else if (p_C_BP_Group_ID != 0)
if (previousLevels!=null && previousLevels.length>0) {
StringBuilder sqlAppend = new StringBuilder();
for (MDunningLevel element : previousLevels) {
sqlAppend.append(" AND i.C_Invoice_ID IN (SELECT C_Invoice_ID FROM C_DunningRunLine WHERE ");
sqlAppend.append("C_DunningRunEntry_ID IN (SELECT C_DunningRunEntry_ID FROM C_DunningRunEntry WHERE ");
sqlAppend.append("C_DunningRun_ID IN (SELECT C_DunningRun_ID FROM C_DunningRunEntry WHERE ");
sqlAppend.append("C_DunningLevel_ID=");
sqlAppend.append(" AND i.C_Invoice_ID IN (SELECT C_Invoice_ID FROM C_DunningRunLine cd"
+ " JOIN C_DunningRunEntry cd2 ON cd.C_DunningRunEntry_ID = cd2.C_DunningRunEntry_ID"
+ " JOIN C_DunningRun cd3 ON cd2.C_DunningRun_ID = cd3.C_DunningRun_ID"
+ " WHERE cd.Processed <> 'N'"
+ " AND cd2.C_DunningLevel_ID = ");
sqlAppend.append(element.get_ID ());
sqlAppend.append(")) AND Processed<>'N')");
sqlAppend.append(")");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First you need to create a dunning with a least three levels and 'create levels sequentially'. Then create an invoice some days in the past and create dunning runs so that this invoice is dunned at the last level. Now create another invoice some days in the past and let it run in the first dunning. Now try to let it run in the second level. This will lead to run in the last level instead, because there is one invoice dunned in the last level. Because this invoice has no connection to the new invoice that behaviour is wrong. The new invoice should be dunned with the second level, not the last.

}
sql.append(sqlAppend.toString());
}
Expand Down
2 changes: 1 addition & 1 deletion org.adempiere.base/src/org/compiere/model/MDunningLevel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public MDunningLevel[] getPreviousLevels()
if (!getParent().isCreateLevelsSequentially ())
return null;
ArrayList<MDunningLevel> list = new ArrayList<MDunningLevel>();
String sql = "SELECT * FROM C_DunningLevel WHERE C_Dunning_ID=? AND DaysAfterDue+DaysBetweenDunning<?";
String sql = "SELECT * FROM C_DunningLevel WHERE C_Dunning_ID=? AND DaysAfterDue+DaysBetweenDunning<? AND IsActive='Y'";
PreparedStatement pstmt = null;
ResultSet rs = null;
try
Expand Down
Loading