Staging #712
Staging #712
Conversation
[QF-921]: introduces country_lanaguage_preference endpoint
QF-1475 filter by permission to share
WalkthroughAdds resource permission enums/scope, a CountryLanguagePreference model plus QDC API endpoint and presenters, schema and migration updates (languages unique index, resource permissions), host/test/Sentry config tweaks, health/readiness endpoints, a pinned Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Router as API Router
participant C as Api::Qdc::ResourcesController
participant Pref as CountryLanguagePreference
participant RC as ResourceContent (approved + allowed_to_share)
participant View as country_language_preference.json.streamer
Client->>Router: GET /api/qdc/resources/country_language_preference?user_device_language=xx&country=US
Router->>C: country_language_preference(params)
C->>C: validate params[:user_device_language] (Language)
alt country provided
C->>Pref: with_includes.where(user_device_language:'xx', country:'US').first
alt found
C->>RC: load defaults (translations/tafsir/mushaf/reciter) using approved + allowed_to_share
C->>View: render payload
else not found
C->>Pref: with_includes.where(user_device_language:'xx', country:nil).first
alt found
C->>RC: load defaults using approved + allowed_to_share
C->>View: render payload
else
C-->>Client: 404
end
end
else no country
C->>Pref: with_includes.where(user_device_language:'xx', country:nil).first || any language match
alt found
C->>RC: load defaults using approved + allowed_to_share
C->>View: render payload
else
C-->>Client: 404
end
end
View-->>Client: 200 JSON (when found)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 35
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (19)
app/presenters/tafsirs_presenter.rb (1)
74-85: Minor: simplify lookup and fix local var naming.Use idiomatic snake_case and avoid building an OR scope twice.
- approved_Tafsirs = ResourceContent + approved_tafsirs = ResourceContent .approved .tafsirs .allowed_to_share - approved_tafsir = approved_Tafsirs - .where(id: id_or_slug) - .or(approved_Tafsirs.where(slug: id_or_slug)) - .first + approved_tafsir = approved_tafsirs.find_by(id: id_or_slug) || + approved_tafsirs.find_by(slug: id_or_slug)app/finders/verse_finder.rb (1)
55-61: Use a narrow subquery to reduce planner work.Selecting only ids in the subquery avoids projecting unnecessary columns and can help the planner use covering indexes.
- approved_word_by_word_translations = ResourceContent.approved.one_word.translations_only.allowed_to_share + approved_word_by_word_translations = ResourceContent + .approved + .one_word + .translations_only + .allowed_to_share + .select(:id)app/controllers/api/v3/tafsirs_controller.rb (1)
9-9: Rename typo: tafisr_id → tafsir_id, and avoid loading full rows.Minor readability fix; also pick only the id to reduce IO.
- .find_by(resource_content_id: tafisr_id) + .find_by(resource_content_id: tafsir_id) ... - def tafisr_id + def tafsir_id - tafsir = approved_tafsir.where(id: params[:tafsir]) - .or(approved_tafsir.where(slug: params[:tafsir])) - .first - - tafsir&.id || raise_not_found("tafsir not found") + id = approved_tafsir + .where(id: params[:tafsir]) + .or(approved_tafsir.where(slug: params[:tafsir])) + .pick(:id) + id || raise_not_found("tafsir not found")Also applies to: 22-33
app/models/concerns/resourceable.rb (1)
14-20: Prefer association when already loaded to avoid an extra query.If resource_content is eager-loaded, reuse it; otherwise fall back to the scoped find.
def get_resource_content if respond_to? :resource_content_id - ResourceContent.allowed_to_share.find(resource_content_id) + return resource_content if association(:resource_content).loaded? + ResourceContent.allowed_to_share.find(resource_content_id) else resource_content end endapp/controllers/api/v3/search_controller.rb (1)
28-37: Indexing advice to keep queries fast.Given frequent filtering by permission_to_share, consider DB indexes:
- resource_contents(permission_to_share)
- resource_contents(slug) (likely exists)
- Optional composite covering typical chains, e.g., resource_contents(type, permission_to_share, priority)
app/controllers/mobile/translations_controller.rb (1)
11-16: Guard against nil resource in download
find_bymay return nil; current code would raise onincrement_download_count!. Return 404 when not found.- resource = ResourceContent.find_by(mobile_translation_id: params[:id]) - resource.increment_download_count! - - redirect_to resource.sqlite_file_url + resource = ResourceContent.find_by(mobile_translation_id: params[:id]) + unless resource + head :not_found and return + end + resource.increment_download_count! + redirect_to resource.sqlite_file_urlapp/finders/v4/verse_finder.rb (1)
188-199: Restrict default-language fallback to shareable resources
In app/finders/v4/verse_finder.rb, update thewords_with_default_translationscope to includeresource_content_id: approved_word_by_word_translationsso it only returns allowed_to_share entries. Verified no other one-word translations-only fallbacks are missing this constraint.app/controllers/api/v4/api_controller.rb (2)
9-16: DRY the repeated scope chainConsider named scopes to centralize these chains and reduce drift, e.g.,
ResourceContent.shareable_translations_one_verse_approvedandshareable_tafsirs_one_verse_approved.Example in ResourceContent (illustrative):
scope :shareable_translations_one_verse_approved, -> { translations.one_verse.approved.allowed_to_share } scope :shareable_tafsirs_one_verse_approved, -> { tafsirs.one_verse.approved.allowed_to_share }Also applies to: 18-26
1-4: Add supporting DB indexes for the new filterGiven frequent use of
allowed_to_sharewithapprovedand resource type, add composite/partial indexes on resource_contents to keep queries fast, e.g.,(permission_to_share, approved, resource_type)or tailored to actual columns used in scopes.app/presenters/translations_presenter.rb (1)
48-61: Minor: clarify local variable name and keep usage consistent
translations = params[:resource_id].to_sis confusing. Rename toresource_keyfor readability.- if params[:resource_id] - translations = params[:resource_id].to_s + if params[:resource_id] + resource_key = params[:resource_id].to_s approved_translations = ResourceContent .approved .translations .one_verse .allowed_to_share - params[:resource_id] = approved_translations - .where(id: translations) - .or(approved_translations.where(slug: translations)) + params[:resource_id] = approved_translations + .where(id: resource_key) + .or(approved_translations.where(slug: resource_key)) .pick(:id)app/controllers/api/v3/options_controller.rb (1)
32-41: Align recitations with share-permission gating to avoid surfacing unusable itemsV3 recitations still return all approved entries. This can surface recitations whose underlying resource_content is rejected, leading to later 404s. Gate here too.
Apply:
def recitations list = Recitation - .eager_load(reciter: :translated_name) - .approved + .eager_load(reciter: :translated_name) + .joins(:resource_content) + .merge(ResourceContent.allowed_to_share) + .approved .order('translated_names.language_priority desc')app/controllers/api/v4/resources_controller.rb (1)
77-83: Also gate recitation_info by share permissionsEnsure info endpoint doesn’t expose recitations tied to rejected resource_content.
- @recitation = Recitation - .includes(:resource_content) - .approved - .find_by(id: params[:recitation_id]) + @recitation = Recitation + .joins(:resource_content) + .merge(ResourceContent.allowed_to_share) + .includes(:resource_content) + .approved + .find_by(id: params[:recitation_id])app/controllers/api/qdc/resources_controller.rb (7)
59-66: Recitations: also gate by share permissionsMirror the v4 controller by joining resource_content and merging allowed_to_share.
- list = Recitation - .eager_load(reciter: :translated_name) - .approved - .order('translated_names.language_priority desc') + list = Recitation + .eager_load(reciter: :translated_name) + .joins(:resource_content) + .merge(ResourceContent.allowed_to_share) + .approved + .order('translated_names.language_priority desc')
70-85: Recitation info: enforce allowed_to_sharePrevent exposing metadata of rejected content.
- @recitation = Recitation - .approved - .find_by(id: params[:recitation_id]) + @recitation = Recitation + .joins(:resource_content) + .merge(ResourceContent.allowed_to_share) + .approved + .find_by(id: params[:recitation_id]) @@ - resource = ResourceContent - .eager_load(:translated_name) - .where(id: @recitation&.resource_content_id) + resource = ResourceContent + .eager_load(:translated_name) + .allowed_to_share + .where(id: @recitation&.resource_content_id)
31-41: Tafsirs: missing allowed_to_shareAdd the scope to stay consistent with v4 and sharing policy.
list = ResourceContent .eager_load(:translated_name) .tafsirs - .approved + .approved + .allowed_to_share .order('priority ASC')
43-49: Word-by-word translations: missing allowed_to_shareGate the list to exclude rejected content.
- list = ResourceContent.eager_load(:translated_name).approved.one_word.translations_only.order('priority ASC') + list = ResourceContent.eager_load(:translated_name).approved.allowed_to_share.one_word.translations_only.order('priority ASC')
91-101: Chapter infos: apply allowed_to_shareAlign with v4 controller.
list = ResourceContent .eager_load(:translated_name) .chapter_info .one_chapter - .approved + .approved + .allowed_to_share
103-110: Verse media: apply allowed_to_share (and keep one_verse)Match v4 behavior.
@media = ResourceContent .includes(:language) .media - .one_verse.approved + .one_verse.approved + .allowed_to_share
202-209: Translations loader: missing allowed_to_shareEnsure QDC translations list honors share permissions.
list = ResourceContent .eager_load(:translated_name) .one_verse .translations - .approved + .approved + .allowed_to_share .order('priority ASC')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
Gemfile(1 hunks)app/controllers/api/qdc/quran_controller.rb(1 hunks)app/controllers/api/qdc/recitations_controller.rb(1 hunks)app/controllers/api/qdc/resources_controller.rb(1 hunks)app/controllers/api/qdc/tafsirs_controller.rb(1 hunks)app/controllers/api/qdc/translations_controller.rb(1 hunks)app/controllers/api/v3/options_controller.rb(3 hunks)app/controllers/api/v3/search_controller.rb(1 hunks)app/controllers/api/v3/tafsirs_controller.rb(1 hunks)app/controllers/api/v4/api_controller.rb(2 hunks)app/controllers/api/v4/resources_controller.rb(7 hunks)app/controllers/api/v4/search_controller.rb(1 hunks)app/controllers/application_controller.rb(1 hunks)app/controllers/mobile/translations_controller.rb(1 hunks)app/finders/v4/verse_finder.rb(1 hunks)app/finders/verse_finder.rb(1 hunks)app/models/concerns/resourceable.rb(1 hunks)app/models/country_language_preference.rb(1 hunks)app/models/language.rb(2 hunks)app/models/resource_content.rb(7 hunks)app/presenters/qdc/recitations_presenter.rb(1 hunks)app/presenters/qdc/tafsirs_presenter.rb(1 hunks)app/presenters/qdc/translations_presenter.rb(1 hunks)app/presenters/qdc/verses_presenter.rb(2 hunks)app/presenters/recitations_presenter.rb(1 hunks)app/presenters/tafsirs_presenter.rb(1 hunks)app/presenters/translations_presenter.rb(1 hunks)app/presenters/verses_presenter.rb(2 hunks)app/views/api/qdc/recitations/_audio_files.json.streamer(2 hunks)app/views/api/qdc/resources/country_language_preference.json.streamer(1 hunks)app/views/api/v4/resources/chapter_reciters.json.streamer(1 hunks)config/environments/test.rb(1 hunks)config/initializers/hosts.rb(1 hunks)config/initializers/raven.rb(1 hunks)config/routes/api/qdc.rb(1 hunks)db/migrate/20240918072140_language_iso_code_unique.rb(1 hunks)db/migrate/20240918072240_create_country_language_preferences.rb(1 hunks)db/migrate/20241228151537_add_resource_permissions.rb(1 hunks)db/schema.rb(47 hunks)spec/requests/api/qdc/resources_country_language_preference_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (16)
app/controllers/api/qdc/quran_controller.rb (2)
app/controllers/api/v4/quran_controller.rb (2)
resource_filters(110-122)humanize_filter_names(124-130)app/controllers/api/v4/api_controller.rb (1)
fetch_tafsir_resource(18-27)
app/controllers/mobile/translations_controller.rb (4)
app/controllers/api/v4/resources_controller.rb (1)
translations(11-24)app/controllers/api/v3/options_controller.rb (1)
translations(19-30)app/controllers/api/v3/search_controller.rb (1)
translations(21-40)app/controllers/api/v4/search_controller.rb (1)
translations(21-40)
config/environments/test.rb (1)
config/application.rb (1)
config(22-49)
app/controllers/api/qdc/tafsirs_controller.rb (3)
app/controllers/api/qdc/recitations_controller.rb (2)
protected(4-9)init_presenter(6-8)app/presenters/qdc/tafsirs_presenter.rb (1)
protected(3-29)app/controllers/api/qdc/translations_controller.rb (1)
init_presenter(9-11)
app/models/country_language_preference.rb (1)
app/models/mushaf.rb (1)
scope(28-59)
app/models/concerns/resourceable.rb (1)
app/finders/verse_finder.rb (2)
find(3-125)find(4-12)
app/controllers/api/v3/tafsirs_controller.rb (2)
app/controllers/api/v4/resources_controller.rb (1)
tafsirs(42-54)app/controllers/api/v3/options_controller.rb (1)
tafsirs(58-69)
app/controllers/api/qdc/recitations_controller.rb (6)
app/presenters/qdc/recitations_presenter.rb (1)
protected(4-15)app/controllers/api/qdc/tafsirs_controller.rb (2)
protected(4-9)init_presenter(6-8)app/controllers/api/qdc/translations_controller.rb (1)
init_presenter(9-11)app/controllers/api/qdc/verses_controller.rb (1)
init_presenter(12-14)app/controllers/api/qdc/audio/recitations_controller.rb (1)
init_presenter(48-50)app/controllers/api/qdc/chapters_controller.rb (1)
init_presenter(16-18)
db/migrate/20240918072140_language_iso_code_unique.rb (1)
db/migrate/20240918072240_create_country_language_preferences.rb (2)
change(1-23)change(2-22)
db/migrate/20240918072240_create_country_language_preferences.rb (1)
db/migrate/20240918072140_language_iso_code_unique.rb (2)
change(1-6)change(2-5)
app/controllers/api/qdc/resources_controller.rb (1)
app/controllers/application_controller.rb (2)
render_bad_request(25-27)render_404(21-23)
app/presenters/qdc/verses_presenter.rb (2)
app/presenters/verses_presenter.rb (2)
fetch_tafsirs(257-274)fetch_translations(276-294)app/lib/quran_utils/strong_memoize.rb (1)
strong_memoize(23-29)
app/presenters/qdc/translations_presenter.rb (3)
app/presenters/translations_presenter.rb (2)
resource_id(47-66)translations(38-40)app/lib/quran_utils/strong_memoize.rb (1)
strong_memoize(23-29)app/controllers/api/v4/resources_controller.rb (1)
translations(11-24)
app/presenters/qdc/tafsirs_presenter.rb (5)
app/controllers/api/qdc/tafsirs_controller.rb (1)
protected(4-9)app/presenters/qdc/translations_presenter.rb (1)
protected(3-26)app/presenters/tafsirs_presenter.rb (1)
resource(69-92)app/lib/quran_utils/strong_memoize.rb (1)
strong_memoize(23-29)app/presenters/base_presenter.rb (1)
raise_404(39-41)
app/presenters/qdc/recitations_presenter.rb (3)
app/controllers/api/qdc/recitations_controller.rb (1)
protected(4-9)app/presenters/recitations_presenter.rb (1)
recitation_id(48-61)app/lib/quran_utils/strong_memoize.rb (1)
strong_memoize(23-29)
db/schema.rb (2)
app/controllers/mobile/translations_controller.rb (2)
index(4-17)index(5-9)app/controllers/api/v3/audio_files_controller.rb (2)
index(4-23)index(5-12)
🔇 Additional comments (34)
app/views/api/v4/resources/chapter_reciters.json.streamer (2)
36-36: LGTM (no functional change).
23-31: Approved_recitations is N+1 safe
Audio::RecitationPresenter#recitations (app/presenters/audio/recitation_presenter.rb) preloads :recitation_style, :qirat_type, and reciter: :translated_name, covering translated_name.Gemfile (1)
56-56: Relax thecountriesgem pin to allow patch updatesSwitch to a pessimistic constraint so you can receive future bug-fix and security releases:
-gem 'countries', '= 6.0.1' +gem 'countries', '~> 6.0'Then, in your local environment (ensure you have bundler-audit installed via
gem install bundler-audit), run:# List all 6.x patch versions curl -s https://rubygems.org/api/v1/versions/countries.json | jq '.[].number | select(startswith("6."))' # Check for known advisories bundle audit update && bundle audit check --verbosedb/migrate/20240918072140_language_iso_code_unique.rb (1)
1-6: Manual verification required for existing duplicates
Cannot confirm absence of duplicate iso_code entries in your languages table due to environment limitations—please run the provided SQL check against your database before applying the concurrent unique index migration.db/migrate/20241228151537_add_resource_permissions.rb (1)
1-6: Enforce stricter enum constraints on permissions
- Change both columns to
null: false, retaindefault: 0, and add a descriptivecomment: '0: unknown, 1: requested, 2: granted, 3: rejected'- Add check constraints to guarantee values are in
(0,1,2,3)- If you frequently filter by
permission_to_share, add an index on it- For large tables on PG < 11, rollout zero-downtime: add nullable cols without default → backfill in batches → set
NOT NULL+ defaultclass AddResourcePermissions < ActiveRecord::Migration[7.0] def change - add_column :resource_contents, :permission_to_host, :integer, default: 0 - add_column :resource_contents, :permission_to_share, :integer, default: 0 + add_column :resource_contents, :permission_to_host, :integer, null: false, default: 0, comment: '0: unknown, 1: requested, 2: granted, 3: rejected' + add_column :resource_contents, :permission_to_share, :integer, null: false, default: 0, comment: '0: unknown, 1: requested, 2: granted, 3: rejected' + add_check_constraint :resource_contents, 'permission_to_host IN (0,1,2,3)', name: 'rc_permission_to_host_valid' + add_check_constraint :resource_contents, 'permission_to_share IN (0,1,2,3)', name: 'rc_permission_to_share_valid' + # add_index :resource_contents, :permission_to_share end endapp/presenters/tafsirs_presenter.rb (1)
77-77: Good: enforce share-permissions with allowed_to_share.This aligns selection with visibility rules.
app/models/language.rb (1)
4-4: Schema comment reflects UNIQUE iso_code. Verify data readiness.Ensure no duplicate iso_codes exist before applying the unique index migration; otherwise it will fail.
If needed, add a pre-migration to dedupe or block duplicates and run the index concurrently in production.
Also applies to: 19-19
app/views/api/qdc/recitations/_audio_files.json.streamer (1)
1-7: LGTM: compute audio_files in-view via presenter.Removes reliance on controller instance state; clearer data flow.
config/routes/api/qdc.rb (1)
31-32: Route mapping and spec coverage confirmed. The GET country_language_preference route under api/qdc/resources correctly maps to Api::Qdc::ResourcesController#country_language_preference, and the request specs exercise 200 (global, country-specific, fallback), 404 when no match, and invalid‐country (ISO-3166) scenarios.config/environments/test.rb (1)
51-52: Allowing all hosts in test is fine; keep it scoped to test only.This prevents HostAuthorization 403s in request specs and is safely limited to test env. No action needed.
app/controllers/api/qdc/tafsirs_controller.rb (1)
5-8: Presenter initialization override is consistent with QDC pattern.Matches the translations/recitations controllers approach. Good to go.
app/controllers/api/qdc/recitations_controller.rb (1)
5-8: Presenter initialization looks correct.Consistent with QDC namespacing; no issues spotted.
app/controllers/api/qdc/translations_controller.rb (1)
10-10: Wiring to QDC presenter looks correct.Instantiation of Qdc::TranslationsPresenter aligns with the new QDC-specific resolution logic.
app/finders/verse_finder.rb (1)
55-66: Edge case: no share-allowed WWB resources.If allowed_to_share filters everything out, this will silently return no words. If that’s acceptable, fine; otherwise consider a fallback (e.g., raise or log) to make the behavior explicit.
app/controllers/api/v3/tafsirs_controller.rb (1)
23-27: Gating tafsir by allowed_to_share is correct and consistent.app/models/concerns/resourceable.rb (1)
16-16: Behavior change: find now scoped to allowed_to_share.This will raise ActiveRecord::RecordNotFound for previously associated but non-shareable content. Confirm global exception handling maps this to the intended API response (likely 404).
app/controllers/api/v3/search_controller.rb (1)
28-31: LGTM: translations constrained to shareable set.Consistent with other controllers/presenters applying allowed_to_share.
app/controllers/api/v4/search_controller.rb (1)
28-33: Good: shareability is now enforced for search translationsAdding
.allowed_to_sharehere aligns search with the rest of the API and prevents returning disallowed content.app/controllers/api/v4/api_controller.rb (2)
9-16: LGTM: translations now restricted to shareable, approved one-verse resourcesScope addition is correct and consistent.
18-26: LGTM: tafsirs now restricted to shareable, approved one-verse resourcesMatches the intended policy across endpoints.
app/presenters/translations_presenter.rb (2)
52-57: Good: presenter respects shareability when resolving resource_idScope change is correct.
57-61: Usingpick(:id)is correct asV4::TranslationFinder#load_translationsexpects a scalarresource_id.app/controllers/api/v3/options_controller.rb (3)
25-26: Good: translations are now gated by share-permissionAdding allowed_to_share here aligns with the new sharing policy.
51-52: Good: chapter_info is now gated by share-permissionConsistent with translations/tafsirs.
63-64: Good: tafsirs are now gated by share-permissionMatches V4 behavior.
app/presenters/qdc/tafsirs_presenter.rb (1)
11-15: Consider scoping to one_verse for parity with V4If tafsir resources are one-verse in QDC too, include one_verse to match V4 selection and avoid mismatches.
Apply if applicable:
approved_tafsirs = ResourceContent .approved .tafsirs + .one_verse .allowed_to_shareapp/controllers/api/v4/resources_controller.rb (6)
16-18: LGTM: translations now respect share permissionsAdding allowed_to_share before approved is correct and consistent.
35-35: LGTM: word-by-word translations gatedScope application looks good.
47-48: LGTM: tafsirs gated by share permissionsThis prevents leaking rejected content.
100-101: LGTM: chapter_infos gatedConsistent with other endpoints.
111-114: LGTM: verse_media now properly filteredGood addition of one_verse, approved, and allowed_to_share.
130-131: LGTM: changes endpoint gatedPrevents listing rejected content in change logs.
app/models/country_language_preference.rb (1)
29-35: Confirm FK delete behavior — cascade may delete whole preferences unexpectedly.FKs to mushafs/reciters/resource_contents/languages are optional; ON DELETE CASCADE will drop CountryLanguagePreference rows if the parent is removed. Usually ON DELETE SET NULL is safer here.
Would you like to switch these FKs to nullify in the migration?
app/models/resource_content.rb (1)
54-67: Enums definition looks good.
| def tafsir | ||
| @presenter = Qdc::TafsirsPresenter.new(params) | ||
| filters = resource_filters(@resource) | ||
| @filter_names = humanize_filter_names(filters) | ||
|
|
||
| @tafsirs = if (@resource = fetch_tafsir_resource) | ||
| Tafsir.order('verse_id ASC').where(filters) | ||
| else | ||
| [] | ||
| end | ||
|
|
||
| render | ||
| end |
There was a problem hiding this comment.
Bug: filters computed before @resource is set, dropping resource_content_id
resource_filters(@resource) runs with nil, so queries miss resource scoping.
Move filter construction after assigning @resource (and build @filter_names from the corrected filters):
def tafsir
- @presenter = Qdc::TafsirsPresenter.new(params)
- filters = resource_filters(@resource)
- @filter_names = humanize_filter_names(filters)
-
- @tafsirs = if (@resource = fetch_tafsir_resource)
- Tafsir.order('verse_id ASC').where(filters)
- else
- []
- end
+ @presenter = Qdc::TafsirsPresenter.new(params)
+ if (@resource = fetch_tafsir_resource)
+ filters = resource_filters(@resource)
+ @filter_names = humanize_filter_names(filters)
+ @tafsirs = Tafsir.where(filters).order(verse_id: :asc)
+ else
+ @filter_names = {}
+ @tafsirs = []
+ end
render
end📝 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.
| def tafsir | |
| @presenter = Qdc::TafsirsPresenter.new(params) | |
| filters = resource_filters(@resource) | |
| @filter_names = humanize_filter_names(filters) | |
| @tafsirs = if (@resource = fetch_tafsir_resource) | |
| Tafsir.order('verse_id ASC').where(filters) | |
| else | |
| [] | |
| end | |
| render | |
| end | |
| def tafsir | |
| @presenter = Qdc::TafsirsPresenter.new(params) | |
| if (@resource = fetch_tafsir_resource) | |
| filters = resource_filters(@resource) | |
| @filter_names = humanize_filter_names(filters) | |
| @tafsirs = Tafsir.where(filters).order(verse_id: :asc) | |
| else | |
| @filter_names = {} | |
| @tafsirs = [] | |
| end | |
| render | |
| end |
🤖 Prompt for AI Agents
In app/controllers/api/qdc/quran_controller.rb around lines 5 to 17,
resource_filters(@resource) is called before @resource is assigned so the
computed filters omit resource_content_id; move the filters =
resource_filters(@resource) line (and the subsequent @filter_names =
humanize_filter_names(filters)) to after the @resource = fetch_tafsir_resource
assignment and use the resulting filters when building @tafsirs (so filters
reflect the real @resource), leaving render as-is.
| def country_language_preference | ||
| user_device_language = request.query_parameters[:user_device_language].presence | ||
| country = request.query_parameters[:country].presence&.upcase | ||
|
|
||
| # Require a valid user_device_language always | ||
| if user_device_language.blank? | ||
| return render_bad_request('user_device_language is required') | ||
| end | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize language param; ensure case-insensitive matching
Downcase the ISO code before validation to avoid false negatives.
- user_device_language = request.query_parameters[:user_device_language].presence
+ user_device_language = request.query_parameters[:user_device_language].to_s.downcase.presence📝 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.
| def country_language_preference | |
| user_device_language = request.query_parameters[:user_device_language].presence | |
| country = request.query_parameters[:country].presence&.upcase | |
| # Require a valid user_device_language always | |
| if user_device_language.blank? | |
| return render_bad_request('user_device_language is required') | |
| end | |
| def country_language_preference | |
| user_device_language = request.query_parameters[:user_device_language].to_s.downcase.presence | |
| country = request.query_parameters[:country].presence&.upcase | |
| # Require a valid user_device_language always | |
| if user_device_language.blank? | |
| return render_bad_request('user_device_language is required') | |
| end |
🤖 Prompt for AI Agents
In app/controllers/api/qdc/resources_controller.rb around lines 119 to 127, the
user_device_language ISO code must be normalized for case-insensitive matching;
change the assignment so you convert the incoming param to a string, strip
whitespace and downcase it before presence validation (e.g. use
request.query_parameters[:user_device_language].to_s.strip.presence&.downcase or
equivalent) and then use that normalized variable for the blank check and any
subsequent validation/lookup.
| def country_language_preference | ||
| user_device_language = request.query_parameters[:user_device_language].presence | ||
| country = request.query_parameters[:country].presence&.upcase | ||
|
|
||
| # Require a valid user_device_language always | ||
| if user_device_language.blank? | ||
| return render_bad_request('user_device_language is required') | ||
| end | ||
|
|
||
| unless Language.exists?(iso_code: user_device_language) | ||
| return render_bad_request('Invalid user_device_language') | ||
| end | ||
|
|
||
| # Validate country only if provided | ||
| if country.present? | ||
| valid_countries = ISO3166::Country.all.map(&:alpha2) | ||
| unless valid_countries.include?(country) | ||
| return render_bad_request('Invalid country code') | ||
| end | ||
| end | ||
|
|
||
| if country.present? | ||
| # First try to find country-specific preference | ||
| preferences = CountryLanguagePreference.with_includes | ||
| .where(user_device_language: user_device_language, country: country) | ||
| @preference = preferences.first | ||
|
|
||
| # If no country-specific preference found, try global preference | ||
| unless @preference | ||
| @preference = CountryLanguagePreference.with_includes | ||
| .find_by(user_device_language: user_device_language, country: nil) | ||
| end | ||
| else | ||
| # No country provided: search by user_device_language only | ||
| # Prefer global (country: nil), then fall back to any match for that language | ||
| @preference = CountryLanguagePreference.with_includes | ||
| .find_by(user_device_language: user_device_language, country: nil) | ||
|
|
||
| unless @preference | ||
| @preference = CountryLanguagePreference.with_includes | ||
| .where(user_device_language: user_device_language) | ||
| .first | ||
| end | ||
| end | ||
|
|
||
| if @preference | ||
| # Filter out unapproved resources when building the response | ||
| @data = build_preference_data(@preference) | ||
| render | ||
| else | ||
| render_404("No matching country language preference found") | ||
| end | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
End-to-end verification suggestion
After applying the gating changes, verify no QDC/V4 endpoints return rejected resources.
🏁 Script executed:
#!/bin/bash
# Find ResourceContent queries missing allowed_to_share in controllers/presenters
rg -nP --type=ruby -C2 "(ResourceContent|Recitation).*approved" app/controllers app/presenters \
| sed -n '1,200p'
echo "---- Potential misses (heuristic):"
rg -nP --type=ruby -C2 "(ResourceContent|Recitation).*\\.(eager_load|includes|joins)\\(.*\\).*\\.(translations|tafsirs|chapter_info|media|one_verse|one_chapter)" app/controllers app/presenters \
| rg -v "allowed_to_share" -n -C2Length of output: 5273
Add allowed_to_share scope to QDC resource queries
- In
app/controllers/api/qdc/resources_controller.rb:44, changetoResourceContent .eager_load(:translated_name) .approved .one_word .translations_only
ResourceContent .eager_load(:translated_name) .approved .allowed_to_share .one_word .translations_only
- In the default translations block (
…; default_translations: ids.any? ? ResourceContent.where(id: ids).approved.includes(:translated_name) : []), insert.allowed_to_shareafter.approved.
Re-run the end-to-end check to confirm no QDC/V4 endpoints surface rejected resources.
🤖 Prompt for AI Agents
In app/controllers/api/qdc/resources_controller.rb around lines 119-171, the
resource queries used when building preference responses are missing the
allowed_to_share scope; update the ResourceContent query chains so that
.allowed_to_share is inserted immediately after .approved (e.g. .approved.then
.allowed_to_share.then the rest of the chain) for both the main resource query
and the default_translations branch (the conditional that builds
default_translations using ResourceContent.where(id:
ids).approved.includes(:translated_name) should become
.approved.allowed_to_share.includes(:translated_name)); rerun the end-to-end
checks to confirm rejected resources are excluded.
| if country.present? | ||
| valid_countries = ISO3166::Country.all.map(&:alpha2) | ||
| unless valid_countries.include?(country) | ||
| return render_bad_request('Invalid country code') | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optimize country validation
Avoid allocating all countries on each request; use a direct lookup.
- valid_countries = ISO3166::Country.all.map(&:alpha2)
- unless valid_countries.include?(country)
+ unless ISO3166::Country.find_country_by_alpha2(country)
return render_bad_request('Invalid country code')
end📝 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.
| if country.present? | |
| valid_countries = ISO3166::Country.all.map(&:alpha2) | |
| unless valid_countries.include?(country) | |
| return render_bad_request('Invalid country code') | |
| end | |
| if country.present? | |
| unless ISO3166::Country.find_country_by_alpha2(country) | |
| return render_bad_request('Invalid country code') | |
| end |
🤖 Prompt for AI Agents
In app/controllers/api/qdc/resources_controller.rb around lines 133 to 137, the
code builds ISO3166::Country.all.map(&:alpha2) on every request which allocates
the entire country list; replace this with a direct lookup such as using
ISO3166::Country.find_country_by_alpha2(country) (or ISO3166::Country[country])
and check whether that returns nil to determine validity, rendering the same
bad-request response if the lookup fails.
| def build_preference_data(preference) | ||
| # Sanitize CSV IDs for default translations | ||
| ids = if preference.default_translation_ids.present? | ||
| preference.default_translation_ids | ||
| .split(',') | ||
| .map(&:strip) | ||
| .reject(&:blank?) | ||
| .map(&:to_i) | ||
| else | ||
| [] | ||
| end | ||
|
|
||
| { | ||
| preference: preference, | ||
| default_mushaf: preference.mushaf&.enabled ? preference.mushaf : nil, | ||
| default_translations: ids.any? ? | ||
| ResourceContent.where(id: ids).approved.includes(:translated_name) : [], | ||
| default_tafsir: preference.tafsir&.approved? ? preference.tafsir : nil, | ||
| default_wbw_language: preference.wbw_language, | ||
| default_reciter: preference.reciter, | ||
| ayah_reflections_languages: Language.where(iso_code: preference.ayah_reflections_languages&.split(',') || []), | ||
| learning_plan_languages: Language.where(iso_code: preference.learning_plan_languages&.split(',') || []) | ||
| } | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Build payload with share-permission filters and robust parsing
- Gate default_translations/tafsir/reciter by allowed_to_share.
- Strip/downcase comma-separated ISO codes; de-duplicate.
- Optional: preserve caller-specified order of default_translation_ids.
# Sanitize CSV IDs for default translations
ids = if preference.default_translation_ids.present?
preference.default_translation_ids
.split(',')
.map(&:strip)
.reject(&:blank?)
.map(&:to_i)
else
[]
end
- {
+ {
preference: preference,
default_mushaf: preference.mushaf&.enabled ? preference.mushaf : nil,
- default_translations: ids.any? ?
- ResourceContent.where(id: ids).approved.includes(:translated_name) : [],
- default_tafsir: preference.tafsir&.approved? ? preference.tafsir : nil,
+ default_translations: ids.any? ?
+ ResourceContent.allowed_to_share.where(id: ids).approved.includes(:translated_name)
+ : [],
+ default_tafsir: preference.tafsir_id ?
+ ResourceContent.allowed_to_share.approved.find_by(id: preference.tafsir_id) : nil,
default_wbw_language: preference.wbw_language,
- default_reciter: preference.reciter,
- ayah_reflections_languages: Language.where(iso_code: preference.ayah_reflections_languages&.split(',') || []),
- learning_plan_languages: Language.where(iso_code: preference.learning_plan_languages&.split(',') || [])
+ default_reciter: preference.reciter_id ?
+ Recitation.joins(:resource_content).merge(ResourceContent.allowed_to_share).approved.find_by(id: preference.reciter_id) : nil,
+ ayah_reflections_languages: Language.where(
+ iso_code: (preference.ayah_reflections_languages || '').split(',').map { |c| c.strip.downcase }.uniq
+ ),
+ learning_plan_languages: Language.where(
+ iso_code: (preference.learning_plan_languages || '').split(',').map { |c| c.strip.downcase }.uniq
+ )
}Optional (preserve order for default_translations; Postgres):
# After the where(...):
.order(Arel.sql("array_position(ARRAY[#{ids.join(',')}], resource_contents.id)"))| add_foreign_key :country_language_preferences, :languages, column: :user_device_language, primary_key: :iso_code, on_delete: :cascade | ||
| add_foreign_key :country_language_preferences, :mushafs, column: :default_mushaf_id, on_delete: :cascade | ||
| add_foreign_key :country_language_preferences, :resource_contents, column: :default_tafsir_id, on_delete: :cascade | ||
| add_foreign_key :country_language_preferences, :languages, column: :default_wbw_language, primary_key: :iso_code, on_delete: :cascade | ||
| add_foreign_key :country_language_preferences, :reciters, column: :default_reciter, on_delete: :cascade | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Revisit ON DELETE cascade behavior.
Cascading deletes from languages/reciters/mushafs can wipe preferences unexpectedly; nullify may be safer.
- add_foreign_key :country_language_preferences, :languages, column: :user_device_language, primary_key: :iso_code, on_delete: :cascade
+ add_foreign_key :country_language_preferences, :languages, column: :user_device_language, primary_key: :iso_code, on_delete: :nullify
- add_foreign_key :country_language_preferences, :mushafs, column: :default_mushaf_id, on_delete: :cascade
+ add_foreign_key :country_language_preferences, :mushafs, column: :default_mushaf_id, on_delete: :nullify
- add_foreign_key :country_language_preferences, :resource_contents, column: :default_tafsir_id, on_delete: :cascade
+ add_foreign_key :country_language_preferences, :resource_contents, column: :default_tafsir_id, on_delete: :nullify
- add_foreign_key :country_language_preferences, :languages, column: :default_wbw_language, primary_key: :iso_code, on_delete: :cascade
+ add_foreign_key :country_language_preferences, :languages, column: :default_wbw_language, primary_key: :iso_code, on_delete: :nullify
- add_foreign_key :country_language_preferences, :reciters, column: :default_reciter, on_delete: :cascade
+ add_foreign_key :country_language_preferences, :reciters, column: :default_reciter, on_delete: :nullifyIf cascade is intentional, add a comment explaining the rationale.
📝 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.
| add_foreign_key :country_language_preferences, :languages, column: :user_device_language, primary_key: :iso_code, on_delete: :cascade | |
| add_foreign_key :country_language_preferences, :mushafs, column: :default_mushaf_id, on_delete: :cascade | |
| add_foreign_key :country_language_preferences, :resource_contents, column: :default_tafsir_id, on_delete: :cascade | |
| add_foreign_key :country_language_preferences, :languages, column: :default_wbw_language, primary_key: :iso_code, on_delete: :cascade | |
| add_foreign_key :country_language_preferences, :reciters, column: :default_reciter, on_delete: :cascade | |
| end | |
| add_foreign_key :country_language_preferences, :languages, column: :user_device_language, primary_key: :iso_code, on_delete: :nullify | |
| add_foreign_key :country_language_preferences, :mushafs, column: :default_mushaf_id, on_delete: :nullify | |
| add_foreign_key :country_language_preferences, :resource_contents, column: :default_tafsir_id, on_delete: :nullify | |
| add_foreign_key :country_language_preferences, :languages, column: :default_wbw_language, primary_key: :iso_code, on_delete: :nullify | |
| add_foreign_key :country_language_preferences, :reciters, column: :default_reciter, on_delete: :nullify | |
| end |
🤖 Prompt for AI Agents
In db/migrate/20240918072240_create_country_language_preferences.rb around lines
17–22 the foreign keys for user_device_language, default_mushaf_id,
default_tafsir_id, default_wbw_language and default_reciter all use on_delete:
:cascade which will remove user preferences when related
language/reciter/mushaf/resource records are deleted; change the on_delete
behavior to :nullify for these preference FKs so deletions null out the
preference instead of dropping the row, and if cascade was intentionally
required for any specific FK leave it as :cascade but add an inline comment
documenting the rationale for that choice.
| t.index ["mushaf_id", "verse_id", "position_in_verse"], name: "index_on_mushad_word_position" | ||
| t.index ["mushaf_id", "word_id"], name: "index_mushaf_words_on_mushaf_id_and_word_id" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Typos/table naming and FK delete strategy need confirmation.
- Possible typos:
- Index name “index_on_mushad_word_position” (Line 717) — “mushaf”?
- Table “mushas_pages” (Lines 736-745) — intended “mushaf_pages”?
- CountryLanguagePreferences FKs (Lines 1638-1643) use ON DELETE CASCADE though columns are optional; SET NULL is usually preferable to avoid accidental row loss.
Run to locate/confirm the migrations and adjust if unintended:
Also applies to: 736-745, 1638-1643
🏁 Script executed:
#!/bin/bash
# Find suspicious names
rg -nC2 -g 'db/migrate/*.rb' -e 'mushas_pages|mushad_word_position'
# Check FK strategies for CLP
rg -nC2 -g 'db/migrate/*.rb' -e 'add_foreign_key.*:country_language_preferences.*on_delete:\s*:cascade'Length of output: 1418
🏁 Script executed:
#!/bin/bash
# Inspect schema for index typo and table naming
echo "Index typo context:"
rg -n -C2 'index_on_mushad_word_position' -g 'db/schema.rb' || echo "No matches"
echo
echo "Schema around lines 700-760:"
sed -n '700,760p' db/schema.rb || echo "Unable to read schema.rb"
echo
echo "Check mushas_pages vs mushaf_pages:"
rg -n -C2 'mushas_pages' -g 'db/schema.rb' || echo "No 'mushas_pages' matches"
rg -n -C2 'mushaf_pages' -g 'db/schema.rb' || echo "No 'mushaf_pages' matches"
echo
echo "CountryLanguagePreferences create_table context:"
rg -n -C2 'create_table \"country_language_preferences\"' -g 'db/schema.rb' || echo "No CLP table definition found"
echo
echo "Schema around CLP lines 1600-1660:"
sed -n '1600,1660p' db/schema.rb || echo "Unable to read schema.rb"Length of output: 8608
🏁 Script executed:
#!/bin/bash
# Locate the create_table block for country_language_preferences
echo "CLP table definition:"
rg -n -C5 'create_table "country_language_preferences"' -g db/schema.rb || echo "Not found"
echo
# Dump the CLP schema block
start_line=$(rg -n 'create_table "country_language_preferences"' -g db/schema.rb | cut -d: -f1)
end_line=$((start_line + 30))
sed -n "${start_line},${end_line}p" db/schema.rb || echo "Unable to extract CLP block"Length of output: 1211
🏁 Script executed:
#!/bin/bash
# Locate migration file for country_language_preferences
file=$(ls db/migrate/*create_country_language_preferences*.rb)
echo "Migration file: $file"
# Dump the create_table block for country_language_preferences
sed -n -e '/create_table "country_language_preferences"/,/^ end/p' "$file"Length of output: 448
🏁 Script executed:
#!/bin/bash
cat db/migrate/20240918072240_create_country_language_preferences.rbLength of output: 1207
Fix index and table naming typos, and adjust FK delete strategies
- Rename the misspelled index in db/schema.rb (and its originating migration) from
index_on_mushad_word_position→index_on_mushaf_word_position. - Correct the unintended table name
mushas_pages→mushaf_pages(update the migration that creates “mushas_pages”). - In db/migrate/20240918072240_create_country_language_preferences.rb, change
on_delete: :cascade→on_delete: :nullifyfor optional FKs (default_mushaf_id, default_tafsir_id, default_wbw_language, default_reciter) to prevent unintended row removal.
| unless response.status == 200 | ||
| warn "DEBUG status=#{response.status} headers=#{response.headers.inspect} body=#{response.body}" | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove debug logging from specs
The warn() block adds noisy logs and can leak payloads in CI.
- unless response.status == 200
- warn "DEBUG status=#{response.status} headers=#{response.headers.inspect} body=#{response.body}"
- end📝 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.
| unless response.status == 200 | |
| warn "DEBUG status=#{response.status} headers=#{response.headers.inspect} body=#{response.body}" | |
| end |
🤖 Prompt for AI Agents
spec/requests/api/qdc/resources_country_language_preference_spec.rb lines 21-23:
remove the debug warn block that prints response status/headers/body to avoid
noisy and sensitive logs in CI; instead rely on the spec's assertion for the
expected status (or replace with an assertion that includes a safe, minimal
failure message) so no debug payload is emitted.
| it 'returns 404 when nothing matches' do | ||
| get '/api/qdc/resources/country_language_preference', params: { user_device_language: 'zz' } | ||
|
|
||
| expect(response).to have_http_status(:not_found) | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Cover invalid user_device_language
Assert 400 when the language doesn’t exist.
it 'returns 404 when nothing matches' do
get '/api/qdc/resources/country_language_preference', params: { user_device_language: 'zz' }
expect(response).to have_http_status(:not_found)
end
+
+ it 'returns 400 for unknown user_device_language' do
+ get '/api/qdc/resources/country_language_preference', params: { user_device_language: 'unknown' }
+ expect(response).to have_http_status(:bad_request)
+ end📝 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.
| it 'returns 404 when nothing matches' do | |
| get '/api/qdc/resources/country_language_preference', params: { user_device_language: 'zz' } | |
| expect(response).to have_http_status(:not_found) | |
| end | |
| it 'returns 404 when nothing matches' do | |
| get '/api/qdc/resources/country_language_preference', params: { user_device_language: 'zz' } | |
| expect(response).to have_http_status(:not_found) | |
| end | |
| it 'returns 400 for unknown user_device_language' do | |
| get '/api/qdc/resources/country_language_preference', params: { user_device_language: 'unknown' } | |
| expect(response).to have_http_status(:bad_request) | |
| end |
🤖 Prompt for AI Agents
In spec/requests/api/qdc/resources_country_language_preference_spec.rb around
lines 45-49, the test currently expects a 404 for an invalid
user_device_language but should assert a 400; update the example to
expect_http_status(:bad_request) (or 400) and, if the controller currently
returns 404, modify the controller/action to validate the user_device_language
and return a 400 bad request when the language is not found, including a clear
error message in the response.
| it 'validates country code when provided' do | ||
| get '/api/qdc/resources/country_language_preference', params: { user_device_language: 'xx', country: 'ZZ' } | ||
|
|
||
| expect(response).to have_http_status(:bad_request) | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add a test for lowercase country codes
Controller uppercases the input; ensure it’s case-insensitive.
it 'validates country code when provided' do
get '/api/qdc/resources/country_language_preference', params: { user_device_language: 'xx', country: 'ZZ' }
expect(response).to have_http_status(:bad_request)
end
+
+ it 'accepts lowercase country codes' do
+ get '/api/qdc/resources/country_language_preference', params: { user_device_language: 'xx', country: 'us' }
+ expect(response).to have_http_status(:ok)
+ json = JSON.parse(response.body)
+ expect(json['id']).to eq(us_test_pref.id)
+ end📝 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.
| it 'validates country code when provided' do | |
| get '/api/qdc/resources/country_language_preference', params: { user_device_language: 'xx', country: 'ZZ' } | |
| expect(response).to have_http_status(:bad_request) | |
| end | |
| it 'validates country code when provided' do | |
| get '/api/qdc/resources/country_language_preference', params: { user_device_language: 'xx', country: 'ZZ' } | |
| expect(response).to have_http_status(:bad_request) | |
| end | |
| it 'accepts lowercase country codes' do | |
| get '/api/qdc/resources/country_language_preference', params: { user_device_language: 'xx', country: 'us' } | |
| expect(response).to have_http_status(:ok) | |
| json = JSON.parse(response.body) | |
| expect(json['id']).to eq(us_test_pref.id) | |
| end |
🤖 Prompt for AI Agents
In spec/requests/api/qdc/resources_country_language_preference_spec.rb around
lines 57 to 61, add a test that submits a lowercase country code to ensure the
controller treats country codes case-insensitively; call GET
'/api/qdc/resources/country_language_preference' with params including
user_device_language: 'xx' and country: 'zz' and assert the response
has_http_status(:bad_request) (mirroring the existing uppercase test).
add QR country configs fields
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (9)
app/models/country_language_preference.rb (1)
31-33: Make language association required (DB already enforces NOT NULL).Mirror the NOT NULL + FK on
user_device_language; fail fast at the model layer.- belongs_to :language, foreign_key: :user_device_language, primary_key: :iso_code, optional: true + belongs_to :language, foreign_key: :user_device_language, primary_key: :iso_code, optional: falsedb/schema.rb (3)
718-720: Fix index name typo (“mushad” → “mushaf”).Keeps schema greppable and avoids confusion.
Migration:
class RenameMushafWordIndex < ActiveRecord::Migration[7.0] def change remove_index :mushaf_words, name: "index_on_mushad_word_position" add_index :mushaf_words, [:mushaf_id, :verse_id, :position_in_verse], name: "index_on_mushaf_word_position" end end
738-747: Rename table “mushas_pages” → “mushaf_pages”.Very likely a typo; keep naming consistent with
mushaf_pages.Migration:
class RenameMushasPages < ActiveRecord::Migration[7.0] def change rename_table :mushas_pages, :mushaf_pages end end
1640-1644: Use ON DELETE NULLIFY for optional CLP FKs (avoid unintended row deletes).Current
:cascadecan wipe CLP rows when a referenced record is removed.Migration:
class FixClpFkDeleteStrategies < ActiveRecord::Migration[7.0] def change remove_foreign_key :country_language_preferences, column: :default_wbw_language remove_foreign_key :country_language_preferences, column: :user_device_language remove_foreign_key :country_language_preferences, column: :default_mushaf_id remove_foreign_key :country_language_preferences, column: :default_reciter remove_foreign_key :country_language_preferences, column: :default_tafsir_id add_foreign_key :country_language_preferences, :languages, column: :default_wbw_language, primary_key: :iso_code, on_delete: :nullify add_foreign_key :country_language_preferences, :languages, column: :user_device_language, primary_key: :iso_code, on_delete: :restrict add_foreign_key :country_language_preferences, :mushafs, column: :default_mushaf_id, on_delete: :nullify add_foreign_key :country_language_preferences, :reciters, column: :default_reciter, on_delete: :nullify add_foreign_key :country_language_preferences, :resource_contents, column: :default_tafsir_id, on_delete: :nullify end endapp/controllers/api/qdc/resources_controller.rb (3)
120-121: Normalize params to avoid false negatives (strip/downcase language, strip/upcase country).Current checks can reject valid inputs like " En " or "AR". Normalize before validation.
- user_device_language = request.query_parameters[:user_device_language].presence - country = request.query_parameters[:country].presence&.upcase + user_device_language = request.query_parameters[:user_device_language].to_s.strip.downcase.presence + country = request.query_parameters[:country].to_s.strip.upcase.presence
133-137: Avoid allocating all countries; use direct ISO3166 lookup.Prevents per-request allocation and speeds validation.
- valid_countries = ISO3166::Country.all.map(&:alpha2) - unless valid_countries.include?(country) + unless ISO3166::Country.find_country_by_alpha2(country) return render_bad_request('Invalid country code') end
175-212: Gate all resources by allowed_to_share; sanitize CSV lists; preserve client-specified order.Without this, disallowed resources may leak; language CSV parsing is brittle; order is lost.
def build_preference_data(preference) # Sanitize CSV IDs for default translations ids = if preference.default_translation_ids.present? preference.default_translation_ids .split(',') .map(&:strip) .reject(&:blank?) .map(&:to_i) else [] end # QR specific default translations ids qr_ids = if preference.qr_default_translations_ids.present? preference.qr_default_translations_ids .split(',') .map(&:strip) .reject(&:blank?) .map(&:to_i) else [] end { preference: preference, default_mushaf: preference.mushaf&.enabled ? preference.mushaf : nil, - default_translations: ids.any? ? - ResourceContent.where(id: ids).approved.includes(:translated_name) : [], + default_translations: ids.any? ? + ResourceContent.approved.allowed_to_share.where(id: ids) + .includes(:translated_name) + .order(Arel.sql("array_position(ARRAY[#{ids.join(',')}], resource_contents.id)")) + : [], - qr_default_translations: qr_ids.any? ? - ResourceContent.where(id: qr_ids).approved.includes(:translated_name) : [], + qr_default_translations: qr_ids.any? ? + ResourceContent.approved.allowed_to_share.where(id: qr_ids) + .includes(:translated_name) + .order(Arel.sql("array_position(ARRAY[#{qr_ids.join(',')}], resource_contents.id)")) + : [], - default_tafsir: preference.tafsir&.approved? ? preference.tafsir : nil, + default_tafsir: preference.tafsir_id ? + ResourceContent.approved.allowed_to_share.find_by(id: preference.tafsir_id) : nil, default_wbw_language: preference.wbw_language, - default_reciter: preference.reciter, - ayah_reflections_languages: Language.where(iso_code: preference.ayah_reflections_languages&.split(',') || []), - qr_reflection_languages: Language.where(iso_code: preference.qr_reflection_languages&.split(',') || []), - learning_plan_languages: Language.where(iso_code: preference.learning_plan_languages&.split(',') || []) + default_reciter: preference.reciter_id ? + Recitation.joins(:resource_content).merge(ResourceContent.allowed_to_share) + .approved.find_by(id: preference.reciter_id) : nil, + ayah_reflections_languages: Language.where( + iso_code: (preference.ayah_reflections_languages || '') + .split(',').map { |c| c.strip.downcase }.reject(&:blank?).uniq + ), + qr_reflection_languages: Language.where( + iso_code: (preference.qr_reflection_languages || '') + .split(',').map { |c| c.strip.downcase }.reject(&:blank?).uniq + ), + learning_plan_languages: Language.where( + iso_code: (preference.learning_plan_languages || '') + .split(',').map { |c| c.strip.downcase }.reject(&:blank?).uniq + ) } endRe-run the end-to-end check to ensure no QDC endpoints surface disallowed resources:
#!/bin/bash # Controllers/Presenters queries missing allowed_to_share rg -nP --type=ruby -C2 '(ResourceContent|Recitation)\b(?!.*allowed_to_share).*approved' app/controllers app/presentersapp/views/api/qdc/resources/country_language_preference.json.streamer (2)
18-23: Guard against nil translated_name in default_translations.json.extract! on nil will raise.
- json.translated_name do - translated_name = translation.translated_name - json.object! do - json.extract! translated_name, :name, :language_name - end - end + translated_name = translation.translated_name + if translated_name + json.translated_name do + json.object! do + json.extract! translated_name, :name, :language_name + end + end + else + json.translated_name nil + end
33-37: Guard against nil translated_name in qr_default_translations.Same issue as above.
- json.translated_name do - translated_name = translation.translated_name - json.object! do - json.extract! translated_name, :name, :language_name - end - end + translated_name = translation.translated_name + if translated_name + json.translated_name do + json.object! do + json.extract! translated_name, :name, :language_name + end + end + else + json.translated_name nil + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
app/controllers/api/qdc/resources_controller.rb(1 hunks)app/models/country_language_preference.rb(1 hunks)app/views/api/qdc/resources/country_language_preference.json.streamer(1 hunks)db/migrate/20250909000000_add_qr_fields_to_country_language_preferences.rb(1 hunks)db/schema.rb(47 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
app/models/country_language_preference.rb (1)
app/models/mushaf.rb (1)
scope(28-59)
app/controllers/api/qdc/resources_controller.rb (1)
app/controllers/application_controller.rb (2)
render_bad_request(25-27)render_404(21-23)
db/schema.rb (2)
app/controllers/mobile/translations_controller.rb (2)
index(4-17)index(5-9)app/controllers/api/v3/audio_files_controller.rb (2)
index(4-23)index(5-12)
| class CountryLanguagePreference < ApplicationRecord | ||
| belongs_to :reciter, foreign_key: :default_reciter, optional: true | ||
| belongs_to :language, foreign_key: :user_device_language, primary_key: :iso_code, optional: true | ||
| belongs_to :wbw_language, class_name: 'Language', foreign_key: :default_wbw_language, primary_key: :iso_code, optional: true | ||
| belongs_to :mushaf, foreign_key: :default_mushaf_id, optional: true | ||
| belongs_to :tafsir, class_name: 'ResourceContent', foreign_key: :default_tafsir_id, optional: true | ||
|
|
||
| validates :user_device_language, presence: true | ||
|
|
||
| scope :with_includes, -> { | ||
| includes(:reciter, :language, :wbw_language, :mushaf, :tafsir) | ||
| } | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add normalized accessors/parsers for QR list fields (if you keep them as strings).
Prevents scattered split(',') across controllers/serializers and centralizes parsing.
Example (place inside the model):
def qr_default_translation_ids_list
(qr_default_translation_ids || '').split(',').map!(&:strip).reject!(&:blank?) || []
end
def qr_reflections_languages_list
(qr_reflections_languages || '').split(',').map!(&:strip).reject!(&:blank?) || []
end🤖 Prompt for AI Agents
In app/models/country_language_preference.rb around lines 30 to 42, add
normalized accessor methods for the QR list string fields (e.g.,
qr_default_translation_ids and qr_reflections_languages) to centralize parsing;
each method should live in this model, return an array, safely handle nil by
treating it as an empty string, split on commas, strip whitespace, and reject
blank entries so callers get a clean array (e.g.,
qr_default_translation_ids_list and qr_reflections_languages_list).
| add_column :country_language_preferences, :qr_default_translations_ids, :string | ||
| add_column :country_language_preferences, :qr_reflection_languages, :string |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer structured types (array/JSONB) over comma-separated strings.
These fields hold lists; model/query code will be simpler and faster with arrays/JSON.
Option A (string[]):
- add_column :country_language_preferences, :qr_default_translation_ids, :string
- add_column :country_language_preferences, :qr_reflections_languages, :string
+ add_column :country_language_preferences, :qr_default_translation_ids, :string, array: true, default: [], null: false
+ add_column :country_language_preferences, :qr_reflections_languages, :string, array: true, default: [], null: falseOption B (jsonb):
- add_column :country_language_preferences, :qr_default_translation_ids, :string
- add_column :country_language_preferences, :qr_reflections_languages, :string
+ add_column :country_language_preferences, :qr_default_translation_ids, :jsonb, default: [], null: false
+ add_column :country_language_preferences, :qr_reflections_languages, :jsonb, default: [], null: falseCommittable suggestion skipped: line range outside the PR's diff.
🛠️ Refactor suggestion
Align column names with existing conventions (avoid “translations_ids”).
Use singular “translation” before “_ids” and keep “reflections” plural to match default_translation_ids and ayah_reflections_languages.
- add_column :country_language_preferences, :qr_default_translations_ids, :string
- add_column :country_language_preferences, :qr_reflection_languages, :string
+ add_column :country_language_preferences, :qr_default_translation_ids, :string
+ add_column :country_language_preferences, :qr_reflections_languages, :string📝 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.
| add_column :country_language_preferences, :qr_default_translations_ids, :string | |
| add_column :country_language_preferences, :qr_reflection_languages, :string | |
| add_column :country_language_preferences, :qr_default_translation_ids, :string | |
| add_column :country_language_preferences, :qr_reflections_languages, :string |
🤖 Prompt for AI Agents
In db/migrate/20250909000000_add_qr_fields_to_country_language_preferences.rb
around lines 3 to 4, the new column names use incorrect pluralization and should
align with existing conventions; change add_column
:country_language_preferences, :qr_default_translations_ids, :string to use
singular "translation" before "_ids" (qr_default_translation_ids) and change
:qr_reflection_languages to use plural "reflections" (qr_reflections_languages);
update the migration to add the two columns with those corrected names so they
match default_translation_ids and ayah_reflections_languages.
| create_table "country_language_preferences", force: :cascade do |t| | ||
| t.string "country" | ||
| t.string "user_device_language", null: false | ||
| t.integer "default_mushaf_id" | ||
| t.string "default_translation_ids" | ||
| t.integer "default_tafsir_id" | ||
| t.string "default_wbw_language" | ||
| t.integer "default_reciter" | ||
| t.string "ayah_reflections_languages" | ||
| t.string "learning_plan_languages" | ||
| t.datetime "created_at", null: false | ||
| t.datetime "updated_at", null: false | ||
| t.string "qr_default_translations_ids" | ||
| t.string "qr_reflection_languages" | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Index CLP foreign-key columns for joins.
Joins via with_includes will benefit from indexes; FKs don’t auto-create them.
Add migration:
class AddIndexesToClp < ActiveRecord::Migration[7.0]
def change
add_index :country_language_preferences, :user_device_language
add_index :country_language_preferences, :default_wbw_language
add_index :country_language_preferences, :default_mushaf_id
add_index :country_language_preferences, :default_tafsir_id
add_index :country_language_preferences, :default_reciter
end
end🤖 Prompt for AI Agents
In db/schema.rb around lines 328 to 342, the country_language_preferences table
defines several foreign-key/lookup columns used in joins (user_device_language,
default_wbw_language, default_mushaf_id, default_tafsir_id, default_reciter) but
lacks indexes; add a new Rails migration (e.g., AddIndexesToClp) that calls
add_index for each of those columns to improve join performance and ensure
queries using with_includes benefit from indexed FK/lookup columns.
| t.string "qr_default_translations_ids" | ||
| t.string "qr_reflection_languages" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
If you rename the QR columns, ensure schema regeneration reflects it.
After updating the migration, re-run db:migrate so schema.rb shows qr_default_translation_ids and qr_reflections_languages.
🏁 Script executed:
#!/bin/bash
# Verify all code references match the final column names
rg -n "qr_default_translation[s]?_ids|qr_default_translations_ids|qr_reflection[s]?_languages" -C2
# Check serializers/views for these fields
rg -n --type=ruby "country_language_preference" app/views app/presenters app/controllers -C3Length of output: 3950
Update migration and regenerate schema for QR column renames
- Rename the columns in
db/migrate/20250909000000_add_qr_fields_to_country_language_preferences.rbtoqr_default_translation_idsandqr_reflections_languages - Rerun
rails db:migratesodb/schema.rbreflects those exact names - Verify and update all code references (controllers, views, serializers) to match the new column names
🤖 Prompt for AI Agents
In db/schema.rb around lines 340-341 the column names are incorrect (t.string
"qr_default_translations_ids" and t.string "qr_reflection_languages"); update
the migration
db/migrate/20250909000000_add_qr_fields_to_country_language_preferences.rb to
rename the columns to qr_default_translation_ids and qr_reflections_languages,
run rails db:migrate (or create a new migration that renames the columns if
already run), regenerate db/schema.rb, and then search & update all code
references (controllers, models, views, serializers, tests) to use the new exact
column names.
Support transliterated_name of chapters
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/finders/chapter_finder.rb (2)
66-76: Include plural associations to back your WHEREs and prevent N+1.You reference
translated_namesandtransliterationselsewhere; include them here to force a single SQL with proper JOINs.- eager_load = [:translated_name, :transliteration] + eager_load = [:translated_name, :translated_names, :transliteration, :transliterations]
48-49: Avoid duplicate rows after ORs/LEFT JOINs.The combined ORs on translated/transliteration joins can duplicate chapters. Add DISTINCT before ordering.
- @chapters = chapters.order('translated_names.language_priority DESC') + @chapters = chapters.distinct.order('translated_names.language_priority DESC')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
app/finders/chapter_finder.rb(3 hunks)app/models/chapter.rb(1 hunks)app/models/concerns/name_transliterateable.rb(1 hunks)app/presenters/chapter_presenter.rb(1 hunks)app/views/api/qdc/chapters/_chapter.json.streamer(1 hunks)app/views/api/qdc/chapters/index.json.streamer(1 hunks)app/views/api/qdc/chapters/show.json.streamer(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
app/presenters/chapter_presenter.rb (1)
app/presenters/base_presenter.rb (1)
include_in_response?(58-62)
app/finders/chapter_finder.rb (8)
app/presenters/chapter_presenter.rb (1)
chapters(17-19)app/graphql/types/query_type.rb (1)
chapters(11-13)app/models/language.rb (2)
default(42-44)default?(37-39)app/controllers/api/v3/search_controller.rb (1)
language(17-19)app/controllers/api/v4/search_controller.rb (1)
language(17-19)app/controllers/api/qdc/search_controller.rb (1)
language(26-28)app/presenters/base_presenter.rb (1)
language(79-83)app/controllers/api/v3/suggest_controller.rb (1)
language(18-20)
app/models/chapter.rb (6)
app/models/resource_content.rb (1)
include(50-150)app/models/language.rb (1)
include(22-55)app/models/author.rb (1)
include(14-17)app/models/reciter.rb (1)
include(18-20)app/models/translated_name.rb (1)
include(25-29)app/models/recitation_style.rb (1)
include(21-23)
🔇 Additional comments (5)
app/presenters/chapter_presenter.rb (1)
29-31: LGTM on the new flag.Matches existing include_*? pattern and BasePresenter semantics.
app/views/api/qdc/chapters/show.json.streamer (1)
6-6: LGTM: flag plumbed through show endpoint.Ensure public docs mention the new query param
transliterated_name=true.app/views/api/qdc/chapters/index.json.streamer (1)
7-7: LGTM: flag plumbed through index endpoint.Keeps index/show consistent.
app/models/chapter.rb (1)
32-32: LGTM: concern inclusion is appropriate.No conflicts with existing NameTranslateable usage observed.
app/finders/chapter_finder.rb (1)
43-46: Double-check slug JOIN used by load_language_slug. load_language_slug uses a where(slugs: …) but this path only includes :default_slug (not :slugs); if load_language_slug doesn't add a join this will fail. Either add left_joins(:slugs) inside load_language_slug or include :slugs here instead of :default_slug. Location: app/finders/chapter_finder.rb:43-46
| # Eager load transliteration for chapter names with fallback to English (mirror translated_names pattern) | ||
| with_default_trans = chapters.where(transliterations: { language_id: Language.default.id }) | ||
|
|
||
| chapters = if language.nil? || language.default? | ||
| with_default_trans | ||
| else | ||
| chapters | ||
| .where(transliterations: { language_id: language.id }) | ||
| .or(with_default_trans) | ||
| .order(Arel.sql("CASE WHEN transliterations.language_id = #{language.id} THEN 0 ELSE 1 END")) | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Offer: I can push a cohesive fix PR to stabilize the query plan.
I can refactor to use left_joins for both plural associations, add distinct, and keep includes for singular presenter needs.
Also applies to: 66-76
🤖 Prompt for AI Agents
In app/finders/chapter_finder.rb around lines 31-41 (and similarly 66-76), the
query should be refactored to use left_joins for plural associations (e.g.
transliterations/translated_names) instead of where-based joins so fallback rows
are preserved; add distinct to the final relation to remove duplicate rows
introduced by the joins; keep using includes for any singular
presenter/association that needs eager loading; and preserve the CASE order
logic for preferring the requested language by moving that ORDER clause onto the
distinct relation. Implement these changes in both code blocks mentioned.
Broken JOINs and potential SQL error when filtering by transliterations.
You where(transliterations: ...) but don’t eager-load or join the plural association, risking “missing FROM-clause entry” and N+1s. Also, the raw interpolated CASE is brittle.
Minimal fix: ensure the association is included (or left-joined) and make the CASE numeric-safe.
- with_default_trans = chapters.where(transliterations: { language_id: Language.default.id })
+ with_default_trans = chapters.where(transliterations: { language_id: Language.default.id })
- .order(Arel.sql("CASE WHEN transliterations.language_id = #{language.id} THEN 0 ELSE 1 END"))
+ .order(
+ Arel.sql(
+ "CASE WHEN transliterations.language_id = #{language.id.to_i} THEN 0 ELSE 1 END"
+ )
+ )Follow-up: see change below to chapter_eager_loads to actually include the plural associations so the where works reliably.
📝 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.
| # Eager load transliteration for chapter names with fallback to English (mirror translated_names pattern) | |
| with_default_trans = chapters.where(transliterations: { language_id: Language.default.id }) | |
| chapters = if language.nil? || language.default? | |
| with_default_trans | |
| else | |
| chapters | |
| .where(transliterations: { language_id: language.id }) | |
| .or(with_default_trans) | |
| .order(Arel.sql("CASE WHEN transliterations.language_id = #{language.id} THEN 0 ELSE 1 END")) | |
| end | |
| # Eager load transliteration for chapter names with fallback to English (mirror translated_names pattern) | |
| with_default_trans = chapters.where(transliterations: { language_id: Language.default.id }) | |
| chapters = if language.nil? || language.default? | |
| with_default_trans | |
| else | |
| chapters | |
| .where(transliterations: { language_id: language.id }) | |
| .or(with_default_trans) | |
| .order( | |
| Arel.sql( | |
| "CASE WHEN transliterations.language_id = #{language.id.to_i} THEN 0 ELSE 1 END" | |
| ) | |
| ) | |
| end |
🤖 Prompt for AI Agents
In app/finders/chapter_finder.rb around lines 31 to 41, the code filters on
transliterations using where(transliterations: ...) without guaranteeing a
join/include on the plural association and interpolates a raw CASE expression,
which can produce missing FROM-clause SQL errors and is brittle; fix it by
ensuring you either left_joins(:transliterations) or includes(:transliterations)
before applying the where/or so the transliterations table is present, and
replace the raw interpolated CASE with a sanitized numeric-safe expression
(build the CASE with Arel or use ActiveRecord::Base.sanitize_sql_array to inject
the language id safely) so the ordering clause cannot be manipulated or break
when language is nil.
| has_many :transliterations, as: :resource | ||
| has_one :transliteration, as: :resource | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add dependent cleanup and inverse associations to avoid orphans and improve caching.
Polymorphic associations should specify dependent and inverse_of to prevent orphaned rows and reduce object churn.
- has_many :transliterations, as: :resource
- has_one :transliteration, as: :resource
+ has_many :transliterations, as: :resource, dependent: :destroy, inverse_of: :resource
+ has_one :transliteration, as: :resource, inverse_of: :resource📝 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.
| has_many :transliterations, as: :resource | |
| has_one :transliteration, as: :resource | |
| end | |
| has_many :transliterations, as: :resource, dependent: :destroy, inverse_of: :resource | |
| has_one :transliteration, as: :resource, inverse_of: :resource | |
| end |
🤖 Prompt for AI Agents
In app/models/concerns/name_transliterateable.rb around lines 7–9, the
polymorphic associations lack dependent cleanup and inverse definitions; update
the has_many and has_one to include dependent: :destroy (or :delete_all if you
prefer SQL-level removal) and add inverse_of to enable proper bi-directional
caching (e.g. has_many :transliterations, as: :resource, dependent: :destroy,
inverse_of: :resource and has_one :transliteration, as: :resource, dependent:
:destroy, inverse_of: :resource), and also update the Transliteration model to
declare belongs_to :resource, polymorphic: true with matching inverse_of entries
(e.g. inverse_of: :transliterations / :transliteration) so orphans are removed
and ActiveRecord can reuse in-memory associations.
| def localised_transliteration | ||
| transliteration&.text | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: Provide a US-spelling alias for ergonomics.
Small DX polish; keep existing name for backward-compat and add an alias.
def localised_transliteration
transliteration&.text
end
+ alias_method :localized_transliteration, :localised_transliteration📝 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.
| def localised_transliteration | |
| transliteration&.text | |
| end | |
| def localised_transliteration | |
| transliteration&.text | |
| end | |
| alias_method :localized_transliteration, :localised_transliteration |
🤖 Prompt for AI Agents
In app/models/concerns/name_transliterateable.rb around lines 11 to 13, add a
US-spelling alias method named localized_transliteration that simply delegates
to the existing localised_transliteration to preserve backward compatibility and
improve ergonomics; implement it as a one-line method that returns
localised_transliteration (or uses alias_method if preferred) so callers using
US spelling get the same behavior without changing existing code.
| if render_transliterated_name && chapter.transliteration | ||
| json.transliterated_name do | ||
| json.object! do | ||
| json.extract! chapter.transliteration, :language_name, :text | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Confirm API contract re: omission vs null.
You conditionally render transliterated_name. If clients expect a stable shape, consider emitting null when render_transliterated_name is true but data is absent; otherwise omission is fine.
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)app/controllers/health_controller.rb(1 hunks)app/controllers/readiness_controller.rb(1 hunks)config/routes.rb(1 hunks)spec/requests/health_spec.rb(1 hunks)spec/requests/readiness_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/controllers/health_controller.rb (1)
app/controllers/readiness_controller.rb (2)
show(3-37)show(5-28)
app/controllers/readiness_controller.rb (1)
app/controllers/health_controller.rb (2)
show(3-8)show(5-7)
🔇 Additional comments (3)
.gitignore (1)
40-40: Good call ignoring .env.Prevents accidental secrets commits.
app/controllers/health_controller.rb (1)
5-7: LGTM: minimal, correct liveness endpoint.spec/requests/health_spec.rb (1)
5-15: LGTM: verifies status and no-store headers.
| /config/credentials/production.key | ||
|
|
||
| /config/credentials/development.key | ||
|
|
||
| .env |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Broaden secrets ignore patterns and keep a checked-in example.
Avoid leaking future credentials keys and env variants.
Apply:
-/config/credentials/production.key
-
-/config/credentials/development.key
+/# ignore all Rails credentials keys
+/config/credentials/*.key
+
+# allow a sample env file but ignore real envs
+.env
+.env.*
+!.env.example📝 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.
| /config/credentials/production.key | |
| /config/credentials/development.key | |
| .env | |
| # ignore all Rails credentials keys | |
| /config/credentials/*.key | |
| # allow a sample env file but ignore real envs | |
| .env | |
| .env.* | |
| !.env.example |
🤖 Prompt for AI Agents
In .gitignore around lines 36-40 replace the specific secret file entries with
broader ignore patterns to avoid leaking future keys and variants: add
/config/credentials/*.key to cover all credential keys, ignore all env variants
with .env* but explicitly allow a checked-in example by adding !.env.example (or
other example filename you use), and remove the hard-coded individual filenames;
ensure the example file is committed so consumers have a template.
| class HealthController < ApplicationController | ||
| # Lightweight liveness endpoint: confirms app process & middleware stack respond. | ||
| def show | ||
| render json: { status: 'ok' } | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Skip heavy filters (auth, locale, rate limiting) if any.
Health should bypass non-essential before_actions to stay fast/reliable.
Run to check for inherited filters and add skips if present:
If needed:
class HealthController < ApplicationController
+ skip_before_action :authenticate_user!, :set_locale, :verify_authenticity_token, raise: false
def show
render json: { status: 'ok' }
end
end🏁 Script executed:
#!/bin/bash
rg -nC2 -g 'app/controllers/**/*.rb' -P 'class\s+ApplicationController\b|before_action|around_action'Length of output: 10266
Skip non-essential ApplicationController before_actions for the /health endpoint
ApplicationController registers before_action :set_cache_headers and :set_default_response_format — add skip_before_action :set_cache_headers, :set_default_response_format, raise: false in app/controllers/health_controller.rb so /health remains minimal (see app/controllers/application_controller.rb).
🤖 Prompt for AI Agents
In app/controllers/health_controller.rb around lines 3 to 7, the health endpoint
currently inherits ApplicationController before_actions and should skip
non-essential ones; add skip_before_action :set_cache_headers,
:set_default_response_format, raise: false at the top of this controller (above
the show action) so the /health endpoint remains minimal and unaffected by
ApplicationController middleware.
| checks[:redis] = dep_check('redis') do | ||
| k = Kredis.string("readiness:#{Process.pid}") | ||
| k.value = '1' | ||
| k.value == '1' ? 'up' : 'down' | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid unbounded Redis key cardinality; add TTL.
Using PID in the key can create lots of persistent keys under load.
Apply:
- k = Kredis.string("readiness:#{Process.pid}")
+ k = Kredis.string('readiness', expires_in: 5)
k.value = '1'
k.value == '1' ? 'up' : 'down'📝 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.
| checks[:redis] = dep_check('redis') do | |
| k = Kredis.string("readiness:#{Process.pid}") | |
| k.value = '1' | |
| k.value == '1' ? 'up' : 'down' | |
| end | |
| checks[:redis] = dep_check('redis') do | |
| k = Kredis.string('readiness', expires_in: 5) | |
| k.value = '1' | |
| k.value == '1' ? 'up' : 'down' | |
| end |
🤖 Prompt for AI Agents
In app/controllers/readiness_controller.rb around lines 13–17, the readiness
check writes a Redis key that includes Process.pid which can create unbounded
persistent keys; modify the code so the key is short-lived by setting an
expiration when creating the key (e.g., use Kredis.string with an expires_in
option if available, or call Redis EXPIRE / k.expire after setting the value) or
avoid per-PID keys entirely; choose a short TTL (eg. 60s) so keys automatically
expire and do not accumulate.
| checks[:elasticsearch] = dep_check('elasticsearch') do | ||
| client = Elasticsearch::Model.client | ||
| resp = client.cluster.health(timeout: '1s') | ||
| resp && resp['status'].present? ? 'up' : 'down' | ||
| end |
There was a problem hiding this comment.
Treat Elasticsearch cluster “red” as down; wait for at least yellow.
Current check marks any non-blank status as up, including red. Also request a yellow threshold.
Apply:
- client = Elasticsearch::Model.client
- resp = client.cluster.health(timeout: '1s')
- resp && resp['status'].present? ? 'up' : 'down'
+ client = Elasticsearch::Model.client
+ resp = client.cluster.health(wait_for_status: 'yellow', timeout: '1s')
+ %w[green yellow].include?(resp['status']) ? 'up' : 'down'📝 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.
| checks[:elasticsearch] = dep_check('elasticsearch') do | |
| client = Elasticsearch::Model.client | |
| resp = client.cluster.health(timeout: '1s') | |
| resp && resp['status'].present? ? 'up' : 'down' | |
| end | |
| checks[:elasticsearch] = dep_check('elasticsearch') do | |
| client = Elasticsearch::Model.client | |
| resp = client.cluster.health(wait_for_status: 'yellow', timeout: '1s') | |
| %w[green yellow].include?(resp['status']) ? 'up' : 'down' | |
| end |
🤖 Prompt for AI Agents
In app/controllers/readiness_controller.rb around lines 19 to 23, the health
check currently treats any non-blank cluster.status as up (including "red") and
does not wait for a yellow threshold; change the cluster.health call to request
at least yellow (e.g. pass wait_for_status: 'yellow', timeout: '1s') and update
the up/down logic to return 'up' only when resp is present and resp['status'] is
not 'red' (or explicitly is 'green' or 'yellow'), otherwise return 'down'.
| overall_ok = checks.values.all? { |v| v == 'up' } | ||
| status_code = overall_ok ? :ok : :service_unavailable | ||
| render json: { status: overall_ok ? 'ok' : 'degraded', checks: checks }, status: status_code | ||
| end |
There was a problem hiding this comment.
Don’t leak per-dependency status publicly in production.
Expose only overall status in prod unless explicitly enabled.
Apply:
- render json: { status: overall_ok ? 'ok' : 'degraded', checks: checks }, status: status_code
+ payload = { status: overall_ok ? 'ok' : 'degraded' }
+ if !Rails.env.production? || ENV['READINESS_DETAILS'] == 'true'
+ payload[:checks] = checks
+ end
+ render json: payload, status: status_code📝 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.
| overall_ok = checks.values.all? { |v| v == 'up' } | |
| status_code = overall_ok ? :ok : :service_unavailable | |
| render json: { status: overall_ok ? 'ok' : 'degraded', checks: checks }, status: status_code | |
| end | |
| overall_ok = checks.values.all? { |v| v == 'up' } | |
| status_code = overall_ok ? :ok : :service_unavailable | |
| payload = { status: overall_ok ? 'ok' : 'degraded' } | |
| if !Rails.env.production? || ENV['READINESS_DETAILS'] == 'true' | |
| payload[:checks] = checks | |
| end | |
| render json: payload, status: status_code | |
| end |
🤖 Prompt for AI Agents
In app/controllers/readiness_controller.rb around lines 25 to 28, the current
code renders per-dependency checks publicly; change it to only expose the
overall status in production by default: compute overall_ok as now, set
status_code accordingly, and when Rails.env.production? (or a dedicated feature
flag like ENV['EXPOSE_READINESS_CHECKS']=='true') omit the checks key and render
only { status: overall_ok ? 'ok' : 'degraded' } with the appropriate
status_code; when the flag is enabled (or non-production), include the checks
payload as currently implemented. Ensure the default behavior in production does
not include any per-dependency data.
| def dep_check(label) | ||
| yield | ||
| rescue => e | ||
| Rails.logger.warn("readiness #{label} error: #{e.class}: #{e.message}") | ||
| 'down' | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Bound check latency; readiness should fail fast.
Consider short per-check time budgets to avoid hanging on network issues.
If you can’t tune client timeouts globally, wrap individual checks with a safe timeout helper (avoid Ruby Timeout for DB if you can). Example:
def with_deadline(seconds)
start = Process.clock_gettime(Process::CLOCK_MONOTONIC)
yield
ensure
# optionally log elapsed and enforce budget across checks if needed
endOr configure:
- DB: low connect/statement timeout in the pool config for this endpoint.
- ES: use request_timeout at client or transport level.
- Redis: set low read_timeout on the client used by Kredis.
🤖 Prompt for AI Agents
In app/controllers/readiness_controller.rb around lines 31 to 36, the dep_check
helper yields without any per-check timeout so a slow external dependency can
hang the whole readiness probe; wrap each check in a short, safe deadline so
readiness fails fast. Implement a with_deadline wrapper that uses
Process.clock_gettime/monotonic to enforce a per-check budget (or configure
per-client timeouts for DB, ES and Redis) and ensure the wrapper returns 'down'
on timeout while logging elapsed time and the timeout event; avoid using Ruby's
Timeout for DB calls and prefer client-level connect/read/statement/request
timeouts where possible.
| # Simple health check endpoint (non-cached) | ||
| get '/health', to: 'health#show' | ||
| # Readiness endpoint (returns 503 if dependencies down) | ||
| get '/ready', to: 'readiness#show' |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Pin JSON format for health/readiness.
Avoid content negotiation surprises and keep payloads lightweight.
Apply:
- get '/health', to: 'health#show'
+ get '/health', to: 'health#show', defaults: { format: :json }
# Readiness endpoint (returns 503 if dependencies down)
- get '/ready', to: 'readiness#show'
+ get '/ready', to: 'readiness#show', defaults: { format: :json }📝 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.
| # Simple health check endpoint (non-cached) | |
| get '/health', to: 'health#show' | |
| # Readiness endpoint (returns 503 if dependencies down) | |
| get '/ready', to: 'readiness#show' | |
| # Simple health check endpoint (non-cached) | |
| get '/health', to: 'health#show', defaults: { format: :json } | |
| # Readiness endpoint (returns 503 if dependencies down) | |
| get '/ready', to: 'readiness#show', defaults: { format: :json } |
🤖 Prompt for AI Agents
In config/routes.rb around lines 15 to 18, the health and readiness endpoints
should be pinned to JSON to avoid content negotiation surprises; update the two
routes to force JSON responses by adding defaults: { format: :json } (e.g., get
'/health', to: 'health#show', defaults: { format: :json } and likewise for
'/ready').
| it 'returns checks and ok when all up (or degraded if something down)' do | ||
| get '/ready' | ||
| expect([200, 503]).to include(response.status) | ||
| json = JSON.parse(response.body) | ||
| expect(json).to include('status', 'checks') | ||
| expect(json['checks']).to be_a(Hash) | ||
| %w[database redis elasticsearch].each do |k| | ||
| expect(json['checks']).to have_key(k) | ||
| end | ||
| expect(%w[ok degraded]).to include(json['status']) | ||
| if response.status == 200 | ||
| expect(json['status']).to eq('ok') | ||
| else | ||
| expect(json['status']).to eq('degraded') | ||
| end | ||
| expect(response.headers['Cache-Control']).to match(/no-store/) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen assertions for headers and per-check outcomes.
Ensure all checks are up on 200; at least one down on 503, and align headers with health spec.
Apply:
expect(%w[ok degraded]).to include(json['status'])
if response.status == 200
- expect(json['status']).to eq('ok')
+ expect(json['status']).to eq('ok')
+ expect(json['checks'].values).to all(eq('up'))
else
- expect(json['status']).to eq('degraded')
+ expect(json['status']).to eq('degraded')
+ expect(json['checks'].values).to include('down')
end
- expect(response.headers['Cache-Control']).to match(/no-store/)
+ expect(response.headers['Cache-Control']).to match(/no-store/)
+ expect(response.headers['Pragma']).to eq('no-cache')
+ expect(response.headers['Expires']).to eq('0')📝 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.
| it 'returns checks and ok when all up (or degraded if something down)' do | |
| get '/ready' | |
| expect([200, 503]).to include(response.status) | |
| json = JSON.parse(response.body) | |
| expect(json).to include('status', 'checks') | |
| expect(json['checks']).to be_a(Hash) | |
| %w[database redis elasticsearch].each do |k| | |
| expect(json['checks']).to have_key(k) | |
| end | |
| expect(%w[ok degraded]).to include(json['status']) | |
| if response.status == 200 | |
| expect(json['status']).to eq('ok') | |
| else | |
| expect(json['status']).to eq('degraded') | |
| end | |
| expect(response.headers['Cache-Control']).to match(/no-store/) | |
| end | |
| it 'returns checks and ok when all up (or degraded if something down)' do | |
| get '/ready' | |
| expect([200, 503]).to include(response.status) | |
| json = JSON.parse(response.body) | |
| expect(json).to include('status', 'checks') | |
| expect(json['checks']).to be_a(Hash) | |
| %w[database redis elasticsearch].each do |k| | |
| expect(json['checks']).to have_key(k) | |
| end | |
| expect(%w[ok degraded]).to include(json['status']) | |
| if response.status == 200 | |
| expect(json['status']).to eq('ok') | |
| expect(json['checks'].values).to all(eq('up')) | |
| else | |
| expect(json['status']).to eq('degraded') | |
| expect(json['checks'].values).to include('down') | |
| end | |
| expect(response.headers['Cache-Control']).to match(/no-store/) | |
| expect(response.headers['Pragma']).to eq('no-cache') | |
| expect(response.headers['Expires']).to eq('0') | |
| end |
🤖 Prompt for AI Agents
In spec/requests/readiness_spec.rb around lines 7-23, strengthen the test so
headers and per-check results strictly follow the health spec: assert
exact/expected Cache-Control and Content-Type values (e.g., include no-store and
match the app's health spec) rather than a loose match; when response.status ==
200 assert json['status'] == 'ok' AND every entry in json['checks'] equals 'ok';
when response.status == 503 assert json['status'] == 'degraded' AND at least one
entry in json['checks'] is not 'ok' (use a predicate to ensure at-least-one
failing check); keep existing structure but replace the loose header and
per-check assertions with these stricter checks.
Summary by CodeRabbit