Fix 409 conflict on Clientify merge by taxpayer_identification_number - #259
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02b08cce27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| foreach ( $active_feeds as $feed ) { | ||
| if ( $addon->is_feed_condition_met( $feed, $form, $entry ) ) { | ||
| $matched_feed = $feed; | ||
| break; |
There was a problem hiding this comment.
When two active feeds have conditions that both match the same entry—including two unconditional feeds—each feed can produce its own error log, but this loop selects the first matching feed for every log. A resend of a failure from the second feed therefore inherits the first feed's module and merge strategy, potentially updating the wrong object or creating a duplicate; the originating feed ID needs to be stored with the error and used directly during retries.
Useful? React with 👍 / 👎.
| foreach ( $matched_feed['meta'] as $key => $value ) { | ||
| if ( ! empty( $value ) ) { | ||
| $settings[ $key ] = $value; | ||
| } |
There was a problem hiding this comment.
For a feed configured with a custom CRM/account, its credentials are stored as fc_crm_custom_*, while CRM clients read fc_crm_*; the original submission handles this conversion in GFCRM::get_api_settings_custom(). Copying the feed metadata verbatim leaves the global credential in fc_crm_apipassword, so manual and scheduled retries can authenticate against the wrong account or fail even though the custom feed credentials are valid; apply the same custom-setting translation here.
Useful? React with 👍 / 👎.
| * Reproduces the real-world case: Antonio Luque Oliveros (DNI 50997453J) | ||
| * already exists in the account with merge_strategy set to | ||
| * taxpayer_identification_number — this must resolve to an update, not a 409. |
There was a problem hiding this comment.
This comment explicitly identifies the fixture as a real-world case and publishes a person's full name together with a checksum-valid Spanish DNI. Committing that sensitive identifier exposes it through the repository and any distributed source archives; replace every occurrence with clearly synthetic data and treat the committed value as already disclosed when cleaning up history or logs.
Useful? React with 👍 / 👎.
3 participants
Summary:
When merge_strategy is set to taxpayer_identification_number, form submissions for contacts whose NIF/DNI already exists in Clientify were failing with a 409 conflict instead of updating the existing contact.
Root cause:
determine_search_by() in CRMLIB_Clientify mapped email and business_name to Clientify's generic query search param, but passed taxpayer_identification_number through literally as a query param. Clientify's API does not support filtering GET /contacts/ by that field name directly — confirmed against the real API, where GET /contacts/?query= correctly matches the contact but GET /contacts/?taxpayer_identification_number= does not. As a result, the existence check always reported "not found," the integration fell through to POST (create), and Clientify rejected it with 409 since the NIF was already a duplicate.
Fix:
Added taxpayer_identification_number => query to the search-field mapping in determine_search_by() (includes/crm-library/class-crmlib-clientify.php), so merging by NIF now correctly finds the existing contact via GET /contacts/?query= and updates it via PATCH instead of attempting a duplicate POST.
Tests:
Added to tests/API/test-clientify.php:
test_create_entry_v2_merge_nif_found_patched — existing NIF is found and updated via PATCH (no 409).
test_create_entry_v2_merge_nif_not_found_posted — new NIF is created via POST.
Regression guard in the POST mock: simulates the real 409 Clientify returns for a duplicate NIF, so this suite would catch a future regression in the determine_search_by() mapping.