CLDC-1091: User Account URLs (#448)

* Works but relies on routes ordering

* Add helper test

* Actually work

* Test edit password auth even though we don't have a route for it

* Add spec description

* Consistency

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!

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

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

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 %>
 

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 %>
 

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.key { "Password" }
-        row.value { '••••••••' }
+        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

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
+  resource :account, only: %i[show edit], controller: "users" do
-    member do
+    get "edit/password", to: "users#edit_password"
-      get "password/edit", to: "users#edit_password"
-    end
   end
 
+  resources :users
+
   resources :organisations do
     member do
       get "details", to: "organisations#details"

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

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

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

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
+        it "there is no route" do
-          expect(response).to have_http_status(:not_found)
+          expect {
+            get "/users/#{other_user.id}/password/edit", headers: headers, params: {}
+          }.to raise_error(ActionController::RoutingError)
         end
       end
     end