feat: implement granular filtering for earthquake data - #2
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
Note
|
| Layer / File(s) | Summary |
|---|---|
Sismo scopes and query indexes app/models/sismo.rb, db/migrate/..., db/schema.rb |
Adds six filter scopes and indexes for mag, magType, and created_at. |
Controller filter application app/controllers/sismos_controller.rb |
Reads nested filter parameters, validates numeric, date, and boolean values, and applies filter scopes. |
Filter fixtures and validation test/fixtures/sismos.yml, test/models/sismo_test.rb, test/controllers/sismos_controller_test.rb, test/controllers/sismos_controller_filters_test.rb |
Adds realistic fixtures and coverage for validations, filters, combined queries, pagination, response structure, and associations. |
Lint workflow .rubocop.yml, Makefile |
Adds RuboCop exclusions and host and Docker lint commands. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Sequence Diagram(s)
sequenceDiagram
participant Client
participant SismosController
participant Sismo
participant Database
Client->>SismosController: Request nested filters
SismosController->>Sismo: Parse and validate filters
SismosController->>Sismo: Apply filter scopes
Sismo->>Database: Query indexed sismos
Database-->>Sismo: Return matching records
Sismo-->>SismosController: Return filtered relation
SismosController-->>Client: Return data and pagination
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and concisely describes the main change: adding granular filtering for earthquake data. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feature/advanced-sismos-filters
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/sismos_controller.rb`:
- Around line 45-49: Update apply_tsunami_filter to treat blank tsunami
parameters as absent, validate non-blank values before passing them to
by_tsunami, and accept only true or false case-insensitively. Raise the shared
ApplicationController::InvalidFilter for unsupported values, and define/rescue
that exception in ApplicationController with a structured 400 JSON response.
- Around line 39-43: Update the controller’s date-filter flow by adding an
InvalidFilter path that parses and validates date_from and date_to before
applying by_date_from and by_date_to; normalize date-only date_to values to
end_of_day, raise InvalidFilter for malformed ISO 8601 values, and have index
render the error message with HTTP 400 instead of allowing a database exception.
In `@app/models/sismo.rb`:
- Line 15: Update the by_tsunami scope in the Sismo model so a false filter
includes both false and NULL tsunami values, matching the existing
sismo.tsunami? serialization behavior; retain the true filter as tsunami: true
and reuse ActiveModel::Type::Boolean casting.
- Around line 11-12: Update the magnitude-bound handling around the by_mag_min
and by_mag_max scopes to use strict Float conversion instead of to_f, rejecting
malformed or empty values. Rescue ArgumentError and TypeError in the relevant
request flow and return a 400 response with the specified numeric-bounds error
for invalid mag_min or mag_max inputs.
In `@db/migrate/20260803034001_add_filter_indexes_to_sismos.rb`:
- Around line 1-7: Update AddFilterIndexesToSismos to disable the migration
transaction and create each index on sismos with PostgreSQL’s concurrent
algorithm. Apply this to the mag, magType, and created_at indexes while
preserving their existing columns.
In `@test/models/sismo_test.rb`:
- Around line 41-80: Add explicit count assertions to prevent vacuous filter
tests: in test/models/sismo_test.rb lines 41-80, update the named tests with
expected result counts of 3, 2, 2, 2, 1, 2, and 3 respectively; in
test/controllers/sismos_controller_test.rb lines 21-81, assert
json['data'].length equals 2, 3, 2, 2, and 2 for the listed filter tests; in
test/controllers/sismos_controller_filters_test.rb lines 16-66, assert lengths
of 2, 1, 3, and 1 for the listed tests. Keep the existing predicate assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 50f302ca-0a04-4634-886b-6410e5619d23
📒 Files selected for processing (8)
app/controllers/sismos_controller.rbapp/models/sismo.rbdb/migrate/20260803034001_add_filter_indexes_to_sismos.rbdb/schema.rbtest/controllers/sismos_controller_filters_test.rbtest/controllers/sismos_controller_test.rbtest/fixtures/sismos.ymltest/models/sismo_test.rb
0d8be74 to
3b1d72b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/sismos_controller.rb`:
- Around line 34-35: Update the mag_types parsing in the sismos filtering flow
to trim leading and trailing whitespace from each value after splitting
filter_param(:mag_type), while preserving the existing
sismos.by_mag_type(mag_types) call.
- Around line 31-36: Validate the raw filters parameter before any filter_param,
dig, or split calls, including in apply_mag_type_filter and the surrounding
filtering flow. Accept only nil or ActionController::Parameters whose values are
all strings; for invalid values such as scalar filters or array-valued mag_type,
render { error: 'Invalid filters' } with a 400 status and return without
applying filters.
In `@test/controllers/sismos_controller_filters_test.rb`:
- Around line 5-14: Strengthen the assertions in the `filters by date_from` and
`filters by date range (date_from and date_to combined)` tests to compare
returned records against the exact expected `Sismo` scope count, following the
pattern used by `filters by date_to`; retain the existing boundary checks for
each returned record.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f755b709-a098-4728-9e97-573989067c69
📒 Files selected for processing (8)
app/controllers/sismos_controller.rbapp/models/sismo.rbdb/migrate/20260803034001_add_filter_indexes_to_sismos.rbdb/schema.rbtest/controllers/sismos_controller_filters_test.rbtest/controllers/sismos_controller_test.rbtest/fixtures/sismos.ymltest/models/sismo_test.rb
3b1d72b to
5155194
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/controllers/sismos_controller.rb (1)
31-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
mag_typeis a String, and trim whitespace before matching.
filter_param(:mag_type).split(',')fails withNoMethodErrorif a client sendsfilters[mag_type][]=ml(an Array). It also keeps leading/trailing spaces, sofilters[mag_type]=ml, mwwproduces" mww", which never matches amagTypevalue.🔧 Fix
def apply_mag_type_filter(sismos) - return sismos unless filter_param(:mag_type).present? + mag_type = filter_param(:mag_type) + return sismos unless mag_type.is_a?(String) && mag_type.present? - mag_types = filter_param(:mag_type).split(',') + mag_types = mag_type.split(',').map(&:strip).reject(&:blank?) sismos.by_mag_type(mag_types) end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/sismos_controller.rb` around lines 31 - 36, Update apply_mag_type_filter to process mag_type only when filter_param(:mag_type) is a String, avoiding split calls on array inputs; split string values by commas and trim surrounding whitespace from each value before passing the normalized mag_types to sismos.by_mag_type.test/controllers/sismos_controller_filters_test.rb (1)
5-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStrengthen weak assertions to exact counts.
json['data'].length < Sismo.count(line 10) andjson['pagination']['total'] >= 1(line 32) pass even when the date filter returns the wrong number of records. Fixture data supports exact expected counts, as already done for'filters by date_to'in the same file.🔧 Fix
test 'filters by date_from' do get sismos_url, params: { filters: { date_from: 2.days.ago.iso8601 } } json = JSON.parse(response.body) assert_response :success - assert json['data'].length < Sismo.count + assert_equal 1, json['data'].length json['data'].each do |sismo| assert Time.parse(sismo['attributes']['time']) >= 2.days.ago.beginning_of_day end endtest 'filters by date range (date_from and date_to combined)' do get sismos_url, params: { filters: { date_from: 5.days.ago.iso8601, date_to: Time.current.iso8601 } } json = JSON.parse(response.body) assert_response :success - assert json['pagination']['total'] >= 1 + assert_equal 2, json['pagination']['total'] endAlso applies to: 27-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/controllers/sismos_controller_filters_test.rb` around lines 5 - 14, Strengthen the assertions in the date filter tests, especially the `'filters by date_from'` test and the pagination assertion around `json['pagination']['total']`, by replacing broad comparisons with the exact record counts supported by the fixtures. Follow the exact-count assertion pattern already used by `'filters by date_to'`, while preserving the per-record date validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.rubocop.yml:
- Around line 24-33: Remove the broad test/**/* exclusions from
Metrics/ClassLength and Metrics/BlockLength, and remove the whole-file
app/controllers/sismos_controller.rb exclusion. Retain only the
lib/tasks/**/*.rake block-length exemption, adding narrowly targeted class or
block exemptions only for currently required violations.
In `@app/controllers/sismos_controller.rb`:
- Around line 135-138: Update filter_param in SismosController to validate that
params[:filters] is a Hash before traversing it; return nil for scalar or
otherwise invalid filters values, while preserving the existing lookup for valid
Hash input so the apply_*_filter methods can continue to produce their
structured responses.
In `@Makefile`:
- Around line 83-90: Update the docker compose exec commands in the dev-lint and
dev-lint-fix targets to include the -T option, disabling TTY allocation for
non-interactive lint and auto-correct runs.
---
Duplicate comments:
In `@app/controllers/sismos_controller.rb`:
- Around line 31-36: Update apply_mag_type_filter to process mag_type only when
filter_param(:mag_type) is a String, avoiding split calls on array inputs; split
string values by commas and trim surrounding whitespace from each value before
passing the normalized mag_types to sismos.by_mag_type.
In `@test/controllers/sismos_controller_filters_test.rb`:
- Around line 5-14: Strengthen the assertions in the date filter tests,
especially the `'filters by date_from'` test and the pagination assertion around
`json['pagination']['total']`, by replacing broad comparisons with the exact
record counts supported by the fixtures. Follow the exact-count assertion
pattern already used by `'filters by date_to'`, while preserving the per-record
date validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2223e276-2d19-429a-8a80-42fc8137ebf1
📒 Files selected for processing (10)
.rubocop.ymlMakefileapp/controllers/sismos_controller.rbapp/models/sismo.rbdb/migrate/20260803034001_add_filter_indexes_to_sismos.rbdb/schema.rbtest/controllers/sismos_controller_filters_test.rbtest/controllers/sismos_controller_test.rbtest/fixtures/sismos.ymltest/models/sismo_test.rb
5155194 to
8549947
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/controllers/sismos_controller.rb (1)
31-36: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
mag_typeagainst non-String values beforesplit.
filter_param(:mag_type)can return anArrayorActionController::Parameterswhen a client sendsfilters[mag_type][]=mlor a nested hash..present?passes for a non-emptyArray, then.split(',')raisesNoMethodErrorbecauseArrayandActionController::Parametersdo not definesplit. This is unrescued and crashes the public, unauthenticated/v1/sismosendpoint with a 500 instead of the structured 400 every other filter uses.Separately,
.split(',')does not strip whitespace, sofilters[mag_type]=ml, mwwproduces' mww', which silently matches nothing.Fix both by validating the type and trimming each value:
🔧 Proposed fix
def apply_mag_type_filter(sismos) - return sismos unless filter_param(:mag_type).present? + raw = filter_param(:mag_type) + return sismos unless raw.is_a?(String) && raw.present? - mag_types = filter_param(:mag_type).split(',') + mag_types = raw.split(',').map(&:strip).reject(&:blank?) sismos.by_mag_type(mag_types) endIf you instead want to render a structured
400for non-Stringmag_type(for consistency withmag_min/mag_max/tsunami), add areturn unless sismoscheck afterapply_mag_type_filterinfilter_sismos(line 21), since it currently assumes this method never returnsnil.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/sismos_controller.rb` around lines 31 - 36, Update apply_mag_type_filter to accept only String mag_type values, returning the existing invalid-filter result for non-String inputs so malformed arrays or nested parameters produce the structured 400 path. Split valid strings on commas and trim each value before passing the normalized list to sismos.by_mag_type; update filter_sismos to handle a nil result if that is the established invalid-filter convention.Source: Path instructions
test/controllers/sismos_controller_filters_test.rb (1)
5-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStrengthen weak count assertions in date-range filter tests.
'filters by date_from'only assertsjson['data'].length < Sismo.count, and'filters by date range (date_from and date_to combined)'only assertsjson['pagination']['total'] >= 1. Both pass even if the filter returns the wrong number of records. Use exact counts, matching the pattern already used in'filters by date_to'.🔧 Proposed fix
test 'filters by date_from' do get sismos_url, params: { filters: { date_from: 2.days.ago.iso8601 } } json = JSON.parse(response.body) assert_response :success - assert json['data'].length < Sismo.count + assert_equal 1, json['data'].length json['data'].each do |sismo| assert Time.parse(sismo['attributes']['time']) >= 2.days.ago.beginning_of_day end endtest 'filters by date range (date_from and date_to combined)' do get sismos_url, params: { filters: { date_from: 5.days.ago.iso8601, date_to: Time.current.iso8601 } } json = JSON.parse(response.body) assert_response :success - assert json['pagination']['total'] >= 1 + assert_equal 2, json['pagination']['total'] endAlso applies to: 27-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/controllers/sismos_controller_filters_test.rb` around lines 5 - 14, Strengthen the assertions in the date-range filter tests, specifically “filters by date_from” and “filters by date range (date_from and date_to combined),” by asserting the exact expected record counts as done in “filters by date_to.” Keep the existing per-record date validation and response checks while replacing the weak length/total comparisons.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 91-92: Update the dev-shell-backend target’s docker compose exec
command to remove the -T option, preserving the pseudo-TTY required for the
interactive bash shell.
---
Duplicate comments:
In `@app/controllers/sismos_controller.rb`:
- Around line 31-36: Update apply_mag_type_filter to accept only String mag_type
values, returning the existing invalid-filter result for non-String inputs so
malformed arrays or nested parameters produce the structured 400 path. Split
valid strings on commas and trim each value before passing the normalized list
to sismos.by_mag_type; update filter_sismos to handle a nil result if that is
the established invalid-filter convention.
In `@test/controllers/sismos_controller_filters_test.rb`:
- Around line 5-14: Strengthen the assertions in the date-range filter tests,
specifically “filters by date_from” and “filters by date range (date_from and
date_to combined),” by asserting the exact expected record counts as done in
“filters by date_to.” Keep the existing per-record date validation and response
checks while replacing the weak length/total comparisons.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 963ce6be-4e51-4ad6-bc0c-01cfd4aa7f14
📒 Files selected for processing (10)
.rubocop.ymlMakefileapp/controllers/sismos_controller.rbapp/models/sismo.rbdb/migrate/20260803034001_add_filter_indexes_to_sismos.rbdb/schema.rbtest/controllers/sismos_controller_filters_test.rbtest/controllers/sismos_controller_test.rbtest/fixtures/sismos.ymltest/models/sismo_test.rb
- add database indexes for query optimization
8549947 to
395e596
Compare
1 participant
Summary by CodeRabbit
New Features
Bug Fixes