Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReportsController を Report スコープ中心のクエリへ切替し、Practice/Report に source_id を考慮するスコープ／メソッドを追加。ビューに給付金コース用タブを追加し、関連フィクスチャとテストを追加・更新。 Changes
Sequence Diagram(s)sequenceDiagram
participant User as 利用者 (ブラウザ)
participant Controller as Practices::ReportsController
participant Practice as Practice
participant Report as Report
User->>Controller: GET /practices/:id/reports?grant_only=...
Controller->>Practice: find(practice_id)
Controller->>Report: Report.list
alt practice_id present
Report->>Report: with_practice_and_source(practice_id)
alt grant_only == 'true'
Report->>Report: without_original_practice
end
end
Controller->>Report: paginate / fetch
Report-->>Controller: filtered reports
Controller-->>User: HTML (タブ, 一覧, ページネーション)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
karlley
force-pushed
the
feature/reports-grant-course-filter-v2
branch
from
4c9f6c9 to
f9558e8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
test/integration/api/reports/unchecked_test.rb (1)
16-16: 総件数の固定値が fixture 追加に引っ張られやすいです。今回も fixture 追加だけで
65件→71件の更新が必要になっていて、この assertion は別機能のデータ変更にかなり依存しています。ここで確認したい契約が API の文言やレスポンス形式なら、件数をこのテスト内で閉じる形に寄せたほうが保守しやすいです。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/api/reports/unchecked_test.rb` at line 16, The test currently asserts a hardcoded total string with assert_match '71件', response.body which is brittle; change it to assert the presence/format of a total (e.g. assert_match /\d+件/, response.body) or compute the expected count from the fixtures/models used in the test (e.g. derive expected = Report.unchecked.count or from the fixtures created in this test) and compare that parsed/derived value against response.body so the assertion no longer depends on an external fixed number; update the assertion that references assert_match and response.body accordingly.app/models/practice.rb (1)
214-217: 集計条件はReport.with_practice_and_sourceに寄せたいです。同じ practice/source の絞り込み条件が
app/models/report.rbにもあるので、ここだけ別実装にすると件数表示と一覧表示の条件がズレやすいです。既存スコープを使って 1 箇所に寄せておく方が安全です。As per coding guidelines, "複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。"♻️ まとめ先を 1 箇所にする例
def reports_count_with_source - ids = [id, source_id].compact - Report.joins(:practices).where(practices: { id: ids }).distinct.count + Report.with_practice_and_source(id).count end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/practice.rb` around lines 214 - 217, The reports_count_with_source method currently builds its own ids/join query; replace that with the existing Report.with_practice_and_source scope to keep filtering consistent: call Report.with_practice_and_source(id, source_id) (or pass the ids array as the scope expects) and then use .distinct.count to return the count. Update reports_count_with_source to delegate the filtering to Report.with_practice_and_source so listing and counting share the same query logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/report.rb`:
- Around line 70-72: The current without_original_practice scope excludes any
report that has any practice with source_id: nil, which wrongly removes reports
that are also linked to non-original practices; change the scope implementation
so it returns only reports that are associated with at least one practice where
source_id IS NOT NULL (e.g. replace the where.not(...) subquery with a positive
inclusion like where(id: Report.joins(:practices).where.not(practices: {
source_id: nil }).select(:id)) so reports with dual associations
(with_practice_and_source + original) are kept).
In `@app/views/practices/reports/index.html.slim`:
- Around line 18-22: The active-tab logic should compare params[:grant_only] to
the string 'true' to match controller behavior: update both occurrences so the
"all" tab uses ('is-active' unless params[:grant_only] == 'true') (or
('is-active' if params[:grant_only] != 'true')) and the "給付金コース" tab uses
('is-active' if params[:grant_only] == 'true'); locate and change the
conditional expressions around the link_to calls that build the pill-nav items
(refer to practice_reports_path and params[:grant_only]).
In `@db/fixtures/categories.yml`:
- Around line 116-119: The category24 fixture has mismatched name and slug
(name: "Mac OS X（Reスキル）" vs slug: "ready-for-learn-reskill"); update the
category24 entry so the slug consistently represents the same category as the
name (e.g., change slug to a normalized form of the name like
"mac-os-x-re-skill" or "mac-os-x-reskill"), and then search for and update any
references that still use "ready-for-learn-reskill" to the new slug to keep
fixture lookups consistent.
In `@db/fixtures/practices.yml`:
- Around line 773-776: The fixture has a missing closing ERB tag: in the
practice114 entry set the source_id expression using
ActiveRecord::FixtureSet.identify(:practice2) and add the missing "%>" to close
the ERB tag so the line becomes a valid ERB expression; update the source_id
line (referencing practice114, source_id and
ActiveRecord::FixtureSet.identify(:practice2)) to include the trailing ">".
In `@test/models/report_test.rb`:
- Around line 77-98: The test uses practices(:practice1) whose source_id is nil
so Report.with_practice_and_source isn’t exercising the "source" side; update
the test to use (or create) a practice fixture that has a non-nil source_id (or
add fixtures for a practice_with_source and its source) and add reports fixtures
covering: a report tied only to the practice, a report tied only to the source,
a report tied to both (copy/original), and an unrelated report; then call
Report.with_practice_and_source(practice.id) and assert it includes the
practice-only and source-only reports and also verifies the handling of the
report tied to both (included once or included as expected by your scope), and
keep the existing without_original_practice test unchanged except to ensure its
fixtures (reports(:report76)/(:report77)) still reflect original vs grant
semantics.
---
Nitpick comments:
In `@app/models/practice.rb`:
- Around line 214-217: The reports_count_with_source method currently builds its
own ids/join query; replace that with the existing
Report.with_practice_and_source scope to keep filtering consistent: call
Report.with_practice_and_source(id, source_id) (or pass the ids array as the
scope expects) and then use .distinct.count to return the count. Update
reports_count_with_source to delegate the filtering to
Report.with_practice_and_source so listing and counting share the same query
logic.
In `@test/integration/api/reports/unchecked_test.rb`:
- Line 16: The test currently asserts a hardcoded total string with assert_match
'71件', response.body which is brittle; change it to assert the presence/format
of a total (e.g. assert_match /\d+件/, response.body) or compute the expected
count from the fixtures/models used in the test (e.g. derive expected =
Report.unchecked.count or from the fixtures created in this test) and compare
that parsed/derived value against response.body so the assertion no longer
depends on an external fixed number; update the assertion that references
assert_match and response.body accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 475f4b0a-760d-4003-bba4-a40d86231b63
📒 Files selected for processing (19)
app/controllers/practices/reports_controller.rbapp/helpers/page_tabs/practices_helper.rbapp/models/practice.rbapp/models/report.rbapp/views/practices/reports/index.html.slimdb/fixtures/categories.ymldb/fixtures/categories_practices.ymldb/fixtures/courses_categories.ymldb/fixtures/discord_profiles.ymldb/fixtures/practices.ymldb/fixtures/reports.ymldb/fixtures/talks.ymldb/fixtures/users.ymltest/fixtures/learning_times.ymltest/fixtures/practices.ymltest/fixtures/reports.ymltest/integration/api/reports/unchecked_test.rbtest/models/practice_test.rbtest/models/report_test.rb
karlley
force-pushed
the
feature/reports-grant-course-filter-v2
branch
2 times, most recently
from
e3be37c to
914577e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/views/practices/reports/index.html.slim (1)
16-22:⚠️ Potential issue | 🟡 Minorタブの active 判定が絞り込み条件とずれています。
Line 18 と Line 22 は
params[:grant_only]の有無だけを見ているので、grant_only=falseのような値でも「給付金コース」タブが active になります。コントローラーはparams[:grant_only] == 'true'のときだけ絞り込むため、表示と実際の一覧がずれます。💡 修正案
li.pill-nav__item = link_to '全て', practice_reports_path(`@practice`), - class: ['pill-nav__item-link', ('is-active' unless params[:grant_only])] + class: ['pill-nav__item-link', ('is-active' unless params[:grant_only] == 'true')] li.pill-nav__item = link_to '給付金コース', practice_reports_path(`@practice`, params: { grant_only: true }), - class: ['pill-nav__item-link', ('is-active' if params[:grant_only])] + class: ['pill-nav__item-link', ('is-active' if params[:grant_only] == 'true')]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/practices/reports/index.html.slim` around lines 16 - 22, タブの active 判定が文字列比較になっていないため表示と実際の絞り込みがずれているので、params[:grant_only] を真偽ではなく文字列で比較するように修正してください：practice_reports_path を使っている2つの link_to の class 条件式で現状の params[:grant_only]／!params[:grant_only] チェックをそれぞれ params[:grant_only] == 'true' と params[:grant_only] != 'true'（または逆に if params[:grant_only] == 'true' / unless params[:grant_only] == 'true'）に置き換え、"給付金コース" タブは params[:grant_only] == 'true' のときのみ 'is-active' が付くようにしてください。app/models/report.rb (1)
70-72:⚠️ Potential issue | 🟠 Major元プラクティスにも紐づく日報まで除外してしまいます。
Line 71 の否定サブクエリは「元プラクティスを1つでも持つ report」を落とすので、給付金コースと元プラクティスの両方に紐づく日報も消えます。
app/controllers/practices/reports_controller.rbでwith_practice_and_sourceと組み合わせると、grant_only 一覧が欠けます。💡 修正案
scope :without_original_practice, lambda { - where.not(id: Report.joins(:practices).where(practices: { source_id: nil }).select(:id)) + where( + id: Report.joins(:practices) + .where.not(practices: { source_id: nil }) + .select(:id) + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/report.rb` around lines 70 - 72, scope :without_original_practice currently excludes any report that has at least one practice with source_id NULL, removing reports that also have grant practices; change it to exclude only reports whose all associated practices are original (i.e. every practice has source_id NULL). Replace the subquery with a left_joins(:practices).group('reports.id').having('COUNT(practices.id FILTER (WHERE practices.source_id IS NOT NULL)) = 0') selection and then where.not(id: that_selection) so that reports with at least one non-null source_id practice are preserved; update the scope :without_original_practice to use left_joins(:practices), group(:id) and the HAVING count of non-null source_id = 0 to identify pure-original reports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db/fixtures/categories_practices.yml`:
- Around line 360-368: The entries categories_practice67 and
categories_practice68 reference a newly added category (category24) but their
position values start at 16 and 17; change the position for
categories_practice67 (practice113 → category24) to 1 and for
categories_practice68 (practice114 → category24) to 2 so the positions for
category24 start at 1 and form a contiguous sequence like other categories.
---
Duplicate comments:
In `@app/models/report.rb`:
- Around line 70-72: scope :without_original_practice currently excludes any
report that has at least one practice with source_id NULL, removing reports that
also have grant practices; change it to exclude only reports whose all
associated practices are original (i.e. every practice has source_id NULL).
Replace the subquery with a
left_joins(:practices).group('reports.id').having('COUNT(practices.id FILTER
(WHERE practices.source_id IS NOT NULL)) = 0') selection and then where.not(id:
that_selection) so that reports with at least one non-null source_id practice
are preserved; update the scope :without_original_practice to use
left_joins(:practices), group(:id) and the HAVING count of non-null source_id =
0 to identify pure-original reports.
In `@app/views/practices/reports/index.html.slim`:
- Around line 16-22: タブの active
判定が文字列比較になっていないため表示と実際の絞り込みがずれているので、params[:grant_only]
を真偽ではなく文字列で比較するように修正してください：practice_reports_path を使っている2つの link_to の class
条件式で現状の params[:grant_only]／!params[:grant_only] チェックをそれぞれ params[:grant_only]
== 'true' と params[:grant_only] != 'true'（または逆に if params[:grant_only] == 'true'
/ unless params[:grant_only] == 'true'）に置き換え、"給付金コース" タブは params[:grant_only] ==
'true' のときのみ 'is-active' が付くようにしてください。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b976f88d-e0e9-4b7a-b8ac-59977904f151
📒 Files selected for processing (13)
app/controllers/practices/reports_controller.rbapp/helpers/page_tabs/practices_helper.rbapp/models/practice.rbapp/models/report.rbapp/views/practices/reports/index.html.slimdb/fixtures/categories.ymldb/fixtures/categories_practices.ymldb/fixtures/courses_categories.ymldb/fixtures/discord_profiles.ymldb/fixtures/practices.ymldb/fixtures/reports.ymldb/fixtures/talks.ymldb/fixtures/users.yml
🚧 Files skipped from review as they are similar to previous changes (6)
- db/fixtures/practices.yml
- app/controllers/practices/reports_controller.rb
- db/fixtures/courses_categories.yml
- db/fixtures/talks.yml
- db/fixtures/reports.yml
- app/models/practice.rb
karlley
force-pushed
the
feature/reports-grant-course-filter-v2
branch
from
914577e to
3c5f10e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/fixtures/practices.yml (1)
394-411:goalフィールドが未設定です。新しく追加されたフィクスチャ（practice64〜67）には
goalフィールドがありません。Practiceモデルではvalidates :goal, presence: trueが設定されており、既存のフィクスチャはすべてgoalフィールドを持っています。フィクスチャのロード時にはバリデーションがスキップされるため動作上は問題ありませんが、一貫性のために追加することを推奨します。
♻️ 修正案
practice64: title: "給付金コースのプラクティス" description: "Railsコースのプラクティスをコピーしたプラクティスです。" + goal: "goal..." source_id: <%= ActiveRecord::FixtureSet.identify(:practice3) %> # PracticeProgressMigratorTestで使用しないpracticeの指定が必要 practice65: title: "日報が存在しない給付金コースのプラクティス" description: "Railsコースのプラクティスをコピーしたプラクティスです。" + goal: "goal..." source_id: <%= ActiveRecord::FixtureSet.identify(:practice4) %> # PracticeProgressMigratorTestで使用しないpracticeの指定が必要 practice66: title: "日報が存在しないプラクティス" description: "日報が存在しないプラクティスです。" + goal: "goal..." practice67: title: "source_idを持っている日報が存在しないプラクティス" description: "source_idを持っている日報が存在しないプラクティスです。" + goal: "goal..." source_id: <%= ActiveRecord::FixtureSet.identify(:practice66) %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/practices.yml` around lines 394 - 411, Add a missing required goal field to the new fixtures practice64, practice65, practice66, and practice67 so they comply with Practice's validates :goal, presence: true; open the fixture entries for practice64–practice67 and add a goal attribute with an appropriate string value (matching the style/format used by existing practice fixtures), ensuring practice67 keeps its source_id and practice64/65 keep theirs so tests like PracticeProgressMigratorTest keep expected references.app/controllers/practices/reports_controller.rb (1)
6-8:params[:practice_id]の条件チェックが冗長、@practice.idの使用を推奨このコントローラーはネストされたルート（
/practices/:practice_id/reports）配下にあるため、params[:practice_id]は常に存在します。また、Line 5 でPractice.find(params[:practice_id])が成功した時点で、パラメータの存在と有効性は保証されています。
@practice.idを使用することで：
- 既にロード済みのモデルを再利用でき一貫性が向上
- 冗長な条件チェックを削除可能
♻️ リファクタ提案
def index `@practice` = Practice.find(params[:practice_id]) - `@reports` = Report.list.page(params[:page]) - `@reports` = `@reports.with_practice_and_source`(params[:practice_id]) if params[:practice_id].present? - `@reports` = `@reports.without_original_practice` if params[:practice_id].present? && params[:grant_only] == 'true' + `@reports` = Report.list + .with_practice_and_source(`@practice.id`) + .then { |reports| params[:grant_only] == 'true' ? reports.without_original_practice : reports } + .page(params[:page]) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/practices/reports_controller.rb` around lines 6 - 8, The checks against params[:practice_id] are redundant because Practice.find(params[:practice_id]) already ran and `@practice` is loaded; update the Report query to use `@practice.id` instead of params[:practice_id] and remove the params presence guards: call `@reports` = `@reports.with_practice_and_source`(`@practice.id`) unconditionally (since route is nested) and change the third line to apply without_original_practice only when params[:grant_only] == 'true' (keep that condition but no longer check params[:practice_id]); adjust references to Report.list.page, with_practice_and_source, without_original_practice and ensure `@practice` is used consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/controllers/practices/reports_controller.rb`:
- Around line 6-8: The checks against params[:practice_id] are redundant because
Practice.find(params[:practice_id]) already ran and `@practice` is loaded; update
the Report query to use `@practice.id` instead of params[:practice_id] and remove
the params presence guards: call `@reports` =
`@reports.with_practice_and_source`(`@practice.id`) unconditionally (since route is
nested) and change the third line to apply without_original_practice only when
params[:grant_only] == 'true' (keep that condition but no longer check
params[:practice_id]); adjust references to Report.list.page,
with_practice_and_source, without_original_practice and ensure `@practice` is used
consistently.
In `@test/fixtures/practices.yml`:
- Around line 394-411: Add a missing required goal field to the new fixtures
practice64, practice65, practice66, and practice67 so they comply with
Practice's validates :goal, presence: true; open the fixture entries for
practice64–practice67 and add a goal attribute with an appropriate string value
(matching the style/format used by existing practice fixtures), ensuring
practice67 keeps its source_id and practice64/65 keep theirs so tests like
PracticeProgressMigratorTest keep expected references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 84d89efa-5e64-4649-9199-d2a2d6e18ba9
📒 Files selected for processing (15)
app/controllers/practices/reports_controller.rbapp/helpers/page_tabs/practices_helper.rbapp/models/practice.rbapp/views/practices/reports/index.html.slimdb/fixtures/categories.ymldb/fixtures/categories_practices.ymldb/fixtures/courses_categories.ymldb/fixtures/discord_profiles.ymldb/fixtures/practices.ymldb/fixtures/reports.ymldb/fixtures/talks.ymldb/fixtures/users.ymltest/fixtures/practices.ymltest/models/practice_test.rbtest/system/practice/reports_test.rb
🚧 Files skipped from review as they are similar to previous changes (11)
- test/models/practice_test.rb
- app/helpers/page_tabs/practices_helper.rb
- test/system/practice/reports_test.rb
- db/fixtures/categories_practices.yml
- db/fixtures/courses_categories.yml
- db/fixtures/practices.yml
- db/fixtures/categories.yml
- db/fixtures/discord_profiles.yml
- db/fixtures/users.yml
- app/models/practice.rb
- app/views/practices/reports/index.html.slim
karlley
force-pushed
the
feature/reports-grant-course-filter-v2
branch
4 times, most recently
from
b368afb to
3036884
Compare
karlley
force-pushed
the
feature/reports-grant-course-filter-v2
branch
6 times, most recently
from
589ed0b to
cdc431d
Compare
karlley
force-pushed
the
feature/reports-grant-course-filter-v2
branch
from
cdc431d to
0845134
Compare
karlley
force-pushed
the
feature/reports-grant-course-filter-v2
branch
from
0845134 to
cff99c7
Compare
karlley
force-pushed
the
feature/reports-grant-course-filter-v2
branch
from
cff99c7 to
28e71a9
Compare
🚀 Review AppURL: https://bootcamp-pr-9733-fvlfu45apq-an.a.run.app
|
karlley
force-pushed
the
feature/reports-grant-course-filter-v2
branch
from
8191412 to
73c6b2a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
前提
wiki: 給付金コースの人が元コースの関連情報を見れるようにする
概要
給付金コースのプラクティスの関連日報一覧(/practices/:id/reports)にコピー元プラクティスの関連日報の表示/非表示を切り替えるフィルターを追加
全て: コピー元プラクティス + 給付金コースのプラクティスの日報給付金コース: 給付金コースのプラクティスの日報補足
元々はReactベースで実装を進めていましたが
Reports.jsxコンポーネントが廃止されたため、Viewベースの実装に変更しました。以下PRはReactベースで作成していたものです。
#9515
変更確認方法
事前準備
feature/reports-grant-course-filter-v2をローカルに取り込むフィルター表示/非表示
komagataでログインgrant-courseでログインフィルタリング
grant-courseでログイン全て選択時:給付金コース選択時:日報の表示件数
grant-courseでログイン日報（n）の表示件数と実際に表示される日報数が同一であることを確認日報（n）の表示件数がフィルターの「全て」選択時の表示件数と同一であることを確認給付金が日報が0件時の表示
grant-courseでログイン給付金コースを選択Screenshot
変更前
変更後
Summary by CodeRabbit