Duplicate error messages on User objects

[mokolabs]

My app is using message_block to show all errors and flash messages.

So I've got this in my application layout:
= message_block on: :all

It's been working great... with one exception. When I try to update my password while being logged in, if there is a validation error, the error output appears twice:

• Password is too short (minimum is 3 characters)
• Password confirmation doesn't match Password
• Password confirmation can't be blank
• Password is too short (minimum is 3 characters)
• Password confirmation doesn't match Password
• Password confirmation can't be blank

It took me a long time to figure out why this was happening. But I finally did.

When I try to change my password while I'm logged in, there are actually two instances of the User model loaded by the view.

One is an instance of current_user (the convenience method used by most Rails authentication gems as a proxy for a logged in user). The other is a more generic instance of the User model -- accessed via @user -- that I'm using to update the password.

And, yes, @user is really a copy of current_user, but there are still technically two User objects being loaded in the view.

Here are the relevant bits of the controller for reference

class PasswordsController < ApplicationController
  before_action :require_login

  def update
    @user = current_user
    
    if params[:user][:password].blank? and params[:user][:password_confirmation].blank?
      flash.now[:error] = "You forgot to enter a new password."
      render :edit
      return
    end

    @user.password_confirmation = params[:user][:password_confirmation]

    if @user.change_password!(params[:user][:password])
      @user.update_attribute(:reset_password_email_sent_at, nil)
      flash[:notice] = "Your password was successfully updated."
      redirect_to root_path
    else
      render :edit
    end
  end

end

One way to fix this would be to reject the current_user object from the list of objects that are checked for errors in the message_block helper.

model_objects = options[:on].map do |model_object|
  if model_object == :all
    assigns.delete("current_user") # <- ADD THIS LINE OF CODE
    assigns.values.select {|o| o.respond_to?(:errors) && o.errors.is_a?(ActiveModel::Errors) }
  elsif model_object.instance_of?(String) or model_object.instance_of?(Symbol)
    instance_variable_get("@#{model_object}")
  else
    model_object
  end
end.flatten.select {|m| !m.nil? }

But, ultimately, I think a better approach for the :all option would be to grab every user model from the app/models directory -- which is how we're doing it with Graffletopia, albeit as an initializer.

MODELS = Dir['app/models/*.rb'].map {|f| File.basename(f, '.rb').downcase.to_sym}

Thoughts?

[eigenben]

Dating back to a discussion a while ago, I am still not a fan of the design decision to even have :all in this gem, and issues like this are a great example of why. Implicitly puling instance variables set in the controller and taking any and all of them for consideration in display here is just pretty ugly. Although we commonly use instance variables as part of the API contract between the controller & view within Rails, there are plenty of other uses of these variables, many of which would manifestly not be related to anything message_block should be dealing with. I favor explicit over implicit in this case, and that is why the preferred usage is to use :on with specific instances-with-errors message_block should consult.

Nevertheless, this :all thing is in the gem now, and barring removing it with a major version change, we might be able to find a way to accommodate your use case.

The gem is pulling these values by checking assigns.values. I would play around in your app with what assigns is actually exposing here, as it is definitely not the current_user method directly. Something must be setting that current user as an instance variable (perhaps @current_user) that is thus being exposed as the view.

If that's the case, one "solution" to your problem is to just reuse that instance variable name instead of a different @user one.

Another option is to potentially try the gem with calling .uniq on the set of model objects it finds, to remove duplicates:

assigns.values.select {|o| o.respond_to?(:errors) && o.errors.is_a?(ActiveModel::Errors) }.uniq

This would generally handle any case where you re-assign an instance variable that may also be present, not just for this common current_user case.

Another potential option is to extend the API with an :except option which would serve as the inverse of :on, explicitly excluding certain names.

I am definitely against introducing a hardcoded list of instance variable names that we exclude (assigns.delete("current_user")), as the word current_user is way app-specific, and there are plenty of other examples potentially running into this very same problem beyond current_user.

I am also against having this gem pulling a list of model names dynamically from app/models and merely presuming that this is a proper mapping of names-to-instance-variables for :all. There are lots of potential issues going down that route for use of this gem more widely.

[mokolabs]

Yeah, you make some great points.

The potential solution I shared isn't perfect and could definitely be problematic in some scenarios. But my goal, however, is perfectly valid.

I would like message_block to be even easier to install. I don't want to think about which models and flash messages to display. I want it to just show all of them... unless and until I require a more selective approach. With convention over configuration in mind, I think this is probably true of most Rails apps. Just show me errors on my stuff without a lot of fuss.

So, given that, you've got the selective approach pretty well covered. Let's work on improving support for the zero-config approach. :)

PS: Indeed, it looks like sorcery is creating an instance variable for current_user:

def current_user
  unless defined?(@current_user)
    @current_user = login_from_session || login_from_other_sources || nil
  end
  @current_user
end

I'll think about this some more and get back to you.

[mokolabs]

I ran into this duplicate flash message bug again, googled around, and rediscovered this issue. LOL.

So... five years later, here's how I fixed it:

list = flash_messages[type.to_sym].uniq.map {|message| "<li>#{message}</li>" }.join

This removes any duplicate flash_messages before we render them into HTML -- regardless if they're caused by duplicate model instances or some other arcane reason.