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| %>
+ <%= f.govuk_error_summary %> +

<%= content_for(:title) %>

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| %>
+ <%= f.govuk_error_summary %> +

<%= content_for(:title) %>

- <%= 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" %>
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 %>
+ <%= f.govuk_error_summary %> +

<%= content_for(:title) %>

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