diff --git a/CHANGELOG.md b/CHANGELOG.md index a7ef089..1906775 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,13 @@ change public APIs; each entry says how. ### Added +- Confirmed per-entry saved-search deletion through the CSRF-protected POST + form. Stores implement `delete(context:, id:)` with a current-user-scoped + lookup and a boolean result. Deletion preserves the active query, sort and + page, updates the Saved star, and refuses denied scopes and GET mutations. + The saved-search popover stays inside narrow mobile viewports. Each entry + shares one hover/focus highlight with its inline delete icon, while Apply + and Delete keep separate, non-overlapping click targets. - `config.logger`: querylane logs why a search was unavailable (Meilisearch unreachable, timed out, or an API error such as a missing index). API errors are logged by code, type and HTTP status only, never their message, @@ -72,6 +79,9 @@ change public APIs; each entry says how. ### Fixed +- The saved-search POST-only guard and request parser share normalization for + symbol- and string-keyed params, including false-valued host params, even + when GET search queries are enabled. - Space on a record right after a new answer drew its rows opens the preview. The preview state was reset on `turbo:frame-render`, which Turbo fires a few repaints after the rows are in, so the preview's answer was dropped as if diff --git a/app/assets/stylesheets/querylane.css b/app/assets/stylesheets/querylane.css index 9298a38..c233885 100644 --- a/app/assets/stylesheets/querylane.css +++ b/app/assets/stylesheets/querylane.css @@ -278,11 +278,14 @@ .ql-saved-refusal { margin: 0; padding: 10px 16px; border-bottom: 1px solid var(--ql-rule); font-size: 13px; color: var(--ql-text); } .ql-saved-heading { padding: 10px 16px 4px; font-size: 13px; font-weight: 600; letter-spacing: 0.06em; font-variant-caps: all-small-caps; color: var(--ql-muted); } .ql-saved-none { margin: 0; padding: 4px 16px 14px; color: var(--ql-muted); font-size: 13px; } -.ql-saved-item { display: grid; grid-template-columns: 1fr auto; gap: 2px 12px; width: 100%; padding: 8px 16px; border: 0; - background: none; text-align: left; } -.ql-saved-item:hover, .ql-saved-item:focus-visible { background: var(--ql-accent-soft); } -.ql-saved-name { font-weight: 600; color: var(--ql-heading); } -.ql-saved-query { grid-column: 1; font: 400 12px/18px var(--ql-font-mono); color: var(--ql-muted); } +.ql-saved-row { display: grid; grid-template-columns: minmax(0, 1fr) auto; align-items: center; margin: 2px 8px; padding-right: 4px; border-radius: 4px; } +.ql-saved-row:hover, .ql-saved-row:focus-within { background: var(--ql-accent-soft); } +.ql-saved-delete { min-width: 32px; height: 32px; padding: 4px; border: 0; border-radius: 4px; background: none; color: var(--ql-muted); } +.ql-saved-delete:hover, .ql-saved-delete:focus-visible { color: var(--ql-error); } +.ql-saved-item { display: grid; grid-template-columns: 1fr auto; gap: 2px 12px; width: 100%; padding: 8px; border: 0; + min-width: 0; background: none; text-align: left; } +.ql-saved-name { overflow-wrap: anywhere; font-weight: 600; color: var(--ql-heading); } +.ql-saved-query { grid-column: 1; overflow-wrap: anywhere; font: 400 12px/18px var(--ql-font-mono); color: var(--ql-muted); } .ql-saved-used { grid-column: 2; grid-row: 1 / span 2; align-self: center; font-size: 12px; color: var(--ql-faint); } .ql-notice { margin-top: 8px; font-size: 13px; color: var(--ql-text); } .ql-notice-refused, .ql-notice-store_failed { color: var(--ql-error); } @@ -290,6 +293,14 @@ .ql-box-saved { anchor-name: --ql-saved; } .ql-saved { position-anchor: --ql-saved; inset: auto; top: anchor(bottom); right: anchor(right); margin: 8px 0 0; } } +@media (max-width: 600px) { + .ql-saved { left: 16px; right: 16px; width: auto; } + .ql-saved-head { grid-template-columns: minmax(0, 1fr) auto; gap: 8px; } + .ql-saved-head label { grid-column: 1 / -1; } + .ql-saved-head input { min-width: 0; } + .ql-saved-item { grid-template-columns: minmax(0, 1fr); } + .ql-saved-used { grid-column: 1; grid-row: auto; } +} /* Searching, too short, reasons, unavailable, denied ------------------------ */ .ql-box-busy-label { display: none; flex-shrink: 0; font-size: 13px; color: var(--ql-muted); } diff --git a/app/javascript/querylane/controllers/box_controller.js b/app/javascript/querylane/controllers/box_controller.js index 37f6d9e..f1a64ea 100644 --- a/app/javascript/querylane/controllers/box_controller.js +++ b/app/javascript/querylane/controllers/box_controller.js @@ -90,7 +90,7 @@ export default class extends Controller { // new revision; the rewrite comes back at that same revision. submitted(event) { clearTimeout(this.timer) - if (["toggle", "remove", "clear", "compose_submit", "saved", "fix", "instead", "widen", "relax", "as_words"].includes(event.submitter?.name)) this.bump() + if (["toggle", "remove", "clear", "compose_submit", "saved", "delete_saved", "fix", "instead", "widen", "relax", "as_words"].includes(event.submitter?.name)) this.bump() } setQuery(event) { diff --git a/app/javascript/querylane/controllers/results_controller.js b/app/javascript/querylane/controllers/results_controller.js index f0f9a6e..a307aa1 100644 --- a/app/javascript/querylane/controllers/results_controller.js +++ b/app/javascript/querylane/controllers/results_controller.js @@ -81,6 +81,10 @@ export default class extends Controller { document.getElementById(event.target.dataset.querylaneEnter)?.click() } + deleteSaved(event) { + if (!window.confirm(event.currentTarget.dataset.confirm)) event.preventDefault() + } + closePreview() { if (!this.hasPreviewTarget) return this.previewTarget.innerHTML = "" diff --git a/app/views/querylane/_saved.html.erb b/app/views/querylane/_saved.html.erb index 1507886..8101e3a 100644 --- a/app/views/querylane/_saved.html.erb +++ b/app/views/querylane/_saved.html.erb @@ -3,6 +3,7 @@ Name and Save belong to the search form; choosing one sends its id and the server rewrites the box. %>
+
<%= t("querylane.saved.none") %>

