Automattic / Automattic/jetpack-crm

Merging contacts drops date custom fields from the secondary record

Open
#28 1 comment 0 reactions 0 assignees View on GitHub
bug needs-triage
Dominant language
PHP
Stars
13
Forks
8
Avg merge
1d 10h
Merged PRs (30d)
13

Description

### Quick summary

Merging two contacts silently drops a date custom field that only the secondary record has. The merge is supposed to patch empty fields on the main record from the secondary one, and it does that for every other field type. Date custom fields go missing without any error or log entry.

This is the same double normalisation as #21, reached without going anywhere near the API. It's worth recording separately because a fix confined to the API layer, like #27, doesn't reach it.

### Steps to reproduce

1. In Jetpack CRM → Settings → Custom Fields, add a Contact custom field of type Date (e.g. label `Contract Date`, slug `contract-date`).
2. Create contact A and leave the date custom field empty.
3. Create contact B and set the date custom field to a real date.
4. Merge B into A, with A as the main record.

**Expected:** A ends up with B's date, the same as any other field the main record was missing. The merge log says "Copied field ... from secondary record over main record, (main was empty)."

**Actual:** A's date custom field is still empty. The merge reports the copy as a change, so the log claims it happened.

### Root cause

`zeroBSCRM_mergeCustomers()` loads the main record with `zeroBS_getCustomer()`, which returns date custom fields as the raw UTS the DAL stores:

```php
$master = zeroBS_getCustomer( $dominantID ); // includes/ZeroBSCRM.DAL3.Helpers.php:1303
$masterNewMeta = $master; // :1343
```

The patch loop then copies the secondary record's value in, also a raw UTS:

```php
$masterNewMeta[ $fieldPrefix . $fieldKey ] = $slave[ $fieldKey ]; // :1362
```

and the whole array goes to `zeroBS_addUpdateCustomer()` at `:1471`, which normalises it again with `$removeEmpties` on:

```php
$zbsCustomerMeta = zeroBS_buildContactMeta( $cFields, $existingMeta, $metaBuilderPrefix, '', true ); // :1863
```

The `date` branch of the builder expects `Y-m-d`, gets `"1784764800"`, and `DateTime::createFromFormat( '!Y-m-d', '1784764800' )` fails. `jpcrm_date_str_to_uts()` returns `false`, `$removeEmpties` drops the key at `:3049`, and `addUpdateContact()` skips absent custom fields at `ZeroBSCRM.DAL3.Obj.Contacts.php:3087`. Nothing is written and nothing complains.

Three callers hand the builder an array that has already been through it: `api/create_customer.php:51`, `api/create_company.php:30`, and this one. Everything else normalises once and goes straight to `addUpdateContact()`.

### Fixed by #25

The idempotency fix in #25 covers this, because it makes the `date` branch pass an already-converted timestamp through untouched. Verified both ways:

```
Trunk: ✘ Merging contacts copies a date custom field from the secondary record.
Failed asserting that '' matches expected 1784764800.
Branch: OK (1 test, 2 assertions)
```

Filing it anyway so the case is recorded, since it's the clearest argument that the builder is the right place to fix this rather than the API endpoints.

### The regression test is blocked

I've written the test but left it out of #25. It trips a pre-existing deprecation, and `phpunit.11.xml.dist` sets `failOnDeprecation="true"`, so the suite exits 1 with it in:

```
ZeroBSCRM.DAL3.ObjectLayer.php:934
strlen(): Passing null to parameter #1 ($string) of type string is deprecated
Tests: 53, Assertions: 222, Deprecations: 1
```

The null values are `tw`, `fb` and `li`. A contact created without socials has NULL in those columns, `getContact()` hands the nulls back, and the merge passes them straight into the max-length check:

```php
if ( strlen( $val ) > $this->objectModel[ $fieldKey ]['max_len'] ) {
```

It fires identically on trunk and on the #25 branch, so it has nothing to do with dates. Setting the socials in the test fixture doesn't avoid it, the nulls originate inside the merge.

Two ways to unblock the test, and I don't have a strong view on which:

- Cast at the check, `strlen( (string) $val )`. One line, and it fixes every other caller that round-trips a contact through `addUpdateContact()`.
- Stop the merge handing null socials down in the first place, which is the real bug but a bigger change.

The test itself is ready and I can attach it to whichever gets picked.

### Related

- #21, the original report, contacts and companies via the API
- #25, the idempotency fix
- #27, the `create_customer.php` rework

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with zeroBSCRM_mergeCustomers() in includes/ZeroBSCRM.DAL3.Helpers.php around lines 1303-1471, then inspect zeroBS_buildContactMeta() and the contact update path at ZeroBSCRM.DAL3.Obj.Contacts.php:3087. Run the mentioned merge regression test and account for the existing deprecation at ZeroBSCRM.DAL3.ObjectLayer.php:934; done means the secondary date remains on the main contact and the test suite passes.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.