Skip to content

fix(search,trust): correct stale docstring and replace unsafe test fake#15

Open
kevinelliott wants to merge 1 commit into
developfrom
fix/search-trust-test-cleanup
Open

fix(search,trust): correct stale docstring and replace unsafe test fake#15
kevinelliott wants to merge 1 commit into
developfrom
fix/search-trust-test-cleanup

Conversation

@kevinelliott

Copy link
Copy Markdown
Collaborator

Summary

Closes out two unresolved threads from PR #13.

1. QueryTranslator docstring

The class docstring showed user input manufacturer:Boeing producing filter aircraft_manufacturer = "Boeing", implying a logical→physical name mapping. There is no such mapping: FieldRegistry.meilisearch_attribute_for/2 is an identity function (per its own inline comment "Since we use the same names, this just validates and returns the field"), and manufacturer is not in Aircraft's filterable_attributes. The example as written would have produced a silently dropped filter, not the documented output.

Updated the docstring to use aircraft_manufacturer: directly and added a note that this class does not alias user-friendly names to physical attributes. The test was already correct.

2. TrustCalculatorTest fake

fake_source overrode Object#class via source.define_singleton_method(:class) { FakeClass.new(...) }. It works today because TrustCalculator only reaches for .class.name.demodulize, but the override silently breaks is_a?, case..when, and pattern matching on the fake — a future footgun.

Replaced with an anonymous class whose .name returns the desired string. source.class.name.demodulize still works, and class introspection now behaves correctly.

Not changing

The trust score expectations in source_config_test.rb (40 → 85/95) — verified these match SourceConfig's HIGH_TRUST = 85 and VERY_HIGH_TRUST = 95 for CASAAircraftSource. No drift; no change needed.

Test plan

  • ./bin/rails test test/services/trust_calculator_test.rb test/services/search/query_translator_test.rb — 18 runs, 0 failures.
  • bundle exec rubocop clean on touched files (the one pre-existing offence is on untouched code at query_translator.rb:111).

🤖 Generated with Claude Code

Two unresolved threads from PR #13 worth closing out:

QueryTranslator's docstring claimed user input `manufacturer:Boeing`
produced filter `aircraft_manufacturer = "Boeing"`, suggesting a
logical→physical name mapping. FieldRegistry has always been an
identity function (see its own inline comment), and `manufacturer`
is not in Aircraft's filterable_attributes — so the example was
misleading. Updated to use `aircraft_manufacturer:` directly and
added a note that this class does not alias names.

TrustCalculatorTest's `fake_source` helper overrode `Object#class`
via `define_singleton_method(:class)`. It works today because
TrustCalculator only calls `.class.name.demodulize`, but the
override silently breaks `is_a?`, `case..when`, and pattern
matching on the fake — a footgun for future code. Replaced with
an anonymous class whose own `.name` returns the desired string,
so the fake behaves correctly under all class introspection.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 28, 2026 08:06

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Cleans up two leftover items from PR #13: corrects a misleading docstring in QueryTranslator that suggested a non-existent field-name mapping, and replaces a fragile test fake that overrode Object#class with an anonymous class that exposes the desired .name instead.

Changes:

  • Update QueryTranslator docstring examples to use the real aircraft_manufacturer attribute and note that no aliasing exists.
  • Replace fake_source implementation in TrustCalculatorTest to avoid overriding #class, preserving is_a?/pattern-matching behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
app/services/search/query_translator.rb Corrects docstring examples to use the actual filterable attribute name and clarifies no aliasing is performed.
test/services/trust_calculator_test.rb Replaces Struct-based class override with an anonymous class whose .name returns the desired string.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@shaneshort

Copy link
Copy Markdown
Contributor

Adversarial review finding:

  1. The visible search help still teaches the invalid Aircraft field alias. app/views/shared/search/_field_search_form.html.erb still shows examples like manufacturer:Boeing, manufacturer:Boe*, -manufacturer:Boeing, and manufacturer:Boeing OR manufacturer:Airbus. This PR updates QueryTranslator to document that Aircraft requires aircraft_manufacturer, but users following the help modal will still enter manufacturer, which QueryTranslator silently drops as an invalid field. If this PR is closing the stale-doc issue, the user-facing help copy needs to be corrected too.

  2. Search::FieldRegistry still has contradictory docs. app/services/search/field_registry.rb documents fields_for("Aircraft") as including :manufacturer and says FieldRegistry.valid_field?("Aircraft", "manufacturer") # => true, but Aircraft does not expose that filterable attribute. The new QueryTranslator doc is correct, but it now conflicts with the adjacent registry docs that define the field-validation rule.

I could not run the Rails test command locally because PostgreSQL is not listening at /tmp/.s.PGSQL.5432. I did verify ruby -c test/services/trust_calculator_test.rb, and RuboCop on the touched files only reports the known pre-existing has_field_filters? naming offense.

@tardoe

tardoe commented Jun 13, 2026

Copy link
Copy Markdown
Member

@kevinelliott need to fix linting and review Shorty's comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants