From 3b7bc3f2bb7fcd3a295127f6fc3dd59116f88d6b Mon Sep 17 00:00:00 2001
From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com>
Date: Thu, 9 Dec 2021 15:32:07 +0000
Subject: [PATCH] Validate that password match when updating and show errors if
 they don't (#158)

* Validate that password match and show error if not

* User spec

* Test user update failure

* Format error messages for Form builder gem

* Test error message
---
 app/controllers/users_controller.rb     | 20 +++++++++-
 app/views/users/edit.html.erb           |  2 +
 app/views/users/edit_password.html.erb  | 14 ++++---
 app/views/users/reset_password.html.erb |  2 +
 spec/features/user_spec.rb              |  2 +-
 spec/requests/user_controller_spec.rb   | 49 +++++++++++++++++++++++++
 6 files changed, 81 insertions(+), 8 deletions(-)

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index a65b1f137..33f23e41a 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -10,6 +10,12 @@ class UsersController < ApplicationController
       bypass_sign_in @user
       flash[:notice] = I18n.t("devise.passwords.updated") if user_params.key?("password")
       redirect_to user_path(@user)
+    elsif user_params.key?("password")
+      format_error_messages
+      render :edit_password, status: :unprocessable_entity
+    else
+      format_error_messages
+      render :edit, status: :unprocessable_entity
     end
   end
 
@@ -39,6 +45,18 @@ class UsersController < ApplicationController
 
 private
 
+  def format_error_messages
+    errors = @user.errors.to_hash
+    @user.errors.clear
+    errors.each do |attribute, message|
+      @user.errors.add attribute.to_sym, format_error_message(attribute, message)
+    end
+  end
+
+  def format_error_message(attribute, message)
+    [attribute.to_s.humanize.capitalize, message].join(" ")
+  end
+
   def password_params
     { password: SecureRandom.hex(8) }
   end
@@ -48,7 +66,7 @@ private
   end
 
   def user_params
-    params.require(:user).permit(:email, :name, :password, :role)
+    params.require(:user).permit(:email, :name, :password, :password_confirmation, :role)
   end
 
   def find_resource
diff --git a/app/views/users/edit.html.erb b/app/views/users/edit.html.erb
index 3d4d5e7b8..f355ba993 100644
--- a/app/views/users/edit.html.erb
+++ b/app/views/users/edit.html.erb
@@ -10,6 +10,8 @@
 <%= form_for(current_user, as: :user, html: { method: :patch }) do |f| %>
   <div class="govuk-grid-row">
     <div class="govuk-grid-column-two-thirds">
+      <%= f.govuk_error_summary %>
+
       <h1 class="govuk-heading-l">
         <%= content_for(:title) %>
       </h1>
diff --git a/app/views/users/edit_password.html.erb b/app/views/users/edit_password.html.erb
index e95f4e869..9c1f29088 100644
--- a/app/views/users/edit_password.html.erb
+++ b/app/views/users/edit_password.html.erb
@@ -7,23 +7,25 @@
   ) %>
 <% end %>
 
-<%= form_for(current_user, as: :user, html: { method: :patch }) do |f| %>
+<%= form_for(@user, as: :user, html: { method: :patch }) do |f| %>
   <div class="govuk-grid-row">
     <div class="govuk-grid-column-two-thirds">
+      <%= f.govuk_error_summary %>
+
       <h1 class="govuk-heading-l">
         <%= content_for(:title) %>
       </h1>
 
-      <%= f.govuk_password_field :current_password,
-        label: { text: "Current password" },
-        autocomplete: "current-password"
-      %>
-
       <%= f.govuk_password_field :password,
+        label: { text: "New password" },
         hint: @minimum_password_length ? { text: "Your password must be at least #{@minimum_password_length} characters and hard to guess." } : nil,
         autocomplete: "new-password"
       %>
 
+      <%= f.govuk_password_field :password_confirmation,
+        label: { text: "Confirm new password" }
+      %>
+
       <%= f.govuk_submit "Update" %>
     </div>
   </div>
diff --git a/app/views/users/reset_password.html.erb b/app/views/users/reset_password.html.erb
index 656eaa264..e12a0beed 100644
--- a/app/views/users/reset_password.html.erb
+++ b/app/views/users/reset_password.html.erb
@@ -11,6 +11,8 @@
   <%= f.hidden_field :reset_password_token %>
   <div class="govuk-grid-row">
     <div class="govuk-grid-column-two-thirds">
+      <%= f.govuk_error_summary %>
+
       <h1 class="govuk-heading-l">
         <%= content_for(:title) %>
       </h1>
diff --git a/spec/features/user_spec.rb b/spec/features/user_spec.rb
index f14cd43fd..7e4e957f5 100644
--- a/spec/features/user_spec.rb
+++ b/spec/features/user_spec.rb
@@ -146,8 +146,8 @@ RSpec.describe "User Features" do
       visit("/users/#{user.id}")
       find('[data-qa="change-password"]').click
       expect(page).to have_content("Change your password")
-      fill_in("user[current_password]", with: "pAssword1")
       fill_in("user[password]", with: "Password123!")
+      fill_in("user[password_confirmation]", with: "Password123!")
       click_button("Update")
       expect(page).to have_current_path("/users/#{user.id}")
     end
diff --git a/spec/requests/user_controller_spec.rb b/spec/requests/user_controller_spec.rb
index c247ced48..4f55ac00a 100644
--- a/spec/requests/user_controller_spec.rb
+++ b/spec/requests/user_controller_spec.rb
@@ -44,6 +44,25 @@ RSpec.describe UsersController, type: :request do
         get "/users/password/edit?reset_password_token=#{enc}"
         expect(page).to have_css("h1", class: "govuk-heading-l", text: "Reset your password")
       end
+
+      context "update password" do
+        let(:params) do
+          {
+            id: user.id, user: { password: new_value, password_confirmation: "something_else" }
+          }
+        end
+
+        before do
+          sign_in user
+          put "/users/#{user.id}", headers: headers, params: params
+        end
+
+        it "shows an error if passwords don't match" do
+          expect(response).to have_http_status(:unprocessable_entity)
+          expect(page).to have_selector("#error-summary-title")
+          expect(page).to have_content("Password confirmation doesn't match Password")
+        end
+      end
     end
   end
 
@@ -132,6 +151,18 @@ RSpec.describe UsersController, type: :request do
       end
     end
 
+    context "update fails to persist" do
+      before do
+        allow_any_instance_of(User).to receive(:update).and_return(false)
+        sign_in user
+        patch "/users/#{user.id}", headers: headers, params: params
+      end
+
+      it "show an error" do
+        expect(response).to have_http_status(:unprocessable_entity)
+      end
+    end
+
     context "current user is another user" do
       let(:params) { { id: unauthorised_user.id, user: { name: new_value } } }
 
@@ -144,5 +175,23 @@ RSpec.describe UsersController, type: :request do
         expect(response).to have_http_status(:not_found)
       end
     end
+
+    context "update password" do
+      let(:params) do
+        {
+          id: user.id, user: { password: new_value, password_confirmation: "something_else" }
+        }
+      end
+
+      before do
+        sign_in user
+        patch "/users/#{user.id}", headers: headers, params: params
+      end
+
+      it "shows an error if passwords don't match" do
+        expect(response).to have_http_status(:unprocessable_entity)
+        expect(page).to have_selector("#error-summary-title")
+      end
+    end
   end
 end