Skip to content

add coverage for removing all user settings on deletion + test coverage#24099

Merged
nathangavin merged 39 commits into5.x-devfrom
dev-19607
Mar 5, 2026
Merged

add coverage for removing all user settings on deletion + test coverage#24099
nathangavin merged 39 commits into5.x-devfrom
dev-19607

Conversation

@nathangavin
Copy link
Copy Markdown
Contributor

@nathangavin nathangavin commented Feb 18, 2026

Description

Please include a description of this change and which issue it fixes. If no issue exists yet please include context and what problem it solves.

fixes #23672

Checklist

  • [✔] I have understood, reviewed, and tested all AI outputs before use
  • [✔] All AI instructions respect security, IP, and privacy rules

Review

@nathangavin nathangavin added this to the 5.8.0 milestone Feb 18, 2026
@nathangavin nathangavin requested a review from a team February 20, 2026 05:45
@nathangavin nathangavin marked this pull request as ready for review February 20, 2026 05:45
Copy link
Copy Markdown
Contributor

@chippison chippison 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 checked the code and done some functional tests, overall it everything works as intended. Deleting a user does remove the settings from the DB

There are a couple of edge case issues codex found though:

[P1] User deletion can leave legacy per-user data behind when one observer throws
Basically, if a plugin throws an exception when deleting a user, it could leave some data behind and not clean it up

  • Was able to recreate this by creating a local plugin that throws exception on UserDelete
  • Codex recommendations (you can just use this to get a starting point):
    1. Add direct legacy cleanup in deleteUserOptions() for MobileMessaging + ProfessionalServices keys (smallest blast radius).
    2. Keep event-based cleanup, but isolate observer failures per callback for this event path.

[P2] Migration strategy for ProfessionalServices scales poorly with user count

  • wasn't really able to recreate this one, but seems like some DB query optimisation needed
  • Codex recommendations: (you can just use this to get a starting point):
    1. Load ProfessionalServices.DismissedWidget.% once, group by login in PHP, then write/delete once.
    2. Keep logic but batch by login chunks with tighter SQL (still more churn than option 1).

@sgiehl sgiehl modified the milestones: 5.8.0, 5.9.0 Feb 26, 2026
Comment thread plugins/UsersManager/Model.php Outdated
Comment thread plugins/UsersManager/Model.php
@chippison
Copy link
Copy Markdown
Contributor

I have checked the changes and it looks good from my end.
I also have done another round of testing and can confirm that:

  • when a user is deleted, its legacy settings are also deleted.
  • when a new user is created using the previous username, it does not follow/reuse the settings of the old user.

chippison
chippison previously approved these changes Feb 27, 2026
@nathangavin nathangavin requested a review from sgiehl February 27, 2026 05:24
Copy link
Copy Markdown
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Didn't have time for any testing and only performed a rough code review

Comment thread core/Updates/5.8.0-b2.php Outdated
Comment thread plugins/ProfessionalServices/ProfessionalServices.php Outdated
Comment thread core/Settings/Storage/LegacyUserSettingsMigration.php Outdated
Comment thread core/Settings/Storage/LegacyUserSettingsMigration.php
Comment thread core/Settings/Storage/UserScopedSettingsStore.php Outdated
Comment thread core/Settings/Storage/UserScopedSettingsStore.php Outdated
Comment thread plugins/MobileMessaging/tests/UI/MobileMessaging_spec.js
Comment thread plugins/MobileMessaging/MobileMessaging.php Outdated
@nathangavin nathangavin requested a review from sgiehl March 2, 2026 07:59
Copy link
Copy Markdown
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Tried doing a more detailed review now and left some further comments around possible improvements/fixes.

Comment thread core/Settings/Storage/Backend/PluginSettingsTable.php
Comment thread core/Settings/Storage/Backend/PluginSettingsTable.php
Comment thread core/Settings/Storage/LegacyUserSettingsMigration.php
Comment thread core/Settings/Storage/LegacyUserSettingsMigration.php Outdated
Comment thread core/Updates/5.9.0-b1.php Outdated
Comment thread plugins/ProfessionalServices/PromoWidgetDismissal.php Outdated
Comment thread plugins/UsersManager/API.php Outdated
Comment thread plugins/UsersManager/Model.php
Comment thread plugins/UsersManager/Model.php
@nathangavin nathangavin requested a review from sgiehl March 4, 2026 04:28
Copy link
Copy Markdown
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

some last comments, afterwards that should be good to go.

Comment thread core/Settings/Storage/Backend/PluginSettingsTable.php Outdated
Comment thread core/Settings/Storage/LegacyUserSettingsMigration.php Outdated
Comment thread plugins/CoreAdminHome/Commands/MigrateUserScopedSettings.php Outdated
Comment thread plugins/MobileMessaging/tests/UI/MobileMessaging_spec.js
Comment thread core/Settings/Storage/LegacyUserSettingsMigration.php
@nathangavin nathangavin requested a review from sgiehl March 5, 2026 05:06
@nathangavin nathangavin merged commit a9e2f0f into 5.x-dev Mar 5, 2026
120 of 135 checks passed
@nathangavin nathangavin deleted the dev-19607 branch March 5, 2026 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Deleted user’s settings persist when username is reused

3 participants