Skip to content

Conversation

putzwasser
Copy link
Contributor

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 mautic/user-documentation#...
Related developer documentation PR URL mautic/developer-documentation-new#...
Issue(s) addressed Fixes #...

Description

This PR allows the anniversary date filter in combination with a datetime field. This would allow it to use anniversary along with the existing date_identified property.


📋 Steps to test this PR:

  1. Modify the date_identified of an existing contact in PHPMyAdmin and set it to the current date one year ago.
  2. Create a segment filtering Date Identified - equal anniversary
    image
  3. Run php bin/console mautic:segments:rebuild
  4. Nothing happens
  5. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  6. Run php bin/console mautic:segments:rebuild again. The contact gets added to the segment.

Copy link

codecov bot commented Jun 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.17%. Comparing base (968aec7) to head (b81615a).
Report is 2 commits behind head on 5.1.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.1   #13871      +/-   ##
============================================
- Coverage     62.18%   62.17%   -0.01%     
  Complexity    34220    34220              
============================================
  Files          2252     2252              
  Lines        102328   102328              
============================================
- Hits          63628    63627       -1     
- Misses        38700    38701       +1     
Files Coverage Δ
...e/Segment/Decorator/Date/Other/DateAnniversary.php 54.54% <100.00%> (ø)

... and 1 file with indirect coverage changes

@andersonjeccel andersonjeccel added T2 Medium difficulty to fix (issue) or test (PR) ready-to-test PR's that are ready to test code-review-needed PR's that require a code review before merging enhancement Any improvement to an existing feature or functionality segments Anything related to segments labels Jun 21, 2024
@escopecz
Copy link
Member

Please add a functional test that creates the segment that would show the problem before the fix and will pass with the fix.

@escopecz escopecz added the needs-automated-tests PR's that need automated tests before they can be merged label Jun 28, 2024
Copy link
Contributor

@oltmanns-leuchtfeuer oltmanns-leuchtfeuer left a comment

Choose a reason for hiding this comment

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

Tested with "date_identified" and a custom field of type "date/time" and it worked as expected. Thanks for the fix. :)

@oltmanns-leuchtfeuer oltmanns-leuchtfeuer added pending-test-confirmation PR's that require one test before they can be merged and removed ready-to-test PR's that are ready to test labels Jun 28, 2024
@putzwasser
Copy link
Contributor Author

Please add a functional test that creates the segment that would show the problem before the fix and will pass with the fix.

I'm not quite sure how I would do this/what you mean exactly.

The fix formats the date parameter for SQL in %-m-d%. The percent sign % represents zero, one, or multiple characters. So, a test with just a date still would pass. I'm sure you know that, and I didn't get what you really meant.

The only additional test I could come up with would be something like this

        $matchingDateOnlyResult = '%'.(new \DateTime('now', new \DateTimeZone('UTC')))->format('-m-d');
        $this->assertNotEquals($matchingDateOnlyResult, $filterDecorator->getParameterValue($contactSegmentFilterCrate));

This additional test seems redundant to me as it basically tests the same thing as the test that is already there.

Could you please clarify what additional test you would like to see?

@escopecz
Copy link
Member

escopecz commented Jul 1, 2024

@putzwasser the test should do the same steps that you have in your "steps to test" section. Does it make sense?

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.

This is the same fix as #14010 which was approved twice. I'm merging this as 3 times approved.

