diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index e156fbffd..bf4130bf9 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -10,8 +10,10 @@ class UsersController < ApplicationController if @user == current_user bypass_sign_in @user flash[:notice] = I18n.t("devise.passwords.updated") if user_params.key?("password") + redirect_to account_path + else + redirect_to user_path(@user) end - redirect_to user_path(@user) elsif user_params.key?("password") format_error_messages @minimum_password_length = User.password_length.min @@ -87,7 +89,7 @@ private end def find_resource - @user = User.find_by(id: params[:id]) + @user = params[:id] ? User.find_by(id: params[:id]) : current_user end def authenticate_scope! diff --git a/app/helpers/user_helper.rb b/app/helpers/user_helper.rb new file mode 100644 index 000000000..a9f6c920d --- /dev/null +++ b/app/helpers/user_helper.rb @@ -0,0 +1,9 @@ +module UserHelper + def aliased_user_edit(user, current_user) + current_user == user ? edit_account_path : edit_user_path(user) + end + + def pronoun(user, current_user) + current_user == user ? "you" : "they" + end +end diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 84814d199..aabcbc173 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -52,7 +52,7 @@ elsif component.navigation_item(text: 'Logs', href: case_logs_path) component.navigation_item(text: 'Your organisation', href: "/organisations/#{current_user.organisation.id}") - component.navigation_item(text: 'Your account', href: user_path(current_user)) + component.navigation_item(text: 'Your account', href: account_path) component.navigation_item(text: 'Sign out', href: destroy_user_session_path) end end diff --git a/app/views/users/edit.html.erb b/app/views/users/edit.html.erb index 985dd4d6d..fb7a885ba 100644 --- a/app/views/users/edit.html.erb +++ b/app/views/users/edit.html.erb @@ -36,7 +36,7 @@ :id, :name, inline: true, - legend: { text: "Are #{current_user == @user ? "you" : "they"} a data protection officer?", size: "m" } + legend: { text: "Are #{pronoun(@user, current_user)} a data protection officer?", size: "m" } %> <%= f.govuk_collection_radio_buttons :is_key_contact, @@ -44,7 +44,7 @@ :id, :name, inline: true, - legend: { text: "Are #{current_user == @user ? "you" : "they"} a key contact?", size: "m" } + legend: { text: "Are #{pronoun(@user, current_user)} a key contact?", size: "m" } %> <% end %> diff --git a/app/views/users/new.html.erb b/app/views/users/new.html.erb index bc9e0b1f0..7af8daa0c 100644 --- a/app/views/users/new.html.erb +++ b/app/views/users/new.html.erb @@ -37,7 +37,7 @@ :id, :name, inline: true, - legend: { text: "Are #{current_user == @user ? "you" : "they"} a data protection officer?", size: "m" } + legend: { text: "Are #{pronoun(@user, current_user)} a data protection officer?", size: "m" } %> <%= f.govuk_collection_radio_buttons :is_key_contact, @@ -45,7 +45,7 @@ :id, :name, inline: true, - legend: { text: "Are #{current_user == @user ? "you" : "they"} a key contact?", size: "m" } + legend: { text: "Are #{pronoun(@user, current_user)} a key contact?", size: "m" } %> <% end %> diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index 319ab692d..6222fc471 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -12,66 +12,66 @@ <%= govuk_summary_list do |summary_list| %> <%= summary_list.row do |row| - row.key { 'Name' } + row.key { "Name" } row.value { @user.name } if current_user == @user || current_user.data_coordinator? - row.action(visually_hidden_text: 'name', href: edit_user_path, html_attributes: { 'data-qa': 'change-name' }) + row.action(visually_hidden_text: "name", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-name" }) else row.action() end end %> <%= summary_list.row() do |row| - row.key { 'Email address' } + row.key { "Email address" } row.value { @user.email } if current_user == @user || current_user.data_coordinator? - row.action(visually_hidden_text: 'email address', href: edit_user_path, html_attributes: { 'data-qa': 'change-email' }) + row.action(visually_hidden_text: "email address", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-email-address" }) else row.action() end end %> <%= summary_list.row do |row| - row.key { 'Password' } - row.value { '••••••••' } + row.key { "Password" } + row.value { "••••••••" } if current_user == @user - row.action(visually_hidden_text: 'password', href: password_edit_user_path, html_attributes: { 'data-qa': 'change-password' }) + row.action(visually_hidden_text: "password", href: edit_password_account_path, html_attributes: { "data-qa": "change-password" }) else row.action() end end %> <%= summary_list.row do |row| - row.key { 'Organisation' } + row.key { "Organisation" } row.value { @user.organisation.name } row.action() end %> <%= summary_list.row do |row| - row.key { 'Role' } + row.key { "Role" } row.value { @user.role.humanize } if current_user.data_coordinator? - row.action(visually_hidden_text: "role", href: edit_user_path, html_attributes: { "data-qa": "role" }) + row.action(visually_hidden_text: "role", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-role" }) else row.action() end end %> <%= summary_list.row do |row| - row.key { 'Data protection officer' } + row.key { "Data protection officer" } row.value { @user.is_data_protection_officer? ? "Yes" : "No" } if current_user.data_coordinator? - row.action(visually_hidden_text: "are #{current_user == @user ? "you" : "they"} a data protection officer?", href: edit_user_path, html_attributes: { "data-qa": "change-are-#{current_user == @user ? "you" : "they"}-a-data-protection-officer" }) + row.action(visually_hidden_text: "are #{pronoun(@user, current_user)} a data protection officer?", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-are-#{pronoun(@user, current_user)}-a-data-protection-officer" }) else row.action() end end %> <%= summary_list.row do |row| - row.key { 'Key contact' } + row.key { "Key contact" } row.value { @user.is_key_contact? ? "Yes" : "No" } if current_user.data_coordinator? - row.action(visually_hidden_text: "are #{current_user == @user ? "you" : "they"} a key contact?", href: edit_user_path, html_attributes: { "data-qa": "change-are-#{current_user == @user ? "you" : "they"}-a-key-contact" }) + row.action(visually_hidden_text: "are #{pronoun(@user, current_user)} a key contact?", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-are-#{pronoun(@user, current_user)}-a-key-contact" }) else row.action() end diff --git a/config/routes.rb b/config/routes.rb index 25a0a8e43..591f9cad5 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -36,6 +36,7 @@ Rails.application.routes.draw do devise_scope :user do get "account/password/reset-confirmation", to: "auth/passwords#reset_confirmation" + put "account", to: "users#update" end get "/health", to: ->(_) { [204, {}, [nil]] } @@ -48,12 +49,12 @@ Rails.application.routes.draw do get "/privacy-notice", to: "content#privacy_notice" get "/data-sharing-agreement", to: "content#data_sharing_agreement" - resources :users do - member do - get "password/edit", to: "users#edit_password" - end + resource :account, only: %i[show edit], controller: "users" do + get "edit/password", to: "users#edit_password" end + resources :users + resources :organisations do member do get "details", to: "organisations#details" diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb new file mode 100644 index 000000000..36d7b8da0 --- /dev/null +++ b/spec/controllers/users_controller_spec.rb @@ -0,0 +1,20 @@ +require "rails_helper" + +RSpec.describe UsersController, type: :controller do + let(:params) { { id: other_user.id } } + let(:user) { FactoryBot.create(:user, :data_coordinator) } + let(:other_user) { FactoryBot.create(:user, organisation: user.organisation) } + + before do + sign_in user + end + + describe "GET #edit_password" do + context "when trying to view the edit page for another user in your organisation" do + it "does not let you and returns not found" do + get :edit_password, params: params + expect(response).to have_http_status(:not_found) + end + end + end +end diff --git a/spec/features/user_spec.rb b/spec/features/user_spec.rb index 8ff9bed98..d7326dd7e 100644 --- a/spec/features/user_spec.rb +++ b/spec/features/user_spec.rb @@ -182,13 +182,13 @@ RSpec.describe "User Features" do end it "does not have change links for dpo and key contact" do - visit("/users/#{user.id}") + visit("/account") expect(page).not_to have_selector('[data-qa="change-are-you-a-data-protection-officer"]') expect(page).not_to have_selector('[data-qa="change-are-you-a-key-contact"]') end it "does not have dpo and key contact as editable fields" do - visit("/users/#{user.id}/edit") + visit("/account/edit") expect(page).not_to have_field("user[is_dpo]") expect(page).not_to have_field("user[is_key_contact]") end @@ -210,31 +210,31 @@ RSpec.describe "User Features" do visit("/logs") expect(page).to have_link("Your account") click_link("Your account") - expect(page).to have_current_path("/users/#{user.id}") + expect(page).to have_current_path("/account") end it "can navigate to change your password page from main account page" do - visit("/users/#{user.id}") + visit("/account") find('[data-qa="change-password"]').click expect(page).to have_content("Change your password") 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}") + expect(page).to have_current_path("/account") end it "allow user to change name" do - visit("/users/#{user.id}") + visit("/account") find('[data-qa="change-name"]').click expect(page).to have_content("Change your personal details") fill_in("user[name]", with: "Test New") click_button("Save changes") - expect(page).to have_current_path("/users/#{user.id}") + expect(page).to have_current_path("/account") expect(page).to have_content("Test New") end it "has dpo and key contact as editable fields" do - visit("/users/#{user.id}") + visit("/account") expect(page).to have_selector('[data-qa="change-are-you-a-data-protection-officer"]') expect(page).to have_selector('[data-qa="change-are-you-a-key-contact"]') end diff --git a/spec/helpers/user_helper_spec.rb b/spec/helpers/user_helper_spec.rb new file mode 100644 index 000000000..6386ddd4f --- /dev/null +++ b/spec/helpers/user_helper_spec.rb @@ -0,0 +1,38 @@ +require "rails_helper" + +RSpec.describe UserHelper do + let(:current_user) { FactoryBot.create(:user, :data_coordinator) } + let(:user) { FactoryBot.create(:user, :data_coordinator) } + + describe "aliased_user_edit" do + context "when the current logged in user is the same as the user being viewed" do + let(:user) { current_user } + + it "returns the edit account path" do + expect(aliased_user_edit(user, current_user)).to eq(edit_account_path) + end + end + + context "when the current logged in user is not the same as the user being viewed" do + it "returns the edit user path" do + expect(aliased_user_edit(user, current_user)).to eq(edit_user_path(user)) + end + end + end + + describe "pronoun" do + context "when the current logged in user is the same as the user being viewed" do + let(:user) { current_user } + + it "returns 'you'" do + expect(pronoun(user, current_user)).to eq("you") + end + end + + context "when the current logged in user is not the same as the user being viewed" do + it "returns 'they'" do + expect(pronoun(user, current_user)).to eq("they") + end + end + end +end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 68ddc139d..26ada9557 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -34,7 +34,7 @@ RSpec.describe UsersController, type: :request do describe "#password" do it "does not let you edit user passwords" do - get "/users/#{user.id}/password/edit", headers: headers, params: {} + get "/account/edit/password", headers: headers, params: {} expect(response).to redirect_to("/account/sign-in") end end @@ -63,7 +63,7 @@ RSpec.describe UsersController, type: :request do before do sign_in user - put "/users/#{user.id}", headers: headers, params: params + put "/account", headers: headers, params: params end it "shows an error if passwords don't match" do @@ -204,7 +204,7 @@ RSpec.describe UsersController, type: :request do context "when the current user matches the user ID" do before do sign_in user - get "/users/#{user.id}/password/edit", headers: headers, params: {} + get "/account/edit/password", headers: headers, params: {} end it "shows the edit password page" do @@ -453,7 +453,7 @@ RSpec.describe UsersController, type: :request do context "when the current user matches the user ID" do before do sign_in user - get "/users/#{user.id}/password/edit", headers: headers, params: {} + get "/account/edit/password", headers: headers, params: {} end it "shows the edit password page" do @@ -468,11 +468,12 @@ RSpec.describe UsersController, type: :request do context "when the current user does not matches the user ID" do before do sign_in user - get "/users/#{other_user.id}/password/edit", headers: headers, params: {} end - it "returns not found 404" do - expect(response).to have_http_status(:not_found) + it "there is no route" do + expect { + get "/users/#{other_user.id}/password/edit", headers: headers, params: {} + }.to raise_error(ActionController::RoutingError) end end end