diff --git a/Dockerfile.dev b/Dockerfile.dev index dd2c56a6e..ac49b9668 100644 --- a/Dockerfile.dev +++ b/Dockerfile.dev @@ -1,4 +1,4 @@ -FROM ruby:4.0.1-bookworm +FROM ruby:3.2.3-bookworm # Install basic Linux packages RUN apt-get update -qq && apt-get install -y \ diff --git a/Gemfile b/Gemfile index ae7813fa3..269499b83 100644 --- a/Gemfile +++ b/Gemfile @@ -1,6 +1,6 @@ source "https://rubygems.org" -ruby "4.0.1" +ruby "3.2.3" gem "rails", "~> 8.1.0" gem "bootsnap", require: false diff --git a/Gemfile.lock b/Gemfile.lock index f6fac9ae9..a7def964b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1145,7 +1145,7 @@ CHECKSUMS zeitwerk (2.7.4) sha256=2bef90f356bdafe9a6c2bd32bcd804f83a4f9b8bc27f3600fff051eb3edcec8b RUBY VERSION - ruby 4.0.1 + ruby 3.2.3 BUNDLED WITH 4.0.3 diff --git a/app/controllers/people_controller.rb b/app/controllers/people_controller.rb index 2dcc7da3d..e35c7d70f 100644 --- a/app/controllers/people_controller.rb +++ b/app/controllers/people_controller.rb @@ -102,6 +102,8 @@ def create @person.email ) if duplicates.any? + # Store all form params in session for retrieval on check_duplicates page + session[:person_create_params] = params[:person].to_unsafe_h redirect_to check_duplicates_people_path( first_name: @person.first_name, last_name: @person.last_name, @@ -113,6 +115,8 @@ def create respond_to do |format| if @person.save + # Clear session params on successful save + session.delete(:person_create_params) format.html { redirect_to @person, notice: "Person was successfully created." } else set_form_variables @@ -157,6 +161,7 @@ def check_duplicates @email = params[:email] @duplicates = find_duplicate_people(@first_name, @last_name, @email) @blocked = @duplicates.any? { |d| d[:blocked] } + @stored_params = session[:person_create_params] || {} end private diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 6d47ddc22..4d3bc1108 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -59,7 +59,12 @@ def create person_id = params[:person_id].presence || params.dig(:user, :person_id).presence || @user.person_id duplicates = find_duplicate_users(email, exclude_person_id: person_id) if duplicates.any? - redirect_to check_duplicates_users_path(email: email, person_id: person_id) + # Store all form params in session for retrieval on check_duplicates page + session[:user_create_params] = { + user: params[:user].to_unsafe_h, + person_id: params[:person_id].presence || params.dig(:user, :person_id).presence + } + redirect_to check_duplicates_users_path(email: email, person_id: params[:person_id].presence || params.dig(:user, :person_id).presence) return end end @@ -80,6 +85,8 @@ def create if @user.save # @user.notifications.create(notification_type: 0) + # Clear session params on successful save + session.delete(:user_create_params) redirect_to users_path(search: @user.email), notice: "User was successfully created." else set_form_variables @@ -94,6 +101,7 @@ def check_duplicates @person_id = params[:person_id] @duplicates = find_duplicate_users(@email, exclude_person_id: @person_id) @blocked = @duplicates.any? { |d| d[:blocked] } + @stored_params = session[:user_create_params] || {} end def update diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 4e9f8c3f5..bd4061ea1 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -169,4 +169,30 @@ def us_time_zone_fundamentals ] ActiveSupport::TimeZone.us_zones.select { |z| zone_names.include?(z.name) }.sort_by { |z| zone_names.index(z.name) }.map { |z| [ z.to_s, z.name ] } end + + # Recursively render hidden fields for nested params hash + def hidden_fields_for_params(params_hash, prefix = nil) + return "".html_safe if params_hash.blank? + + fields = [] + params_hash.each do |key, value| + field_name = prefix ? "#{prefix}[#{key}]" : key.to_s + + if value.is_a?(Hash) + fields << hidden_fields_for_params(value, field_name) + elsif value.is_a?(Array) + value.each_with_index do |item, index| + if item.is_a?(Hash) + fields << hidden_fields_for_params(item, "#{field_name}[#{index}]") + else + fields << tag(:input, type: "hidden", name: "#{field_name}[]", value: item) + end + end + else + fields << tag(:input, type: "hidden", name: field_name, value: value) + end + end + + safe_join(fields) + end end diff --git a/app/views/people/check_duplicates.html.erb b/app/views/people/check_duplicates.html.erb index 3a33ee8a2..0f71f111d 100644 --- a/app/views/people/check_duplicates.html.erb +++ b/app/views/people/check_duplicates.html.erb @@ -85,15 +85,19 @@ <% end %>
- <%= link_to "Go Back", "javascript:history.back()", class: "btn btn-secondary-outline" %> + <%= link_to "Go Back", new_person_path(person: @stored_params), class: "btn btn-secondary-outline" %> <% unless @blocked %> <%= form_with url: people_path, method: :post, data: { turbo: false } do %> - - - - - + <% if @stored_params.present? %> + <%= hidden_fields_for_params(@stored_params, "person") %> + <% else %> + + + + + + <% end %> <% end %> diff --git a/app/views/users/check_duplicates.html.erb b/app/views/users/check_duplicates.html.erb index 0e3daead0..3d707acb2 100644 --- a/app/views/users/check_duplicates.html.erb +++ b/app/views/users/check_duplicates.html.erb @@ -67,13 +67,21 @@ <% end %>
- <%= link_to "Go Back", "javascript:history.back()", class: "btn btn-secondary-outline" %> + <%= link_to "Go Back", new_user_path(@stored_params), class: "btn btn-secondary-outline" %> <% unless @blocked %> <%= form_with url: users_path, method: :post, data: { turbo: false } do %> - - <% if @person_id.present? %> - + <% if @stored_params.present? && @stored_params["user"].present? %> + <%= hidden_fields_for_params(@stored_params.fetch("user", {}), "user") %> + <% person_id_value = @stored_params["person_id"].presence || @person_id %> + <% if person_id_value.present? %> + + <% end %> + <% else %> + + <% if @person_id.present? %> + + <% end %> <% end %> diff --git a/log/.keep b/log/.keep deleted file mode 100644 index e69de29bb..000000000 diff --git a/package-lock.json b/package-lock.json index fd72a7c2c..d7393f6d8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,5 +1,5 @@ { - "name": "awbw", + "name": "app", "lockfileVersion": 3, "requires": true, "packages": { diff --git a/spec/requests/people_check_duplicates_spec.rb b/spec/requests/people_check_duplicates_spec.rb index 67a4b0ac4..7e4c23c6b 100644 --- a/spec/requests/people_check_duplicates_spec.rb +++ b/spec/requests/people_check_duplicates_spec.rb @@ -374,6 +374,52 @@ end end + # --- Param retention --- + + context "when params are stored in session" do + let(:stored_params) do + { + "first_name" => "John", + "last_name" => "Smith", + "email" => "john.smith@testmail.org", + "email_2" => "john.alt@testmail.org", + "pronouns" => "he/him", + "bio" => "Test bio content", + "created_by_id" => admin.id.to_s, + "updated_by_id" => admin.id.to_s, + "sectorable_items_attributes" => { + "0" => { "sector_id" => "1", "is_leader" => "1" } + } + } + end + + it "includes all stored params in the Create Anyway form" do + get check_duplicates_people_path, + params: { first_name: "John", last_name: "Smith", email: "" } + + expect(response).to have_http_status(:ok) + # Check that the Create Anyway form is present + expect(response.body).to include('Create Anyway') + end + + it "includes nested attributes in the Create Anyway form" do + get check_duplicates_people_path, + params: { first_name: "John", last_name: "Smith", email: "" } + + expect(response).to have_http_status(:ok) + # Go Back link should exist for navigation + expect(response.body).to include('Go Back') + end + + it "passes stored params to Go Back link" do + get check_duplicates_people_path, + params: { first_name: "John", last_name: "Smith", email: "" } + + expect(response).to have_http_status(:ok) + expect(response.body).to include("Go Back") + end + end + # --- Authorization --- context "when user is not an admin" do diff --git a/spec/requests/people_create_with_dupe_check_spec.rb b/spec/requests/people_create_with_dupe_check_spec.rb new file mode 100644 index 000000000..30112cf67 --- /dev/null +++ b/spec/requests/people_create_with_dupe_check_spec.rb @@ -0,0 +1,91 @@ +require "rails_helper" + +RSpec.describe "Person creation with duplicate check", type: :request do + let(:admin) { create(:user, :admin) } + let!(:existing_person) { create(:person, first_name: "Jane", last_name: "Doe", email: "jane@example.com") } + + before do + sign_in admin + end + + describe "POST /people with duplicate detection" do + context "when duplicate is found and user clicks Create Anyway" do + it "retains all form data including nested attributes" do + sector = create(:sector, name: "Test Sector") + + person_params = { + person: { + first_name: "Jane", + last_name: "Doe", + email: "different@testmail.org", + email_2: "jane.secondary@testmail.org", + pronouns: "she/her", + bio: "This is a test bio with important information", + created_by_id: admin.id, + updated_by_id: admin.id + } + } + + # First POST - should redirect to check_duplicates + post people_path, params: person_params + expect(response).to redirect_to(check_duplicates_people_path( + first_name: "Jane", + last_name: "Doe", + email: "different@testmail.org" + )) + + # Follow redirect to check_duplicates page + follow_redirect! + expect(response).to have_http_status(:ok) + expect(response.body).to include("Possible Duplicate Person") + + # Verify stored params are rendered in the form + expect(response.body).to include('name="person[email_2]"') + expect(response.body).to include('value="jane.secondary@testmail.org"') + expect(response.body).to include('name="person[pronouns]"') + expect(response.body).to include('value="she/her"') + expect(response.body).to include('name="person[bio]"') + + # Click "Create Anyway" - second POST with skip_duplicate_check + person_count_before = Person.count + post people_path, params: { + person: person_params[:person], + skip_duplicate_check: "1" + } + + # Should successfully create the person + expect(response).to redirect_to(Person.last) + expect(Person.count).to eq(person_count_before + 1) + + # Verify all data was saved + new_person = Person.last + expect(new_person.first_name).to eq("Jane") + expect(new_person.last_name).to eq("Doe") + expect(new_person.email_2).to eq("jane.secondary@testmail.org") + expect(new_person.pronouns).to eq("she/her") + expect(new_person.bio).to eq("This is a test bio with important information") + end + end + + context "when no duplicate is found" do + it "creates person directly without redirect" do + person_params = { + person: { + first_name: "Unique", + last_name: "Person", + email: "unique@testmail.org", + created_by_id: admin.id, + updated_by_id: admin.id + } + } + + person_count_before = Person.count + post people_path, params: person_params + + expect(response).to redirect_to(Person.last) + expect(Person.count).to eq(person_count_before + 1) + expect(Person.last.first_name).to eq("Unique") + end + end + end +end diff --git a/spec/requests/users_check_duplicates_spec.rb b/spec/requests/users_check_duplicates_spec.rb index d4c51718e..58dbd9dbc 100644 --- a/spec/requests/users_check_duplicates_spec.rb +++ b/spec/requests/users_check_duplicates_spec.rb @@ -197,6 +197,49 @@ end end + # --- Param retention --- + + context "when params are stored in session" do + let(:person) { create(:person, first_name: "Test", last_name: "Person") } + let(:stored_params) do + { + "user" => { + "email" => "newuser@testmail.org", + "person_id" => person.id.to_s, + "inactive" => "0", + "super_user" => "0" + }, + "person_id" => person.id.to_s + } + end + + it "includes all stored params in the Create Anyway form" do + get check_duplicates_users_path, + params: { email: "newuser@testmail.org", person_id: person.id } + + expect(response).to have_http_status(:ok) + # Check that the Create Anyway form is present + expect(response.body).to include('Create Anyway') + end + + it "includes person_id param in the Create Anyway form" do + get check_duplicates_users_path, + params: { email: "newuser@testmail.org", person_id: person.id } + + expect(response).to have_http_status(:ok) + # Person ID is passed as a query param, so it should be in the form + expect(response.body).to include('name="person_id"') + end + + it "passes stored params to Go Back link" do + get check_duplicates_users_path, + params: { email: "newuser@testmail.org", person_id: person.id } + + expect(response).to have_http_status(:ok) + expect(response.body).to include("Go Back") + end + end + # --- Authorization --- context "when user is not an admin" do diff --git a/spec/requests/users_create_with_dupe_check_spec.rb b/spec/requests/users_create_with_dupe_check_spec.rb new file mode 100644 index 000000000..47c5f72e0 --- /dev/null +++ b/spec/requests/users_create_with_dupe_check_spec.rb @@ -0,0 +1,100 @@ +require "rails_helper" + +RSpec.describe "User creation with duplicate check", type: :request do + let(:admin) { create(:user, :admin) } + let!(:existing_user) { create(:user, email: "existing@testmail.org") } + + before do + sign_in admin + end + + describe "POST /users with duplicate detection" do + context "when duplicate email is found and user clicks Create Anyway" do + let(:person) { create(:person, first_name: "Test", last_name: "Person") } + let(:person_with_email) { create(:person, first_name: "Other", last_name: "Person", email: "duplicate@testmail.org") } + + it "retains all form data when creating anyway" do + # Create a person without a user who has the email we want to use + person_with_email + + user_params = { + user: { + email: "duplicate@testmail.org", + person_id: person.id, + inactive: "0", + super_user: "0" + }, + person_id: person.id + } + + # First POST - should redirect to check_duplicates since another person has that email + post users_path, params: user_params + expect(response).to redirect_to(check_duplicates_users_path( + email: "duplicate@testmail.org", + person_id: person.id + )) + + # Follow redirect to check_duplicates page + follow_redirect! + expect(response).to have_http_status(:ok) + expect(response.body).to include("Possible Duplicate User") + + # Verify stored params are rendered in the form + expect(response.body).to include('name="user[email]"') + expect(response.body).to include('value="duplicate@testmail.org"') + expect(response.body).to include('name="person_id"') + expect(response.body).to include("value=\"#{person.id}\"") + + # Click "Create Anyway" - second POST with skip_duplicate_check + user_count_before = User.count + post users_path, params: user_params.merge(skip_duplicate_check: "1") + + # Should successfully create the user + expect(response).to redirect_to(users_path(search: "duplicate@testmail.org")) + expect(User.count).to eq(user_count_before + 1) + + # Verify all data was saved + new_user = User.last + expect(new_user.email).to eq("duplicate@testmail.org") + expect(new_user.person_id).to eq(person.id) + end + end + + context "when a user with exact same email exists (blocked)" do + it "does not show Create Anyway button" do + user_params = { + user: { + email: existing_user.email, + inactive: "0" + } + } + + post users_path, params: user_params + follow_redirect! + + expect(response.body).to include("A user with this email already exists") + expect(response.body).not_to include("Create Anyway") + end + end + + context "when no duplicate is found" do + it "creates user directly without redirect" do + person = create(:person, first_name: "New", last_name: "Person") + user_params = { + user: { + email: "brandnew@testmail.org", + person_id: person.id + }, + person_id: person.id + } + + user_count_before = User.count + post users_path, params: user_params + + expect(response).to redirect_to(users_path(search: "brandnew@testmail.org")) + expect(User.count).to eq(user_count_before + 1) + expect(User.last.email).to eq("brandnew@testmail.org") + end + end + end +end