@escopecz escopecz added bug Issues or PR's relating to bugs code-review-passed PRs which have passed code review and removed 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 enhancement Any improvement to an existing feature or functionality needs-automated-tests PR's that need automated tests before they can be merged labels Jul 26, 2024
@escopecz escopecz added this to the 5.1.1 milestone Jul 26, 2024
@escopecz escopecz merged commit 49b3c4d into mautic:5.1 Jul 26, 2024
escopecz added a commit that referenced this pull request Jul 26, 2024
* Improve grammar for unhide (#13835)

* Improve grammar for unhide

* Fix test

* fix: LeadBundle template errors (#13862)

Co-authored-by: Ruth Cheesley <ruth@ruthcheesley.co.uk>

* [UI] fix CSS flexbox broken in campaign insert clone view (#13878)

* [UI] Replaces table action button `arrow-down` with `more-2` icon (#13841)

* [UI] Replaces table action button `arrow-down` with `more-2` icon

This replaces the icon for the action toggle button in tables (e.g. contact list table or segment list table) with a 3 dot icon.

The previous arrow down icon indicates that an collapsed element would get unfolded/shown. This is not the case in these tables. The button shows more actions. In any UI that I know a 3 dot icon is used for that.

The arrow down icon would be more suitable if it would show more details on click. Hence, this PR.

* TASK: Reverts change for `page_actions.html.twig`

Co-authored-by: Anderson, from Dropsolid <116097999+andersonjeccel@users.noreply.github.com>

---------

Co-authored-by: Anderson, from Dropsolid <116097999+andersonjeccel@users.noreply.github.com>

* add js code to hide profile pic if missing (#13838)

* Segment membership as a new filter for dynamic email content (#13528)

* Merge pull request #1088 from mautic-inc/MAUT-4688

Maut 4688 - Use segment membership as a filter for dynamic content in email

* Changes needed after rebase to M5

* Merge pull request #1407 from acquia/MAUT-5729

MAUT-5729 Dynamic Content Error - Segment Membership filter not showing correct content

* Fixing STAN

* CS fixes

* Test fix

* test fix part 2

* Adding back JS methods removed in a bad conflict resolution

---------

Co-authored-by: Lukáš Drahý <lukas@drahy.net>
Co-authored-by: Rohit Pavaskar <66303837+rohitp19@users.noreply.github.com>
Co-authored-by: Ruth Cheesley <ruth@ruthcheesley.co.uk>

* [UI] Increased icon size (#13825)

* increased icon size

* decrease by 1px

* Make the search regular expression match both "&" and "&amp;" for link replacement within an email body (#13858)

* Guide users to build optimized segments (#13860)

* Merge pull request #2172 from acquia/MAUT-11343

MAUT-11343 : Guide users to build optimized segments

* fix twig file issue

* Apply suggestions from code review

Co-authored-by: John Linhart <admin@escope.cz>

* push suggested changes

---------

Co-authored-by: John Linhart <admin@escope.cz>

* fix CSS flexbox broken in campaign insert clone view

* Revert "[UI] Replaces table action button `arrow-down` with `more-2` icon (#13841)"

This reverts commit fadcf26.

* Revert "Segment membership as a new filter for dynamic email content (#13528)"

This reverts commit 71031b7.

* Revert "[UI] Increased icon size (#13825)"

This reverts commit c00df3c.

* Revert "Make the search regular expression match both "&" and "&amp;" for link replacement within an email body (#13858)"

This reverts commit d3e6ceb.

* Revert "Guide users to build optimized segments (#13860)"

This reverts commit bdabc26.

* Revert "add js code to hide profile pic if missing (#13838)"

This reverts commit 76dd7d6.

---------

Co-authored-by: putzwasser <26040044+putzwasser@users.noreply.github.com>
Co-authored-by: John Linhart <admin@escope.cz>
Co-authored-by: Lukáš Drahý <lukas@drahy.net>
Co-authored-by: Rohit Pavaskar <66303837+rohitp19@users.noreply.github.com>
Co-authored-by: Ruth Cheesley <ruth@ruthcheesley.co.uk>
Co-authored-by: Miroslav Fedeleš <miroslav.fedeles@gmail.com>
Co-authored-by: Saurabh Gupta <48244990+dadarya0@users.noreply.github.com>

* DPMMA-1020 Fix search email with special characters in campaign action (#10306)

* fix: [DPMMA-1020] fix getLookupChoiceListAction for search with special characters

* fix: [DPMMA-1020] phpstan

* fix: focus item published (#13944)

* Update .gitpod.Dockerfile 5.1 branch (#13955)

* fix [DPMMA-2661] mapped field form 5.1 (#13938)

* fix: [DPMMA-2661] Correct form mapper Mautic 5

* feat: [DPMMA-2661] Correct CSFixer

* FIX: Removes onConfigSave which invokes htmlspecialchars and escapes tracking script (#13859)

This fixes #13355

This removes the HTML escaping logic in the onConfigSave method of `ConfigSubscriber`. This change prevents the UI from displaying escaped HTML in the custom tracking JS field after saving the form.

Previously, the form erroneously escaped HTML entities. By removing this code, users will now see the original HTML as expected when they revisit the form.

The main issue, however, was that re-saving  meant saving escaped HTML which would break inserting the tracking HTML code.

**Before**
1. Add custom HTML in `Configuration > Landing Page Settings > Analytics script (i.e. Google Analytics) `
2. Save
3. Open this view again → see escaped HTML
4. Save again
5. Open a landing page and see escaped HTML tracking code injected.

**After**
1. Add custom HTML in `Configuration > Landing Page Settings > Analytics script (i.e. Google Analytics) `
2. Save
3. Open this view again → see unescaped HTML
4. Save again
5. Open a landing page and see correct HTML tracking code injected.

* rename from tweet to tweets (#13967)

* Fix: Create custom fields for lookup list. (#13946)

* FIX: Makes `anniversary` date filter compatible with datetime (#13871)

* FIX: Makes `anniversary` date filter compatible with datetime

* TASK: Fix unit test

* FIX: $expectedResult docstring

---------

Co-authored-by: Ruth Cheesley <ruth@ruthcheesley.co.uk>
Co-authored-by: Frettyl <173093231+Frettyl@users.noreply.github.com>
Co-authored-by: Anderson, from Dropsolid <116097999+andersonjeccel@users.noreply.github.com>
Co-authored-by: putzwasser <26040044+putzwasser@users.noreply.github.com>
Co-authored-by: Lukáš Drahý <lukas@drahy.net>
Co-authored-by: Rohit Pavaskar <66303837+rohitp19@users.noreply.github.com>
Co-authored-by: Miroslav Fedeleš <miroslav.fedeles@gmail.com>
Co-authored-by: Saurabh Gupta <48244990+dadarya0@users.noreply.github.com>
Co-authored-by: Patryk Gruszka <patryk.gruszka@comarch.pl>
Co-authored-by: Tomasz Kowalczyk <39382654+tomekkowalczyk@users.noreply.github.com>
Co-authored-by: Martin Vooremäe <martin.vooremae@gmail.com>
Co-authored-by: Abhisek Mazumdar <abhisekmazumdar@users.noreply.github.com>
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 code-review-passed PRs which have passed code review segments Anything related to segments T2 Medium difficulty to fix (issue) or test (PR)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants