fix: use the right user id when changing the email

[jvillafanez]

Description

Change the email of the right user. Note that the code path isn't reachable from the web UI.
Note that there are checks some lines above for the existence of the user, so if we reach this particular line we know that there is an existing user with that id, no need to check it again.

Related Issue

• Fixes <issue_link>

Motivation and Context

How Has This Been Tested?

Manually tested using curl. It now changes the target user's email instead of the admin's (requester) email.

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:
• Changelog item, see TEMPLATE

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[DeepDiver1975]

unit testable? Thank you

[DeepDiver1975]

Code Review

Overview

This PR fixes a bug in UsersController::setMailAddress() where an admin changing another user's email address would accidentally update the admin's own email instead of the target user's email. The root cause is a one-character variable name bug: $userId (the logged-in admin's ID) was used instead of $id (the function parameter — the target user's ID).

---

Source Fix

settings/Controller/UsersController.php:975

- $this->setEmailAddress($userId, $mailAddress);
+ $this->setEmailAddress($id, $mailAddress);

Correct and minimal. The fix is exactly right — $id is the parameter passed to setMailAddress($id, $mailAddress), i.e. the target user. $userId is the currently logged-in user (the admin). The PR author's note is important: this code path is only reachable via direct API calls, not the web UI — which explains why this wasn't caught earlier.

---

Test Changes

Improvements:

• Typo fixed: $chanChangeMailAddress → $canChangeMailAddress — good cleanup.
• The test now correctly verifies that setEMailAddress is called on $user2 (the target, anotherUserId) rather than $user (the admin, foo). This is the right way to test the actual behavior of the fix.
• willReturnMap for the Config::getUserValue mock is cleaner than the previous single-value stub.

Concerns:

1. Switch without a default case — the UserManager::get() callback returns null implicitly for unknown IDs. A default: return null; would make intent explicit.

2. Test condition may not match production logic — setEMailAddress is only expected when $isValid && $canChangeMailAddress. But the production code path being tested is the admin path ($this->isAdmin = true), which skips the canChangeMailAddress check. If canChangeMailAddress is false, should the admin path still set the email? The test says no — but this needs verification against the controller logic.

3. No never() assertion on the admin user mock — $user->setEMailAddress is no longer expected at all. If production code accidentally called setEMailAddress on $user (the admin) as well, no assertion would catch it. Adding $user->expects($this->never())->method('setEMailAddress') would make the test more defensive.

---

Process

• No linked issue — Fixes <issue_link> is unfilled.
• "Unit tests added" is unchecked — tests were updated, so this should be checked.
• No changelog entry — the checklist item is unchecked and no changelog file is present in the diff.

---

Summary

Category	Assessment
Correctness	The one-line fix is correct
Test coverage	Good improvement, but missing never() assertion on admin mock
Test logic accuracy	Needs verification that $isValid && $canChangeMailAddress matches the admin code path
Security	Low-severity (API-only), but the fix is still important for correctness

The core fix is sound. I'd suggest:

1. Adding $user->expects($this->never())->method('setEMailAddress') to guard against regressions,
2. Clarifying whether the admin path truly respects canChangeMailAddress, and
3. Adding a linked issue + changelog entry before merging.