Skip to content

Fix issue #14457: Contact names with ampersands not showing in search #14818

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Apr 4, 2025
Merged

Fix issue #14457: Contact names with ampersands not showing in search #14818

merged 3 commits into from
Apr 4, 2025

Conversation

goma101
Copy link
Contributor

@goma101 goma101 commented Mar 31, 2025

Q A
Bug fix? (use the a.b branch) ✔️
New feature/enhancement? (use the a.x branch) ❌
Deprecations? ❌
BC breaks? (use the c.x branch) ❌
Automated tests included? ✔️
Related user documentation PR URL N/A
Related developer documentation PR URL N/A
Issue(s) addressed Fixes #14457

Description

This PR fixes an issue where a global search would not return contacts (or other entities) with ampersands in one of their fields such as their names, if searched with the ampersand. This bug occurred because the InputHelper::clean method encodes characters such as ampersand into html encoding, thus causing a mismatch when executing a global search.

This fix decodes search strings before global search execution, thereby eliminating this mismatch and yielding correct results. The PR also includes a phpunit test, for the search example provided in the issue (Global search for Contact with name "R&D"), asserting the search is successful.

Before After
before after

📋 Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Create a contact or other entity with "&" in one of its fields
  3. Execute a global search for the contact/entity by that field, including the & in the search
  4. Confirm that the results are correct.

@goma101
fix mautic#14457: decode special characters in searches
32c2658 
fixes searches for fields with characters otherwise encoded like
ampersand

adds test to check ampersand shows in search, on created contact
@Copilot Copilot AI review requested due to automatic review settings March 31, 2025 10:51
Copilot
Copilot AI reviewed Mar 31, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

Files not reviewed (20)
  • app/bundles/ApiBundle/EventListener/SearchSubscriber.php: Language not supported
  • app/bundles/AssetBundle/EventListener/SearchSubscriber.php: Language not supported
  • app/bundles/CampaignBundle/EventListener/SearchSubscriber.php: Language not supported
  • app/bundles/ChannelBundle/EventListener/SearchSubscriber.php: Language not supported
  • app/bundles/DynamicContentBundle/EventListener/SearchSubscriber.php: Language not supported
  • app/bundles/EmailBundle/EventListener/SearchSubscriber.php: Language not supported
  • app/bundles/FormBundle/EventListener/SearchSubscriber.php: Language not supported
  • app/bundles/LeadBundle/EventListener/SearchSubscriber.php: Language not supported
  • app/bundles/LeadBundle/Tests/Functional/SearchTestHelper.php: Language not supported
  • app/bundles/LeadBundle/Tests/Functional/SearchWithCustomFieldDataFunctionalTest.php: Language not supported
  • app/bundles/LeadBundle/Tests/Functional/SearchWithSpecialCharactersInFieldTest.php: Language not supported
  • app/bundles/NotificationBundle/EventListener/SearchSubscriber.php: Language not supported
  • app/bundles/PageBundle/EventListener/SearchSubscriber.php: Language not supported
  • app/bundles/PointBundle/EventListener/SearchSubscriber.php: Language not supported
  • app/bundles/ReportBundle/EventListener/SearchSubscriber.php: Language not supported
  • app/bundles/SmsBundle/EventListener/SearchSubscriber.php: Language not supported
  • app/bundles/StageBundle/EventListener/SearchSubscriber.php: Language not supported
  • app/bundles/UserBundle/EventListener/SearchSubscriber.php: Language not supported
  • plugins/MauticFocusBundle/EventListener/SearchSubscriber.php: Language not supported
  • plugins/MauticTagManagerBundle/EventListener/SearchSubscriber.php: Language not supported

patrykgruszka
patrykgruszka requested changes Apr 2, 2025
View reviewed changes
Copy link
Member

@patrykgruszka patrykgruszka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest removing the InputHelper::clean in the controller instead of decoding the query in every subscriber.

@patrykgruszka patrykgruszka added bug Issues or PR's relating to bugs search Issues relating to the search facility in Mautic labels Apr 2, 2025
@goma101
fix mautic#14457: remove InputHelper::clean from global search action
1693034 
reverts previous individual search subscriber modification, removes
clean method from global search action in ajax controller
@goma101
Copy link
Contributor Author

