avoid back trace when importing preferred communication method

[jmcclelland]

Overview

When importing a contact with preferred communication method set, I get:

CiviCRM_API3_Exception: "'' is not a valid option for field preferred_communication_method"

The problem surfaced after upgrading from 5.33 to 5.39.

I traced it back to this code and can fix it with this simple change.

The problem with the original code is that $formatted[$fieldName] is an
array with the key set to the chosen value and the value set to 1. So, the array_search always fails since the key it's looking for is an array, not a string.

I think the line:

&& empty($fieldSpec['options'][$formatted[$fieldName]])) {

Is also not quite right but not sure how to change it.

Before

Importing a contact with preferred communication method set results in:

'' is not a valid option for field preferred_communication_method

After

The contact record is imported.

Technical Details

The problem may have been introduced via cdfa664.

Comments

I don't think this fix is quite there yet but would love to get some feedback.

Also, in this particular use case, the label for the SMS option has been changed to "Text message" but I don't think that has an impact on this issue.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[jmcclelland]

I think I have the right fix now (the previous commit causing the regression was not properly tested against a multi-select field).

And it's locked in with a test.

[jmcclelland]

Woops - one more commit + squash (I accidentally left in my incorrect earlier fix).

[demeritcowboy]

It's interesting that this works in the test, because I can't get numbers to work for this field in the UI, either before or after the patch or using single-value or multi.
Mystery?

[jmcclelland]

Huh - interesting. I put the values into the test because I thought, why not expand the test coverage? But didn't test entering the values in the UI. What happens when you try? Do you get an error?

[demeritcowboy]

You can't get past the import screens to see what would happen - it gives you the excel file to download with errors which says it thinks the field has an invalid value.

[demeritcowboy]

• General standards
  • [r-explain] PASS Can reproduce.
  • [r-user] PASS
  • [r-doc] PASS
  • [r-run] PASS
    • Tried a couple different things including custom multivalue fields. There's just the comment about values for this communication field, but it didn't work before the patch either.
• Developer standards
  • [r-tech] PASS
    • The code is hard to follow already, but the comment maybe helps.
  • [r-code] PASS
  • [r-maint] PASS
  • [r-test] PASS
    • That the test works but UI doesn't might just be that the prevalidation is wrong but the import itself is ok.

[jmcclelland]

@demeritcowboy - ahhh.... see that now. The test code is executing the same code as the UI, but the code doesn't consider it a fatal error (it's more of an FYI) so the test code happily continues. I think it's good for the test to remain and a new issue could be opened to:

1. Fix all import testing code to check for this kind of error
2. Fix CRM_Contact_Import_Parser_Contact::isErrorInCoreData to allow values for this field (and it looks like a few others).