From 49d4c392171719bab7bd2ff6067eec4c24478c7e Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Wed, 14 Oct 2026 10:45:34 -0400 Subject: [PATCH 1/3] Let organizations merge without a status The stored organization_status is derived from affiliations and many legacy orgs have none, so requiring it blocked admins from merging duplicates. The preview also silently stamped the first status onto a blank keeper. Co-Authored-By: Claude Opus 5.5 (1M context) --- app/controllers/concerns/dedupable.rb | 9 ++++-- app/models/organization.rb | 4 +-- app/views/dedupes/_preview.html.erb | 2 +- spec/models/organization_spec.rb | 9 +++++- spec/requests/dedupable_spec.rb | 42 +++++++++++++++++++++++++++ 5 files changed, 60 insertions(+), 6 deletions(-) diff --git a/app/controllers/concerns/dedupable.rb b/app/controllers/concerns/dedupable.rb index 527c613e1b..43be6e8863 100644 --- a/app/controllers/concerns/dedupable.rb +++ b/app/controllers/concerns/dedupable.rb @@ -4,6 +4,8 @@ module Dedupable extend ActiveSupport::Concern CandidateGroup = Struct.new(:label, :records, :reasons, keyword_init: true) + # Lets a model relax a rule for merges (`except_on: :dedupe`) while :update rules still run. + DEDUPE_VALIDATION_CONTEXT = %i[update dedupe].freeze def dedupe_index authorize! :dedupe, to: :dedupe? @@ -61,7 +63,10 @@ def dedupe_update_keep record = mc.find(params[:id]) keep_param_key = "#{mn}_to_keep" - record.update!(params.require(keep_param_key).permit(dedupe_editable_columns(config))) if params[keep_param_key].present? + if params[keep_param_key].present? + record.assign_attributes(params.require(keep_param_key).permit(dedupe_editable_columns(config))) + record.save!(context: DEDUPE_VALIDATION_CONTEXT) + end head :ok rescue ActionPolicy::Unauthorized @@ -124,7 +129,7 @@ def dedupe_perform # need it (e.g. re-crediting a moved scholarship to the keeper's registrant). # Runs inside the transaction so a failure rolls the whole merge back. config[:after_merge]&.call(record_to_keep) - record_to_keep.save! if record_to_keep.changed? + record_to_keep.save!(context: DEDUPE_VALIDATION_CONTEXT) if record_to_keep.changed? end label = mc.model_name.human.pluralize diff --git a/app/models/organization.rb b/app/models/organization.rb index a507aa8dae..3332628b9b 100644 --- a/app/models/organization.rb +++ b/app/models/organization.rb @@ -3,7 +3,7 @@ class Organization < ApplicationRecord include Communicable include FilemakerCoded filemaker_archived_in FmProject, FmOrganization - belongs_to :organization_status + belongs_to :organization_status, optional: true belongs_to :created_by, class_name: "User", optional: true belongs_to :updated_by, class_name: "User", optional: true belongs_to :organization_obligation, optional: true @@ -86,7 +86,7 @@ def self.info_email content_type: %w[image/png image/jpeg image/webp], size: { less_than: 5.megabytes } validates :name, presence: true, length: { maximum: 255 } - validates :organization_status_id, presence: true + validates :organization_status_id, presence: true, except_on: :dedupe validates :email, format: { with: URI::MailTo::EMAIL_REGEXP, message: "must be a valid email address" }, allow_blank: true, length: { maximum: 255 } validates :organization_type_other, length: { maximum: 255 } validates :website_url, length: { maximum: 255 } diff --git a/app/views/dedupes/_preview.html.erb b/app/views/dedupes/_preview.html.erb index 94d1d02647..8b51bec02e 100644 --- a/app/views/dedupes/_preview.html.erb +++ b/app/views/dedupes/_preview.html.erb @@ -192,7 +192,7 @@ class: "w-full px-2 py-1 border border-gray-300 rounded text-gray-900 focus:border-blue-500 focus:ring-blue-200", data: { controller: "remote-select", remote_select_model_value: remote_select_options[col.name] } %> <% elsif col_assoc && belongs_to_options[col.name].present? %> - <%= keep_f.collection_select col.name.to_sym, belongs_to_options[col.name], :id, :name, {}, class: "w-full px-2 py-1 border border-gray-300 rounded text-gray-900 focus:border-blue-500 focus:ring-blue-200" %> + <%= keep_f.collection_select col.name.to_sym, belongs_to_options[col.name], :id, :name, { include_blank: true }, class: "w-full px-2 py-1 border border-gray-300 rounded text-gray-900 focus:border-blue-500 focus:ring-blue-200" %> <% elsif col.type == :integer && col_assoc.nil? %> <%= keep_f.number_field col.name.to_sym, class: "w-32 px-2 py-1 border border-gray-300 rounded text-gray-900 focus:border-blue-500 focus:ring-blue-200" %> <% elsif col.type == :text %> diff --git a/spec/models/organization_spec.rb b/spec/models/organization_spec.rb index c0b543ee52..756b603d0e 100644 --- a/spec/models/organization_spec.rb +++ b/spec/models/organization_spec.rb @@ -6,7 +6,7 @@ describe 'associations' do it { should belong_to(:location).optional } it { should belong_to(:windows_type).optional } - it { should belong_to(:organization_status) } + it { should belong_to(:organization_status).optional } it { should belong_to(:parent).class_name("Organization").optional } it { should have_many(:children).class_name("Organization").with_foreign_key(:parent_id).dependent(:nullify) } it { should have_many(:affiliations) } @@ -20,6 +20,13 @@ it { should validate_presence_of(:name) } it { should validate_presence_of(:organization_status_id) } + it "allows a blank status when saved for a dedupe merge" do + organization = create(:organization) + organization.organization_status_id = nil + + expect(organization.valid?(Dedupable::DEDUPE_VALIDATION_CONTEXT)).to be true + end + it { should allow_value("info@example.com").for(:email) } it { should allow_value("").for(:email) } it { should allow_value(nil).for(:email) } diff --git a/spec/requests/dedupable_spec.rb b/spec/requests/dedupable_spec.rb index a0645e6e20..4906a1bf70 100644 --- a/spec/requests/dedupable_spec.rb +++ b/spec/requests/dedupable_spec.rb @@ -340,6 +340,32 @@ expect(response.body).to include("Website url") expect(response.body).not_to include("Profile show age ranges") end + + it "leaves a blank status blank instead of preselecting one" do + keep.update_column(:organization_status_id, nil) + + get dedupe_preview_organizations_path( + organization_to_keep_id: keep.id, + organization_to_delete_id: delete_rec.id + ) + + status_select = Nokogiri::HTML(response.body).at_css("select[name$='[organization_status_id]']") + expect(status_select.at_css("option[selected]")).to be_nil + expect(status_select.at_css("option[value='']")).to be_present + end + end + + describe "PATCH dedupe_update_keep" do + it "autosaves a keeper that has no status" do + keep = create(:organization, name: "Keep Org") + keep.update_column(:organization_status_id, nil) + + patch dedupe_update_keep_organizations_path, + params: { id: keep.id, organization_to_keep: { name: "Renamed Org" } } + + expect(response).to have_http_status(:ok) + expect(keep.reload.name).to eq("Renamed Org") + end end describe "POST dedupe_perform" do @@ -401,6 +427,22 @@ expect(keep.reload.filemaker_code).to eq("FM-100, FM-273") end + it "merges when neither organization has a status" do + keep.update_column(:organization_status_id, nil) + delete_rec.update_column(:organization_status_id, nil) + + expect { + post dedupe_perform_organizations_path, params: { + organization_to_delete_id: delete_rec.id, + organization_to_keep_id: keep.id, + organization_to_keep: { name: "Canonical Org", organization_status_id: "" } + } + }.to change(Organization, :count).by(-1) + + expect(response).to redirect_to(organizations_path) + expect(keep.reload).to have_attributes(name: "Canonical Org", organization_status_id: nil) + end + it "reassigns the duplicate's invoices instead of blocking the merge" do invoice = create(:invoice, invoicee: delete_rec) From d71faea704a4a4c3e2d23e40051ba411f828e016 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Wed, 14 Oct 2026 10:48:21 -0400 Subject: [PATCH 2/3] Stop requiring a status on organizations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stored status is derived from affiliations and nothing reads it as an input anymore (ADR-0003 D4), so requiring it only blocked saves — dedupe merges included — on legacy orgs that never had one. Co-Authored-By: Claude Opus 5.5 (1M context) --- app/controllers/concerns/dedupable.rb | 9 ++------- app/models/organization.rb | 1 - app/views/organizations/_form.html.erb | 1 - spec/models/organization_spec.rb | 9 +-------- 4 files changed, 3 insertions(+), 17 deletions(-) diff --git a/app/controllers/concerns/dedupable.rb b/app/controllers/concerns/dedupable.rb index 43be6e8863..527c613e1b 100644 --- a/app/controllers/concerns/dedupable.rb +++ b/app/controllers/concerns/dedupable.rb @@ -4,8 +4,6 @@ module Dedupable extend ActiveSupport::Concern CandidateGroup = Struct.new(:label, :records, :reasons, keyword_init: true) - # Lets a model relax a rule for merges (`except_on: :dedupe`) while :update rules still run. - DEDUPE_VALIDATION_CONTEXT = %i[update dedupe].freeze def dedupe_index authorize! :dedupe, to: :dedupe? @@ -63,10 +61,7 @@ def dedupe_update_keep record = mc.find(params[:id]) keep_param_key = "#{mn}_to_keep" - if params[keep_param_key].present? - record.assign_attributes(params.require(keep_param_key).permit(dedupe_editable_columns(config))) - record.save!(context: DEDUPE_VALIDATION_CONTEXT) - end + record.update!(params.require(keep_param_key).permit(dedupe_editable_columns(config))) if params[keep_param_key].present? head :ok rescue ActionPolicy::Unauthorized @@ -129,7 +124,7 @@ def dedupe_perform # need it (e.g. re-crediting a moved scholarship to the keeper's registrant). # Runs inside the transaction so a failure rolls the whole merge back. config[:after_merge]&.call(record_to_keep) - record_to_keep.save!(context: DEDUPE_VALIDATION_CONTEXT) if record_to_keep.changed? + record_to_keep.save! if record_to_keep.changed? end label = mc.model_name.human.pluralize diff --git a/app/models/organization.rb b/app/models/organization.rb index 3332628b9b..e5686a54e1 100644 --- a/app/models/organization.rb +++ b/app/models/organization.rb @@ -86,7 +86,6 @@ def self.info_email content_type: %w[image/png image/jpeg image/webp], size: { less_than: 5.megabytes } validates :name, presence: true, length: { maximum: 255 } - validates :organization_status_id, presence: true, except_on: :dedupe validates :email, format: { with: URI::MailTo::EMAIL_REGEXP, message: "must be a valid email address" }, allow_blank: true, length: { maximum: 255 } validates :organization_type_other, length: { maximum: 255 } validates :website_url, length: { maximum: 255 } diff --git a/app/views/organizations/_form.html.erb b/app/views/organizations/_form.html.erb index 572f087472..18f243d0b3 100644 --- a/app/views/organizations/_form.html.erb +++ b/app/views/organizations/_form.html.erb @@ -285,7 +285,6 @@ collection: @organization_statuses, include_blank: true, label: "Organization status", - required: true, wrapper_html: { class: "mb-0" }, input_html: { class: "w-full rounded-md border-gray-300 shadow-sm focus:border-blue-500 focus:ring focus:ring-blue-200" diff --git a/spec/models/organization_spec.rb b/spec/models/organization_spec.rb index 756b603d0e..aff1b0d5f5 100644 --- a/spec/models/organization_spec.rb +++ b/spec/models/organization_spec.rb @@ -18,14 +18,7 @@ describe 'validations' do subject { build(:organization) } it { should validate_presence_of(:name) } - it { should validate_presence_of(:organization_status_id) } - - it "allows a blank status when saved for a dedupe merge" do - organization = create(:organization) - organization.organization_status_id = nil - - expect(organization.valid?(Dedupable::DEDUPE_VALIDATION_CONTEXT)).to be true - end + it { should_not validate_presence_of(:organization_status_id) } it { should allow_value("info@example.com").for(:email) } it { should allow_value("").for(:email) } From a6757f6468259e41a2c5ca459a472020df0ef140 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Wed, 14 Oct 2026 11:16:03 -0400 Subject: [PATCH 3/3] Label the empty status option "(none)" A bare blank option reads like a missing render, so an org without a status looked like it had one or like the select was broken. Co-Authored-By: Claude Opus 5.5 (1M context) --- app/views/dedupes/_preview.html.erb | 2 +- app/views/organizations/_form.html.erb | 2 +- spec/requests/dedupable_spec.rb | 2 +- spec/requests/organizations_spec.rb | 9 +++++++++ 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/app/views/dedupes/_preview.html.erb b/app/views/dedupes/_preview.html.erb index 8b51bec02e..06416e6f03 100644 --- a/app/views/dedupes/_preview.html.erb +++ b/app/views/dedupes/_preview.html.erb @@ -192,7 +192,7 @@ class: "w-full px-2 py-1 border border-gray-300 rounded text-gray-900 focus:border-blue-500 focus:ring-blue-200", data: { controller: "remote-select", remote_select_model_value: remote_select_options[col.name] } %> <% elsif col_assoc && belongs_to_options[col.name].present? %> - <%= keep_f.collection_select col.name.to_sym, belongs_to_options[col.name], :id, :name, { include_blank: true }, class: "w-full px-2 py-1 border border-gray-300 rounded text-gray-900 focus:border-blue-500 focus:ring-blue-200" %> + <%= keep_f.collection_select col.name.to_sym, belongs_to_options[col.name], :id, :name, { include_blank: "(none)" }, class: "w-full px-2 py-1 border border-gray-300 rounded text-gray-900 focus:border-blue-500 focus:ring-blue-200" %> <% elsif col.type == :integer && col_assoc.nil? %> <%= keep_f.number_field col.name.to_sym, class: "w-32 px-2 py-1 border border-gray-300 rounded text-gray-900 focus:border-blue-500 focus:ring-blue-200" %> <% elsif col.type == :text %> diff --git a/app/views/organizations/_form.html.erb b/app/views/organizations/_form.html.erb index 18f243d0b3..edc5f91269 100644 --- a/app/views/organizations/_form.html.erb +++ b/app/views/organizations/_form.html.erb @@ -283,7 +283,7 @@ <%= f.input :organization_status_id, as: :select, collection: @organization_statuses, - include_blank: true, + include_blank: "(none)", label: "Organization status", wrapper_html: { class: "mb-0" }, input_html: { diff --git a/spec/requests/dedupable_spec.rb b/spec/requests/dedupable_spec.rb index 4906a1bf70..ff18dda1c8 100644 --- a/spec/requests/dedupable_spec.rb +++ b/spec/requests/dedupable_spec.rb @@ -351,7 +351,7 @@ status_select = Nokogiri::HTML(response.body).at_css("select[name$='[organization_status_id]']") expect(status_select.at_css("option[selected]")).to be_nil - expect(status_select.at_css("option[value='']")).to be_present + expect(status_select.at_css("option[value='']").text).to eq("(none)") end end diff --git a/spec/requests/organizations_spec.rb b/spec/requests/organizations_spec.rb index 0f73e9eba6..d94d757678 100644 --- a/spec/requests/organizations_spec.rb +++ b/spec/requests/organizations_spec.rb @@ -175,6 +175,15 @@ expect(response).to be_successful end + it "shows (none) in the admin status select when the organization has no status" do + organization = Organization.create!(valid_attributes.merge(organization_status_id: nil)) + get edit_organization_url(organization, admin: 1) + + status_select = Nokogiri::HTML(response.body).at_css("select[name='organization[organization_status_id]']") + expect(status_select.at_css("option[selected]")).to be_nil + expect(status_select.at_css("option[value='']").text).to eq("(none)") + end + it "hides the Monthly reports row when there are no monthly reports" do organization = Organization.create!(valid_attributes) get edit_organization_url(organization)