goma101 commented Apr 2, 2025

@patrykgruszka Indeed much more sensical, my bad.

Please tell me if it needs anything else, or if you'd like me to squash or something.

@patrykgruszka patrykgruszka self-requested a review April 2, 2025 16:57
@codecov Codecov
Copy link

codecov bot commented Apr 2, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.75%. Comparing base (12f062e) to head (bae0473).
Report is 4 commits behind head on 6.0.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##                6.0   #14818   +/-   ##
=========================================
  Coverage     64.75%   64.75%           
  Complexity    34728    34728           
=========================================
  Files          2275     2275           
  Lines        103739   103739           
=========================================
+ Hits          67171    67172    +1     
+ Misses        36568    36567    -1
Files with missing lines Coverage Δ
...p/bundles/CoreBundle/Controller/AjaxController.php 30.25% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

patrykgruszka
patrykgruszka reviewed Apr 3, 2025
View reviewed changes
@patrykgruszka patrykgruszka added the ready-to-test PR's that are ready to test label Apr 3, 2025
@patrykgruszka patrykgruszka moved this to 🦸🏻 Needs 2 tests in Open Source Fridays Apr 3, 2025
@goma101 goma101 requested a review from patrykgruszka April 3, 2025 08:45
@goma101
refactor SearchTestHelper into AbstractSearchTest
bae0473 
renames SearchTestHelper and moves auxiliary functions from the subclass
to the parent
@goma101 goma101 force-pushed the fix-ampersands-not-found-in-search branch from 9bb65e4 to bae0473 Compare April 3, 2025 14:33
patrykgruszka
patrykgruszka approved these changes Apr 3, 2025
View reviewed changes
Copy link
Member

@patrykgruszka patrykgruszka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks fine and fixes the problem 👍
image

@patrykgruszka patrykgruszka moved this from 🦸🏻 Needs 2 tests to ⏳︎ Needs 1 more test in Open Source Fridays Apr 3, 2025
@patrykgruszka patrykgruszka added pending-test-confirmation PR's that require one test before they can be merged code-review-passed PRs which have passed code review and removed ready-to-test PR's that are ready to test labels Apr 3, 2025
@patrykgruszka patrykgruszka reopened this Apr 4, 2025
@github-project-automation github-project-automation bot moved this from ⏳︎ Needs 1 more test to 🥳 Done in Open Source Fridays Apr 4, 2025
@patrykgruszka patrykgruszka moved this from 🥳 Done to ⏳︎ Needs 1 more test in Open Source Fridays Apr 4, 2025
@escopecz escopecz added this to the 6.0.1 milestone Apr 4, 2025
escopecz
escopecz approved these changes Apr 4, 2025
View reviewed changes
Copy link
Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also believe that removing InputHelper::clean in this case is the right way to solve this 👍

@escopecz escopecz added user-testing-passed PRs which have been successfully tested by the required number of people. and removed pending-test-confirmation PR's that require one test before they can be merged labels Apr 4, 2025
@escopecz escopecz merged commit 6397525 into mautic:6.0 Apr 4, 2025
32 checks passed
@github-project-automation github-project-automation bot moved this from ⏳︎ Needs 1 more test to 🥳 Done in Open Source Fridays Apr 4, 2025
@escopecz escopecz mentioned this pull request Apr 4, 2025
Closed
1 task
@matbcvo
Copy link
Contributor

matbcvo commented Apr 4, 2025

@all-contributors please add @goma101 for code test

@allcontributors allcontributors bot mentioned this pull request Apr 4, 2025
Merged
@allcontributors AllContributors
Copy link
Contributor

@matbcvo

I've put up a pull request to add @goma101! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@escopecz escopecz escopecz approved these changes

@patrykgruszka patrykgruszka patrykgruszka approved these changes

Assignees
No one assigned
Labels
bug Issues or PR's relating to bugs code-review-passed PRs which have passed code review search Issues relating to the search facility in Mautic user-testing-passed PRs which have been successfully tested by the required number of people.
Projects
Open Source Fridays
Status: 🥳 Done
Milestone
6.0.1
Development

Successfully merging this pull request may close these issues.
4 participants
@goma101 @matbcvo @escopecz @patrykgruszka