-
-
Notifications
You must be signed in to change notification settings - Fork 367
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.
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
Fix typos in user creation with initial password #1030
Conversation
Good catch. Looks like I missed that line when I updated it in acf4d60. |
with the test failure, I'm not so sure anymore if this was actually the correct fix... |
I think the test may need to be adjusted, as before it assumed there was never a password field to handle. |
UserFrosting/app/sprinkles/admin/src/Controller/UserController.php Lines 789 to 790 in 1f3edc0
|
@@ -167,7 +167,7 @@ public function create(Request $request, Response $response, $args) | |||
$passwordRequest = $this->ci->repoPasswordReset->create($user, $config['password_reset.timeouts.create']); | |||
|
|||
// If the password_mode is manual, do not send an email to set it. Else, send the email. | |||
if (!isset($data['value'])) { | |||
if ($data['password'] === '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test failure was actually due to the isset
here - it's set to empty string if not set some lines earlier.
Just a note, it would be helpful to include some details about what's actually broken for others to be able to tell what exactly this is fixing. :-) "Such that it actually works" is not very helpful. |
@amosfolz sure, sorry about that - for the record, the problem was that the frontend validation wouldn't allow entering a password, because it always failed the check against the password confirmation field. |
Not sure if I picked the right base branch, let me know :)
This fixes some (I guess) typos in #1017 such that it actually works (on my machine™️).
issue: #764