-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Don’t cast as boolean, do validate as boolean for User validation #13546
Don’t cast as boolean, do validate as boolean for User validation #13546
Conversation
Signed-off-by: snipe <[email protected]>
PR Summary
|
Hm, looks like those tests are failing, but I'm not sure (yet) if that's because the tests need to be updated or whether this actually breaks stuff. |
👀 gonna check it out |
@snipe yeah for the tests we can change the assertions to $this->assertEquals(1, $user->refresh()->activated);
$this->assertEquals(0, $user->refresh()->activated);
// and
$this->assertEquals(1, $admin->refresh()->activated); Want me to push a commit to your branch? |
Thanks! I ran out for a snack but I’ll handle that when I get back to my desk. |
Totes up to you. We’re on our midday break (a little late tho) so we won’t be back until the standup |
Pushed 👍🏾 |
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.
This seems to make sense.
Should we check other models for the same issue? byod
on Asset for example.
Yeah, probably - it's the postman fuckery that made it misleading when I first wrote it. :( |
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
This should make the behavior of the API for boolean fields a little more consistent. What's goofy though is if you're using postman, this fails using true/false, but works with 1 or 0.
I guess we just have to document that in the API docs. :-/