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

No validation messages for sign up #554

Open
iamajvillalobos opened this issue Apr 5, 2015 · 11 comments
Open

No validation messages for sign up #554

iamajvillalobos opened this issue Apr 5, 2015 · 11 comments
Milestone

Comments

@iamajvillalobos
Copy link

When I clicked sign up when no email and password inputted, it doesn't have flash message compare like signing in. Is this normal? and I need to set my own?

@derekprior
Copy link
Contributor

Do you have a suggestion for what the flash should say? Unfortunately, I can't think of anything particularly useful that wouldn't then be an impediment for people with different sign up requirements.

I thin the proper way to handle this is to customize the sign up template to display errors as you would see fit. It's a tough thing for us to do for you because error styling is generally very app-by-app.

What do you think?

@iamajvillalobos
Copy link
Author

Hi Derek,

You are right. I did use Devise before trying out Clearance, and in Devise
I think there is a default error message. For me I would like have a
default error message where we could customize to our liking. It's not
really important just a little thing that I observed.

On Fri, May 8, 2015 at 10:51 PM, Derek Prior [email protected]
wrote:

Do you have a suggestion for what the flash should say? Unfortunately, I
can't think of anything particularly useful that wouldn't then be an
impediment for people with different sign up requirements.

I thin the proper way to handle this is to customize the sign up template
to display errors as you would see fit. It's a tough thing for us to do for
you because error styling is generally very app-by-app.

What do you think?


Reply to this email directly or view it on GitHub
#554 (comment)
.

@DavidVII
Copy link

I posted a similar issue today with #559. I agree that perhaps there should be a default error. Even the "Bad email or password." would be good. That being said, this feels more like a form validation issue. Adding form errors in the sign up form makes some sense to me. Thoughts?

@jessieay
Copy link
Contributor

I usually see people create a place for flash messages in Rails view layouts, as @derekprior mentioned.

Looks like we are setting the flash message in the sign in form like @iamarmanjon mentioned. (reference: https://github.com/thoughtbot/clearance/blob/master/app/controllers/clearance/sessions_controller.rb#L13) - I think it makes sense to do the same in the UsersController so that the error messages display in the same way during sign up.

One challenge with this is that we are using a single default failure message when a sign in fails. For a sign up form where a user record is being created, failure messages are usually determined by ActiveRecord validations. There are validations here:

module Validations
- but I would have to think a little bit about how to use them in the controller template because we'd have to make sure to include any additional validations on the class that is using clearance.

@DavidVII
Copy link

@jessieay would it make sense to include form errors within the sign up form?

Something like...

<% if object.errors.any? %>
  <h3>Please fix the following errors:</h3>
  <ul>
    <% object.errors.full_messages.each do |msg| %>
      <li>
        <%= msg %>
      </li>
    <% end %>
  </ul>
<% end %>

That should allow us to display validation errors directly in the form instead of displaying a general flash message. Thoughts?

@derekprior
Copy link
Contributor

This is my least favorite part of Clearance to make decisions on... It's hard to get shared UI right at all. I'd be perfectly fine making some of these changes if generating the views into your own app was required and this was just a default option. But it's not required -- you can run with internal views.

I like the idea of setting the flash and as @jessieay points out, we're already doing that elsewhere. But I'm just not sure what that message should actually say. I don't think I'd consider this a breaking change, though some could argue it is. Clearance already assumes you display and style flashes. If your layout doesn't show them, then you won't get them, if it does, it should be displayed acceptably.

Presenting errors as @DavidVII mentions makes it obvious how to handle the messaging, but its also something I would consider a breaking change. There's no expectation for your sign up form to have styling to handle that markup in a nice way. That markup is almost exactly what error_messages_for did in Rails 2.3, but it was removed because it pre-supposes too much about how end apps should display errors. Error display is seldom identical across apps, in my experience.

@DavidVII
Copy link

Good points, @derekprior. I definitely think this requires some more thought.

IMO, the user flow is off when using internal views or even default controllers. Clearance has user validations in place, yet we're not showing their errors by default. Not showing these error messages when a user signs up adds a hurdle to the user experience. I feel like presenting errors as I described above makes it clear why there was an interruption. Given, my idea may not be the best solution, but I definitely feel like we need something. Thoughts?

@derekprior
Copy link
Contributor

I'm leaning towards making this a Clearance 2.0 change, adding errors to the view, and possibly requiring that views be dumped to your app and not served internally.

@DavidVII
Copy link

+1 on requiring the views

@derekprior derekprior added this to the 2.0 milestone Jan 6, 2016
@anhari
Copy link

anhari commented Feb 2, 2016

Hi all! For some reason when I was using clearance my Active Record validations were not showing error messages when they were being validated. I ended modifying the create action in my own UsersController so that it always shows at least one of the errors:

  def create
    @user = user_from_params

    if @user.save
      sign_in @user
      redirect_back_or url_after_create
    else
      flash.now[:error] = @user.errors.full_messages[0]
      render template: "users/new"
    end
  end

Hopefully this applies to this discussion!

@mjankowski mjankowski modified the milestones: 2.0, 2.1 Nov 15, 2019
@mjankowski mjankowski modified the milestones: 2.1, 2.2 Jan 28, 2020
@ahmgeek
Copy link

ahmgeek commented Jun 7, 2020

Although the solution is easy for customized signup process, but there's no code comments or docs saying please add it yourself 😓
although it should be obvious, but still, it would be nice to remind devs that there will be no flashing errors there.

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

No branches or pull requests

7 participants