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#249 Fix crash when performing export all contacts from search #12447

Merged
merged 1 commit into from
Jul 10, 2018

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Jul 9, 2018

Overview

Regression since #12341, #12349, related to #12320.

Error: undefined function getContactIds()

To reproduce:

  1. Do an empty search
  2. Select all contacts
  3. Choose "Export" from the task list
  4. Export the default fields

Before

Crash.

After

Export works again.

@civibot
Copy link

civibot bot commented Jul 9, 2018

(Standard links)

@mattwire mattwire force-pushed the allcontactexport_regression branch from 9409b8e to b7250ea Compare July 9, 2018 22:43
@totten
Copy link
Member

totten commented Jul 9, 2018

A fix for this should target 5.4 (RC) because the regression appears in the RC.

When testing patches here, we need to check both:

  1. Export contacts (Ex: Search for "Jerome". Select all contacts. In the task-list, choose to export.)
  2. Batch update (Ex: Search for "Jerome". Select all contacts. In the task-list, choose to update multiple.)

With this patch, it fixes 1 but breaks 2. :(

@mattwire mattwire force-pushed the allcontactexport_regression branch from b7250ea to 094208c Compare July 9, 2018 23:09
@totten
Copy link
Member

totten commented Jul 9, 2018

@mattwire posted a revision here along with some explanatory comments on Mattermost.

I think part of the issue here is the introduction of the Core_Form_Task classes has required some changes to class inheritance etc. And in particular, getContactIds() is no longer part of the class being called.

This does smell like a better theory of the problem (although I still don't claim to fully understand what we're juggling around). In any event, the following use-cases are passing r-run with the lastest 094208c:

  • Search for "Jerome".
    • Select all. Export. ==> Observe same 3 exported 👍
    • Select all. Update multiple. ==> Observe same 3 available for update 👍 (Also, make some changes and observe they work. 👍)
  • Search for all contacts
    • Select all. Export. => Observe all 203 exported. 👍
    • Select four arbitrary contacts. Export. => Observe all 4 exported. 👍
  • Search for all contributions.
    • Select all. Export. => Observe all 93 exported. 👍

@mattwire could you update to target 5.4?

@seamuslee001 seamuslee001 added the merge ready PR will be merged after a few days if there are no objections label Jul 9, 2018
@seamuslee001
Copy link
Contributor

Given @totten's comments i have added the merge ready label, noting that the target of the PR needs to change to be 5.4, ping also @lcdservices Brian you may want to try applying this as well and testing it.

totten pushed a commit to totten/civicrm-core that referenced this pull request Jul 10, 2018
@seamuslee001
Copy link
Contributor

Jenkins re test this please

@mattwire mattwire force-pushed the allcontactexport_regression branch from 094208c to fe9b66c Compare July 10, 2018 11:23
@mattwire mattwire changed the base branch from master to 5.4 July 10, 2018 11:26
@mattwire
Copy link
Contributor Author

Rebased against 5.4

@seamuslee001 seamuslee001 changed the title Fix crash when performing export all contacts from search dev/core#249 Fix crash when performing export all contacts from search Jul 10, 2018
@seamuslee001
Copy link
Contributor

(CiviCRM Review Template WORD-1.1)

@seamuslee001 seamuslee001 merged commit 4e2822a into civicrm:5.4 Jul 10, 2018
@mattwire mattwire deleted the allcontactexport_regression branch September 25, 2018 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.4 master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants