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

Disambiguate Address.state_province_id:abbr #25550

Merged
merged 1 commit into from
Feb 17, 2023

Conversation

totten
Copy link
Member

@totten totten commented Feb 10, 2023

Overview

The test ContactJoinTest::testCreateWithPrimaryAndBilling is flaky. This stems from following:

  'address_billing.state_province_id:abbr' => 'AK',

The symbol 'AK' can resolve to three places: Akwa Ibom (Nigeria), Atakora (Benin), and Alaska (USA). This is an ambiguous choice. It should be resolved in a consistent way.

ping @colemanw

Before

There are two layers of flakiness:

  • MySQL: When it fetches civicrm_state_province records, it uses ORDER BY abbreviation. This is ambiguous.
  • PHP: After receiving the list, it maps the results into various arrays and does some extra sorts. This provides a similar ordering. However, depending on the PHP version, it may swap around equivalent abbreviations. (Thus, Akwa Ibom and Alaska can flip-flop their relative positions.)

The PHP flakiness is more apparent than the MySQL flakiness. You can easily switch PHP versions to observe the difference (which is why testCreateWithPrimaryAndBilling usually fails on min and usually passes on max). For MySQL, the problem depends more on internal/arbitrary details, and the test-assertions match the usual behavior -- so failures there are less common.

After

Both layers should be consistent:

  • MySQL: It fetches civicrm_state_province with ORDER BY abbreviation, id. This is unambiguous.
  • PHP: PHP 7 now uses a different sort method - one which comes closer to the PHP 8 behavior.

@civibot
Copy link

civibot bot commented Feb 10, 2023

(Standard links)

@totten
Copy link
Member Author

totten commented Feb 10, 2023

The PHP aspect seems to be easier than the MySQL aspect. I've extracted that as a separate PR (#25552).

@totten
Copy link
Member Author

totten commented Feb 10, 2023

Rebased to build on top of PHP-only PR (25552).

Changed the approach on the MySQL fix.

(The earlier patch had filled in the <orderColumn>, because there were some hints in the code toward using orderColumn. However, there were test-failures which suggest that orderColumn is a bit nonsensical: the "pseudoconstant" has several flavors -- name, label, id, abbreviation, etc. You can't use a singular orderColumn for all flavors.)

Pushed up a simpler approach that doesn't require extra metadata.

@@ -1499,7 +1499,8 @@ public static function renderOptionsFromTablePseudoconstant($pseudoconstant, &$p
$select = 'SELECT %1 AS id, %2 AS label';
$from = 'FROM %3';
$wheres = [];
$order = 'ORDER BY %2';
$order = 'ORDER BY %2, id';
// Example: 'ORDER BY abbreviation, id' because abbreviations are not unique.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe check that the id is in the fields? Import tables do not have an _id field -

It looks like you can use some variant of

   if (in_array('id', $availableFields), TRUE) {
     ...
    }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton Good idea. Updated.

Also, rebased on top of current master. (Since the related PR was merged, the diff should now look smaller.)

Consider `ContactJoinTest::testCreateWithPrimaryAndBilling` which writes the value:

  'address_billing.state_province_id:abbr' => 'AK',

The symbol 'AK' can map to three places: Akwa Ibom (Nigeria), Atakora
(Benin), and Alaska (USA).  This is an ambiguous choice.  It should be
resolved in a consistent way.

One flavor of ambiguity comes from MySQL.  When loading abbreviations, Civi
queries with `ORDER BY abbreviation`.  This is a *typically* stable, but it
has no *guaranteed* resolution.  Adding a secondary sort key makes the
outcome clear/unambiguous.
@totten
Copy link
Member Author

totten commented Feb 17, 2023

civibot, test this please

@eileenmcnaughton
Copy link
Contributor

It looks very small indeed. This seems safe & sensible to me

@eileenmcnaughton eileenmcnaughton merged commit 3ed3d2c into civicrm:master Feb 17, 2023
@totten totten deleted the master-abbr2 branch February 18, 2023 00:07
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.

2 participants