Skip to content

test(dashboard): regression for widget removal sync (#15390)#16864

Open
Ibochkarev wants to merge 1 commit intomodxcms:3.xfrom
Ibochkarev:fix/14753-default-dashboard-widgets
Open

test(dashboard): regression for widget removal sync (#15390)#16864
Ibochkarev wants to merge 1 commit intomodxcms:3.xfrom
Ibochkarev:fix/14753-default-dashboard-widgets

Conversation

@Ibochkarev
Copy link
Copy Markdown
Collaborator

@Ibochkarev Ibochkarev commented Feb 22, 2026

What does it do?

Adds testRemovedWidgetsNotShownAfterUpdate() in _build/test/Tests/Model/Dashboard/modDashboardTest.php: after saving a customizable dashboard template (user=0) with fewer widgets, user-specific placements stay in sync and render() output matches.

Why is it needed?

Per review on this PR: runtime behavior is already covered by #15390. This test documents and guards that path. #14753 remains the original report context.

How to test

With a configured test DB:

core/vendor/bin/phpunit -c _build/test/phpunit.xml --filter testRemovedWidgetsNotShownAfterUpdate

Related issue(s)/PR(s)

@Ibochkarev Ibochkarev force-pushed the fix/14753-default-dashboard-widgets branch 3 times, most recently from 2572bd6 to 80125b2 Compare February 22, 2026 16:38
@biz87
Copy link
Copy Markdown

biz87 commented Feb 25, 2026

Code Review

Summary

Fixes the default dashboard not respecting widget removal (#14753). When a template dashboard (user=0) is updated and a widget is removed, user-specific placements are now also updated. The Dashboard Update processor's setWidgets() is refactored to track added/removed widgets and propagate changes to user placements. The addUserWidgets() method in modDashboard is fixed to sort by rank and use getCollection() instead of getMany(). Includes a comprehensive regression test.

Assessment

Well-structured fix. The setWidgets() refactoring properly separates concerns: syncTemplatePlacements() for the template, propagateAddedWidgets() and propagateRemovedWidgets() for user-specific placements. The test is thorough — creates dashboard, widgets, placements, verifies initial state, performs update, and checks that removed widgets disappear from both template and user placements. Proper cleanup in the test ensures no test data leaks.

Verdict: Approve

@Ibochkarev Ibochkarev force-pushed the fix/14753-default-dashboard-widgets branch from 30cbb79 to 9189c98 Compare February 26, 2026 02:16
Copy link
Copy Markdown
Member

@mkschell mkschell left a comment

Choose a reason for hiding this comment

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

This solution does not appear to take into account the discussion and the lack of consensus on the solution in #14753 . It looks like a low resolution shortcut to resolve the issue as stated, without considering the nuance of the issue.

@mkschell
Copy link
Copy Markdown
Member

On further exploration, I could not reproduce the issue. It appears to have already been solved in 2021 via #15390

The new test testRemovedWidgetsNotShownAfterUpdate is defensible, so perhaps re-submitting the PR with a test that covers the 2021 change would be welcome.

Thanks!

Guards behavior from modxcms#15390: saving the template dashboard with fewer widgets
updates user placements; render shows only remaining widgets. modxcms#14753 context.
@Ibochkarev Ibochkarev force-pushed the fix/14753-default-dashboard-widgets branch from 9189c98 to ee82917 Compare March 28, 2026 07:41
@Ibochkarev
Copy link
Copy Markdown
Collaborator Author

@mkschell Thanks for the detailed review. I agree the runtime behavior is already covered by #15390 in Dashboard\\Update::setWidgets().

Following your suggestion, I narrowed this PR to a regression test only (testRemovedWidgetsNotShownAfterUpdate), with the docblock explicitly referencing #15390 as the behavior under test and #14753 as the original report context. The earlier refactor, addUserWidgets / getCollection change, and empty-widgets handling are removed so the diff does not duplicate the 2021 fix.

If we want to follow up on the edge case where widgets is an empty array (the old if ($widgets) guard), I am happy to propose that as a separate small PR after maintainer consensus.

@Ibochkarev Ibochkarev requested a review from mkschell March 28, 2026 07:41
@Ibochkarev Ibochkarev changed the title fix(dashboard): default dashboard not respecting widget removal (#14753) test(dashboard): regression for widget removal sync (#15390) Mar 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants