Skip to content
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

Improve UserValidator default behavior [4.0 beta] #311

Closed
wants to merge 4 commits into from

Conversation

gabrielalmeida
Copy link
Contributor

UserValidator present design doesn't provide good UX practices and easy client-side validation via
$user->errors.

It overwrites $user->errors each time UserValidator->attachErrorMsg() method runs, providing only 'one' error at time instead of all of them, making user progress at completing unvalidated fields slower and unintelligent.

It also doesn't follow MessageBag default behavior, which is a key corresponding to input names/model attributes and its respective value. It actually adds a MessageBag with confide key, so client-side validation by matching keys from $user->errors isn't possible..

Note that UserValidator->ValidateIsUnique() method is still showing only first error but now at least exports correct error key to $user->errors.

Unity test to UserValidator->attachErrorMsg() must be updated if merged since it was redone to work directly with MessageBag object instead of ConfideUserInterface.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.21%) when pulling c5699fe on gabrielalmeida:huge-update into 19f3eb7 on Zizaco:huge-update.

@Zizaco Zizaco changed the title Improve UserValidator default behavior Improve UserValidator default behavior [4.0 beta1] Jul 15, 2014
@Zizaco
Copy link
Owner

Zizaco commented Jul 15, 2014

@gabrielalmeida Good catch!

My only concern is about changing the first parameter of attachErrorMsg to a MessageBag.

For me it seems better to look for the error attribute inside attachErrorMsg method. I believe it's a better design since the attachErrorMsg is a simpler method and that have the single responsibility of attaching error messages. It can look if the error attribute contains a MessageBag, if so, call merge else create a new one.

About the the $key attribute, I think it's very good. Seems very reasonable to me.

This way you will not have to pass $user->errors everytime, but simply the $user and the only test that you'd have to tweak is UserValidatorTest::testShouldAttachErrorMsg

What do you think about making these changes? You can create another PR if you like.

@Zizaco Zizaco changed the title Improve UserValidator default behavior [4.0 beta1] Improve UserValidator default behavior [4.0 beta] Jul 15, 2014
@@ -63,6 +66,9 @@ public function validate(ConfideUserInterface $user, $ruleset = 'create')
// Set the $repo as a ConfideRepository object
$this->repo = App::make('confide.repository');

// Set $user->errors as a MessageBag object
Copy link
Contributor

Choose a reason for hiding this comment

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

Please indent with spaces. :P

@GrahamCampbell
Copy link
Contributor

The indentation here is wrong. There's a mix of tabs and spaces. There should only be spaces.

@Zizaco
Copy link
Owner

Zizaco commented Jul 15, 2014

@GrahamCampbell Yes you're right. I plan to fix then if @gabrielalmeida did not.

@Zizaco
Copy link
Owner

Zizaco commented Jul 17, 2014

I did this: 139d44b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants