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

Return 422 (was 500) when empty body for sign up and account update #204

Merged
merged 1 commit into from
Apr 9, 2015
Merged

Conversation

mchavarriagam
Copy link
Contributor

Hello.

As mentioned on issue #203, currently DTA returns 500 error when an empty POST or PUT is made on the registration controller. For those using DTA for API authentication without control over REST clients, this may be an issue. I went ahead and made some changes. I think I can do some refactoring on the tests and also on the before filters, but wanted to check if the idea is welcomed before moving forward.

Thanks!

@lynndylanhurley
Copy link
Owner

Thanks again @mchavarriagam, and thanks again for the tests!!

lynndylanhurley added a commit that referenced this pull request Apr 9, 2015
Return 422 (was 500) when empty body for sign up and account update. Fixes #203
@lynndylanhurley lynndylanhurley merged commit 16d1993 into lynndylanhurley:master Apr 9, 2015
@lynndylanhurley
Copy link
Owner

And congratulations, you've made it to the callouts list.

@alex88
Copy link

alex88 commented Apr 13, 2015

Shouldn't this be handled with validations? Like saying "Email shouldn't be blank"?

@mchavarriagam
Copy link
Contributor Author

@alex88 I'm not sure what the protocol should be here but my assumption was that multiple fields would have to be validated and I wasn't certain if the fields would be pre-defined and constant. Regardless of that, I didn't feel it was appropriate for the gem to return 500 (server error) for something that was in reality a user error. This is just a long way of saying "yes it should probably be done with validations but I didn't know what to validate for so I just checked that the body wasn't empty" :) (that didn't turn out short either).

I'm open to suggestions on this and wouldn't mind working on the PR a bit more to make it better.

@alex88
Copy link

alex88 commented Apr 14, 2015

@mchavarriagam maybe we should stay in line with devise https://github.com/plataformatec/devise/blob/master/lib/devise/models/validatable.rb#L29-L35 since "Please submit proper sign up data in request body" doesn't let the user know what's wrong with his submission neither it tells what to fix.

Devise in its controllers just tries to persist the entity, the checks are done on the validation side.

Maybe we can just use downcase only on non blank email params and let the validations do the work

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.

3 participants