CRM: Fix export crash when custom field name clashes with linked objects #38851

gogdzl · 2024-08-13T04:14:08Z

Fixes https://github.com/Automattic/zero-bs-crm/issues/3477

Proposed changes:

This PR fixes a fatal error that occurs when exporting contacts with a custom field that had the same name as a linked field (e.g., 'company').
The way this works is by adding a suffix to a key when there is a name clash, then before the export process uses custom field keys, it checks for clashes. The former is a manual check that has to be added in every place we may have a name clash.

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

https://github.com/Automattic/zero-bs-crm/issues/3477

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

Steps to replicate this error:

Add a custom text field named 'Company' to contacts
Add some information to this new custom field for at least one contact
Try to export the contact list ( wp-admin/admin.php?page=zbs-export-tools) - Make sure the custom field is selected

The export process will crash.
With this PR the crash no longer happens.

github-actions · 2024-08-13T04:15:20Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
🔴 Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Choose a review path based on your changes:
- A. Team Review: add the "[Status] Needs Team Review" label
  - For most changes, including minor cross-team impacts.
  - Example: Updating a team-specific component or a small change to a shared library.
- B. Crew Review: add the "[Status] Needs Review" label
  - For significant changes to core functionality.
  - Example: Major updates to a shared library or complex features.
- C. Both: Start with Team, then request Crew
  - For complex changes or when you need extra confidence.
  - Example: Refactor affecting multiple systems.
Get at least one approval before merging.

Still unsure? Reach out in #jetpack-developers for guidance!

projects/plugins/crm/includes/ZeroBSCRM.DAL3.Obj.Contacts.php

projects/plugins/crm/includes/ZeroBSCRM.DAL3.ObjectLayer.php

coder-karen

Thanks for tackling this! I left a few questions after looking specifically at the test failures.

projects/plugins/crm/includes/ZeroBSCRM.DAL3.Obj.Contacts.php

Previously this was installed as a dependency of mailcap, which was a dependency of mime-support, which was a dependency of various things. But apparently things depending on mime-support are transitioning to media-types which doesn't have that dependency. We need xz-utils for extracting `build.tar.xz` in CI, so make sure it's installed.

* Ensure all product cards on Jetpack Dashboard have icons when inactive * changelog * Add icon to boost install section * Fix AI missing icon when active * Move CRM back to where it was

* Remove connection nudges for cards that don't need it on dashboard * Ask for user connection on Blaze when not present and remove nudge on Akismet * changelog * Get rid of connection nudge for Search modules * Fix blaze tests

Co-authored-by: Phabricator Bot <[email protected]>

* Blocks: convert to a module Co-authored-by: Brad Jorsch <[email protected]>

