diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index a5c669b8b..c39468fff 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -23,7 +23,7 @@ def index records = records.where.not(id: exclude_ids) end - records = records.limit(25) + records = records.order(Arel.sql(model_class.remote_search_columns.first)).limit(25) render json: records.map(&:remote_search_label) end diff --git a/app/frontend/javascript/controllers/remote_select_controller.js b/app/frontend/javascript/controllers/remote_select_controller.js index 19976b810..9765d9a8d 100644 --- a/app/frontend/javascript/controllers/remote_select_controller.js +++ b/app/frontend/javascript/controllers/remote_select_controller.js @@ -4,25 +4,48 @@ import TomSelect from "tom-select"; export default class extends Controller { static values = { model: String, exclude: String }; + get personModel() { + return this.modelValue === "person" || this.modelValue === "user"; + } + connect() { - this.select = new TomSelect(this.element, { + const options = { valueField: "id", labelField: "label", searchField: "label", + score: () => () => 1, create: false, load: (query, callback) => { if (!query.length) return callback(); + this.select.clearOptions(); + let url = `/search/${this.modelValue}?q=${encodeURIComponent(query)}` if (this.hasExcludeValue && this.excludeValue) { url += `&exclude=${encodeURIComponent(this.excludeValue)}` } fetch(url) .then((r) => r.json()) - .then((json) => callback(json)) + .then((json) => { + callback(json); + this.updateScrollHint(); + }) .catch(() => callback()); }, - }); + }; + + if (this.personModel) { + const renderFn = (data, escape) => { + const match = data.label.match(/^(.+?)\s*\(([^)]+)\)\s*$/); + if (match) { + return `
${escape(match[1].trim())} (${escape(match[2])})
`; + } + return `
${escape(data.label)}
`; + }; + options.render = { option: renderFn, item: renderFn }; + } + + this.select = new TomSelect(this.element, options); // Inject CSS to remove some default tom-select styles -might be a better way to do this. const style = document.createElement("style"); style.textContent = ` @@ -46,10 +69,42 @@ export default class extends Controller { margin: 0 !important; /* Remove padding/margin from selected items */ padding: 0 !important; } + .ts-dropdown-content { + max-height: 400px !important; + overflow-y: auto !important; + } + .ts-dropdown .scroll-hint { + text-align: center; + padding: 4px 0; + color: #9ca3af; + font-size: 0.75rem; + border-top: 1px solid #e5e7eb; + background: #f9fafb; + } `; document.head.appendChild(style); } + updateScrollHint() { + requestAnimationFrame(() => { + const dropdown = this.select.dropdown; + if (!dropdown) return; + + const content = dropdown.querySelector(".ts-dropdown-content"); + if (!content) return; + + const existing = dropdown.querySelector(".scroll-hint"); + if (existing) existing.remove(); + + if (content.scrollHeight > content.clientHeight) { + const hint = document.createElement("div"); + hint.className = "scroll-hint"; + hint.textContent = `Scroll for more results`; + dropdown.appendChild(hint); + } + }); + } + disconnect() { if (this.select) this.select.destroy(); } diff --git a/app/models/concerns/remote_searchable.rb b/app/models/concerns/remote_searchable.rb index be631f359..8ae53efbd 100644 --- a/app/models/concerns/remote_searchable.rb +++ b/app/models/concerns/remote_searchable.rb @@ -14,18 +14,18 @@ def remote_search(query) return none if query.blank? raise "remote_searchable_by not defined for #{name}" if remote_search_columns.empty? - words = query.split.flat_map { |w| w.split(/[\s\-]+/) }.reject(&:blank?) - return none if words.blank? + terms = query.split + scope = all - conditions = words.each_with_index.map do |word, i| - bind_var = "pattern_#{i}".to_sym - column_conditions = remote_search_columns.map { |column| "#{table_name}.#{column} LIKE :#{bind_var}" } - "(#{column_conditions.join(' OR ')})" + terms.each_with_index do |term, i| + pattern_key = :"pattern_#{i}" + conditions = remote_search_columns + .map { |column| "#{table_name}.#{column} LIKE :#{pattern_key}" } + .join(" OR ") + scope = scope.where(conditions, pattern_key => "%#{term}%") end - bindings = words.each_with_index.each_with_object({}) do |(word, i), hash| - hash["pattern_#{i}".to_sym] = "%#{word}%" - end - where(conditions.join(" AND "), bindings) + + scope end end diff --git a/app/models/person.rb b/app/models/person.rb index b048028e0..a841051e2 100644 --- a/app/models/person.rb +++ b/app/models/person.rb @@ -187,6 +187,24 @@ def preferred_email remote_searchable_by :first_name, :last_name, :email, :email_2 + def self.remote_search(query) + return none if query.blank? + + terms = query.split + scope = left_joins(:user) + + terms.each_with_index do |term, i| + pattern_key = :"pattern_#{i}" + conditions = remote_search_columns + .map { |col| "#{table_name}.#{col} LIKE :#{pattern_key}" } + .push("users.email LIKE :#{pattern_key}") + .join(" OR ") + scope = scope.where(conditions, pattern_key => "%#{term}%") + end + + scope.distinct + end + def remote_search_label { id: id, diff --git a/spec/models/concerns/remote_searchable_spec.rb b/spec/models/concerns/remote_searchable_spec.rb new file mode 100644 index 000000000..94963d0b3 --- /dev/null +++ b/spec/models/concerns/remote_searchable_spec.rb @@ -0,0 +1,91 @@ +require "rails_helper" + +RSpec.describe RemoteSearchable, type: :model do + describe ".remote_search" do + let(:admin) { create(:user, :admin) } + + let!(:alice) { create(:person, first_name: "Alice", last_name: "Smith", email: "alice@example.com", created_by: admin, updated_by: admin) } + let!(:bob) { create(:person, first_name: "Bob", last_name: "Smith", email: "bob@example.com", created_by: admin, updated_by: admin) } + let!(:carol) { create(:person, first_name: "Carol", last_name: "Jones", email: "carol@test.org", created_by: admin, updated_by: admin) } + + context "with a single term" do + it "matches first name" do + expect(Person.remote_search("Alice")).to include(alice) + expect(Person.remote_search("Alice")).not_to include(bob, carol) + end + + it "matches last name" do + results = Person.remote_search("Smith") + expect(results).to include(alice, bob) + expect(results).not_to include(carol) + end + + it "matches email" do + results = Person.remote_search("carol@test") + expect(results).to include(carol) + expect(results).not_to include(alice, bob) + end + + it "matches email_2" do + person = create(:person, first_name: "Dana", last_name: "White", email: nil, email_2: "dana@secondary.org", created_by: admin, updated_by: admin) + results = Person.remote_search("dana@secondary") + expect(results).to include(person) + end + + it "matches user email" do + user = create(:user, email: "unique-login@corp.com") + person = create(:person, first_name: "Zara", last_name: "Test", user: user, created_by: admin, updated_by: admin) + results = Person.remote_search("unique-login@corp") + expect(results).to include(person) + end + + it "matches partial strings" do + expect(Person.remote_search("ali")).to include(alice) + end + + it "is case insensitive" do + expect(Person.remote_search("ALICE")).to include(alice) + end + end + + context "with multiple terms" do + it "matches across different columns" do + results = Person.remote_search("Alice Smith") + expect(results).to include(alice) + expect(results).not_to include(bob, carol) + end + + it "matches regardless of term order" do + results = Person.remote_search("Smith Alice") + expect(results).to include(alice) + expect(results).not_to include(bob) + end + + it "matches partial terms across columns" do + results = Person.remote_search("ali smi") + expect(results).to include(alice) + expect(results).not_to include(bob, carol) + end + + it "matches names containing spaces" do + mary_ann = create(:person, first_name: "Mary Ann", last_name: "De La Cruz", email: "ma@example.com", created_by: admin, updated_by: admin) + + expect(Person.remote_search("Mary Ann")).to include(mary_ann) + expect(Person.remote_search("Ann Cruz")).to include(mary_ann) + expect(Person.remote_search("De La")).to include(mary_ann) + end + + it "requires all terms to match" do + results = Person.remote_search("Alice Jones") + expect(results).not_to include(alice, carol) + end + end + + context "with a blank query" do + it "returns no results" do + expect(Person.remote_search("")).to be_empty + expect(Person.remote_search(" ")).to be_empty + end + end + end +end diff --git a/spec/requests/search_spec.rb b/spec/requests/search_spec.rb new file mode 100644 index 000000000..11a7c35e4 --- /dev/null +++ b/spec/requests/search_spec.rb @@ -0,0 +1,154 @@ +require "rails_helper" + +RSpec.describe "Search", type: :request do + let(:admin) { create(:user, :admin) } + let(:user) { create(:user) } + + let!(:alice) { create(:person, first_name: "Alice", last_name: "Smith", email: "alice@example.com") } + let!(:bob) { create(:person, first_name: "Bob", last_name: "Smith", email: "bob@example.com") } + let!(:carol) { create(:person, first_name: "Carol", last_name: "Jones", email: "carol@test.org") } + + describe "GET /search/person" do + context "as a guest" do + it "does not return results" do + get "/search/person", params: { q: "Alice" } + expect(response).not_to have_http_status(:ok) + end + end + + context "as a regular user" do + before { sign_in user } + + it "does not return results" do + get "/search/person", params: { q: "Alice" } + expect(response).not_to have_http_status(:ok) + end + end + + context "as an admin" do + before { sign_in admin } + + it "returns matching results as JSON" do + get "/search/person", params: { q: "Alice" } + json = JSON.parse(response.body) + expect(json.length).to eq(1) + expect(json.first["label"]).to include("Alice") + end + + it "searches by last name" do + get "/search/person", params: { q: "Smith" } + json = JSON.parse(response.body) + labels = json.map { |r| r["label"] } + expect(labels).to include(a_string_including("Alice")) + expect(labels).to include(a_string_including("Bob")) + expect(labels).not_to include(a_string_including("Carol")) + end + + it "searches by person email" do + get "/search/person", params: { q: "carol@test" } + json = JSON.parse(response.body) + expect(json.length).to eq(1) + expect(json.first["label"]).to include("Carol") + end + + it "searches by email_2" do + person = create(:person, first_name: "Dana", last_name: "White", email: nil, email_2: "dana@secondary.org") + get "/search/person", params: { q: "dana@secondary" } + json = JSON.parse(response.body) + ids = json.map { |r| r["id"] } + expect(ids).to include(person.id) + end + + it "searches by user email" do + search_user = create(:user, email: "searchable-login@corp.com") + person = create(:person, first_name: "Zara", last_name: "Finder", user: search_user) + get "/search/person", params: { q: "searchable-login@corp" } + json = JSON.parse(response.body) + ids = json.map { |r| r["id"] } + expect(ids).to include(person.id) + end + + it "displays preferred email in label with user email priority" do + email_user = create(:user, email: "login@corp.com") + person = create(:person, first_name: "Pria", last_name: "Email", email: "personal@example.com", email_2: "alt@example.com", user: email_user) + get "/search/person", params: { q: "login@corp" } + json = JSON.parse(response.body) + match = json.find { |r| r["id"] == person.id } + expect(match["label"]).to include("login@corp.com") + end + + it "falls back to person email when no user email" do + person = create(:person, first_name: "Eve", last_name: "Nolan", email: "eve@personal.com", user: nil) + get "/search/person", params: { q: "Eve" } + json = JSON.parse(response.body) + match = json.find { |r| r["id"] == person.id } + expect(match["label"]).to include("eve@personal.com") + end + + it "falls back to email_2 when no user or person email" do + person = create(:person, first_name: "Fay", last_name: "Park", email: nil, email_2: "fay@backup.com", user: nil) + get "/search/person", params: { q: "Fay" } + json = JSON.parse(response.body) + match = json.find { |r| r["id"] == person.id } + expect(match["label"]).to include("fay@backup.com") + end + + it "returns results matched by person email even when label shows user email" do + diff_user = create(:user, email: "login@corp.com") + person = create(:person, first_name: "Gia", last_name: "Diff", email: "personal@home.com", user: diff_user) + get "/search/person", params: { q: "personal@home" } + json = JSON.parse(response.body) + match = json.find { |r| r["id"] == person.id } + expect(match).to be_present + expect(match["label"]).not_to include("personal@home") + expect(match["label"]).to include("login@corp.com") + end + + it "returns results matched by email_2 even when label shows a different email" do + alt_user = create(:user, email: "primary@corp.com") + person = create(:person, first_name: "Hana", last_name: "Alt", email: "work@corp.com", email_2: "secret@hidden.org", user: alt_user) + get "/search/person", params: { q: "secret@hidden" } + json = JSON.parse(response.body) + match = json.find { |r| r["id"] == person.id } + expect(match).to be_present + expect(match["label"]).not_to include("secret@hidden") + end + + it "handles multi-word queries" do + get "/search/person", params: { q: "Alice Smith" } + json = JSON.parse(response.body) + expect(json.length).to eq(1) + expect(json.first["label"]).to include("Alice") + end + + it "excludes specified IDs" do + get "/search/person", params: { q: "Smith", exclude: alice.id.to_s } + json = JSON.parse(response.body) + ids = json.map { |r| r["id"] } + expect(ids).not_to include(alice.id) + expect(ids).to include(bob.id) + end + + it "returns empty array for blank query" do + get "/search/person", params: { q: "" } + expect(JSON.parse(response.body)).to eq([]) + end + + it "returns results in alphabetical order" do + get "/search/person", params: { q: "Smith" } + json = JSON.parse(response.body) + names = json.map { |r| r["label"] } + expect(names).to eq(names.sort) + end + end + end + + describe "GET /search/invalid" do + before { sign_in admin } + + it "returns forbidden for unknown models" do + get "/search/invalid", params: { q: "test" } + expect(response).not_to have_http_status(:ok) + end + end +end