Skip to content
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.

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

Segment - Fix filter URL Visited with regex and more #11541

Merged
merged 5 commits into from
Oct 10, 2022

Conversation

Cr7t3K
Copy link
Contributor

@Cr7t3K Cr7t3K commented Oct 5, 2022

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

Description:

This PR fixes the problem with regex filters, like the visited url.
The filter did not take into account the regex, and display the same number of contacts no matter the regex.

Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Login
  3. Create two segments with filter "Visited URL" and "regexp", but not same expression (depends on your current data in table page_hits)
  4. Update your two segments with command
  5. You will see that the filters work
  6. Enjoy

PR funded by @PierreAmmeloot from WebAnyOne

@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Oct 5, 2022
@npracht npracht added bug Issues or PR's relating to bugs code-review-needed PR's that require a code review before merging pending-test-confirmation PR's that require one test before they can be merged segments Anything related to segments labels Oct 5, 2022
@npracht npracht linked an issue Oct 5, 2022 that may be closed by this pull request
1 task
Copy link
Sponsor 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 added a small request before we merge this PR. Also, please create a duplicate PR against the 5.x branch to confirm it will work there too and all conflicts will be solved. Thanks for the functional tests! It makes me a lot more confident that it works!

@escopecz escopecz added the pending-feedback PR's and issues that are awaiting feedback from the author label Oct 7, 2022
@Cr7t3K Cr7t3K requested a review from escopecz October 7, 2022 16:06
@codecov
Copy link

codecov bot commented Oct 7, 2022

Codecov Report

Merging #11541 (80e8f5c) into 4.4 (4c947e9) will increase coverage by 0.47%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                4.4   #11541      +/-   ##
============================================
+ Coverage     49.54%   50.02%   +0.47%     
- Complexity    35405    35406       +1     
============================================
  Files          2144     2144              
  Lines        105542   105550       +8     
============================================
+ Hits          52296    52806     +510     
+ Misses        53246    52744     -502     
Impacted Files Coverage Δ
...s/LeadBundle/Segment/ContactSegmentFilterCrate.php 96.55% <100.00%> (+0.12%) ⬆️
...nt/Query/Filter/ForeignValueFilterQueryBuilder.php 73.97% <100.00%> (+10.59%) ⬆️
...Bundle/EventListener/GeneratedColumnSubscriber.php 92.30% <0.00%> (-7.70%) ⬇️
...p/bundles/InstallBundle/Command/InstallCommand.php 70.55% <0.00%> (-0.73%) ⬇️
...es/IntegrationsBundle/Sync/SyncJudge/SyncJudge.php 0.00% <0.00%> (ø)
...dles/CoreBundle/Templating/Helper/AssetsHelper.php 72.40% <0.00%> (+2.86%) ⬆️
...dles/InstallBundle/Configurator/Step/CheckStep.php 58.33% <0.00%> (+4.16%) ⬆️
app/bundles/ReportBundle/Entity/Report.php 81.18% <0.00%> (+4.95%) ⬆️
...ugins/GrapesJsBuilderBundle/Integration/Config.php 66.66% <0.00%> (+13.33%) ⬆️
... and 21 more

@escopecz escopecz added this to the 4.4.4 milestone Oct 10, 2022
@escopecz escopecz removed the pending-feedback PR's and issues that are awaiting feedback from the author label Oct 10, 2022
Copy link
Sponsor 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.

Thank you! 👍

@escopecz escopecz added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged labels Oct 10, 2022
@escopecz
Copy link
Sponsor Member

@Cr7t3K create please also PRs with these commits against the 5.x branch. It may need rebase or cherry-pick. See 5.x...Cr7t3K:fix-segment-filter-regex

@escopecz escopecz removed the code-review-needed PR's that require a code review before merging label Oct 10, 2022
@escopecz
Copy link
Sponsor Member

Aha, you already did. #11556 Thank you!

@escopecz escopecz merged commit a56ebce into mautic:4.4 Oct 10, 2022
escopecz added a commit to escopecz/mautic that referenced this pull request Nov 2, 2022
escopecz added a commit that referenced this pull request Nov 7, 2022
* Fix import company error message without unique fields

* Fix PHP stan

* Change clean conditional values  with InputHelper::clean

* Fix unit test

* Fix error with regex operator

* Create functional test for filter with regex

* Add test for segment filter with object behaviors

* Change the regex of test suit for match with MariaDb 10.3 and MySQL 5.7

* Update SpoolTransport.php

* Removing diff from interval and adding initially

* Adding typehint to test

* CS Fixer changes

* Removing Adding interval to grouped contacts

* CS fixes

* Dynamic Content filters fixed

* Dynamic Content filters fixed - return type of test methods specified

* Company city filter test with lead and empty company

* Removed unnecessary change in error reporting in test

* Fix day graph range

* Updating queries in the test for #11541

Co-authored-by: Zdeno Kuzmany <zdeno@kuzmany.biz>
Co-authored-by: Zdeno Kuzmany <zku@webmecanik.com>
Co-authored-by: Guillaume <cguillaumedu85@orange.fr>
Co-authored-by: Tony Eckstrand <teckstrand@lawdepot.com>
Co-authored-by: jazeabby <abhishek.jain@acquia.com>
Co-authored-by: Anna Munk <anna.munk@comarch.pl>
mautibot pushed a commit to mautic/core-lib that referenced this pull request Nov 7, 2022
* Fix import company error message without unique fields

* Fix PHP stan

* Change clean conditional values  with InputHelper::clean

* Fix unit test

* Fix error with regex operator

* Create functional test for filter with regex

* Add test for segment filter with object behaviors

* Change the regex of test suit for match with MariaDb 10.3 and MySQL 5.7

* Update SpoolTransport.php

* Removing diff from interval and adding initially

* Adding typehint to test

* CS Fixer changes

* Removing Adding interval to grouped contacts

* CS fixes

* Dynamic Content filters fixed

* Dynamic Content filters fixed - return type of test methods specified

* Company city filter test with lead and empty company

* Removed unnecessary change in error reporting in test

* Fix day graph range

* Updating queries in the test for mautic/mautic#11541

Co-authored-by: Zdeno Kuzmany <zdeno@kuzmany.biz>
Co-authored-by: Zdeno Kuzmany <zku@webmecanik.com>
Co-authored-by: Guillaume <cguillaumedu85@orange.fr>
Co-authored-by: Tony Eckstrand <teckstrand@lawdepot.com>
Co-authored-by: jazeabby <abhishek.jain@acquia.com>
Co-authored-by: Anna Munk <anna.munk@comarch.pl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs cla-signed The PR contributors have signed the contributors agreement ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged segments Anything related to segments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segment with URL visited and regexp don't filter
4 participants