v4: add QDC-style audio lookup endpoints + /verses/by_range (shared presenter, strict validation, Pagy) with request specs #746
Conversation
* expose /api/v4/audio/reciters/:id/timestamp and /lookup mirroring QDC behavior * add view templates and request specs covering segment and timestamp responses Tests: bundle exec rspec spec/requests/api/v4/audio_recitations_spec.rb
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughAdds three API features: two audio recitation endpoints (timestamp and lookup_ayah) with presenter-backed JSON streamer views; a verses by-range endpoint with controller action and finder logic to compute and load an ayah range; and accompanying request specs for the new endpoints. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Router
participant RecitationsCtrl
participant SegmentPresenter
participant StreamView
Note over Client,StreamView: Audio timestamp endpoint
Client->>Router: GET /api/v4/audio/reciters/:reciter_id/timestamp
Router->>RecitationsCtrl: timestamp(params)
RecitationsCtrl->>SegmentPresenter: new(params)
RecitationsCtrl->>StreamView: render timestamp.json.streamer
StreamView->>SegmentPresenter: find_timestamp
StreamView-->>Client: { "result": ... }
Note over Client,StreamView: Audio lookup_ayah endpoint
Client->>Router: GET /api/v4/audio/reciters/:reciter_id/lookup
Router->>RecitationsCtrl: lookup_ayah(params)
RecitationsCtrl->>SegmentPresenter: new(params)
RecitationsCtrl->>StreamView: render lookup_ayah.json.streamer
StreamView->>SegmentPresenter: lookup_ayah / include_segments?
alt segment present
StreamView-->>Client: { "result": { verse_key, timestamp_from, timestamp_to, duration, segments? } }
else no segment
StreamView-->>Client: { "result": {} }
end
sequenceDiagram
participant Client
participant Router
participant VersesCtrl
participant VerseFinder
participant View
Note over Client,View: Verses by-range endpoint
Client->>Router: GET /api/v4/verses/by_range?from=...&to=...
Router->>VersesCtrl: by_range(params)
VersesCtrl->>VerseFinder: initialize(params)
VersesCtrl->>VerseFinder: fetch_by_range
VerseFinder->>VerseFinder: validate from/to, compute ayah ids
VerseFinder->>VerseFinder: get_ayah_range_to_load -> offset/limit
VerseFinder-->>VersesCtrl: `@results` (verse list or error)
VersesCtrl->>View: render verses partial
View-->>Client: JSON array of verses (or error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
app/controllers/api/v4/audio/recitations_controller.rb(2 hunks)app/views/api/v4/audio/recitations/lookup_ayah.json.streamer(1 hunks)app/views/api/v4/audio/recitations/timestamp.json.streamer(1 hunks)config/routes/api/v4.rb(1 hunks)spec/requests/api/v4/audio_recitations_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/api/v4/audio/recitations_controller.rb (3)
app/controllers/api/v4/chapters_controller.rb (1)
before_action(4-23)app/controllers/api/v4/audio/chapters_controller.rb (1)
before_action(4-19)app/controllers/api/v4/audio/sections_controller.rb (1)
before_action(4-15)
🔇 Additional comments (2)
app/views/api/v4/audio/recitations/timestamp.json.streamer (1)
1-3: LGTM: Clean and minimal JSON template.The template correctly renders the timestamp result from the presenter in a simple, straightforward structure.
config/routes/api/v4.rb (1)
27-28: The review comment's technical concern is incorrect.The new routes are actually correctly parameterized. The
Audio::RecitationPresenterexplicitly expectsparams[:reciter_id](line 68 ofapp/presenters/audio/recitation_presenter.rb):def recitation_id params[:reciter_id] endThis means:
- New routes (
reciters/:reciter_id) correctly passparams[:reciter_id]✓- Existing routes (
qaris/:id) passparams[:id], which wouldn't match the presenter's expectations ifrecitation_idis called ✗The SegmentPresenter inherits from RecitationPresenter and uses the same
recitation_idmethod, so the new routes are compatible with both presenters.The path prefix inconsistency (using both "reciters" and "qaris") remains a valid API design concern, but it's separate from parameter naming and doesn't cause functional issues.
Likely an incorrect or invalid review comment.
| def timestamp | ||
| @presenter = ::Audio::SegmentPresenter.new(params) | ||
|
|
||
| render | ||
| end | ||
|
|
||
| def lookup_ayah | ||
| @presenter = ::Audio::SegmentPresenter.new(params) | ||
|
|
||
| render | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Eliminate code duplication between timestamp and lookup_ayah actions.
Both actions have identical implementations, which violates the DRY principle. This duplication should be consolidated.
Apply this refactor:
+ before_action :init_segment_presenter, only: [:timestamp, :lookup_ayah]
- before_action :init_presenter, except: [:timestamp, :lookup_ayah]
+ before_action :init_presenter, except: [:timestamp, :lookup_ayah]
def timestamp
- @presenter = ::Audio::SegmentPresenter.new(params)
-
render
end
def lookup_ayah
- @presenter = ::Audio::SegmentPresenter.new(params)
-
render
end
protected
def init_presenter
@presenter = ::Audio::RecitationPresenter.new(params)
end
+
+ def init_segment_presenter
+ @presenter = ::Audio::SegmentPresenter.new(params)
+ endCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/controllers/api/v4/audio/recitations_controller.rb around lines 23 to 33,
both timestamp and lookup_ayah actions duplicate presenter initialization;
extract that logic into a private set_presenter method that assigns @presenter =
::Audio::SegmentPresenter.new(params), add a before_action :set_presenter for
only: [:timestamp, :lookup_ayah], and leave the actions to just call render so
the duplicated lines are removed and behavior remains the same.
| end | ||
|
|
||
| def timestamp | ||
| @presenter = ::Audio::SegmentPresenter.new(params) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Verify params handling and consider strong parameters.
The controller passes params directly to Audio::SegmentPresenter without filtering. While this may be acceptable if the presenter handles validation internally, it's worth verifying whether strong parameters should be used here for security and performance.
Run the following script to check how SegmentPresenter handles params and whether other controllers in the codebase use strong parameters:
🏁 Script executed:
#!/bin/bash
# Check SegmentPresenter param handling
ast-grep --pattern 'class SegmentPresenter
$$$
def initialize($PARAMS)
$$$
end
$$$
'
# Check if other audio controllers use strong parameters
rg -nP --type=rb -C5 'params\.(permit|require)' -g '**/audio/**/*controller.rb'
# Check if Audio::RecitationPresenter (used by existing actions) filters params
rg -nP --type=rb -C5 'class RecitationPresenter' -A 20Length of output: 53
I'll re-run the verification with corrected syntax for Ruby file searching:
🏁 Script executed:
#!/bin/bash
# Search for SegmentPresenter class and initialize method
rg -nP --type=ruby 'class SegmentPresenter' -A 30
# Check strong parameters usage in audio controllers
rg -nP --type=ruby 'params\.(permit|require)' -g '**/audio/**/*controller.rb'
# Search for RecitationPresenter to compare param handling
rg -nP --type=ruby 'class RecitationPresenter' -A 30
# Check how params are passed to presenters in audio controllers
rg -nP --type=ruby 'Presenter\.new\(params' -g '**/audio/**/*controller.rb'Length of output: 1274
Now I need to examine the presenter implementations to see how they handle params:
🏁 Script executed:
#!/bin/bash
# Find SegmentPresenter file
fd -t f 'segment_presenter' --type ruby
# Find RecitationPresenter file
fd -t f 'recitation_presenter' --type ruby
# Search for SegmentPresenter class definition in app/presenters
rg -nP --type=ruby 'class.*SegmentPresenter' -B 2 -A 25
# Search for RecitationPresenter class definition
rg -nP --type=ruby 'class.*RecitationPresenter' -B 2 -A 25Length of output: 9994
Now let me examine the BasePresenter to complete the verification:
🏁 Script executed:
#!/bin/bash
# Find and examine BasePresenter
rg -nP 'class BasePresenter' -A 40
# Check SegmentPresenter for any initialize override
rg -nP 'class Audio::SegmentPresenter' -A 50 app/presenters/audio/segment_presenter.rb
# Check what params are accessed in SegmentPresenter/RecitationPresenter
rg -nP 'params\[' app/presenters/audio/Length of output: 5161
Consider adding strong parameters filtering at the controller level for consistency and security best practice.
No strong parameters filtering found in any audio controller. While SegmentPresenter (and other presenters) do implement selective param validation—type conversion on numeric params like word, timestamp, and whitelisting on fields like reciter_fields—this approach is less robust than controller-level filtering. Adopting strong parameters would centralize security concerns and follow Rails conventions. This pattern is consistent across audio presenters (RecitationPresenter, RadioPresenter, etc.), so addressing it here could be part of a broader refactor.
| segment = @presenter.lookup_ayah | ||
| include_segments = @presenter.include_segments? | ||
|
|
||
| json.object! do | ||
| json.result do | ||
| json.object! do | ||
| if segment | ||
| json.verse_key segment.verse_key | ||
| json.timestamp_from segment.timestamp_from | ||
| json.timestamp_to segment.timestamp_to | ||
| json.duration segment.duration_ms | ||
| json.segments segment.segments if include_segments | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider simplifying the nested object structure.
The template works correctly, but the double nesting of json.object! at lines 4 and 6 may be unnecessary depending on the desired JSON structure. If the goal is { "result": { "verse_key": ..., ... } }, the inner json.object! at line 6 can be removed.
If simplification is desired, consider:
json.object! do
json.result do
- json.object! do
- if segment
- json.verse_key segment.verse_key
- json.timestamp_from segment.timestamp_from
- json.timestamp_to segment.timestamp_to
- json.duration segment.duration_ms
- json.segments segment.segments if include_segments
- end
+ if segment
+ json.verse_key segment.verse_key
+ json.timestamp_from segment.timestamp_from
+ json.timestamp_to segment.timestamp_to
+ json.duration segment.duration_ms
+ json.segments segment.segments if include_segments
end
end
endHowever, verify the tests still pass, as the current structure may be intentional to ensure result is always an object even when empty.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| segment = @presenter.lookup_ayah | |
| include_segments = @presenter.include_segments? | |
| json.object! do | |
| json.result do | |
| json.object! do | |
| if segment | |
| json.verse_key segment.verse_key | |
| json.timestamp_from segment.timestamp_from | |
| json.timestamp_to segment.timestamp_to | |
| json.duration segment.duration_ms | |
| json.segments segment.segments if include_segments | |
| end | |
| end | |
| end | |
| end | |
| segment = @presenter.lookup_ayah | |
| include_segments = @presenter.include_segments? | |
| json.object! do | |
| json.result do | |
| if segment | |
| json.verse_key segment.verse_key | |
| json.timestamp_from segment.timestamp_from | |
| json.timestamp_to segment.timestamp_to | |
| json.duration segment.duration_ms | |
| json.segments segment.segments if include_segments | |
| end | |
| end | |
| end |
🤖 Prompt for AI Agents
In app/views/api/v4/audio/recitations/lookup_ayah.json.streamer around lines 1
to 16, the template uses nested json.object! calls producing result: { {} }
which is redundant if the intended output is { "result": { "verse_key": ..., ...
} }; remove the inner json.object! block (the one starting at line 6) and move
its contents directly under json.result so result is a single object, keeping
the conditional for segment and the include_segments behavior unchanged, and run
tests to verify behavior when segment is nil to ensure result remains an object
when required.
| describe 'GET /api/v4/audio/reciters/:reciter_id/timestamp' do | ||
| it 'returns the timestamp range from the presenter' do | ||
| presenter = instance_double(Audio::SegmentPresenter, find_timestamp: { 'timestamp_from' => 10, 'timestamp_to' => 25 }) | ||
|
|
||
| allow(Audio::SegmentPresenter).to receive(:new).and_return(presenter) | ||
|
|
||
| get '/api/v4/audio/reciters/7/timestamp', params: { chapter_number: 1 } | ||
|
|
||
| expect(response).to have_http_status(:ok) | ||
| expect(JSON.parse(response.body)).to eq('result' => { 'timestamp_from' => 10, 'timestamp_to' => 25 }) | ||
| end | ||
| end | ||
|
|
||
| describe 'GET /api/v4/audio/reciters/:reciter_id/lookup' do | ||
| it 'returns the segment details from the presenter' do | ||
| segment = instance_double( | ||
| 'AudioSegment', | ||
| verse_key: '1:1', | ||
| timestamp_from: 1000, | ||
| timestamp_to: 5000, | ||
| duration_ms: 4000, | ||
| segments: [[0, 1000, 2500], [1, 2500, 5000]] | ||
| ) | ||
|
|
||
| presenter = instance_double( | ||
| Audio::SegmentPresenter, | ||
| lookup_ayah: segment, | ||
| include_segments?: true | ||
| ) | ||
|
|
||
| allow(Audio::SegmentPresenter).to receive(:new).and_return(presenter) | ||
|
|
||
| get '/api/v4/audio/reciters/7/lookup', params: { chapter_number: 1, timestamp: 2000, segments: true } | ||
|
|
||
| expect(response).to have_http_status(:ok) | ||
| expect(JSON.parse(response.body)).to eq( | ||
| 'result' => { | ||
| 'verse_key' => '1:1', | ||
| 'timestamp_from' => 1000, | ||
| 'timestamp_to' => 5000, | ||
| 'duration' => 4000, | ||
| 'segments' => [[0, 1000, 2500], [1, 2500, 5000]] | ||
| } | ||
| ) | ||
| end | ||
|
|
||
| it 'returns an empty result when no segment is found' do | ||
| presenter = instance_double( | ||
| Audio::SegmentPresenter, | ||
| lookup_ayah: nil, | ||
| include_segments?: false | ||
| ) | ||
|
|
||
| allow(Audio::SegmentPresenter).to receive(:new).and_return(presenter) | ||
|
|
||
| get '/api/v4/audio/reciters/7/lookup', params: { chapter_number: 1, timestamp: 2000 } | ||
|
|
||
| expect(response).to have_http_status(:ok) | ||
| expect(JSON.parse(response.body)).to eq('result' => {}) | ||
| end | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding edge case and error handling tests.
The current test coverage validates happy path scenarios. Consider adding tests for:
- Invalid or missing
reciter_id - Missing required parameters (e.g.,
chapter_number,timestamp) - Presenter initialization failures
- Invalid parameter types
These tests would ensure robust error handling and input validation.
🤖 Prompt for AI Agents
In spec/requests/api/v4/audio_recitations_spec.rb around lines 6 to 66, the
tests only cover happy paths; add focused specs for edge cases: (1)
invalid/missing reciter_id — issue requests with a non-numeric or absent
reciter_id and expect 404 or 400 as the controller should respond, (2) missing
required params — call the endpoints without chapter_number and/or timestamp and
assert the controller returns an appropriate 400/422 and error body, (3)
presenter initialization failure — stub Audio::SegmentPresenter.new to raise an
error or return nil and assert the endpoint handles it (500 or a structured
error response), and (4) invalid param types — pass strings or arrays where
numbers are expected and assert validation errors; for each test, stub presenter
behavior where needed, perform the GET, and assert HTTP status and JSON
error/result shape to ensure robust error handling.
| it 'returns the timestamp range from the presenter' do | ||
| presenter = instance_double(Audio::SegmentPresenter, find_timestamp: { 'timestamp_from' => 10, 'timestamp_to' => 25 }) | ||
|
|
||
| allow(Audio::SegmentPresenter).to receive(:new).and_return(presenter) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Verify that parameters are correctly passed to the presenter.
The test mocks Audio::SegmentPresenter.new but doesn't verify that the correct parameters are passed to the constructor. Consider adding an expectation to ensure params are properly forwarded.
Apply this enhancement:
- allow(Audio::SegmentPresenter).to receive(:new).and_return(presenter)
+ expect(Audio::SegmentPresenter).to receive(:new).with(hash_including('reciter_id' => '7', 'chapter_number' => '1')).and_return(presenter)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| allow(Audio::SegmentPresenter).to receive(:new).and_return(presenter) | |
| expect(Audio::SegmentPresenter).to receive(:new).with(hash_including('reciter_id' => '7', 'chapter_number' => '1')).and_return(presenter) |
🤖 Prompt for AI Agents
In spec/requests/api/v4/audio_recitations_spec.rb around line 10, the test stubs
Audio::SegmentPresenter.new but does not assert the constructor was called with
the correct parameters; update the test to expect the presenter to be
initialized with the same arguments the controller/action passes (for example
replace the stub with: expect(Audio::SegmentPresenter).to
receive(:new).with(expected_args).and_return(presenter) or use
.with(hash_including(...)) where expected_args are derived from the test’s
segment/request variables), ensuring the expectation checks the concrete params
forwarded to the presenter before returning the presenter.
* expose GET /api/v4/verses/by_range and reuse the shared presenter/view so v4 clients can fetch custom ayah spans like QDC * port range validation + pagination logic from the QDC finder into V4::VerseFinder, including strict verse-key checks and Pagy-based slicing * add deterministic request specs for /api/v4/verses/by_range, with in-spec fixtures that seed the minimal language/mushaf/chapter/verse data needed for the tests Test * bundle exec rspec spec/requests/api/v4/verses_by_range_spec.rb
basit3407
changed the title
* run bundle install so Gemfile.lock records cld3 (= 3.4.3) under DEPENDENCIES * keep the lockfile aligned with the Gemfile to prevent deployment/CI bundle errors
1 participant
Summary
This PR combines two related additions to the v4 API:
QDC-style segment lookup for audio recitations:
GET /api/v4/audio/reciters/:reciter_id/timestampGET /api/v4/audio/reciters/:reciter_id/lookupThese mirror QDC behavior and render via dedicated JSON streamer views for consistent payloads.
A verse-range retrieval endpoint:
GET /api/v4/verses/by_rangeThis reuses the shared presenter/view so v4 clients can fetch custom ayah spans with the same response shape as other verse endpoints.
Endpoints
Audio (added earlier in this PR)
GET /api/v4/audio/reciters/:reciter_id/timestampGET /api/v4/audio/reciters/:reciter_id/lookupBehavior mirrors QDC: timestamp lookup and verse (ayah) lookup including optional segments.
Verses (new commit)
GET /api/v4/verses/by_rangeQuery params:
from: starting verse key, e.g.2:1to: ending verse key, e.g.2:5page,per_pageExample:
GET /api/v4/verses/by_range?from=2:1&to=2:5&per_page=10&page=1Implementation Notes
Audio lookups
timestampandlookup_ayahactions.Verse range
V4::VerseFinder.Tests
/api/v4/audio/reciters/:reciter_id/timestampand/lookup./api/v4/verses/by_rangewith minimal in-spec fixtures (language, mushaf, chapter, verse).Backwards Compatibility
Follow-ups