* Properly close the modal when Cancel is clicked, cleaning up the state * Skip new generation on try again button when there are options already * Changelog

) `jetpack_register_social_logos()` and `jetpack_register_genericons` is called directly, and the files are [loaded](https://github.com/Automattic/jetpack/blob/8d8e0e6ef75b32c52029a57740cb6e13c6312296/projects/plugins/jetpack/class.jetpack.php#L735) on `wp_loaded` after the `init` hook has already run.

* i18n: Remove unnecessary filter to loading translations This filter has been moved to D162485-code * changelog * Add changelog files

* add prop to toggle button text * show button text Generate if the site has no logo set * add changelog * rename prop so it's not misleading

* Update CTA for connection related cards on At A Glance * changelog * Fix JS tests * Fix e2e test * Fix alignment issue on smaller screens * Fix e2e test * Fix cta spacing issue * Add border to connection cards * Fix e2e test * Make border transparent * Align content with title on connection cards

* Remove unused and non-working code. * Deprecate functions that are no longer needed to be removed later.

* Reapply overwritten changes * changelog * Fix import * Move stories to broken directory

* My Jetpack: update notice mobile style * changelog * Update notice style to the same when there's title

…then offer optional survey (#39485)

Initialize connection assets in the Story block to fix the block assets loading issue.

* Fix special chars display in SMP The Site Management Panel may break when special chars are included in the blog's name (quotes, ampersands, etc.). This commit fixes that. * Add ENT_QUOTES * Changelog * Use wp_specialchars_decode instead

Co-authored-by: Phabricator Bot <[email protected]>

Calypso has updated to node 22, which means it's time for us to do so as well.

… is required. (#39892)

Should be no functionality changes, unless some of the `react-hooks/` things were obscure bugs. * `curly` × 2: Missing `{}` around `if` bodies. * `eqeqeq` × 11: Use `===` and `!==` for comparisons. * `no-case-declarations` × 8: If a `case` has `const` or `let` inside, wrap it in a block for scoping. * `no-extra-boolean-cast` × 1: `if ( !! foo )` → `if ( foo )`. * `no-redeclare` × 2: A few useless `/* global JSON */`. * `no-undef` × 3: Remove some unused variables. * `no-unused-expressions` × 7: All ignored by updating `projects/plugins/jetpack/modules/.eslintrc.js`. * `no-useless-escape` × 4: Remove unnecessary `\`s in regexes. * `react-hooks/exhaustive-deps` × 3: Added missing deps. * One case needed a bit of refactoring. * `react-hooks/rules-of-hooks` × 5: Used `useSelect` and `useResizeObserver` unconditionally, and defined one that needed a conditional outside of `render()`. * `valid-typeof` × 2: Broken `undefined !== typeof window` check, changed to just use `window?` in later conditionals. * `@wordpress/no-global-active-element` × 2: Use `ownerDocument`.

…' as spelling errors. (#39880) * AI Assistant: Write Brief should not flag words like '2nd' and '100th' as spelling errors. * changelog * Typescript expects isNaN to be passed a number

* Update dependency svgicons2svgfont to v14 * Update svg-to-font for new svgicons2svgfont version --------- Co-authored-by: Renovate Bot <[email protected]> Co-authored-by: Brad Jorsch <[email protected]>

* Add changes to fix Verbum comments resize logic * changelog

We're waiting on updating to pnpm 9.12+ due to the regression of pnpm/pnpm#3935. Update `.engines.pnpm` so Renovate stops trying to use 9.12 and then failing the "Lock files are up to date" check.

… check (#39836) * Initial State: Replace isSocialImageGeneratorAvailable with feature check * Use constants instead of strings

Co-authored-by: Renovate Bot <[email protected]>

* decouple prompt input from prompt component, export it to make it available for other modals * allow passing down CTA button label as prop * implement AiModalPromptInput on GP image generation modal * changelog * fix wee css issue

Attempting to request a review from a bot account will likely fail, as bot accounts are not "collaborators". Fortunately a bot account is probably going to look like `@something[bot]`, so we can look for that to exclude them without having to make extra API queries.

…ort-crash-when-company-custom-field

… lot more difficult to fix the problem

gogdzl · 2024-10-25T19:09:22Z

Basically we are always calling add_name_clash_suffix_if_needed in our DAL layer, but we are using it only in fix_name_clash_if_needed which is only called in the export method.

This is not ideal, but I can't see other quick alternative as all the methods involved in this problem are huge and hard to read.

coder-karen

This works for me now, the changes look good.

cleacos

Nice. If it fixes the issue.

gogdzl added [Status] Needs Team Review [Plugin] CRM Issues about the Jetpack CRM plugin labels Aug 13, 2024

gogdzl added this to the crm/6.4.5 milestone Aug 13, 2024

gogdzl requested review from cleacos and coder-karen August 13, 2024 04:14

gogdzl self-assigned this Aug 13, 2024

gogdzl commented Aug 13, 2024

View reviewed changes

projects/plugins/crm/includes/ZeroBSCRM.DAL3.Obj.Contacts.php Show resolved Hide resolved

cleacos reviewed Aug 14, 2024

View reviewed changes

projects/plugins/crm/includes/ZeroBSCRM.DAL3.ObjectLayer.php Outdated Show resolved Hide resolved

coder-karen requested changes Aug 20, 2024

View reviewed changes

projects/plugins/crm/includes/ZeroBSCRM.DAL3.Obj.Contacts.php Show resolved Hide resolved

projects/plugins/crm/includes/ZeroBSCRM.DAL3.Obj.Contacts.php Outdated Show resolved Hide resolved

coder-karen added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Team Review labels Aug 20, 2024

anomiex and others added 18 commits October 25, 2024 15:29

Bump wc-calypso-bridge to 2.7.1 and add changelog (#39557)

b197708

Add/icons for all product cards on dashboard (#39513)

051048f

* Ensure all product cards on Jetpack Dashboard have icons when inactive * changelog * Add icon to boost install section * Fix AI missing icon when active * Move CRM back to where it was

Remove/connection nudges where not needed (#39533)

e5275d9

* Remove connection nudges for cards that don't need it on dashboard * Ask for user connection on Blaze when not present and remove nudge on Akismet * changelog * Get rid of connection nudge for Search modules * Fix blaze tests

phan: Update wpcom stubs (#39565)

f05e3b0

Co-authored-by: Phabricator Bot <[email protected]>

wpcom_add_shopping_cart: Use new is_cart_empty function (#39563)

6e3017f

Blocks: convert to module (#39449)

4bf833b

* Blocks: convert to a module Co-authored-by: Brad Jorsch <[email protected]>

Jetpack: Remove WAF deprecation notice (#39568)

4ea9683

Title Optimization: improve button behavior on error screen (#39554)

a17157f

* Properly close the modal when Cancel is clicked, cleaning up the state * Skip new generation on try again button when there are options already * Changelog

Remove/unnecessary textdomain filter (#39552)

ed2c5b6

* i18n: Remove unnecessary filter to loading translations This filter has been moved to D162485-code * changelog * Add changelog files

Jetpack AI Logo generator toolbar button label (#39550)

01cae1e

* add prop to toggle button text * show button text Generate if the site has no logo set * add changelog * rename prop so it's not misleading

Jetpack: remove unused infinite scroll code (#39570)

48effe7

* Remove unused and non-working code. * Deprecate functions that are no longer needed to be removed later.

Protect: Fix blockedRequests destructuring fallback (#39569)

4d374d2

Protect: Fix overwritten FirewallSubheading updates (#39571)

d80aed4

* Reapply overwritten changes * changelog * Fix import * Move stories to broken directory

My Jetpack: update notice style on mobile (#39537)

cce926e

* My Jetpack: update notice mobile style * changelog * Update notice style to the same when there's title

My Jetpack Welcome Flow: Show default recommendations upfront first, …

091d0f9

…then offer optional survey (#39485)

coder-karen and others added 16 commits October 25, 2024 15:29

General: indicate compatibility with upcoming WP 6.7 (#39786)

ed0c437

Blocks: initialize connection assets in Story block (#39870)

6081d78

Initialize connection assets in the Story block to fix the block assets loading issue.

Fix special chars display in SMP (#39756)

0ca758b

* Fix special chars display in SMP The Site Management Panel may break when special chars are included in the blog's name (quotes, ampersands, etc.). This commit fixes that. * Add ENT_QUOTES * Changelog * Use wp_specialchars_decode instead

phan: Update wpcom stubs (#39795)

95ebe43

Co-authored-by: Phabricator Bot <[email protected]>

Update to node 22 (#39886)

46e136a

Calypso has updated to node 22, which means it's time for us to do so as well.

Contact Info: Rearranging how the Contact Info block register process…

6c64126

… is required. (#39892)

AI Assistant: Write Brief should not flag words like '2nd' and '100th…

fca75ed

…' as spelling errors. (#39880) * AI Assistant: Write Brief should not flag words like '2nd' and '100th' as spelling errors. * changelog * Typescript expects isNaN to be passed a number

Update dependency svgicons2svgfont to v14 (#39893)

d3386eb

* Update dependency svgicons2svgfont to v14 * Update svg-to-font for new svgicons2svgfont version --------- Co-authored-by: Renovate Bot <[email protected]> Co-authored-by: Brad Jorsch <[email protected]>

Verbum Comments: Fix iframe resize logic (#39791)

748fce5

* Add changes to fix Verbum comments resize logic * changelog

Add <9.12.0 to .engines.pnpm (#39897)

ec82326

We're waiting on updating to pnpm 9.12+ due to the regression of pnpm/pnpm#3935. Update `.engines.pnpm` so Renovate stops trying to use 9.12 and then failing the "Lock files are up to date" check.

Initial State: Replace isSocialImageGeneratorAvailable with feature…

894d176

… check (#39836) * Initial State: Replace isSocialImageGeneratorAvailable with feature check * Use constants instead of strings

Changelog and readme.txt edits. (#39903)

ccd65f8

Update Eslint packages (#39898)

f232510

Co-authored-by: Renovate Bot <[email protected]>

gogdzl requested a review from a team as a code owner October 25, 2024 18:30

Merge remote-tracking branch 'origin/trunk' into fix/crm/3477-fix-exp…

063c6ac

…ort-crash-when-company-custom-field

gogdzl removed the request for review from a team October 25, 2024 18:33

gogdzl added 3 commits October 25, 2024 15:35

Make suffix shorter

e5ce479

Remove duplicate 'key´

775eb5d

Leave old key as this is not a problem and removing it will make it a…

c8e6376

… lot more difficult to fix the problem

gogdzl requested review from coder-karen and cleacos October 25, 2024 19:07

coder-karen approved these changes Oct 28, 2024

View reviewed changes

cleacos reviewed Oct 30, 2024

View reviewed changes

gogdzl merged commit 0c5ecc5 into trunk Oct 30, 2024
54 checks passed

gogdzl deleted the fix/crm/3477-fix-export-crash-when-company-custom-field branch October 30, 2024 13:00

github-actions bot removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Oct 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM: Fix export crash when custom field name clashes with linked objects #38851

CRM: Fix export crash when custom field name clashes with linked objects #38851

gogdzl commented Aug 13, 2024 •

edited

Loading

github-actions bot commented Aug 13, 2024 •

edited

Loading

coder-karen left a comment

gogdzl commented Oct 25, 2024

coder-karen left a comment

cleacos left a comment

CRM: Fix export crash when custom field name clashes with linked objects #38851

CRM: Fix export crash when custom field name clashes with linked objects #38851

Conversation

gogdzl commented Aug 13, 2024 • edited Loading

Proposed changes:

Other information:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Aug 13, 2024 • edited Loading

coder-karen left a comment

Choose a reason for hiding this comment

gogdzl commented Oct 25, 2024

coder-karen left a comment

Choose a reason for hiding this comment

cleacos left a comment

Choose a reason for hiding this comment

gogdzl commented Aug 13, 2024 •

edited

Loading

github-actions bot commented Aug 13, 2024 •

edited

Loading