FIX: Show error highlighting on password field in finish installation form

ZogStriP · ZogStriP · commit b39afff21417 · 2025-12-13T22:10:38.000+01:00
When submitting the admin registration form with an invalid password (blank or too short), the password field was not visually highlighted with the error state, making it unclear what was wrong. Two issues were causing this: 1. Copy-paste error: The password field container was checking `@user.errors[:username]` instead of `@user.errors[:password]` 2. Wrong error key: Password length validation errors from the UserPassword model are stored under `:"user_password.password"`, not `:password`. The template only checked the latter. Fixed by checking both error keys and displaying all password-related errors with proper visual highlighting. Added system spec with page object for UI behavior testing and trimmed request spec to cover edge cases that require stubs (like empty developer_emails). Ref - https://meta.discourse.org/t/381505
diff --git a/app/views/finish_installation/register.html.erb b/app/views/finish_installation/register.html.erb
@@ -30,7 +30,8 @@
       <%- end %>
     </div>
 
-    <div class='wizard-container__input wizard-container__field text-field <% if @user.errors[:username].present? %>invalid<% end %>'>
+    <%- password_errors = @user.errors[:password] + @user.errors[:"user_password.password"] %>
+    <div class='wizard-container__input wizard-container__field text-field <% if password_errors.present? %>invalid<% end %>'>
       <label for="password">
         <span class="wizard-container__label"><%= t 'js.user.password.title' %></span>
       </label>
@@ -40,7 +41,7 @@
       <div class='input-area'>
         <%= password_field_tag(:password, params[:password], class:"wizard-container__text-input") %>
       </div>
-      <% @user.errors[:password].each do |e| %>
+      <% password_errors.each do |e| %>
         <div class='field-error-description'><%= e.to_s %></div>
       <% end %>
     </div>
diff --git a/spec/requests/finish_installation_controller_spec.rb b/spec/requests/finish_installation_controller_spec.rb
@@ -1,141 +1,65 @@
 # frozen_string_literal: true
 
 RSpec.describe FinishInstallationController do
-  describe "#index" do
-    context "when has_login_hint is false" do
-      before { SiteSetting.has_login_hint = false }
-
-      it "doesn't allow access" do
-        get "/finish-installation"
-        expect(response.status).to eq(403)
-      end
-    end
-
-    context "when has_login_hint is true" do
-      before { SiteSetting.has_login_hint = true }
-
-      it "allows access" do
-        get "/finish-installation"
-        expect(response.status).to eq(200)
-      end
-    end
-  end
-
   describe "#register" do
-    context "when has_login_hint is false" do
-      before { SiteSetting.has_login_hint = false }
-
-      it "doesn't allow access" do
-        get "/finish-installation/register"
-        expect(response.status).to eq(403)
-      end
+    before do
+      SiteSetting.has_login_hint = true
+      GlobalSetting.stubs(:developer_emails).returns("robin@example.com")
     end
 
-    context "when has_login_hint is true" do
-      before do
-        SiteSetting.has_login_hint = true
-        GlobalSetting.stubs(:developer_emails).returns("robin@example.com")
-      end
-
-      it "allows access" do
-        get "/finish-installation/register"
-        expect(response.status).to eq(200)
-      end
-
-      it "raises an error when the email is not in the allowed list" do
-        post "/finish-installation/register.json",
-             params: {
-               email: "notrobin@example.com",
-               username: "eviltrout",
-               password: "disismypasswordokay",
-             }
-        expect(response.status).to eq(400)
-      end
-
-      it "doesn't redirect when fields are wrong" do
-        post "/finish-installation/register",
-             params: {
-               email: "robin@example.com",
-               username: "",
-               password: "disismypasswordokay",
-             }
-
-        expect(response).not_to be_redirect
-      end
-
-      context "with working params" do
-        let(:params) do
-          { email: "robin@example.com", username: "eviltrout", password: "disismypasswordokay" }
-        end
-
-        it "registers the admin when the email is in the list" do
-          expect do post "/finish-installation/register.json", params: params end.to change {
-            Jobs::CriticalUserEmail.jobs.size
-          }.by(1)
-
-          expect(response).to be_redirect
-          expect(User.where(username: "eviltrout").exists?).to eq(true)
-        end
-
-        it "automatically resends the signup email when the user already exists" do
-          expect do post "/finish-installation/register.json", params: params end.to change {
-            Jobs::CriticalUserEmail.jobs.size
-          }.by(1)
-
-          expect(User.where(username: "eviltrout").exists?).to eq(true)
-
-          expect do post "/finish-installation/register.json", params: params end.to change {
-            Jobs::CriticalUserEmail.jobs.size
-          }.by(1)
-
-          expect(response).to be_redirect
-          expect(User.where(username: "eviltrout").exists?).to eq(true)
-        end
-      end
-
-      it "sets the admins trust level" do
-        post "/finish-installation/register.json",
-             params: {
-               email: "robin@example.com",
-               username: "eviltrout",
-               password: "disismypasswordokay",
-             }
+    it "shows no_emails message when developer_emails is empty" do
+      GlobalSetting.stubs(:developer_emails).returns("")
+      get "/finish-installation/register"
+      expect(response.status).to eq(200)
+      expect(response.body).to include(I18n.t("finish_installation.register.no_emails"))
+    end
 
