Skip to content

Conversation

AlanWierzchonCA
Copy link
Contributor

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

Description

Fix for Issue: #14857
Bug: "Is Published" filter in Reports doesn't allow selecting "True" or "False"


📋 Steps to test this PR:

  1. Go to Reports and click "New" or edit an existing report.
  2. In the Data tab, add a filter.
  3. Choose the Is published column.
  4. Observe that while "equal" / "not equal" are available, there's no dropdown or input for selecting true or false.

@AlanWierzchonCA AlanWierzchonCA marked this pull request as ready for review April 23, 2025 09:36
Copy link

codecov bot commented Apr 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.68%. Comparing base (8e40e80) to head (b5124b7).
Report is 371 commits behind head on 5.2.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.2   #14909      +/-   ##
============================================
+ Coverage     63.27%   63.68%   +0.40%     
- Complexity    34619    34676      +57     
============================================
  Files          2272     2274       +2     
  Lines        103558   103729     +171     
============================================
+ Hits          65530    66058     +528     
+ Misses        38028    37671     -357     

see 91 files 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
Copy link
Member

Can you rebase this onto the 5.2 branch? We should have this fix in that version.

@matbcvo matbcvo added the needs-rebase PR's that need to be rebased label Apr 23, 2025
@AlanWierzchonCA AlanWierzchonCA changed the base branch from 7.x to 5.x April 24, 2025 12:48
@AlanWierzchonCA AlanWierzchonCA changed the base branch from 5.x to 7.x April 24, 2025 12:49
@matbcvo
Copy link
Contributor

matbcvo commented Apr 25, 2025

@AlanWierzchonCA, just a heads-up, it needs to be rebased onto the 5.2 branch, not 5.x. The 5.x branch is locked and doesn’t accept any PRs. We'll merge it into the 6.0 and 7.x branches later.

@AlanWierzchonCA AlanWierzchonCA force-pushed the DPMMA-3098_fix_report_data_bool_filter branch from 364aefa to 57f7984 Compare April 25, 2025 16:12
@AlanWierzchonCA AlanWierzchonCA changed the base branch from 7.x to 5.x April 25, 2025 16:12
@AlanWierzchonCA AlanWierzchonCA changed the base branch from 5.x to 5.2 April 25, 2025 16:13
@AlanWierzchonCA
Copy link
Contributor Author

@patrykgruszka @matbcvo rebased this onto the 5.2 branch

@matbcvo matbcvo added this to the 5.2.5 milestone Apr 25, 2025
@matbcvo matbcvo added bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test code-review-needed PR's that require a code review before merging and removed needs-rebase PR's that need to be rebased labels Apr 25, 2025
@matbcvo
Copy link
Contributor

matbcvo commented Apr 26, 2025

This fix makes the boolean appear, but there’s another issue - the boolean value is not being saved. I checked the database and confirmed that the boolean value is not being saved or updated. For example, the "Dynamic?" boolean works correctly.

Screen.Recording.2025-04-26.180515.mp4

@matbcvo matbcvo added the pending-feedback PR's and issues that are awaiting feedback from the author label Apr 28, 2025
@matbcvo matbcvo modified the milestones: 5.2.5, 5.2.6 Apr 28, 2025
@Hugo-Prossaird
Copy link
Contributor

I have tested this PR locally, and everything works correctly with the latest commit ✅

@matbcvo matbcvo 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 Apr 29, 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.

Faced the same issue. And this fixed it 👍

Copy link

@kingsedem kingsedem left a comment

Choose a reason for hiding this comment

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

Tested and works as expected

image

@kingsedem kingsedem removed the pending-test-confirmation PR's that require one test before they can be merged label Apr 29, 2025
@matbcvo matbcvo 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 labels Apr 29, 2025
@matbcvo matbcvo moved this to 🎉 Ready to commit in Open Source Fridays Apr 29, 2025
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.

This approach will cause ID conflicts, I would suggest to try with something like this:

if (filterType == 'bool' || filterType == 'boolean') {
  const yesId = valueId + '_1';
  const noId = valueId + '_0';
  const isYes = valueVal == '1';
  const $template = mQuery(mQuery('#filterValueYesNoTemplate').html());
  const $label = $template.find('#report_value_template_yesno_label');
  const $yesOption = $template.find('#report_value_template_yesno_1');
  const $noOption = $template.find('#report_value_template_yesno_0');

  $yesOption.attr('name', valueName)
    .attr('id', yesId);
  $noOption.attr('name', valueName)
    .attr('id', noId);
  $label.attr('id', valueId + '_bool-label')
    .attr('data-yes-id', yesId)
    .attr('data-no-id', noId);

  mQuery(valueEl).replaceWith($template);

  if (!isYes) {
    Mautic.toggleYesNo($label);
  }
} else if // ....

@github-project-automation github-project-automation bot moved this from 🎉 Ready to commit to 🚨 Developer revision needed in Open Source Fridays Apr 29, 2025
@matbcvo matbcvo added pending-feedback PR's and issues that are awaiting feedback from the author and removed ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged labels Apr 29, 2025
@AlanWierzchonCA AlanWierzchonCA force-pushed the DPMMA-3098_fix_report_data_bool_filter branch 2 times, most recently from 26a6401 to c2f0b78 Compare April 29, 2025 12:26
@AlanWierzchonCA
Copy link
Contributor Author

@kuzmany , @matbcvo I made changes as suggested by @patrykgruszka

@AlanWierzchonCA AlanWierzchonCA force-pushed the DPMMA-3098_fix_report_data_bool_filter branch from 69ad29b to b5124b7 Compare April 29, 2025 13:21
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.

Looks fine now, thanks!

@patrykgruszka patrykgruszka moved this from 🚨 Developer revision needed to ⏳︎ Needs 1 more test in Open Source Fridays Apr 29, 2025
@patrykgruszka patrykgruszka added pending-test-confirmation PR's that require one test before they can be merged and removed pending-feedback PR's and issues that are awaiting feedback from the author user-testing-passed PRs which have been successfully tested by the required number of people. labels Apr 29, 2025
Copy link
Contributor

@shinde-rahul shinde-rahul 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 changes look good

@shinde-rahul
Copy link
Contributor

Once this change is applied, I can toggle the boolean for the isPublished column under filters.

image

@shinde-rahul shinde-rahul 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 May 5, 2025
@shinde-rahul shinde-rahul moved this from ⏳︎ Needs 1 more test to 🎉 Ready to commit in Open Source Fridays May 5, 2025
@escopecz escopecz merged commit 8e87a2a into mautic:5.2 May 20, 2025
17 checks passed
@github-project-automation github-project-automation bot moved this from 🎉 Ready to commit to 🥳 Done in Open Source Fridays May 20, 2025
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 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.

Bug: "Is Published" filter in Reports doesn't allow selecting "True" or "False"
8 participants