Fix Field mapping bug - forgets related fields #23534

eileenmcnaughton · 2022-05-21T03:56:08Z

Overview

Fix Field mapping bug - forgets related fields

I hit this during r-run on imports and initially thought it was a regression but it is there in 5.48 too.

To replicate

do an import with a contact where some of the fields are for a related contact - here is the csv I used for testing
do NOT save the mapping, do not collect $200
once you hit the preview field go back (still no $200)
check if the related contact's field is still populated

Before

After

Technical Details

I think I got to the bottom of what the js was doing & switched to a simple jquery hide (well a whole tonne of them - I'm sure it could be more elegant at the jquery end but figuring out this much was a lot....

Comments

civibot · 2022-05-21T03:56:10Z

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

aydun · 2022-05-24T06:45:41Z

The first time the mapping page is shown, I get additional empty dropdowns after some fields:

If I then 'continue' to the preview and then 'previous' to the mapping I correctly get:

I added a phone field to trigger the 4th dropdown. The mappings are preserved when returning to the mapping screen from the preview:

One small anomaly: when selecting 'street address' for the main contact, the default location is 'Primary', but when selecting 'child of', 'street address', the default location is 'Home' instead of 'Primary'.

aydun · 2022-05-24T06:48:05Z

I see you've pushed more changes - those results were from before that push.

eileenmcnaughton · 2022-05-24T06:48:29Z

@aydun no - just a rebase - there weren't any changes in it - just keeping it fresh

eileenmcnaughton · 2022-05-24T06:49:02Z

@aydun I'm assuming your anomaly is unchanged by this PR?

aydun · 2022-05-24T06:51:04Z

CRM/Contact/Import/Form/MapField.php

+      foreach ([1, 2, 3] as $columnNumber) {
+        if (empty($mapperValues[$columnNumber])) {
+          $rowNumber = str_replace('mapper[', '', substr($row, 0, -1));
+          $js[] = "cj('#mapper_{$rowNumber}_{$columnNumber}').hide();";


Isn't 'cj' deprecated? How about:
$js[] = "CRM.\$('#mapper_{$rowNumber}_{$columnNumber}').hide();";

yeah - I'm always confused about that - it doesn't seem to work on some pages - @colemanw what should I use (Or are we just happy with 'a bit better' here?)

aydun · 2022-05-24T06:57:53Z

@aydun I'm assuming your anomaly is unchanged by this PR?

Yes - this also happens without this PR. I just noticed it while testing this, so not a problem for this PR.

eileenmcnaughton · 2022-05-24T07:05:53Z

@aydun I think the 'Primary' behaviour is better - so I might yet circle round to that (not necessarily in this PR)

aydun · 2022-05-24T08:48:12Z

CRM/Contact/Import/Form/MapField.php

      }
      else {
-        $js .= "swapOptions($formName, 'mapper[$i]', 0, 3, 'hs_mapper_0_');\n";
        if ($hasColumnNames) {
          // do array search first to see if has mapped key
          $columnKey = array_search($this->_columnNames[$i], $this->getFieldTitles());


Not sure how to do a PR on a PR ... but the line just below this that reads:
$defaults["mapper[$i]"] = $columnKey;
can be changed to:
$defaults["mapper[$i]"] = [$columnKey];
to avoid the problem on the initial display of the mapping page that I noted.

@aydun - I can add that in - but do you want to confirm if it works with the other change you suggested? - if so I can include that too

(I think you can actually suggest changes in review & I can commit them through th eUI too)

@aydun well I've pushed up tht change - so no pressure on checking the other

aydun · 2022-05-24T15:25:10Z

See: eileenmcnaughton#14

eileenmcnaughton · 2022-05-24T19:39:46Z

@aydun - I merged your commit in & rebased to cheer up Jenkins - but now it's defaulting to 'Home' for both rather than 'Primary' for both

aydun · 2022-05-24T19:47:50Z

but now it's defaulting to 'Home' for both rather than 'Primary' for both

That's odd ... it's showing Primary for me on initial display:

eileenmcnaughton · 2022-05-24T19:54:30Z

I'll try again

eileenmcnaughton · 2022-05-24T19:58:39Z

@aydun - OK it IS showing 'Primary' for main contact - but when I changed 2 fields to related I got 'Home'

darrick · 2022-05-26T03:57:04Z

CRM/Contact/Import/Form/MapField.php

+    if (!empty($sel2[$key]) && is_array($sel2[$key])) {
+      reset($sel2[$key]);
+      $subkey = key($sel2[$key]);
+      $def[] = $subkey;


I think "Primary" is set here when the form is first loaded and the mapping is guesstimated. But after the form is built there is no default set for $sel2 so "Home" is selected as that is the first item in the select list.

The options are created via JS here: https://github.com/civicrm/civicrm-packages/blob/96b7c2441b851395176bb275a45e24fbe31e3187/HTML/QuickForm/hierselect.php#L427

I've just rebased this - do you see a fix for ^^ @darrick - it was kind of a tangent on this PR - so I'm also happy to drop that bit out of scope for this PR

I think "Primary" is set here when the form is first loaded and the mapping is guesstimated.

Yes, 'Primary' is selected because it is first in the php list.

When you change to a relationship ('spouse of') then the JS `swapoptions()' function is called. No value is initially set so the dropdown shows the first option.

The array ordering in php is:

"Primary":"Primary", "5":"Billing", "1":"Home", "3":"Main", "4":"Other", "2":"Work"

However, although the php values are passed in the same order, when the JS function loops through the array to add the select options, it does so based on the ordering of the keys, not the order added, and so the options are ordered as:

1: "Home" 2: "Work" 3: "Main" 4: "Other" 5: "Billing" Primary: "Primary"

To fix the default, we could explicitly pass through a default value and use that when creating the select options in swapoptions()

If we wanted to solve the more general issue of making the JS options match the PHP order, I think we would need to assign new key values when we pass from PHP to JS, but then also map back from from JS keys to PHP keys. That sounds like a lot of effort for something few people are likely to notice!

This looks like it goes well beyond the scope of the initial aim of this PR.

default mappings. Use CRM.$ instead of cj for new jQuery Fix an assignment to $this->fieldUsed. Not sure why it exists but lets update the right key.

eileenmcnaughton · 2022-05-27T20:50:34Z

@colemanw - this should be mergeable based on ^^ - ie this is an agreed fix - discussion focusses on a tangential bug that came up in review

darrick · 2022-05-28T00:07:51Z

This is a bit out of my skill set. But most things are. But I did step through the code a bit the other night. To me it seemed like setDefault was for setting form defaults when the page is first loaded. And this bug has more to do with the form state being lost or mangled when hitting the back button.

Also, the csv file used for testing has duplicate header field. I recall theleague/cvs throws an error for that. Maybe that should be caught in Validation earlier.

I just tested real quick. If I do the following (i.e. just don't set any defaults if the mapper is set). Then it works fine except the extra selects not be hidden. This is based off master.

    $mapper = $this->getSubmittedValue('mapper');

    for ($i = 0; $i < $this->_columnCount; $i++) {
      $sel = &$this->addElement('hierselect', "mapper[$i]", ts('Mapper for Field %1', [1 => $i]), NULL);
      $sel->setOptions([$sel1, $sel2, $sel3, $sel4]);

      // Don't set any defaults if we are going to the next page.
      // ... or coming back.
      // But do add the js.
      if (!empty($mapper)) {
        //$sel->setValue($mapper[$i]);
        //$js .= "swapOptions($formName, 'mapper[$i]', 0, 3, 'hs_mapper_0_');\n";
      }
      else if ($this->getSubmittedValue('savedMapping') && $processor->getFieldName($i)) {
        $defaults["mapper[$i]"] = $processor->getSavedQuickformDefaultsForColumn($i);
        $js .= $processor->getQuickFormJSForField($i);
      }
      else {
        $js .= "swapOptions($formName, 'mapper[$i]', 0, 3, 'hs_mapper_0_');\n";
        if ($hasColumnNames) {
          // do array search first to see if has mapped key
          $columnKey = array_search($this->_columnNames[$i], $this->getFieldTitles());
          if (isset($this->_fieldUsed[$columnKey])) {
            $defaults["mapper[$i]"] = $columnKey;
            $this->_fieldUsed[$key] = TRUE;
          }
          else {
            // Infer the default from the column names if we have them
            $defaults["mapper[$i]"] = [
              $this->defaultFromColumnName($this->_columnNames[$i]),
              0,
            ];
          }
        }
        else {
          // Otherwise guess the default from the form of the data
          $defaults["mapper[$i]"] = [
            $this->defaultFromData($this->getDataPatterns(), $i),
            //                     $defaultLocationType->id
            0,
          ];
        }
      }

    }

darrick · 2022-05-28T18:48:00Z

The following solves the problem on my end: c9cd1c1

Adding this $js .= "swapOptions($formName, 'mapper[$i]', 0, 3, 'hs_mapper_0_');\n"; is what was causing the second, third and fourth options to get hosed. As that function rebuilds the options for the select lists above the select list it was called from. Usually it's called from the onchange event for the appropriate select list. But above is hard coded to the first select list.

For what @aydun is mentioning about having a default location type maybe the swapOption js could be modified to use a default as it's currently hardcoded to false. Setting it in buildForm will only work for inferred defaults.

civibot bot added the master label May 21, 2022

eileenmcnaughton force-pushed the import_check branch 3 times, most recently from 0f8ce5a to 13be8cc Compare May 24, 2022 06:30

aydun reviewed May 24, 2022

View reviewed changes

eileenmcnaughton force-pushed the import_check branch from 13be8cc to b7834b2 Compare May 24, 2022 09:15

eileenmcnaughton force-pushed the import_check branch from 84ac07a to 51f2502 Compare May 24, 2022 19:38

eileenmcnaughton force-pushed the import_check branch from 51f2502 to c0426f6 Compare May 24, 2022 19:57

eileenmcnaughton force-pushed the import_check branch from c0426f6 to bad6fcf Compare May 25, 2022 04:30

darrick reviewed May 26, 2022

View reviewed changes

eileenmcnaughton force-pushed the import_check branch from bad6fcf to 1ab060d Compare May 26, 2022 04:24

eileenmcnaughton and others added 4 commits May 26, 2022 17:49

Fix Field mapping bug - forgets related fields

7599e38

Additional defaults fix

95011e3

Show secondary fields (eg location for address fields) when setting

0e338ad

default mappings. Use CRM.$ instead of cj for new jQuery Fix an assignment to $this->fieldUsed. Not sure why it exists but lets update the right key.

Update test for latest iteration

11ed05c

eileenmcnaughton force-pushed the import_check branch from 1ab060d to 11ed05c Compare May 26, 2022 05:49

darrick mentioned this pull request May 28, 2022

Fix swapOptions writing over selected options on mapField page load. #23619

Merged

eileenmcnaughton closed this in #23619 May 30, 2022

eileenmcnaughton deleted the import_check branch June 3, 2022 00:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Field mapping bug - forgets related fields #23534

Fix Field mapping bug - forgets related fields #23534

eileenmcnaughton commented May 21, 2022

civibot bot commented May 21, 2022

aydun commented May 24, 2022 •

edited

Loading

aydun commented May 24, 2022

eileenmcnaughton commented May 24, 2022

eileenmcnaughton commented May 24, 2022

aydun May 24, 2022

eileenmcnaughton May 24, 2022

aydun commented May 24, 2022 •

edited

Loading

eileenmcnaughton commented May 24, 2022

aydun May 24, 2022

eileenmcnaughton May 24, 2022

eileenmcnaughton May 24, 2022

aydun commented May 24, 2022

eileenmcnaughton commented May 24, 2022

aydun commented May 24, 2022

eileenmcnaughton commented May 24, 2022

eileenmcnaughton commented May 24, 2022

darrick May 26, 2022

darrick May 26, 2022

eileenmcnaughton May 26, 2022

aydun May 27, 2022

eileenmcnaughton commented May 27, 2022

darrick commented May 28, 2022

darrick commented May 28, 2022

Fix Field mapping bug - forgets related fields #23534

Fix Field mapping bug - forgets related fields #23534

Conversation

eileenmcnaughton commented May 21, 2022

Overview

Before

After

Technical Details

Comments

civibot bot commented May 21, 2022

aydun commented May 24, 2022 • edited Loading

aydun commented May 24, 2022

eileenmcnaughton commented May 24, 2022

eileenmcnaughton commented May 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aydun commented May 24, 2022 • edited Loading

eileenmcnaughton commented May 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aydun commented May 24, 2022

eileenmcnaughton commented May 24, 2022

aydun commented May 24, 2022

eileenmcnaughton commented May 24, 2022

eileenmcnaughton commented May 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented May 27, 2022

darrick commented May 28, 2022

darrick commented May 28, 2022

aydun commented May 24, 2022 •

edited

Loading

aydun commented May 24, 2022 •

edited

Loading