Avoid overwrite only first field with last value

[beto-aveiga]

Overview

Webforms with multiple emails values are not being saved correctly. The last submitted value is written to the first email field. Other emails are not being updated.

Before

1. Data before submission
   

2. Example configuration of the webform
   

3. Data when visiting the webform
   

4. Filling the webform with a second email
   

5. Visiting the webform again we can see that first email was overwritten with last email
   

6. From the Admin UI it confirms what the webform was showing: first email overwritten with last email.
   

After

1. Repeat steps 1 to 4 from "Before" for initial data, filling the webform and saving.

2. Data was stored correctly.
   

Technical Details

• Attaching exported webform with node export:
  webform-multiple-emails.txt

• Site created for testing with civibuild using this command
  civibuild create civi12 --type drupal-demo --civi-ver 4.7.27 --url http://civi12 --web-root /home/beto/buildkit/build/civi12

• CiviCRM currently uses civicrm_webform 7.20, however this module was replaced with the last version found here https://github.com/colemanw/webform_civicrm

Comments

Base on the modifed code (shown below) this is probably not only applicable to emails but for fields which are not addresses.

[colemanw]

Hi @powdevel thanks for the PR! I appreciate the thorough description.
From your screenshots you appear to be configuring a webform to have 2 "home" email addresses, which seems a bit odd. Does the bug you encountered also manifest if the location types are different (e.g. home and work)?

[beto-aveiga]

Hi @colemanw, when all emails are a different type it works as expected, no patch needed for that. However when one or more emails are the same type it fails and overwrites the first email with the last email of the same type.
Using this patch I tested using 2 emails with the same type and one email different and it worked as expected in my tests. I also tested using 3 different types of emails and the patch allows to save correctly (I mean apparently it is not introducing more errors).

[colemanw]

Ok thanks. I'll go ahead and merge this and hopefully folks like @KarinG who use the dev version of this module will catch any issues before the next release.

[KarinG]

@davejenx - you're very familiar with this bit of webform_civicrm code - if you have time can you please have a look at this PR?

[mattwire]

@davejenx @KarinG @powdevel as a related issue #147 would like your feedback on this change: https://github.com/colemanw/webform_civicrm/pull/147/files#diff-59be2e079a691088e15ab3a80e0640a0R2513

Without that change it's only updating the actual phone number from submitted values (presumably the same for email). And it won't save custom data. With this change it will update all submitted values which I think is what we want?

[beto-aveiga]

@mattwire Title on this PR maybe is not so precise as I'm investigating more on this bug.

With this change it will update all submitted values which I think is what we want?
Yes, that is the purpose of this change. My tests were only for email fields but looking at the code it could apply to other fields.

I recently noticed that the code on saveContactLocation is assigning wrong the primary key. Last changed value that matches location of first value will be the new primary.

Also noticed that, if there an email on the webform, the email user account of Drupal is being changed always on first iteration no matter the location set in the email field, no matter if it changes or not. It would produce in some cases to have a primary email for CiviHR and a different Drupal user account email.

I'm trying to fix both errors and submit new PRs as soon as possible.

cc: @KarinG @colemanw

[KarinG]

The last time I looked at this with @davejenx we both were quite confused (more like scared) re: reOrderLocationValues() but managed to solve a key issue by essentially adding a safe workaround; The issue was #121 - please ensure that your work does not cause a regression for that.

[beto-aveiga]