<% end %> <% result.saved_searches.each do |saved| %> - +
+ + +
<% end %>
diff --git a/config/locales/querylane.en.yml b/config/locales/querylane.en.yml index 7785b0c..28cd976 100644 --- a/config/locales/querylane.en.yml +++ b/config/locales/querylane.en.yml @@ -44,6 +44,9 @@ en: title: "Save this search" name: "Name" save: "Save" + delete: "Delete" + delete_label: "Delete saved search %{name}" + delete_confirm: "Delete this saved search?" yours: "Your saved searches" none: "Nothing saved yet." used: "used %{date}" @@ -53,6 +56,8 @@ en: refusal_alone: "Remove it first." notice: saved: "Search saved." + deleted: "Search deleted." + not_available: "Saved search is not available." refused: "This search was not saved: it holds a member ID, SSN or last four." store_failed: "Saved searches are unavailable right now." reasons: diff --git a/docs/guides/saved-searches.md b/docs/guides/saved-searches.md index 38776b9..6e67e91 100644 --- a/docs/guides/saved-searches.md +++ b/docs/guides/saved-searches.md @@ -10,12 +10,16 @@ d.saved store: "SavedSearchStore" ``` The box gets a **Saved** button that opens a popover with a name field, a Save -button and the user's saved searches. +button and the user's saved searches. Each saved entry also has a delete +control, with a confirmation before it is removed. +The entry shares one hover/focus highlight. Apply and Delete remain separate +sibling buttons with non-overlapping click targets; the trash icon sits +inside the row, not inside the Apply button. ## The store contract `store` is an object, or a class or module name resolved on every call. It -responds to three methods. Each gets the request's `Querylane::Context` as +responds to four methods. Each gets the request's `Querylane::Context` as `context:` so you can scope by user. | Method | Returns | @@ -23,6 +27,7 @@ responds to three methods. Each gets the request's `Querylane::Context` as | `all(context:)` | The user's saved searches, in the order to show them. Each responds to `id`, `name`, `query` and `used_at` (a time or `nil`) | | `create(context:, name:, query:)` | Anything (ignored) | | `use(context:, id:)` | The saved search with that id, or `nil`. Record that it was used here | +| `delete(context:, id:)` | `true` after deleting the current user's selected entry; `false` for a missing or unowned entry, or no current user. Scope the lookup before deleting, never trust the submitted id alone | The dummy app keeps them in memory: @@ -37,16 +42,23 @@ module MemorySavedSearches def all(context:) = items.sort_by { |saved| saved.used_at || Time.at(0) }.reverse def create(context:, name:, query:) - items << Saved.new((items.size + 1).to_s, name, query, nil) + @next_id = (@next_id || 0) + 1 + items << Saved.new(@next_id.to_s, name, query, nil) end def use(context:, id:) items.find { |saved| saved.id == id.to_s }&.tap { |saved| saved.used_at = Time.current } end + + def delete(context:, id:) + saved = items.find { |item| item.id == id.to_s } + saved ? !!items.delete(saved) : false + end end ``` -A database-backed store is the same shape: +The reference store is a shared in-memory list, not an authorization example. +A database-backed store must scope deletion to the current user: ```ruby class SavedSearchStore @@ -57,6 +69,16 @@ class SavedSearchStore def self.use(context:, id:) context.user.saved_searches.find_by(id: id)&.tap { |saved| saved.touch(:used_at) } end + + def self.delete(context:, id:) + return false unless context.user && id.to_s.match?(/\A[1-9]\d*\z/) + + saved = context.user.saved_searches.find_by(id: id) + return false unless saved + + saved.destroy! + true + end end ``` @@ -77,6 +99,19 @@ querylane_search(PEOPLE, scope: scope, context: Querylane::Context.new(user: cur - The Saved star is filled while the box holds exactly the text of one of the user's saved searches. - After saving, a notice says "Search saved." +- **Delete** asks "Delete this saved search?". Cancel changes nothing; + confirmation sends `delete_saved=` and `delete_saved_page=` + through the existing POST form and its CSRF token. The host must authorize + access before calling `querylane_search`; a denied scope never calls deletion. +- Successful deletion says "Search deleted." and refreshes the saved list. + The current query, sort and page remain unchanged. The star stays filled if + another saved entry has the same query. Deleting the final entry shows + "Nothing saved yet.". +- Missing and unowned entries both say "Saved search is not available.". + GET deletion is rejected even when `allow_get_query` is enabled. Stores + without `delete` return the unavailable-entry notice rather than mutating data. +- A host with an `icon` helper supplies `icon("trash-2")`; otherwise the + control reads "Delete". Its accessible label and tooltip identify the entry. ## What is refused diff --git a/lib/querylane/controller.rb b/lib/querylane/controller.rb index fd3a82c..cf14f70 100644 --- a/lib/querylane/controller.rb +++ b/lib/querylane/controller.rb @@ -63,7 +63,11 @@ def querylane_preview(definition, params: self.params, config: Querylane.configu # in), and no response is cached. Only the URL's own query string is # checked: a host may pass a query it restored on the server (e.g. a # remembered search) to a GET. - def querylane_guard!(_params, config) + def querylane_guard!(params, config) + if Request.delete_saved_param(params) && !request.post? + raise ActionController::BadRequest, "Saved searches are deleted by POST" + end + url = request.query_parameters carries_text = [config.param_name, :compose, :group].any? { |name| url[name.to_s].present? } if request.get? && carries_text && !config.allow_get_query diff --git a/lib/querylane/request.rb b/lib/querylane/request.rb index 599143b..63e1955 100644 --- a/lib/querylane/request.rb +++ b/lib/querylane/request.rb @@ -7,19 +7,33 @@ module Querylane # remove (chip index), as_words (chip index), clear, instead, widen, relax, compose (with # compose_submit), options, sort, page, per, revision. class Request - attr_reader :fix, :saved, :save_search, :saved_name, :group, :query, :toggle, :remove, :as_words, :clear, :instead, :widen, :relax, :compose, :options, :sort, :page, :per, :revision + attr_reader :fix, :saved, :delete_saved, :save_search, :saved_name, :group, :query, :toggle, :remove, :as_words, :clear, :instead, :widen, :relax, :compose, :options, :sort, :page, :per, :revision # @param params [ActionController::Parameters, Hash] # @param definition [Querylane::Definition] def self.from_params(params, definition:, config: Querylane.configuration) - values = params.respond_to?(:to_unsafe_h) ? params.to_unsafe_h : params.to_h - values = values.transform_keys(&:to_s) + values = normalized_params(params) + values["page"] = values["delete_saved_page"] if values["delete_saved"].present? && values["delete_saved_page"].present? new(definition: definition, query: values[config.param_name.to_s], toggle: values["toggle"], remove: values["remove"], as_words: values["as_words"], clear: values["clear"], instead: values["instead"], widen: values["widen"], relax: values["relax"], compose: (values["compose"] if values["compose_submit"].present?), options: values["options"], - fix: values["fix"], group: values["group"], saved: values["saved"], save_search: values["save_search"], saved_name: values["saved_name"], sort: values["sort"], page: values["page"], per: values["per"], revision: values["revision"]) + fix: values["fix"], group: values["group"], saved: values["saved"], delete_saved: values["delete_saved"], save_search: values["save_search"], saved_name: values["saved_name"], sort: values["sort"], page: values["page"], per: values["per"], revision: values["revision"]) + end + + # The normalized, bounded deletion value used by both the request parser and POST guard. + # + # @param params [ActionController::Parameters, Hash] + # @return [String, nil] + def self.delete_saved_param(params) + normalized_params(params)["delete_saved"].to_s.presence&.first(64) + end + + def self.normalized_params(params) + values = params.respond_to?(:to_unsafe_h) ? params.to_unsafe_h : params.to_h + values.transform_keys(&:to_s) end + private_class_method :normalized_params # instead: "chip.alternative" (use another reading of an interpreted term); # widen: a facet key (drop that facet's terms); relax: match fewer words; @@ -27,9 +41,10 @@ def self.from_params(params, definition:, config: Querylane.configuration) # compose: options-panel fields {"name" => "…", "policy" => "1 2", "is" => [...], "options" => [...]} # (applied only with compose_submit); options: open the options panel; # group: a GroupToken (all records of one group, see Search#related); - # saved: a saved search id to run; save_search with saved_name: save the query. + # saved: a saved search id to run; save_search with saved_name: save the query; + # delete_saved: an owned saved search id to delete without rewriting the query. def initialize(definition:, query: nil, toggle: nil, remove: nil, as_words: nil, clear: nil, instead: nil, widen: nil, relax: nil, - compose: nil, options: nil, fix: nil, group: nil, saved: nil, save_search: nil, saved_name: nil, sort: nil, page: nil, per: nil, revision: nil) + compose: nil, options: nil, fix: nil, group: nil, saved: nil, delete_saved: nil, save_search: nil, saved_name: nil, sort: nil, page: nil, per: nil, revision: nil) @query = query.to_s @clear = clear.present? @instead = instead.to_s.match(/\A(\d+)\.(\d+)\z/)&.captures&.map(&:to_i) @@ -38,8 +53,9 @@ def initialize(definition:, query: nil, toggle: nil, remove: nil, as_words: nil, @options = options.present? @group = group.presence @fix = fix.present? - @saved = saved.to_s.presence&.first(64) - @save_search = save_search.present? + @delete_saved = delete_saved.to_s.presence&.first(64) + @saved = saved.to_s.presence&.first(64) unless @delete_saved + @save_search = save_search.present? && !@delete_saved @saved_name = saved_name.to_s.squish.first(80) @compose = compose.is_a?(Hash) ? compose.to_h.transform_keys(&:to_s) : nil @toggle = parse_toggle(definition, toggle) diff --git a/lib/querylane/search.rb b/lib/querylane/search.rb index 640c6ec..0990c1c 100644 --- a/lib/querylane/search.rb +++ b/lib/querylane/search.rb @@ -33,6 +33,7 @@ def call query = Query.analyze(@definition, @request.query, config: @config) query, rewritten = rewrite(query) @notice = save(query) if @request.save_search && !@scope.denied? + @notice = delete_saved(@request.delete_saved) if @request.delete_saved && !@scope.denied? return result(:first_load, query, rewritten) if query.valid? && query.blank? return result(:invalid, query, rewritten, error: query.error) unless query.valid? return result(:short, query, rewritten, chips: Chips.for(query)) if query.too_short?(@definition.min_text_length) @@ -311,6 +312,15 @@ def saved_text(id) nil end + def delete_saved(id) + store = @definition.saved_searches + return :not_available unless store&.respond_to?(:delete) + + store.delete(context: @context, id: id) ? :deleted : :not_available + rescue => error + saved_store_failed(error) + end + def saved_list store = @definition.saved_searches store && Array(store.all(context: @context)) diff --git a/spec/dummy/app/models/memory_saved_searches.rb b/spec/dummy/app/models/memory_saved_searches.rb index d2160bb..75a4e79 100644 --- a/spec/dummy/app/models/memory_saved_searches.rb +++ b/spec/dummy/app/models/memory_saved_searches.rb @@ -8,15 +8,25 @@ module MemorySavedSearches def items = (@items ||= []) - def reset! = (@items = []) + def reset! + @items = [] + @next_id = 0 + end def all(context:) = items.sort_by { |saved| saved.used_at || Time.at(0) }.reverse def create(context:, name:, query:) - items << Saved.new((items.size + 1).to_s, name, query, nil) + @next_id = (@next_id || 0) + 1 + items << Saved.new(@next_id.to_s, name, query, nil) end def use(context:, id:) items.find { |saved| saved.id == id.to_s }&.tap { |saved| saved.used_at = Time.current } end + + # @return [Boolean] whether the reference store held the selected entry + def delete(context:, id:) + saved = items.find { |item| item.id == id.to_s } + saved ? !!items.delete(saved) : false + end end diff --git a/spec/querylane/request_spec.rb b/spec/querylane/request_spec.rb index 1564cf3..b9e039f 100644 --- a/spec/querylane/request_spec.rb +++ b/spec/querylane/request_spec.rb @@ -15,6 +15,28 @@ expect(described_class.new(definition: definition, page: "3", remove: "0").page).to eq(1) expect(described_class.new(definition: definition, page: "3").page).to eq(3) end + + it "reads a bounded deletion id without rewriting the query or resetting its page" do + request = described_class.from_params({"delete_saved" => "12", "search_query" => "smith", "page" => "3"}, definition: definition) + + expect(request).to have_attributes(delete_saved: "12", query: "smith", page: 3, rewriting?: false) + expect(described_class.from_params({"delete_saved" => "x" * 100}, definition: definition).delete_saved.length).to eq(64) + end + + it "preserves the displayed page for deletion without overriding normal paging" do + deleting = described_class.from_params({delete_saved: "12", delete_saved_page: "3"}, definition: definition) + paging = described_class.from_params({delete_saved_page: "3", page: "2"}, definition: definition) + + expect(deleting.page).to eq(3) + expect(paging.page).to eq(2) + end + + it "normalizes false-valued deletion params the same way for parsing and the guard" do + params = {delete_saved: false} + + expect(described_class.delete_saved_param(params)).to eq("false") + expect(described_class.from_params(params, definition: definition).delete_saved).to eq(described_class.delete_saved_param(params)) + end end RSpec.describe Querylane::Search, "clear" do @@ -34,3 +56,50 @@ expect(described_class.new(definition: TestSearch::DEFINITION, as_words: "x").as_words).to be_nil end end + +RSpec.describe Querylane::Search, "delete saved searches" do + let(:store) { double("saved-search store", all: []) } + let(:context) { Querylane::Context.new(user: :alice) } + let(:definition) do + Querylane::Definition.build(:saved_people) do |definition| + definition.index :employees, name: "people" + definition.text :name, attributes: [:name], default: true + definition.saved store: store + end + end + + def delete_saved(query: "", scope: Querylane::Scope.unrestricted(:employees)) + request = Querylane::Request.from_params({delete_saved: "12", search_query: query}, definition: definition) + Querylane::Search.new(definition: definition, request: request, context: context, scope: scope, searcher: Object.new).call + end + + it "passes the current context and selected id to the store" do + expect(store).to receive(:delete).with(context: context, id: "12").and_return(true) + + expect(delete_saved).to have_attributes(notice: :deleted, query: "", rewritten: false, saved_searches: []) + end + + it "does not mutate the store when access is denied" do + expect(store).not_to receive(:delete) + + expect(delete_saved(scope: Querylane::Scope.denied)).to have_attributes(notice: nil) + end + + it "does not report success for a missing or unowned entry" do + allow(store).to receive(:delete).and_return(false) + + expect(delete_saved).to have_attributes(notice: :not_available) + end + + it "keeps an invalid active query unchanged while deleting" do + allow(store).to receive(:delete).and_return(true) + + expect(delete_saved(query: 'name:"unfinished')).to have_attributes(state: :invalid, notice: :deleted, query: 'name:"unfinished', rewritten: false) + end + + it "reports store failure without claiming deletion succeeded" do + allow(store).to receive(:delete).and_raise(IOError, "unavailable") + + expect(delete_saved).to have_attributes(notice: :store_failed) + end +end diff --git a/spec/requests/search_spec.rb b/spec/requests/search_spec.rb index 9f31a92..7f3c7f3 100644 --- a/spec/requests/search_spec.rb +++ b/spec/requests/search_spec.rb @@ -280,3 +280,66 @@ def group_token(query) end end end + +RSpec.describe "Saved-search deletion endpoint", type: :request do + before { MemorySavedSearches.reset! } + + it "refuses string-keyed host deletion params on GET even when GET queries are allowed" do + MemorySavedSearches.create(context: nil, name: "Keep", query: "team:ops") + controller = SearchesController.new + controller.set_request!(ActionDispatch::TestRequest.create) + controller.set_response!(ActionDispatch::TestResponse.new) + Querylane.configuration.allow_get_query = true + + expect { + controller.querylane_search(DummySearch.definition, scope: Querylane::Scope.unrestricted(:staff), + params: {"delete_saved" => MemorySavedSearches.items.first.id}) + }.to raise_error(ActionController::BadRequest) + expect(MemorySavedSearches.items.size).to eq(1) + end + + it "refuses false-valued host deletion params on GET" do + MemorySavedSearches.items << MemorySavedSearches::Saved.new("false", "Keep", "team:ops", nil) + controller = SearchesController.new + controller.set_request!(ActionDispatch::TestRequest.create) + controller.set_response!(ActionDispatch::TestResponse.new) + Querylane.configuration.allow_get_query = true + + expect { + controller.querylane_search(DummySearch.definition, scope: Querylane::Scope.unrestricted(:staff), + params: {delete_saved: false}) + }.to raise_error(ActionController::BadRequest) + expect(MemorySavedSearches.items.map(&:id)).to eq(["false"]) + end + + it "deletes the selected entry without rewriting the active query" do + MemorySavedSearches.create(context: nil, name: "First", query: "team:ops") + MemorySavedSearches.create(context: nil, name: "Second", query: "team:ops") + first, second = MemorySavedSearches.items + + post "/search", params: {delete_saved: first.id}, headers: {"Turbo-Frame" => "people-results"} + + expect(response).to have_http_status(:ok) + expect(MemorySavedSearches.items).to eq([second]) + expect(response.body).to include("Search deleted.") + expect(response.body).not_to include("querylane_set_query") + end + + it "shows the empty state after deleting the final entry" do + MemorySavedSearches.create(context: nil, name: "Only", query: "team:ops") + + post "/search", params: {delete_saved: MemorySavedSearches.items.first.id} + + expect(MemorySavedSearches.items).to be_empty + expect(response.body).to include("Nothing saved yet.") + end + + it "refuses deletion through GET without touching the store" do + MemorySavedSearches.create(context: nil, name: "Keep", query: "team:ops") + + get "/", params: {delete_saved: MemorySavedSearches.items.first.id} + + expect(response).to have_http_status(:bad_request) + expect(MemorySavedSearches.items.size).to eq(1) + end +end