ensure checkbox custom fields on contributions import properly

[jmcclelland]

Overview

We've been treating checkboxes differently when importing for a while, but perhaps the need for the different behavior is over? I've included a test and am hoping we have enough test coverage to see a failure if the checkbox exception is still needed somewhere.

Before

When importing a Contribution, that includes a custom field of the the type checkbox in which there is no value of "1", an error is returned.

When testing it's important that you use a custom option set that does not use numeric values - you must use string values.

After

The contribution and custom field is properly imported.

Technical Details

The old code unserialized a comma separated list into an array with the array key set to the option value and the value to the number 1. It expected the proper values to be extracted from the keys.

However, when imported, the values are taken from the array values, not the keys, so it's always "1".

Some tests might not detect a failure here. Suppose you have an option list with the name "Blue" assigned the value 1. You can test this option list and it passes simply because you wanted the value "1" and it just so happens that all values get set to 1.

Comments

I expect some test failures which might suggest this is a bad idea or might suggest that we should fix the code that is failing. I'm really not sure.

[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).

[eileenmcnaughton]

Nope - no test fails - this adds a test & code looks better but I haven't figured out what implications there could be yet

[eileenmcnaughton]

if we are using apiv3 the action is still 'create' - api v4 uses 'update' as a preferred action

[jmcclelland]

Thanks for looking it over @eileenmcnaughton - I just changed 'update' to 'create' and squashed the commits into just one commit.

[eileenmcnaughton]

@jmcclelland I've been pondering this PR - the underlying issue is the code is pretty awful so it's hard to know what is going on or what the consequences are.

Test cover of this import class turns out to be 'a bit better than I had expected' in CRM_Contribute_Import_Parser_ContributionTest - and other custom fields tests are still working

This behaviour also seems to be tested to be consistent with the documentation

If you are importing data into multi-choice (e.g. check-box or radio button) custom fields, your data source can use either the label (what's visible to the user in the CiviCRM front end) or the value (what's actually stored in the database for that choice). CiviCRM will recognise it and import it appropriately. When importing into multi-choice core data fields, you can specify only the value(s) in your data source, not the label.

If you are updating multiple choice options, new values will replace the entire field. For example, if you update the value of the Colors field to be "orange" for a contact that currently has Colors set to "blue", the result will be that Colors is set to orange, not orange and blue.

To import multiple values into a multiple choice field use the "," (comma) character as a separator - using either the Label OR the Value of the required option(s) i.e.: "1, 2, 3" or "fred, red, yellow".

WHich leads me to the decision to merge as

1. this PR fixes an identifiable problem
2. it locks in the fix with a test
3. the fix is consistent with the docs
4. other changes are being made to import this merge cycle so if there IS any fallout this would be the release
5. no known issues are caused

[jmcclelland]

Thanks Eileen!!