CRM-21637 - Search Criteria for Card Type ID and Card Number not resp…

[yashodha]

…ected in Batch

Overview

Steps to replicate

1. Create a new accounting batch
2. Filter on Card Type ID and Card Number, click search
3. It shows all results irrespective of credit card type and number

Before

After

[yashodha]

@monishdeb Can you please check this?

[pradpnayak]

@yashodha The SS for after looks wrong.

The changes look good for me. @monishdeb has been working on refactoring the code to search financial transaction list to use api so that the result can be altered using hook.

[yashodha]

@pradpnayak What looks wrong, can you please elaborate?

Also it seems if multiple options are selected for filter criteria it is NOT working
eg, Filter on payment instrument = credit card, debit card fails with DB error while it works for single option

@monishdeb it is because where clause is not built properly

[eileenmcnaughton]

test fail looks related (style issue)

[yashodha]

@eileenmcnaughton @monishdeb
all tests pass

[monishdeb]

@yashodha actually like Pradeep said, I am working on an issue about refactoring the code to replace the query with EntityFinancialTrxn api to retrieve batch transactions and here's the PR #10395 which is 70% done and about to complete by this week itself.

Although the current patch fixes the issue as I have tested on my local and ok to merge. But I am afraid the current fix is obsolete as it will be replaced by #10395 .

[pradpnayak]

@yashodha I meant screenshot(SS) you attached initial was of some report. I guess you updated it.

@yashodha I agree some fields on search criteria fields are not respected. I believe @monishdeb will be covering all this in his fix hopefully.

[eileenmcnaughton]

@monishdeb do you want to make a call here - this is pretty trivial so I think it is probably OK to be merged. The lack of tests gives me pause but I think it could squeak through without them & it seems the bigger change got deprioritised / too hard.

[monishdeb]

@eileenmcnaughton I agree, its a trivial but essential fix. Earlier I was hoping to complete the refactoring changes made in #10395 but it will take a long time as I encountered a limitation while changing the search query. I am happy to merge it without UT as it doesn't hit any big change plus the fix is pretty simple and straightforward. Tested again on local and works fine.