Honor submitted cid value in existing_contact

[bsilvern]

Overview

This PR replaces #966.

It ensures that the contact selected in an existing_contact element is honored during preprocessing, rather than using the default cid value specified for the element or the cid URL parameter.

It corrects Required field fails validation when autocompleted and disabled, even when "Submit disabled field value(s)" checked.

Before

"Field is required" error occurs upon submission of a required field that is locked by an Existing Contact widget when that widget is configured in a non-static mode, e.g. AutoComplete.

After

The submission completes without error.

Technical Details

Currently, the submitted contact_id value from the Existing Contact element in the non-static widget modes is ignored within WebformCivicrmPreProcess::alterForm(). If the cid is changed after the form is loaded, e.g. by choosing a contact from the autocomplete, then the submitted data (provided by an Ajax call) belongs to the selected contact, but the cid used throughout preprocessing remains the default cid value, causing many issues. This change treats the submitted cid value as the highest priority when determining the cid used during preprocessing.

Following are some related issues corrected by this PR:

ISSUE 2: Wrong data saved in locked fields

• Setup
  Perform the same steps as described in https://www.drupal.org/project/webform_civicrm/issues/3402697, except:
  Set the default mode of the existing_contact element to User.
  Enable contributions (Payment Processor Mode: test mode, Payment Processor: Stripe, Enable Billing Address: No).
  Load the form, select a contact other than the current user, and submit
• Actual
  The submissions list and sent emails show the user's values for the locked fields (first/last name).
• Expected
  The submissions list and sent emails shows the selected contact's values for all fields.

ISSUE 3: Wrong data displayed in locked fields after Next/Previous

• Setup
  Same setup as above.
  Load the form, select a contact other than the current user, and click Next and then Previous.
• Actual
  The locked fields (first name, last name) contain values from the current user.
• Expected
  All fields should contain values for the selected contact.

Other

I came across two blocks of code which I believe are leftovers from Drupal 7 and can never be executed in Drupal 8. I marked them as TODO, but I was reluctant to remove them.

[jitendrapurohit]

Tested and confirmed it fixes all the issues mentioned in the description. Thanks for the PR @bsilvern

[AsylumSeekersCentre]

I've tested this and identified an issue. With this change, choosing Create New from an autocomplete existing contact element causes a crash when the form is submitted.

In the patched WebformCivicrmBase.php line 149:
$result = $this->utils->wf_civicrm_api($ent, 'get', $params);

• At this point, $params["contact_id"] = -Firstname Lastname
• Fails with $e->message = id is not a valid integer

Here is the YAML of a sample webform which can be used to demonstrate the issue:
test_create_new_contact.txt

[bsilvern]

With an existing_contact Select widget, locked fields were not actually locked.

[bsilvern]