-        expect(User.find_by(username: "eviltrout").trust_level).to eq 1
-      end
+    it "returns 400 when email is not in the allowed list" do
+      post "/finish-installation/register.json",
+           params: {
+             email: "notrobin@example.com",
+             username: "eviltrout",
+             password: "disismypasswordokay",
+           }
+      expect(response.status).to eq(400)
     end
   end
 
   describe "#confirm_email" do
-    context "when has_login_hint is false" do
-      before { SiteSetting.has_login_hint = false }
-
-      it "shows the page" do
-        get "/finish-installation/confirm-email"
-        expect(response.status).to eq(200)
-      end
+    it "renders without requiring has_login_hint" do
+      SiteSetting.has_login_hint = false
+      get "/finish-installation/confirm-email"
+      expect(response.status).to eq(200)
     end
   end
 
   describe "#resend_email" do
     before do
       SiteSetting.has_login_hint = true
       GlobalSetting.stubs(:developer_emails).returns("robin@example.com")
+    end
 
+    it "resends activation email for user in session" do
       post "/finish-installation/register",
            params: {
              email: "robin@example.com",
              username: "eviltrout",
              password: "disismypasswordokay",
            }
-    end
 
-    it "resends the email" do
-      expect do put "/finish-installation/resend-email" end.to change {
+      expect { put "/finish-installation/resend-email" }.to change {
         Jobs::CriticalUserEmail.jobs.size
       }.by(1)
 
       expect(response.status).to eq(200)
     end
+
+    it "does nothing when user doesn't exist" do
+      expect { put "/finish-installation/resend-email" }.not_to change {
+        Jobs::CriticalUserEmail.jobs.size
+      }
+
+      expect(response.status).to eq(200)
+    end
   end
 end
diff --git a/spec/system/finish_installation_spec.rb b/spec/system/finish_installation_spec.rb
@@ -1,20 +1,98 @@
 # frozen_string_literal: true
 
 RSpec.describe "Finish Installation", type: :system do
-  before do
-    SiteSetting.has_login_hint = true
-    GlobalSetting.stubs(:developer_emails).returns("dev@example.com")
+  let(:finish_installation_page) { PageObjects::Pages::FinishInstallation.new }
+
+  context "when has_login_hint is false" do
+    before { SiteSetting.has_login_hint = false }
+
+    it "denies access" do
+      finish_installation_page.visit_index
+      expect(finish_installation_page).to have_access_denied
+    end
   end
 
-  it "renders first screen" do
-    visit "/finish-installation"
+  context "when has_login_hint is true" do
+    before do
+      SiteSetting.has_login_hint = true
+      GlobalSetting.stubs(:developer_emails).returns("admin@example.com,other@example.com")
+    end
+
+    it "shows validation error when username is blank" do
+      finish_installation_page.visit_register.fill_password("supersecurepassword").submit
+      expect(finish_installation_page).to have_username_error
+    end
+
+    it "shows validation error when password is blank" do
+      finish_installation_page.visit_register.fill_username("newadmin").submit
+      expect(finish_installation_page).to have_password_error
+    end
+
+    it "shows validation error when password is too short" do
+      finish_installation_page
+        .visit_register
+        .fill_username("newadmin")
+        .fill_password("short")
+        .submit
+      expect(finish_installation_page).to have_password_error("too short")
+    end
+
+    it "registers admin and redirects to confirm email page" do
+      finish_installation_page
+        .visit_register
+        .select_email("admin@example.com")
+        .fill_username("newadmin")
+        .fill_password("supersecurepassword")
+        .submit
+
+      expect(finish_installation_page).to be_redirected_to_confirm_email
+      expect(User.find_by(username: "newadmin")).to have_attributes(
+        email: "admin@example.com",
+        trust_level: 1,
+      )
+    end
+
+    it "handles multiple developer emails" do
+      finish_installation_page
+        .visit_register
+        .select_email("other@example.com")
+        .fill_username("otheradmin")
+        .fill_password("supersecurepassword")
+        .submit
+
+      expect(finish_installation_page).to be_redirected_to_confirm_email
+      expect(User.find_by(username: "otheradmin")).to be_present
+    end
+
+    it "resends activation email when user already exists" do
+      Fabricate(:user, email: "admin@example.com")
+
+      expect {
+        finish_installation_page
+          .visit_register
+          .select_email("admin@example.com")
+          .fill_username("differentuser")
+          .fill_password("supersecurepassword")
+          .submit
+      }.to change { Jobs::CriticalUserEmail.jobs.size }.by(1)
+
+      expect(finish_installation_page).to be_redirected_to_confirm_email
+    end
 
-    find(".finish-installation__register").click
+    it "does not send email when user is already active and confirmed" do
+      user = Fabricate(:user, email: "admin@example.com", active: true)
+      user.activate
 
-    expect(page).to have_css(".wizard-container__combobox") # email field
-    expect(page).to have_css(".input-area")
-    expect(page).to have_css(".wizard-container__button") # submit button
+      expect {
+        finish_installation_page
+          .visit_register
+          .select_email("admin@example.com")
+          .fill_username("differentuser")
+          .fill_password("supersecurepassword")
+          .submit
+      }.not_to change { Jobs::CriticalUserEmail.jobs.size }
 
-    # TODO: add more steps here to ensure full flow works
+      expect(finish_installation_page).to be_redirected_to_confirm_email
+    end
   end
 end
diff --git a/spec/system/page_objects/pages/finish_installation.rb b/spec/system/page_objects/pages/finish_installation.rb
@@ -0,0 +1,75 @@
+# frozen_string_literal: true
+
+module PageObjects
+  module Pages
+    class FinishInstallation < PageObjects::Pages::Base
+      def visit_register
+        visit("/finish-installation/register")
+        self
+      end
+
+      def visit_index
+        visit("/finish-installation")
+        self
+      end
+
+      def has_register_form?
+        has_css?("form.wizard-container__fields")
+      end
+
+      def has_no_register_form?
+        has_no_css?("form.wizard-container__fields")
+      end
+
+      def has_no_emails_message?
+        has_css?("p", text: I18n.t("finish_installation.register.no_emails"))
+      end
+
+      def has_access_denied?
+        has_css?(".not-found-container") || page.status_code == 403
+      end
+
+      def fill_username(username)
+        find("#username").fill_in(with: username)
+        self
+      end
+
+      def fill_password(password)
+        find("#password").fill_in(with: password)
+        self
+      end
+
+      def select_email(email)
+        find("#email").select(email)
+        self
+      end
+
+      def submit
+        find("input[type='submit']").click
+        self
+      end
+
+      def has_username_error?(message = nil)
+        field = find(".wizard-container__field", text: "Username")
+        return false if field[:class].exclude?("invalid")
+        return true if message.nil?
+        field.has_css?(".field-error-description", text: message)
+      end
+
+      def has_password_error?(message = nil)
+        field = find(".wizard-container__field", text: "Password")
+        return false if field[:class].exclude?("invalid")
+        return true if message.nil?
+        field.has_css?(".field-error-description", text: message)
+      end
+
+      def has_no_field_errors?
+        has_no_css?(".wizard-container__field.invalid")
+      end
+
+      def redirected_to_confirm_email?
+        has_current_path?("/finish-installation/confirm-email")
+      end
+    end
+  end
+end
