Skip to content

Conversation

biozshock
Copy link
Contributor

@biozshock biozshock commented Feb 12, 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? ✔️

Description

This is another approach to the PR #13226

The issue:
Currently custom filters can not be applied to tokens, that are actually strings, like {dwc=slot-name}, because the current code does not execute ContactFiltersEvaluateEvent in that case, but replaces tokens with an empty string.


📋 Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Install the bundle https://github.com/Leuchtfeuer/mautic-DwcDeviceType-bundle , applying the PR [MTC-6662] Change DWC filter "Device Type" to work without core patch Leuchtfeuer/mautic-DwcDeviceType-bundle#4 and ignoring the patch note about the Enhances filters evaluation for DWCs  #13226
  3. Go to "Dynamic content" and create a non-campaign slot for desktop devices:
    2025-02-12_23-55
    2025-02-12_23-56
  4. Also create a non-campaign for non-desktop devices:
    2025-02-12_23-56_1
    2025-02-12_23-57
  5. Create a page with both types of dynamic content: the HTML data-slot="dwc" and "inline text DWC" {dwc=slot-name}
    2025-02-12_23-58
    2025-02-13_00-01
<div data-slot="dwc" data-param-slot-name="landing_desktop_slot">
  <h1>Default web content for desktop.
  </h1>
</div>
<div data-slot="dwc" data-param-slot-name="landing_mobile_slot">
  <h1>Default web content for mobile
  </h1>
</div>
  1. Check the resulting page in incognito mode, or another browser contains both "desktop" slots (or non-desktop, if you are testing on mobile). Example for a desktop:
    2025-02-13_00-02

If the PR is not applied the result is following:
2025-02-13_00-10

  1. Check the contact event log (you should enable anonymous contacts in the list to see new results):
    On the screenshot below the first entry (3 rows in the bottom) are log entries when the PR is active, and the rest (last four entries) are when the PR is not active.
    2025-02-13_01-05

  2. You could also create additional slots to check if the previous functionality works as expected.

Copy link

codecov bot commented Feb 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.70%. Comparing base (02be5d1) to head (d933e4d).
Report is 83 commits behind head on 5.2.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.2   #14599      +/-   ##
============================================
- Coverage     63.70%   63.70%   -0.01%     
+ Complexity    34684    34682       -2     
============================================
  Files          2274     2274              
  Lines        103771   103766       -5     
============================================
- Hits          66111    66106       -5     
  Misses        37660    37660              
Files with missing lines Coverage Δ
...tBundle/EventListener/DynamicContentSubscriber.php 83.33% <100.00%> (-1.09%) ⬇️
...namicContentBundle/Helper/DynamicContentHelper.php 79.76% <100.00%> (ø)
🚀 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.

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.

Just tested and does exactly what it should. LGTM! :)

@escopecz escopecz added 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 dynamic-content labels Feb 14, 2025
Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

We build fix for this issue #14959
But this works too 👍

@kuzmany kuzmany added code-review-passed PRs which have passed code review ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged 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 labels May 14, 2025
@kuzmany kuzmany moved this from ⏳︎ Needs 1 more test to 🎉 Ready to commit in Open Source Fridays May 14, 2025
@escopecz escopecz merged commit 749e69b into mautic:5.2 Jun 16, 2025
26 of 32 checks passed
@github-project-automation github-project-automation bot moved this from 🎉 Ready to commit to 🥳 Done in Open Source Fridays Jun 16, 2025
@kuzmany kuzmany added this to the 5.2.7 milestone Jun 30, 2025
@kuzmany kuzmany added the enhancement Any improvement to an existing feature or functionality label Jun 30, 2025
@kuzmany kuzmany changed the title Allow external plugins to replace "{dwc" tokens with custom filter evaluation. Fix dynamic content token replacement for external plugins Jun 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-review-passed PRs which have passed code review dynamic-content enhancement Any improvement to an existing feature or functionality ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged
Projects
Status: 🥳 Done
Development

Successfully merging this pull request may close these issues.

4 participants