Default value overwrites on contact merge fix

[escopecz]

Please be sure you are submitting this against the staging branch.

Q	A
Bug fix?	Y
New feature?	N
Automated tests included?	Y
Related user documentation PR URL	/
Related developer documentation PR URL	/
Issues addressed (#s or URLs)	maybe #7558 and #7559
BC breaks?	N
Deprecations?	N

Description:

A contact clicks on a tracked email link that goes to a tracked page. The browser must contain no Mautic cookies. A new contact is created with only default values. If default values from the new contact overwrite the values of the original contact then data are lost. This PR ensures that a default value from newer contact won't overwrite the original value in the older contact.

Steps to reproduce the bug:

Step 1: Create a Custom Field called Email Consent with Select Data Type, with Yes and No labels and values and default value of No
Step 2: Create a Contact from within Mautic with a First Name, Last Name, Email and Email Consent set to Yes.
Step 3. Send an Email to the new Contact with a link to a Mautic-tracked page. Does not matter if a landing page or a page tracked by the tracking script. Just ensure that you are opening the tracked link in a fresh incognito window (no Mautic cookies) and if you test on local then add index_dev.php to the tracking URL. Otherwise it won't track the page it as localhost is excluded from tracked IPs.
Step 4. Refresh the contact edit/detail page. The value is set to No.

Steps to test this PR:

1. Edit the contact and set the value to Yes again.
2. Open the tracking URL in a fresh incognito window.
3. Refresh the contact edit/detail page. The value is still set to Yes.

[pjeby]

Perhaps this is just my confusion, as I am still quite new to the project, so apologies in advance if I am being a bit dense here. But AFAICT, unless #7560 is also merged, this PR will introduce a new problem, whereby it is impossible for a default value to be set in a merge by PATCH of an anonymous contact. Meanwhile, it does not address the issue of merging device records to prevent loss of sessions (i.e. #7559) at all.

This approach seems to me to be a workaround for only part of the problem, rather than a fix for the underlying cause. AFAICT, the issue with PATCH merging contacts was introduced when LeadApiController was refactored to use a common base for API controllers, as the contact merge process was moved to preSaveEntity(), even though it used to happen before the PATCH values were applied. #7560 fixes this by moving the merge operation back to roughly where it was before (i.e., before PATCH values are applied), so the PATCH values take precedence over the values supplied by the merge.

In contrast, this PR only addresses the merging of the fields themselves, meaning that without #7560, it at best addresses only part of the problem, while introducing a new failure case whereby PATCH will not be able to explicitly set a field to its default value in the event of a merge. (It also doesn't address issue #7559 at all, since that has nothing to do with field merging.)

So, even if this PR is accepted, it is important that #7560 and #7561 be applied as well, since even though this PR may fix certain merge scenarios that I myself have not encountered, it does not address the problems I have encountered.

Thus, I would like to respectfully suggest that someone open a new bug report, separate from #7558 and #7559, to describe the conditions that this PR does fix, and then link this PR to that new issue instead of the earlier issues, so that the actual fixes for those earlier issues aren't accidentally set aside. Thanks!

[escopecz]

@pjeby I crossed the issues addressed in the PR description. It's clear from your comment that this is not the solution for those. I thought it may be related, I was wrong.

Let's focus on the issue described in the PR description. It exists and it's fixed by this PR. The use case is that a contact opens an email link in a browser where no Mautic cookie exists. Perhaps another device.

I don't understand what a PATCH has common with contact merge. You need a certain contact ID to make a PATCH request. There should not be any merging involved.

[pjeby]

To answer your question, if you PATCH an anonymous contact using its ID (obtained via cookie), and the PATCH adds a value to a unique field, then a merge occurs. Prior to the API controller refactoring, the merge was done first, then the updates were applied to the merged contacts; now the reverse is done, causing the PATCH updates to be lost.

As to the rest, well, again, apologies if I'm being dense, but in the scenario presented by this PR's description, shouldn't the actual solution be not creating a new anonymous contact in the first place, since the link passes through a redirect that should be able to simply change the session cookies? Or in the event that the browser has an already existing anonymous contact, that the non-anonymous contact be treated as the merge winner, no matter the order of creation or update?

All that being said, I'm not against this PR per se; I only wanted to make sure that the PRs for the problems I encountered didn't get lost in the shuffle, so I have no further case to make here one way or the other.

[escopecz]

I believe the reason why we create an anonymous contact at first is because of the queue feature where the page/email hit is queued at first, response returned in a timely manner and then processed in the background. That's just my theory, but I don't see how the queueing could work otherwise.

[RCheesley]

Tested per the instructions and confirm that after applying the PR, the custom field is not changed back to the default whereas it was pre-patch.

[dennisameling]

@escopecz there is a conflicting file: CustomFieldRepositoryTrait.php.
Would you mind rebasing this PR to see if that fixes the conflict? Thanks in advance :)

[escopecz]

Rebased

[dbhurley]

+1

[pjeby]

Does this bug still exist now that #7595 is merged? The steps-to-reproduce on that one are suspiciously similar to this one, and IIUC that one prevents an anonymous contact from being created in the first place, meaning the new code paths in this PR wouldn't ever be executed, AFAICT.

Also, this PR still has the problem where it would make it impossible for a default value to be deliberately set in a merge by PATCH of an anonymous contact, at least until #7560 is also merged.

(That is, this PR should not be merged unless #7560 is in the same release, or it will introduce a bug.)

[RCheesley]

@pjeby we require two tests on the PR's to be merged, #7560 does not yet have any tests so it is not likely to be merged.

My understanding is that #7595 is preventing field merging when an anonymous contact is immediately merged into an existing contact. This PR fixes the merge code to ensure defaults are handled correctly.

So, if you're saying that #7560 needs to be merged at the same time as this one, I'd recommend we find at least two people to test it pronto, as the beta release for 2.16.1 is slated for today.

Please see https://github.com/orgs/mautic/projects/2 for the outstanding tests that are pending.

[dennisameling]

Merging now since we have 2 approvals and while testing #7560 I couldn't seem to reproduce the bug on the current staging branch. Let's test 7560 when 2.16.1-beta is out so we can see if this PR indeed introduced a bug.

[pjeby]

So, I loaded this PR up in mautibox, and was unable to reproduce the bug I brought up as a possibility last July. So it doesn't look like it will introduce a new bug to merge it... even though I don't think that merging it is necessary or relevant to fix the bugs fixed by #7595 and #7560.

This PR fixes the merge code to ensure defaults are handled correctly.

I just loaded it up in mautibox and manually merged an anonymous contact to an identified one, and the default/empty values from the anon overwrote those on the identified contact. Is that considered "correctly", then?

If it doesn't handle that scenario, I'm not clear on what scenario it does do anything useful for. Certainly the scenarios it's supposed to handle (outside of the one fixed by the already-merged #7595) are not documented here.

[mautibot]

This pull request has been mentioned on Mautic Community Forums. There might be relevant details there:

https://forum.mautic.org/t/announcing-mautic-2-16-1-beta/13438/1