Skip to content

Only validate changed user fields when provided#118

Merged
dmitriim merged 1 commit intocatalyst:MOODLE_405_STABLEfrom
MagnusSamuelsson:fix/validations-in-update_user
Dec 22, 2025
Merged

Only validate changed user fields when provided#118
dmitriim merged 1 commit intocatalyst:MOODLE_405_STABLEfrom
MagnusSamuelsson:fix/validations-in-update_user

Conversation

@MagnusSamuelsson
Copy link
Copy Markdown

Added a check before validation, so validation won't happen if there is nothing to validate (and update)

@dmitriim
Copy link
Copy Markdown
Member

@MagnusSamuelsson Thanks for working on fixing the issue. Can you please add automated tests to support your change?

@MagnusSamuelsson
Copy link
Copy Markdown
Author

@dmitriim Sure!
To what extent?
I can create a test for logging in with userId, and checking that auth method is updated, but no other fields.

Maybe also one or two tests that updates a singlefield? But I think that's redundant.

About empty/isset. We need too stick with isset, or make additional checks. With !empty, it will skip validation, and update it even if it's set to an empty string. Speaking of... That's something we can test to assure that never happens.

@dmitriim
Copy link
Copy Markdown
Member

As there is a bug I'd suggest to take following approach.

  1. Before changing any logic, create a test that will fail because of the existing bug.
  2. Fix the logic and confirm that your test passes now.

@MagnusSamuelsson MagnusSamuelsson force-pushed the fix/validations-in-update_user branch from 7df237f to e4f2872 Compare December 22, 2025 13:06
@MagnusSamuelsson
Copy link
Copy Markdown
Author

@dmitriim
I played around a bit and did some more testing. I noticed that it’s possible to update a username to an empty string.
I think that’s a separate bug.
The question is: does this bug belong to this plugin, or to Moodle core, which doesn’t appear to validate this in user_update_user?

* Test that a user can be updated without providing any other fields than the mappingfield.
* (Only auth field should be updated).
*/
public function test_update_allow_unset_fields(): void {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would be nice to add tests that would cover the error that you're trying to solve when email or/and username is not set in update user request.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah great you've done that.

@dmitriim dmitriim merged commit 16a9f5f into catalyst:MOODLE_405_STABLE Dec 22, 2025
11 checks passed
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.

2 participants