• $editingSubmission changed to $submission_id to avoid obscuring its actual meaning.
• Exception -> \Exception: An obvious bug since there is no Exception function in the Drupal\webform_civicrm namespace. Interestingly, this bug caused xdebug in VS Code to crash whenever hovering over a member function (was driving me crazy for a long time).
• Corrects handling of "-" in existing_contact value (indicating "Create New" was selected".

[bsilvern]

• isFieldHiddenByExistingContactSettings() was operating on stale data so added call to $this->getExistingContactIds();
• Worked around array-to-string conversion warning on submission of a draft.
• Moved up call to $this->fillDataFromSubmission() so that it is executed prior to saving a draft (stale data was being saved previously).
• unsetError($name) was throwing an exception. I think this is old Drupal7 code. Deleted that function and added code to validate() to delete the unwanted errors.

[bsilvern]

Lots of changes here to ensure correct handling when clicking Prev/Next, especially when there are locked/hidden fields due to the existing_contact element and when working with drafts. It looks like a bigger change than it is because I changed the indent level on a big block of code and github diff is going nuts. Recommend viewing with the "hide whitespace" option.

[bsilvern]

• I was getting a failure in testTokensInEmail() because the site URL appears in the email, and the length of that URL affects where the line breaks. I therefore changed the assertion to ignore line breaks when testing the email content.
• Added a test to exercise paging around multi-page forms, saving/loading drafts, selecting/creating contacts.

[bsilvern]

• Added support to test the existing_contact hide_method and specified_contact controls.
• Corrected a race condition where we wait for an ajax message while a pre-existing one may already exist, resulting in a failure to wait and subsequent test failure.

[AsylumSeekersCentre]

I have tested your branch (bsilvern/webform_civicrm) and I can confirm that it solves the linked issue. It also solves another issue we were encountering where the name of a selected existing contact was blanked when the webform was submitted.

I haven't tested the other issues described, but the changes don't seem to have introduced any new issues for the webforms I've tested.

[bsilvern]

@jitendrapurohit I see there are similar failures in a test PR by demitcowboy. I ran the test with Drupal 10.2.* | CiviCRM 5.74.* on my machine and had no failures, so I was unable to investigate the cause of those two ajax.js errors. I did not investigate the 10 errors in Drupal 10.3.* | CiviCRM dev-master as I assume someone else is working on that.

[KarinG]

CiviCRM dev-master started failing because of [dave and I worked on this last night] see here: #1004

Thank you for all your work on this Bob. Could you please break this down into smaller PRs [one per issue] - and preferably show me the fail in a test first - because then there will be a test to safeguard your fixes in the future as well.

[bsilvern]

@KarinG As some of the issues in this PR are masked by other issues addressed in this PR, and as I already have one lengthy test that exercises all the fixes I implemented and didn't want to rewrite it into many tests, I broke the changes down into 11 branches of the form:

• branch_1_fail (based on 6.x): Issue-1 test that fails
• branch_1_pass (based on branch_1_fail): Issue-1 fix that now passes the test.
• branch_2_fail (based on branch_1_pass): Issue-2 test that fails
• branch_2_pass (based on branch_2_fail): Issue-2 fix that now passes the test.
• ....
• branch_5_pass (based on previous branch): Issue-5 fix that now passes the test.

The critical test in all the PRs is the same test that was added in this PR (testNextPrevSaveLoad), but with an early return statement injected. The return is moved further down the test with each successive PR until it is finally removed in the last one.

It was my intention that you would incrementally review each of the 11 PRs. I created all 11 branches, but then when I started to create those PRs, I realized that when you review e.g. branch_5_pass, you're going to be seeing not the diff to my previous branch in the series (as I do), but the diff to the common commit from your perspective which is the 6.x commit that my branch_1_fail was based on (3146aae). So by the time you get to the end, you're going to be seeing the equivalent of this PR, not just a few changed lines.

I'm not very experienced with github. Am I correct that I've created an unholy mess by basing the branch of each PR off of the previous branch in my series, or are you able to diff one PR against another during your review? I'm not sure how to best proceed. Perhaps I could:

• Submit one pair (the failed test and the fix).
• Wait for you to merge the fix.
• Rebase the next pair of PRs onto the current 6.x HEAD.
• Repeat

Please advise.

[KarinG]

@bsilvern - I plan on a new release next weekend so I'm happy to work with you this week to get this in.

Please first submit a PR with just your tests (both) edits and additions (w/ Prince on Guitar and Madonna on Vocals :-)) So create a add-existing-contact-tests branch in your repo, add your commits that include all test edits and additions and then submit a PR with just those. I want to see the new tests fails and review the test/code in PhpStorm. Follow up subsequent PRs will then address one of the fails at a time.

[bsilvern]

@KarinG I can't create a test that will demonstrate, e.g. bug #3, when bugs #1 and #2 must first be fixed to create the conditions that cause bug #3 to manifest. I think we have to do the PRs as I wrote -- add test, add fix, add test, add fix.

I've submitted the first two PRs (#1007, #1008) to demonstrate and then fix the first bug. Please let me know if you want the remaining 9 PRs all at once, or you'd like me me to rebase each of the subsequent PR's after you merge the previous one.

[KarinG]

Sorry for the delay - I've asked one of my team to look at your PR and to help break it down.

[bsilvern]

@KarinG OK, thanks for the update.