Show multiple error messages when there are multiple errors saving a …

…profile edit

Reuse the errors partial from the signup process; make the message more general.

app/controllers/users_controller.rb

@@ -73,9 +73,9 @@ def update
 
         redirect_to user_path(@user)
       else
-        error_message = @user.errors.full_messages.first
-
-        flash[:error] = "Profile could not be saved: #{error_message}"
+        error_message = render_to_string :partial => 'users/errors',
+                                         :locals => {:user => @user}
+        flash[:error] = error_message.html_safe
 
         render :edit
       end

app/models/user.rb

@@ -341,15 +341,6 @@ def self.authenticate(username, pass)
 
   # Edit profile information
   def update_profile!(params)
-    unless params[:password].blank?
-      if params[:password] == params[:password_confirm]
-        self.password = params[:password]
-        self.save
-      else
-        self.errors.add(:password, "doesn't match confirmation.")
-        return false
-      end
-    end
 
     params[:email] = nil if params[:email].blank?
 
@@ -360,21 +351,48 @@ def update_profile!(params)
 
     self.always_send_to_twitter = params[:user] && params[:user][:always_send_to_twitter].to_i
 
-    return false unless self.save
+    # I can't figure out how to use a real rails validator to confirm that
+    # password matches password_confirm, since these two attributes are
+    # virtual and we only want to check this in this particular case of
+    # updating a user.
+
+    # Additionally, running the other validations clears self.errors, so
+    # we need to add our own errors AFTER calling valid?. But we shouldn't
+    # save the record at all if the password change isn't valid.
+
+    self.valid?
+
+    unless params[:password].blank?
+      if params[:password] == params[:password_confirm]
+        self.password = params[:password]
+        self.save
+      else
+        self.errors.add(:password, "doesn't match confirmation.")
+      end
+    end
 
-    author.username = params[:username]
-    author.name     = params[:name]
-    author.email    = params[:email]
-    author.website  = params[:website]
-    author.bio      = params[:bio]
-    author.save
+    # Calling valid? again here would make the validators run again, which
+    # would clear self.errors again. We may have added an error about the
+    # password not matching the confirmation.
+    if self.errors.present?
+      return false
+    else
+      self.save
 
-    # TODO: Send out notice to other nodes
-    # To each remote domain that is following you via hub
-    # and to each remote domain that you follow via salmon
-    author.feed.ping_hubs
+      author.username = params[:username]
+      author.name     = params[:name]
+      author.email    = params[:email]
+      author.website  = params[:website]
+      author.bio      = params[:bio]
+      author.save
 
-    return self
+      # TODO: Send out notice to other nodes
+      # To each remote domain that is following you via hub
+      # and to each remote domain that you follow via salmon
+      author.feed.ping_hubs
+
+      return self
+    end
   end
 
   # A better name would be very welcome.

app/views/users/_errors.haml

@@ -1,7 +1,7 @@
 - if user && user.errors.any?
   %div{:id => "error"}
     %div{:id => "error_explanation"}
-      %strong= "#{pluralize(user.errors.count, "error")} prohibited your account from being created:"
+      %strong= "Sorry, #{pluralize(user.errors.count, "error")} we need you to fix:"
       %ul
         - user.errors.full_messages.each do |msg|
           %li= msg

test/acceptance/edit_profile_test.rb

@@ -102,12 +102,33 @@
       end
 
       within flash do
-        assert has_content?("Profile could not be saved: Password doesn't match confirmation.")
+        assert has_content?("Sorry, 1 error we need you to fix:")
+        assert has_content?("Password doesn't match confirmation.")
       end
 
       assert has_field?("password")
     end
 
+    it "shows multiple error messages if there are multiple problems" do
+      visit "/users/#{@u.username}/edit"
+
+      fill_in "username", :with => "something too_long&with invalid#chars."
+
+      fill_in "password", :with => "new_password"
+      fill_in "password_confirm", :with => "bunk"
+
+      VCR.use_cassette("update_profile_multiple_errors") do
+        click_button "Save"
+      end
+
+      within flash do
+        assert has_content?("Sorry, 3 errors we need you to fix:")
+        assert has_content?("Password doesn't match confirmation.")
+        assert has_content?("Username contains restricted characters.")
+        assert has_content?("Username must be 17 characters or fewer.")
+      end
+    end
+
     it "verifies your email if you change it" do
       visit "/users/#{@u.username}/edit"
       email = "new_email@new_email.com"

test/acceptance/signup_test.rb

@@ -25,7 +25,7 @@
       fill_in "password", :with => "baseball"
       click_button "Log in"
 
-      assert_match /1 error prohibited your account from being created:/, page.body
+      assert_match /Sorry, 1 error we need you to fix:/, page.body
       assert_match /contains restricted characters\./, page.body
     end
 
@@ -34,7 +34,7 @@
       fill_in "password", :with => "baseball"
       click_button "Log in"
 
-      assert_match /1 error prohibited your account from being created:/, page.body
+      assert_match /Sorry, 1 error we need you to fix:/, page.body
       assert_match /Username can't be blank/, page.body
     end
 
@@ -70,7 +70,7 @@
 
       click_button "Log in"
 
-      assert_match /1 error prohibited your account from being created:/, page.body
+      assert_match /Sorry, 1 error we need you to fix:/, page.body
       assert_match /Username must be 17 characters or fewer\./, page.body
     end
   end
@@ -86,7 +86,7 @@
       fill_in "username", :with => "taken"
       click_button "Finish Signup"
 
-      assert_match /1 error prohibited your account from being created:/, page.body
+      assert_match /Sorry, 1 error we need you to fix:/, page.body
       assert_match /Username has already been taken/, page.body
 
       fill_in "username", :with => "nottaken"