mirror of
https://github.com/discourse/discourse.git
synced 2026-03-04 01:15:08 +08:00
bcf33a2901
The monolithic `CategoryHierarchicalSearch` service mixed query
building, eager loading, and pagination logic in a single class. The
query was a large raw SQL string built through conditional string
concatenation. Fragments like `#{matches_sql}`, `#{only_ids_sql}`,
`#{except_ids_sql}` were stitched together, with LIMIT/OFFSET appended
via ternary interpolation and named placeholders passed through a
manually assembled hash. This made the query fragile and hard to follow.
Break it into focused, single-responsibility classes under the
`Category::` namespace:
- `Category::HierarchicalSearch` — service orchestrator using
`Service::Base`, with a contract that owns pagination logic (page
validation, limit/offset computation)
- `Category::Query::HierarchicalSearch` — query object that uses
ActiveRecord's interface where it naturally fits (`.where()` with
parameter binding, `.limit()`, `.offset()`, `.with()`, `.joins()`) and
isolates the genuinely complex SQL (recursive CTEs, term matching) into
small named methods rather than a monolithic heredoc
- `Category::Action::EagerLoadAssociations` — extracted eager loading
into a reusable `Service::ActionBase`
The controller is simplified to a single
`Category::HierarchicalSearch.call(service_params)` call with proper
`on_success` / `on_failed_contract` / `on_failure` handling, replacing
manual param transformation and direct result access.
Specs are rewritten to test each class in isolation: the service spec
stubs its collaborators to verify orchestration, the query spec
exercises actual SQL behavior, and the action spec verifies preloading.
Service structure and spec patterns follow the [Discourse service object
guidelines](https://meta.discourse.org/t/using-service-objects-in-discourse/333641)
and the [RSpec Style Guide](https://rspec.rubystyle.guide/).
65 lines
1.8 KiB
Ruby
65 lines
1.8 KiB
Ruby
# frozen_string_literal: true
|
|
|
|
RSpec.describe Category::HierarchicalSearch do
|
|
describe described_class::Contract, type: :model do
|
|
subject(:contract) { described_class.new(page:) }
|
|
|
|
let(:page) { 1 }
|
|
|
|
it { is_expected.to allow_values(1, 100).for(:page) }
|
|
it { is_expected.not_to allow_values(0, -1).for(:page) }
|
|
|
|
describe "#limit" do
|
|
it { expect(contract.limit).to eq(CategoriesController::MAX_CATEGORIES_LIMIT) }
|
|
end
|
|
|
|
describe "#offset" do
|
|
let(:page) { 3 }
|
|
|
|
it { expect(contract.offset).to eq(2 * CategoriesController::MAX_CATEGORIES_LIMIT) }
|
|
end
|
|
end
|
|
|
|
describe ".call" do
|
|
subject(:result) { described_class.call(guardian:, params:) }
|
|
|
|
fab!(:user)
|
|
fab!(:category)
|
|
|
|
let(:guardian) { Guardian.new(user) }
|
|
let(:params) { { term: "test" } }
|
|
let(:categories) { [category] }
|
|
let(:query_instance) { instance_double(Category::Query::HierarchicalSearch, call: categories) }
|
|
|
|
context "with invalid data" do
|
|
let(:params) { { page: -1 } }
|
|
|
|
it { is_expected.to fail_a_contract }
|
|
end
|
|
|
|
context "when everything's ok" do
|
|
before do
|
|
allow(Category::Query::HierarchicalSearch).to receive(:new).and_return(query_instance)
|
|
allow(Category::Action::EagerLoadAssociations).to receive(:call)
|
|
end
|
|
|
|
it { is_expected.to run_successfully }
|
|
|
|
it "returns categories from hierarchical search" do
|
|
expect(result.categories).to eq(categories)
|
|
expect(Category::Query::HierarchicalSearch).to have_received(:new).with(
|
|
guardian:,
|
|
params: result[:params],
|
|
)
|
|
end
|
|
|
|
it "eager loads associations" do
|
|
result
|
|
expect(Category::Action::EagerLoadAssociations).to have_received(:call).with(
|
|
categories:,
|
|
guardian:,
|
|
)
|
|
end
|
|
end
|
|
end
|
|
end
|
