Merge pull request #5580 from MothOnMars/5549-password-reset-error

display specific error messages for password change (issue #5549)

Changelog.md

@@ -148,6 +148,7 @@ diaspora.yml file**. The existing settings from 0.4.x and before will not work a
 * Correct photo count on profile page [#5751](https://github.com/diaspora/diaspora/pull/5751)
 * Fix mobile sign up from an invitation [#5754](https://github.com/diaspora/diaspora/pull/5754)
 * Set max-width for tag following button on tag page [#5752](https://github.com/diaspora/diaspora/pull/5752)
+* Display error messages for failed password change [#5580](https://github.com/diaspora/diaspora/pull/5580)
 
 ## Features
 * Don't pull jQuery from a CDN by default [#5105](https://github.com/diaspora/diaspora/pull/5105)

app/controllers/users_controller.rb

@@ -4,17 +4,16 @@
 
 class UsersController < ApplicationController
   before_action :authenticate_user!, :except => [:new, :create, :public, :user_photo]
-  layout ->(c) { request.format == :mobile ? "application" : "with_header_with_footer" }, only: [:privacy_settings, :edit]
+
+  layout ->(c) { request.format == :mobile ? "application" : "with_header_with_footer" },
+    only: [:privacy_settings, :edit, :update]
 
   respond_to :html
 
   def edit
     @aspect = :user_edit
-    @user   = current_user
-    @email_prefs = Hash.new(true)
-    @user.user_preferences.each do |pref|
-      @email_prefs[pref.email_type] = false
-    end
+    @user = current_user
+    set_email_preferences
   end
 
   def privacy_settings
@@ -26,16 +25,13 @@ class UsersController < ApplicationController
     @user = current_user
 
     if u = user_params
-      u.delete(:password) if u[:password].blank?
-      u.delete(:password_confirmation) if u[:password].blank? and u[:password_confirmation].blank?
-      u.delete(:language) if u[:language].blank?
 
       # change email notifications
       if u[:email_preferences]
         @user.update_user_preferences(u[:email_preferences])
         flash[:notice] = I18n.t 'users.update.email_notifications_changed'
       # change password
-      elsif u[:current_password] && u[:password] && u[:password_confirmation]
+      elsif params[:change_password]
         if @user.update_with_password(u)
           password_changed = true
           flash[:notice] = I18n.t 'users.update.password_changed'
@@ -79,10 +75,17 @@ class UsersController < ApplicationController
         end
       end
     end
+    set_email_preferences
 
     respond_to do |format|
       format.js   { render :nothing => true, :status => 204 }
-      format.all  { redirect_to password_changed ? new_user_session_path : edit_user_path }
+      format.all  do
+        if password_changed
+          redirect_to new_user_session_path
+        else
+          render :edit
+        end
+      end
     end
   end
 
@@ -206,4 +209,12 @@ class UsersController < ApplicationController
       ]
     )
   end
+
+  def set_email_preferences
+    @email_prefs = Hash.new(true)
+
+    @user.user_preferences.each do |pref|
+      @email_prefs[pref.email_type] = false
+    end
+  end
 end

app/helpers/error_messages_helper.rb

@@ -6,11 +6,10 @@ module ErrorMessagesHelper
   # Render error messages for the given objects. The :message and :header_message options are allowed.
   def error_messages_for(*objects)
     options = objects.extract_options!
-    options[:header_message] ||= I18n.t('error_messages.helper.invalid_fields')
     options[:message] ||= I18n.t('error_messages.helper.correct_the_following_errors_and_try_again')
     messages = objects.compact.map { |o| o.errors.full_messages }.flatten
     unless messages.empty?
-      content_tag(:div, :class => "error_messages") do
+      content_tag(:div, class: "text-error") do
         list_items = messages.map { |msg| content_tag(:li, msg) }
         content_tag(:h2, options[:header_message]) + content_tag(:p, options[:message]) + content_tag(:ul, list_items.join.html_safe)
       end

app/views/passwords/edit.haml

@@ -9,6 +9,8 @@
         = AppConfig.settings.pod_name
 
     = form_for(resource, as: resource_name, url: password_path(resource_name), html: {class: "form-horizontal block-form", method: :put }, autocomplete: 'off') do |f|
+      = f.error_messages
+
       %fieldset
         %label{for: "user_password"}
           = t('devise.passwords.edit.new_password')

app/views/users/edit.html.haml

@@ -43,8 +43,9 @@
         .span12
           %h3 
             = t('.change_password')
-          = form_for 'user', :url => user_path, :html => { :method => :put } do |f|
-            = f.error_messages
+          = form_for @user, :url => user_path, :html => { :method => :put } do |f|
+            - if (@user.errors.keys & [:password, :password_confirmation, :current_password]).present?
+              = f.error_messages
             %p
               = f.label :current_password, t('.current_password')
               = f.password_field :current_password, :placeholder => t('.current_password_expl')
@@ -58,7 +59,7 @@
             .submit_block
               = link_to t('cancel'), edit_user_path
               = t('or')
-              = f.submit t('.change_password'), :class => "btn"
+              = f.submit t('.change_password'), class: "btn", name: 'change_password'
 
       %hr
 
@@ -67,8 +68,6 @@
           %h3 
             = t('.change_language')
           = form_for 'user', :url => user_path, :html => { :method => :put } do |f|
-            = f.error_messages
-
             .form-inline
               = f.select :language, available_language_options
               = f.submit t('.change_language'), :class => "btn"
@@ -81,7 +80,6 @@
           %h3#stream-preferences
             = t('.stream_preferences')
           = form_for current_user, :url => user_path, :html => { :method => :put } do |f|
-            = f.error_messages
 
             = f.fields_for :stream_preferences do |type|
               #stream_prefs
@@ -106,8 +104,6 @@
           %h3#auto-follow-back-preferences
             = t('.following')
           = form_for current_user, :url => user_path, :html => { :method => :put } do |f|
-            = f.error_messages
-          
             = f.label :auto_follow_back, :class => "checkbox" do
               = f.check_box :auto_follow_back
               = t('.auto_follow_back')
@@ -126,8 +122,6 @@
           %h3 
             = t('.receive_email_notifications')
           = form_for 'user', :url => user_path, :html => { :method => :put } do |f|
-            = f.error_messages
-
             = f.fields_for :email_preferences do |type|
               #email_prefs
                 - if current_user.admin?
@@ -173,7 +167,7 @@
 
             .small-horizontal-spacer
             
-            = f.submit t('.change'), :class => "btn"
+            = f.submit t('.change'), :class => "btn", :id => "change_email_preferences"
 
       %hr
 

features/desktop/change_email.feature

@@ -11,3 +11,11 @@ Feature: Change email
     And I follow the "confirm_email" link from the last sent email
     Then I should see "activated"
     And my "email" should be "new_email@newplac.es"
+
+  Scenario: Change my email preferences
+    Given I am signed in
+    When I go to the users edit page
+    And I uncheck "user_email_preferences_mentioned"
+    And I press "change_email_preferences"
+    Then I should see "Email notifications changed"
+    And the "user_email_preferences_mentioned" checkbox should not be checked

features/desktop/change_password.feature

@@ -11,6 +11,15 @@ Feature: Change password
     When I sign in with password "newsecret"
     Then I should be on the stream page
 
+  Scenario: Attempt to change my password with invalid input
+    Given I am signed in
+    When I go to the edit user page
+    And I fill out change password section with my password and "too" and "short"
+    And I press "Change password"
+    Then I should see "Password change failed"
+    And I should see "Password is too short"
+    And I should see "Password confirmation doesn't match"
+
   Scenario: Reset my password
     Given a user named "Georges Abitbol" with email "forgetful@users.net"
     Given I am on forgot password page
@@ -25,6 +34,18 @@ Feature: Change password
     And I sign in manually as "georges_abitbol" with password "supersecret"
     Then I should be on the stream page
 
+  Scenario: Attempt to reset password with invalid password
+    Given a user named "Georges Abitbol" with email "forgetful@users.net"
+    Given I am on forgot password page
+    When I fill out forgot password form with "forgetful@users.net"
+    And I submit forgot password form
+    When I follow the "Change my password" link from the last sent email
+    When I fill out reset password form with "too" and "short"
+    And I press "Change my password"
+    Then I should be on the user password page
+    And I should see "Password is too short"
+    And I should see "Password confirmation doesn't match"
+
   Scenario: Attempt to reset password with invalid email
     Given I am on forgot password page
     When I fill out forgot password form with "notanemail"

spec/controllers/users_controller_spec.rb

@@ -112,9 +112,9 @@ describe UsersController, :type => :controller do
       }.not_to change(@user, :diaspora_handle)
     end
 
-    it 'redirects to the user edit page' do
+    it 'renders the user edit page' do
       put :update, @params
-      expect(response).to redirect_to edit_user_path
+      expect(response).to render_template('edit')
     end
 
     it 'responds with a 204 on a js request' do
@@ -122,17 +122,21 @@ describe UsersController, :type => :controller do
       expect(response.status).to eq(204)
     end
 
-    context 'password updates' do
-      before do
-        @password_params = {:current_password => 'bluepin7',
-                            :password => "foobaz",
-                            :password_confirmation => "foobaz"}
+    describe 'password updates' do
+      let(:password_params) do
+        {:current_password => 'bluepin7',
+         :password => "foobaz",
+         :password_confirmation => "foobaz"}
+      end
+
+      let(:params) do
+        {id: @user.id, user: password_params, change_password: 'Change Password'}
       end
 
       it "uses devise's update with password" do
-        expect(@user).to receive(:update_with_password).with(hash_including(@password_params))
+        expect(@user).to receive(:update_with_password).with(hash_including(password_params))
         allow(@controller).to receive(:current_user).and_return(@user)
-        put :update, :id => @user.id, :user => @password_params
+        put :update, params
       end
     end