Skip to content

Fix of disabling the Dashboard widget cache #14467

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 4 commits into from
Mar 12, 2025
Merged

Fix of disabling the Dashboard widget cache #14467

merged 4 commits into from
Mar 12, 2025

Conversation

JonasLudwig1998
Copy link
Contributor

@JonasLudwig1998 JonasLudwig1998 commented Jan 16, 2025 •
edited
Loading

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 solves the issue #14017. If changing the cached data timeout in the system settings to 0, the cache for dashboard widgets should be disabled (according to the description), however it results in an endless cache. Instead, disabling the cache works when setting the cached data timeout to -1. The logic was now changed so that 0 results in disabled cache. Also it's ensured that you can just enter full numbers greater than or equal 0 in the cached data timeout field (System Settings -> Pagination and data display --> Cached data timeout)

📋 Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Set cached data timeout to 0 and check if dashboard widgets get updated immediately after reloading
  3. Try to set the cached data timeout to -1 and check if you get an error message
  4. Try to leave the cached data timeout field empty and check if you get an error message

@RCheesley
Copy link
Member

I think we should also make some docs updates about this too, even if it's just to explain the numbers to use?

@escopecz
Copy link
Member

Or shall we update the functionality so it would work according to the tooltip?

@escopecz escopecz added the pending-feedback PR's and issues that are awaiting feedback from the author label Jan 21, 2025
@JonasLudwig1998
Copy link
Contributor Author

So, the functionality should be changed so that 0 results in disabled cache?

@escopecz
Copy link
Member

Yes, I think so.

@codecov Codecov
Copy link

codecov bot commented Mar 10, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 63.65%. Comparing base (4035b79) to head (b6828b0).
Report is 141 commits behind head on 5.2.

Files with missing lines Patch % Lines
...p/bundles/DashboardBundle/Model/DashboardModel.php 75.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.2   #14467      +/-   ##
============================================
+ Coverage     63.45%   63.65%   +0.19%     
- Complexity    34636    34659      +23     
============================================
  Files          2273     2273              
  Lines        103603   103691      +88     
============================================
+ Hits          65741    66002     +261     
+ Misses        37862    37689     -173
Files with missing lines Coverage Δ
app/bundles/CoreBundle/Form/Type/ConfigType.php 100.00% <100.00%> (ø)
...p/bundles/DashboardBundle/Model/DashboardModel.php 70.65% <75.00%> (-0.14%) ⬇️

... and 37 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.

@JonasLudwig1998
Copy link
Contributor Author

@escopecz How about this solution? I also changed the ConfigType a bit so that one can only enter integers >= 0 there

escopecz
escopecz reviewed Mar 10, 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 understand that values 0 and -1 do the same thing which is disabling the cache. Is that correct? The PR description isn't up to date, right?

@JonasLudwig1998 JonasLudwig1998 changed the title Provide correct information about cached data timeout in m5 Fix cached data timeout logic in m5 Mar 10, 2025
@JonasLudwig1998
Copy link
Contributor Author

@escopecz The PR description is now up-to-date.
Originally 0 resulted in an endless cache (even though it should disable the cache according to mautic). With this PR 0 results in a disabled cache as it should

escopecz
escopecz approved these changes Mar 10, 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.

These changes look good to me. Thanks for taking care of this!

@escopecz escopecz added bug Issues or PR's relating to bugs pending-test-confirmation PR's that require one test before they can be merged dashboard Anything related to the Dashboard code-review-passed PRs which have passed code review and removed pending-feedback PR's and issues that are awaiting feedback from the author labels Mar 10, 2025
@escopecz escopecz moved this to ⏳︎ Needs 1 more test in Open Source Fridays Mar 10, 2025
@escopecz escopecz added this to the 5.2.4 milestone Mar 10, 2025
matbcvo
matbcvo approved these changes Mar 11, 2025
View reviewed changes
Copy link
Contributor

@matbcvo matbcvo left a comment

Choose a reason for hiding this comment

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

I've tested the PR, and it works as expected. Thanks for the PR!

@matbcvo matbcvo added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged 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 Mar 11, 2025
@matbcvo matbcvo moved this from ⏳︎ Needs 1 more test to 🎉 Ready to commit in Open Source Fridays Mar 11, 2025
@escopecz escopecz merged commit 8ba8d22 into mautic:5.2 Mar 12, 2025
17 checks passed
@github-project-automation github-project-automation bot moved this from 🎉 Ready to commit to 🥳 Done in Open Source Fridays Mar 12, 2025
@escopecz escopecz changed the title Fix cached data timeout logic in m5 Fix of disabling the Dashboard widget cache Mar 12, 2025
@JonasLudwig1998 JonasLudwig1998 mentioned this pull request Mar 12, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matbcvo matbcvo matbcvo approved these changes

@escopecz escopecz escopecz 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 dashboard Anything related to the Dashboard ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged user-testing-passed PRs which have been successfully tested by the required number of people.
Projects
Open Source Fridays
Status: 🥳 Done
Milestone
5.2.4
Development

Successfully merging this pull request may close these issues.
4 participants
@JonasLudwig1998 @RCheesley @escopecz @matbcvo