The import bank statement process reads the import table ordered by "EftStatementDate". Later in the code it groups the created BankStatements by the field Statement date.
It makes no sense to order for one field and group for another. The Eft* fields are for reference only and can not be edited by the user. So if a user creates the import table manual (and not by an electronic import) he has no chance at all to influence the sort order. My conclusion: The sort order field is just wrong.
This patch solves both issues for me. It works since some weeks and I declare it stable.
Carlos accepted my patch but took it partly back with https://bitbucket.org/idempiere/idempiere/commits/e2d638ea8f18c0cd84ef91e6b3b7886226f539d9
I think in the same sense as with the date that the field used for ordering and creating new BankStatement document should be the one that the user can change. That's why I also changed the ordering form EftStatementReference to ReferenceNo. When creating the import table with the help of a BankStatementLoader both fields are initialized with the same value (see https://bitbucket.org/idempiere/idempiere/src/939122320e2844f7b3bc72975b469d7c6df5df95/org.adempiere.base/src/org/compiere/model/MBankStatementLoader.java?at=development#cl-238 ). But The ReferenceNo can be changed by the user and it can also be entered by the user if he wants to create a statement manual (without a BankStatementLoader).
Thomas, I would agree (will need to review it further) - just that the ORDER BY had some ReferenceNo and the grouping logic had EftStatementReference - both must be in sync - I just reverted, but possibly the change must be move both to ReferenceNo (the order by and the logic).
we have to review the use of ReferenceNo so I reopen this
Result of my review: The ReferenceNo field is copied into the field "ReferenceNo" from the StatementLine - not into the BankStatement. So it has really another use than the EftStatementReference. There is not field to enter a manual StatementReference and in the BankStatement is no field to keep this (only the Eft...-field).
As a conclusion we should let this like it is now. Thanks to Carlos review that saved me from introducing a new bug while solving the first.