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.

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

dev/core#2642 Add filter by custom fields in Accounting Batch #20556

Merged
merged 1 commit into from
Jul 22, 2021

Conversation

muniodiego
Copy link
Contributor

@muniodiego muniodiego commented Jun 8, 2021

Overview

When filtering transactions to be included in a batch, custom field filters do not work.

Comments

Issue: https://lab.civicrm.org/dev/core/-/issues/2642

@civibot
Copy link

civibot bot commented Jun 8, 2021

(Standard links)

@civibot civibot bot added the master label Jun 8, 2021
@seamuslee001
Copy link
Contributor

Test fail relates here

CRM_Financial_Page_AjaxTest::testGetFinancialTransactionsList
Invalid argument supplied for foreach()

/home/jenkins/bknix-dfl/build/core-20556-3fg6v/web/sites/all/modules/civicrm/CRM/Batch/BAO/Batch.php:708

@muniodiego
Copy link
Contributor Author

@seamuslee001 , Bug fixed.

@demeritcowboy
Copy link
Contributor

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] PASS
      • Did a light run.
  • Developer standards
    • [r-tech] PASS
    • [r-code] Just needs a squash
      • Can you squash commits?
      • Just making a note that at line 776 the existing code calls CRM_Contact_BAO_Query and the second parameter can include custom fields by just setting the defaultReturnProperties() second parameter to TRUE, however the rest of this batch function doesn't really use the query except for some minor where clause manipulation, so changing it to TRUE doesn't help much and you still need to build the query.
    • [r-maint] ?
      • A test would be nice. It's a public static function so in theory shouldn't depend on too much setup.
    • [r-test] PASS

@demeritcowboy
Copy link
Contributor

@muniodiego Are you able to squash commits and perhaps add a unit test?

@demeritcowboy
Copy link
Contributor

Thanks for updating. I'm going to merge.

@demeritcowboy demeritcowboy merged commit 5327256 into civicrm:master Jul 22, 2021
omarabuhussein added a commit to compucorp/civicrm-core that referenced this pull request Dec 9, 2022
omarabuhussein added a commit to compucorp/civicrm-core that referenced this pull request Dec 12, 2022
@mlutfy mlutfy changed the title Add filter by custom fields in Accounting Batch. https://lab.civicrm.… dev/core#2642 Add filter by custom fields in Accounting Batch Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants