dev/core#4231 Fix failure to import non-default state

[eileenmcnaughton]

Overview

Fixes https://lab.civicrm.org/dev/core/-/issues/4231

Before

As described in https://lab.civicrm.org/dev/core/-/issues/4231 & as replicated in the attached test

After

Fixed

Technical Details

The import is relying on apiv4 getfields to return the available state options here

civicrm-core/CRM/Import/Parser.php

Lines 1741 to 1745 in 68108b0

	$options = civicrm_api4($this->getFieldEntity($fieldName), 'getFields', [
	'loadOptions' => ['id', 'name', 'label', 'abbr'],
	'where' => [['name', '=', $optionFieldName]],
	'select' => ['options'],
	])->first()['options'];

getFields is returning only the states from the default country, not other countries even though the api call does not specify them.

Note that the default country is not configured by default & perhaps is more commonly used on older sites. Also note that when set it literally is the default country, not the only country

Comments

[civibot]

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

• ➡️ PR Demo site (can be rebuilt with the command test this please).
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

[civibot]

The issue associated with the Pull Request can be viewed at https://lab.civicrm.org/dev/core/-/issues/4231

[eileenmcnaughton]

test this please

[colemanw]

Hmm, I just spent about half an hour debugging this test and tracing through the import code... I'm finding those classes particularly difficult to follow.

Generally speaking, the problem is that each field is being processed atomically, the importer classes are failing to pass relevant context into APIv4.getFields, which APIv4 needs so it can handle the interrelatedness of fields like county->state->country.

I don't know how much we'd have to fight against the Importer code to achieve this, but the ideal solution would be to give APIv4 the context data it needs. The general formula for this (as seen in APIv4's own handling of pseudoconstant lookups) is:

1. TopSort the array in order of dependencies (e.g. country should be resolved before state, state before county). There's already a utility function to do this.
2. Pass the array of resolved $values into APIv4::getFields() so that it knows e.g. the value of country_id when looking up options for state_province_id.
3. That's it. It's pretty simple, but the Importer classes might put up a good fight regardless.

[eileenmcnaughton]

@colemanw hmm - the importer code builds up an array of available options for each field it is importing at the start. Then while importing it checks the value against the id, name, label & abbreviation to find a match. It doesn't know all the countries it will be dealing with when it starts, or even if all rows that have a state will have a country field. And if they don't have a country it might not be the site default country - (for a small number of states that would be ambigous & would rightly fail on import)

In my database there are less than 2k states - that doesn't feel to me like the sort of size that I would want to avoid loading all at once, given each row doesn't have that many keys (I would want to avoid serilizing/unserializing it though) - so I'm really looking for the getFields to not filter if no filter is passed to it

[colemanw]

@eileenmcnaughton if you want to fetch all states, there's probably a way to do that. But I think the underlying problem will persist: without passing any context to the api, it has no way of knowing the difference between e.g. Maryland, USA and Maryland, Liberia, as this test demonstrates:

civicrm-core/tests/phpunit/api/v3/AddressTest.php

Lines 487 to 488 in d2ce7f7

	// should find state_province_id of 1019, Maryland, United States ... NOT 3497, Maryland, Liberia
	$this->assertEquals('1019', $address1['values'][0]['state_province_id']);

[eileenmcnaughton]

@colemanw yeah - but the import class handles that - ie it works with the information it has to get the best pick or lets the user know if it is unresolvably ambigous - but it does that row by row whereas it resolves options at the start

[eileenmcnaughton]

Also worth noting that import does a validate process before the import process

[colemanw]

@eileenmcnaughton where does the import code fetch the list of states?

[eileenmcnaughton]

@colemanw it does it here

civicrm-core/CRM/Import/Parser.php

Lines 1741 to 1745 in 68108b0

	$options = civicrm_api4($this->getFieldEntity($fieldName), 'getFields', [
	'loadOptions' => ['id', 'name', 'label', 'abbr'],
	'where' => [['name', '=', $optionFieldName]],
	'select' => ['options'],
	])->first()['options'];

[colemanw]

@eileenmcnaughton I don't think it's specific to APIv4. The API just delegates to CRM_Core_Pseudoconstant which in turn calls BAO::buildOptions which does this:

civicrm-core/CRM/Core/BAO/Address.php

Lines 1221 to 1229 in ab913b2

	if (empty($props['country_id']) && $context !== 'validate') {
	$config = CRM_Core_Config::singleton();
	if (!empty($config->provinceLimit)) {
	$props['country_id'] = $config->provinceLimit;
	}
	else {
	$props['country_id'] = $config->defaultContactCountry;
	}
	}

[eileenmcnaughton]

@colemanw hmm - that if (empty($props['country_id']) && $context !== 'validate') { - if I could get the context to be validate ? Do you know if the api could pass context through?

[colemanw]

@eileenmcnaughton unfortunately not. And that would also affect how the output is formatted.
Seems like the only practical fix at this point would be to add a special case in the Import_Parser class to fetch all states/provinces without using the API.

[eileenmcnaughton]

@colemanw OK - I think I'd gotten to that conclusion - I might see if I can lazy load in some way but will think about that

[colemanw]

Ok this is working now so let's merge it.