@KarinG Looking at #121 I think the error is caused because of these lines on saveContactLocation :
if ($location == 'address' && $params['location_type_id'] == $is_primary_address_location_type) {
$params['is_primary'] = 1;
These lines basically make primary any address that matches $is_primary_address_location_type which is the first address on the webform (and it can be null also). Now, this line is reached every time that a values changes (in this case every time an address changes), and that is why it will assign the primary address to the last changed address.
I think this is also happening with emails.

PD: currently I'm working with version 7-x.4.20 because that is the version that CiviHR uses.

[beto-aveiga]

PD: The GIF that I posted above was done with the module patched with this patch, otherwise it will not save all values.

[KarinG]

I never thought someone would use multiple of type = home addresses (email etc). But what needs to remain fixed is that if in the webform -> CiviCRM tab a user configures an Address of type = home (note the GUI says -> that this will become is_primary) then it should become is_primary; before 121 billing address would become primary simply because it was the last address entered;

Happy for you to rework this and make it better but we can’t regress on 121 and end up with the incorrect address designated as is_primary

[KarinG]

So your test needs to include a payment of some type -> common scenario: collect name + address (home) + hook up a contribution page to a payment processor and make a contribution via the webform (the billing address is pulled in from teh contribution page)

[beto-aveiga]

You are welcome @KarinG. Thanks to you too.

I understand about the tests to avoid regression for #121

What you are telling me is that currently civicrm_webform does not support multiple fields with the same location, like 3 personal emails, right? If so the UI should not allow to submit this configuration, or at least show a warning message in the UI to try to prevent these situations. And much preferably let the admin choose whether he wants the first field of the group to became primary... although I know this would be more work.

Similar for emails, the first email will always be the new Drupal account email, no matter what. I think this should be written in the UI too, and preferably let the admin to choose to overwrite or not the Drupal account email with that first email.

Now I have to rethink some things 😄

[KarinG]

Not saying we don’t want to support multiple same location (like multiple home addresses) - it’s just something we’ve never seen/asked; if you can find a safe way to make that work without causing regression in the is_primary designation then for sure this is ok;

[beto-aveiga]

@KarinG CiviCRM does not supports multiple addresses with the same location, so I think it doesn't have to be supported, however webform_civicrm allows this configuration.

Beyond addresses... for phones, emails and probably some other fields, CiviCRM does support multiple fields with same location. What I'm going to do is to create the PRs to support those fields and probably other fields in the same situation.

Until now the tests I had made saved all fields correctly.

I even tested with multiple emails fields and multiple locations and everything is working fine. Like this:

I going to test fields like phones, which can have additional types like "Landline" or 'Mobile".

Tests to avoid regression on #121

To avoid regression on #121 I tested with another webform with 2 address: one location work (primary) and the other one location personal. I remove all webform fieds from address 1, to not allow editing on first one but also do not change primary address. Like so:

I also set this config in the contribution part:

And the webform looks like this:

Tests made with this webform saved data correctly too, primary keys for addresses where not swapped, wrongly overwritten, or anything else.

PD: please tell me if the config for avoid regression on #121 is enough. Honestly I see #121 and I think that the modifications I'm doing right now will not make a regression, but will allow saving multiple fields correcly without primary keys confusion.

PD2: the tests I am making include the patch in #146 to retrieve information correctly on webform load

[KarinG]

Excellent - and yes this works - I just pulled your edit into one of my test instances - and happy to also confirm is_primary continues to be handled properly. So this one is in - let's keep this merged. And then continue on from here. I've not tested anything other than this PR (thus far). Which one (between you and @mattwire is next)?

[beto-aveiga]

@KarinG Cool! 👍 Tomorrow I will make a PR with the fix for "primary keys confusion" which works at least on my tests, allowing multiple fields to work in one form keeping primary keys correctly.

[beto-aveiga]

@KarinG The following PR avoids swapping of primary keys when using multiple values with same location #148
Very small changes where needed. My tests went well.
I still have to document better the PR.

[KarinG]

I can look at this as is - Can you instead please look @mattwire ‘s PRs - they are related and I’d like your thoughts on it.

[beto-aveiga]

@KarinG @mattwire Sorry I overlooked your message. Now I'm checking. Just to be sure: @mattwire are you saying that for fields like phones additional data is not saving? Would you make a GIF or similar to explain better the bug? If so add it to your PR description and ping me please.

[beto-aveiga]

@KarinG I think @mattwire is adding very important functionality which is different from this PR. I inspected the code and modifies different things from my PR. However as of now I had not make any tests on #147 as I don't know exactly the bug and how to reproduce it, I'm fairly new to CiviCRM.

A summary:
#145 allows multiple fields of same location type
#146 fixes the load of the information when location type is selectable on a field
#147 allows/fixes custom data saving for fields (right?)
#148 prevents primary key swapping between configured fields

PD: I think that #145 #146 #147 #148 are compatible all together.

[mattwire]

@powdevel @KarinG I think as long as you (@powdevel) do your tests on master (ie. with #147 applied) and there are no problems then all is good :-)