給付金コースのプラクティスの関連Docs一覧に、元コースの関連日報できるようにする。#9650
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:
📝 WalkthroughWalkthrough給付金コース（source_id を持つ実践）でページ取得を Changes
Sequence Diagram(s)sequenceDiagram
participant User as ブラウザ（User）
participant App as Rails App（PagesController）
participant DB as Database
participant Src as SourcePractice
User->>App: GET /practices/:id/pages?target=...
App->>DB: load Practice(id)
App->>DB: load practice.pages (includes comments, avatar)
alt practice.grant_course? && params.target != 'only_grant_course'
App->>DB: load source_practice.pages (includes comments, avatar)
App->>App: union practice.pages + source_practice.pages
end
App->>App: apply order, pagination (.order/.page/.per)
App->>User: render pages/index (grant_course filter shown if applicable)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
kutimiti1234
force-pushed
the
feature/docs-grant-course-filter
branch
3 times, most recently
from
71b116d to
0a8fdcf
Compare
|
給付金コースの「全て」に給付金コースのDocsが表示されていない致命的なバグがあったため修正しました。 |
kutimiti1234
force-pushed
the
feature/docs-grant-course-filter
branch
14 times, most recently
from
22740f8 to
ce8dbe8
Compare
kutimiti1234
changed the title
|
@komagata 
こちら原因としては、 practice64:
title: "OS X Mountain Lionをクリーンインストールする（Reスキル）"
description: "Railsコースのプラクティスをコピーしたプラクティスです。"
source_id: <%= ActiveRecord::FixtureSet.identify(:practice1) %>
practice65:
title: "Terminalの基礎を覚える（Reスキル）"
description: "Railsコースのプラクティスをコピーしたプラクティスです。"
source_id: <%= ActiveRecord::FixtureSet.identify(:practice2) %>具体的には copied_practice = Practice.find_by(source_id: practice_id) copied_practice = Practice.find_by(source_id: practice.id)と書かれていたため、 ただし、 そこで、こういった場合にどういった対処法がよいか相談させていただきたいです。 1, - copied_practice = Practice.find_by(source_id: practice_id)
+ copied_practice = Practice.where(source_id: practice_id).order(:created_at).last- copied_practice = Practice.find_by(source_id: practice.id)
+ copied_practice = Practice.where(source_id: practice.id).order(:created_at).lastメリット：fixtureを変更する必要がない。
メリット：今回のスコープ外である 個人的には2かなと思っているのですが、念のため相談させていただきたいです。 |
3個目の案を追加しました。改めて考えるとこの3が一番素直に感じており、良いのかなと考えています |
kutimiti1234
force-pushed
the
feature/docs-grant-course-filter
branch
2 times, most recently
from
1274c71 to
deb4d1c
Compare
|
@kutimiti1234
がよいんじゃないかな〜と思いました！ source_practice1:
title: "コピー元のプラクティス1"
description: "description..."
goal: "goal..."
memo: "memo for mentors..."
source_practice2:
title: "コピー元のプラクティス2"
description: "description..."
goal: "goal..."
memo: "memo for mentors..."
copy_practice1:
title: "コピー先のプラクティス1"
description: "copy1..."
source_practice: source_practice1
copy_practice2:
title: "コピー先のプラクティス2"
description: "copy2..."
source_practice: source_practice2蛇足ですが
こういうときは画面キャプチャよりテキストをぺろっと貼っていただいた方が個人的には嬉しいかな！と思いました（これは本当に個人の感想です） |
|
bootcamp/test/models/practice_progress_migrator_test.rb Lines 13 to 14 in deb4d1c これは @kutimiti1234 が書いているとおり、「destroy の必要性がわからない」ので避けたい気持ちがあります。 なので事情をコメントに書いておくという方法もあるけど、それでも「いや事情はわかったけどここだけ destory するってやっぱり不思議だな？」ってなってしまう気がします。 |
|
@torinoko さんの案が良いと思います。 既存のテストは絶対変更しちゃいけないってことはないですが、この件では変更する必要もないかなとおもうので。 |
kutimiti1234
force-pushed
the
feature/docs-grant-course-filter
branch
2 times, most recently
from
c4e00f8 to
16ab24b
Compare
給付金コースのフィルターはパーシャルとして再利用できるように実装
kutimiti1234
force-pushed
the
feature/docs-grant-course-filter
branch
from
16ab24b to
241f3b2
Compare
kutimiti1234
force-pushed
the
feature/docs-grant-course-filter
branch
from
241f3b2 to
1a264bf
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: 6
🧹 Nitpick comments (1)
app/controllers/practices/pages_controller.rb (1)
9-15:includesの重複を削除できます。10行目で
includesを適用済みのため、13-14行目のincludesは冗長です。.or()で結合された結果にも最初のincludesが適用されます。♻️ 提案する修正
`@pages` = `@practice.pages` - .includes(:comments, :practice, { last_updated_user: { avatar_attachment: :blob } }) if `@practice.grant_course`? && params[:target] != 'only_grant_course' - `@pages` = `@pages.or`(`@practice.source_practice.pages`).includes(:comments, :practice, - { last_updated_user: { avatar_attachment: :blob } }) + `@pages` = `@pages.or`(`@practice.source_practice.pages`) end - `@pages` = `@pages.order`(updated_at: :desc, id: :desc) + `@pages` = `@pages.includes`(:comments, :practice, { last_updated_user: { avatar_attachment: :blob } }) + .order(updated_at: :desc, id: :desc) .page(params[:page]) .per(PAGER_NUMBER)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/practices/pages_controller.rb` around lines 9 - 15, `@pages` is initialized with `@practice.pages.includes`(...), so remove the redundant .includes(...) chained onto the .or(`@practice.source_practice.pages`) branch; keep the initial .includes on the `@pages` assignment and change the conditional branch to simply do `@pages` = `@pages.or`(`@practice.source_practice.pages`) so the original includes apply to the combined relation (refer to `@pages`, `@practice.pages`, .includes and .or(...)).
🤖 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/controllers/practices/pages_controller.rb`:
- Around line 12-15: Change the direct call to `@practice.source_practice.pages`
to a nil-safe call to avoid potential NoMethodError: replace
`@practice.source_practice.pages` with `@practice.source_practice`&.pages in the
conditional block (the code that assigns `@pages` using
`@practice.source_practice.pages`) so it mirrors the nil-safe pattern used in
own_and_source_practice_pages_length in Practice model and keeps the
includes(:comments, :practice, { last_updated_user: { avatar_attachment: :blob }
}) intact.
In `@app/models/practice.rb`:
- Around line 214-216: own_and_source_practice_pages_length calls
source_practice.pages without guarding for source_practice being nil, causing
NoMethodError; update the method (own_and_source_practice_pages_length) to
handle nil by treating missing source_practice as zero pages (e.g. use safe
navigation or a conditional to use 0 when source_practice is nil) and keep
summing with pages.length.
In `@db/fixtures/categories.yml`:
- Around line 117-119: The fixture entry for category24 has a mismatched name
and slug: name is "Mac OS X（Reスキル）" but slug is "ready-for-learn"; update the
slug to reflect the category name (e.g., "mac-os-x-re-skill" or
"mac-osx-re-skill") so the slug semantically matches the name; locate the
category fixture entry labeled category24 in categories.yml and replace the slug
"ready-for-learn" with the corrected slug.
In `@db/fixtures/practices.yml`:
- Around line 768-776: The fixtures practice113 and practice114 are missing the
required Practice attribute "goal"; update the practice113 and practice114
entries to include a goal key (e.g., a short string or copy the goal from the
referenced source practice1/practice2) so the fixtures satisfy Practice
validations (look for the practice113 and practice114 entries and add a goal:
"<appropriate goal text>" field).
In `@test/system/practice/pages_test.rb`:
- Around line 45-51: The test name string in the test case (the one starting
with test 'grant filter shows only grant course reports when "給付金コース" is
selected' do) is misleading because the body asserts Docs page behavior; change
"reports" to "docs" in that test description so the test name accurately
reflects what the test verifies (referencing the test block starting with test
'...').
- Around line 53-59: Rename the test's description string to match what's being
tested: update the test named "grant filter shows empty message when no reports
exist" to something like "grant filter shows empty message when no docs exist"
(or "when no pages exist") so it reflects that the test visits Pages
(visit_with_auth "/practices/#{practices(:copy_practice2).id}/pages") and
asserts the Docs empty message; change the string in the test definition only
(the test ... do line) to keep assertions and behavior in methods like
visit_with_auth and find unchanged.
---
Nitpick comments:
In `@app/controllers/practices/pages_controller.rb`:
- Around line 9-15: `@pages` is initialized with `@practice.pages.includes`(...), so
remove the redundant .includes(...) chained onto the
.or(`@practice.source_practice.pages`) branch; keep the initial .includes on the
`@pages` assignment and change the conditional branch to simply do `@pages` =
`@pages.or`(`@practice.source_practice.pages`) so the original includes apply to the
combined relation (refer to `@pages`, `@practice.pages`, .includes and .or(...)).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/controllers/practices/pages_controller.rbapp/helpers/page_tabs/practices_helper.rbapp/models/practice.rbapp/views/practices/_grant_course_filter.html.slimapp/views/practices/pages/index.html.slimdb/fixtures/categories.ymldb/fixtures/categories_practices.ymldb/fixtures/courses_categories.ymldb/fixtures/discord_profiles.ymldb/fixtures/pages.ymldb/fixtures/practices.ymldb/fixtures/reports.ymldb/fixtures/talks.ymldb/fixtures/users.ymltest/fixtures/pages.ymltest/fixtures/practices.ymltest/models/practice_test.rbtest/system/practice/pages_test.rb
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/system/practice/pages_test.rb`:
- Around line 53-60: Rename the test description string for the test that
currently begins with "grant filter shows only grant course pages when
\"給付金コース\" is selected but source practice has no practices" to accurately
reflect that the setup destroys pages (i.e., change "has no practices" to "has
no pages"); update the test declaration (the test '...') so the human-readable
name matches the action performed on
practices(:source_practice1).pages.destroy_all and run the test suite to
confirm.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/practices/pages_controller.rbtest/system/practice/pages_test.rb
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/models/practice.rb (1)
214-216:⚠️ Potential issue | 🟠 Major
source_practiceが nil のケースで落ちる可能性があります。Line 215 は
source_practiceを直接参照しているため、関連が欠けたデータでNoMethodErrorになります。件数取得はlengthよりsizeが適しています。💡 修正案
def own_and_source_practice_pages_length - pages.length + source_practice.pages.length + pages.size + source_practice&.pages&.size.to_i 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 - 216, own_and_source_practice_pages_length currently calls source_practice.pages.length which raises NoMethodError when source_practice is nil and uses length instead of size; update the method (own_and_source_practice_pages_length) to defensively handle a nil source_practice (e.g., treat missing source_practice as zero pages) and use size on both collections (pages.size and source_practice&.pages&.size || 0) so it returns a correct integer even when source_practice is absent.app/controllers/practices/pages_controller.rb (1)
13-15:⚠️ Potential issue | 🟠 Major条件式で
source_practiceの nil ガードが不足しています。Line 13 の
source_practice.pages.present?は、source_practiceが nil の場合に例外になります。💡 修正案
- if `@practice.grant_course`? && params[:target] != 'only_grant_course' && source_practice.pages.present? + if `@practice.grant_course`? && params[:target] != 'only_grant_course' && source_practice && source_practice.pages.present? `@pages` = `@pages.or`(source_practice.pages).includes(:comments, :practice, { last_updated_user: { avatar_attachment: :blob } }) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/practices/pages_controller.rb` around lines 13 - 15, The condition uses source_practice.pages.present? without guarding for source_practice being nil; update the conditional to check source_practice first (for example change source_practice.pages.present? to source_practice.present? && source_practice.pages.present? or use safe navigation source_practice&.pages.present?) so the `@pages` = `@pages.or`(source_practice.pages).includes(...) assignment only runs when source_practice exists and has pages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/controllers/practices/pages_controller.rb`:
- Around line 13-15: The condition uses source_practice.pages.present? without
guarding for source_practice being nil; update the conditional to check
source_practice first (for example change source_practice.pages.present? to
source_practice.present? && source_practice.pages.present? or use safe
navigation source_practice&.pages.present?) so the `@pages` =
`@pages.or`(source_practice.pages).includes(...) assignment only runs when
source_practice exists and has pages.
In `@app/models/practice.rb`:
- Around line 214-216: own_and_source_practice_pages_length currently calls
source_practice.pages.length which raises NoMethodError when source_practice is
nil and uses length instead of size; update the method
(own_and_source_practice_pages_length) to defensively handle a nil
source_practice (e.g., treat missing source_practice as zero pages) and use size
on both collections (pages.size and source_practice&.pages&.size || 0) so it
returns a correct integer even when source_practice is absent.
9f7e154 to
88f5d05
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/system/practice/pages_test.rb`:
- Line 54: The test is failing due to a fixture key typo: replace the
non-existent practices(:eopy_practice3) used in visit_with_auth with the correct
fixture key practices(:copy_practice3); update the call in
test/system/practice/pages_test.rb (the visit_with_auth line) so it references
practices(:copy_practice3) and rerun the test to confirm the fix.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/controllers/practices/pages_controller.rbapp/models/practice.rbtest/fixtures/pages.ymltest/fixtures/practices.ymltest/models/practice_test.rbtest/system/practice/pages_test.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- app/models/practice.rb
- app/controllers/practices/pages_controller.rb
- test/fixtures/pages.yml
kutimiti1234
force-pushed
the
feature/docs-grant-course-filter
branch
from
88f5d05 to
a08c0fc
Compare
|
@machida |
machida
force-pushed
the
feature/docs-grant-course-filter
branch
from
8baecb7 to
27f653a
Compare
|
@kutimiti1234 お待たせしました！！レイアウト修正をしましたー |
|
@machida |
torinoko
left a comment
torinoko
left a comment
There was a problem hiding this comment.
@kutimiti1234
またシュッと失礼します！
| source_practice = @practice.source_practice | ||
|
|
||
| @pages = @practice.pages | ||
| .includes(:comments, :practice, { last_updated_user: { avatar_attachment: :blob } }) |
There was a problem hiding this comment.
また余計なツッコミをしてしまうのだけど、できれば includes を使用する際は以下のような仕様であることを認識した上で使用指定ただけると嬉しいです。
- 条件に応じて発行される SQL が変わる
- このため、意図しない SQL が発行される可能性がある
これに関してはこちらの記事がわかりやすいかな？
https://tech.smarthr.jp/entry/2025/06/03/113908
あとこういうこともあったりします。
組み立てたリレーションを再利用するときなどに無駄なコストが発生してしまう可能性もあるんですね。
知っておくと良い注意点は、リレーションオブジェクトにincludesがあると、eager loadingが不必要なクエリにおいてでも、pluckがeager loadingを引き起こすことです。以下に例を示します。
https://railsguides.jp/active_record_querying.html#pluck
ほとんどの場合、N + 1 が発生するよりは遥かによい結果になるかもしれません。だけど書き手の意図しない SQL の発行はリスクをはらみます。
なので #includes のご利用は計画的に！
| @pages = @practice.pages | ||
| .includes(:comments, :practice, { last_updated_user: { avatar_attachment: :blob } }) | ||
|
|
||
| if @practice.grant_course? && params[:target] != 'only_grant_course' && source_practice.pages.present? |
There was a problem hiding this comment.
条件式が複雑になる場合は、この式をまるっと private メソッドに切り出すという手段もあります。
- プラクティスが grant_course を持っている
- params[:target] が
only_grant_courseではない - source_practice がページを持っている
def has_source_course?
@practice.grant_course? &&
params[:target] != 'only_grant_course' &&
@practice.source_practice.pages.present?
endあと #grant_course? 自体が source_id の有無を確認しているだけなので、メソッド化するメリットってあるのかな？とちょっと思いました。
5 participants
Issue
概要
変更確認方法
1.
feature/docs-grant-course-filterをローカルに取り込む2. http://localhost:3000/practices/200789251/pages へ遷移
3. ピル型タブで「全て」が表示されていることを確認
4. デフォルトで給付金コースと元コース(http://localhost:3000/practices/315059988) のどちらのDocsも表示されていることを確認する。
5. ピル型タブで「給付金コース」をクリック
6.
http://localhost:3000/practices/200789251/pages?target=only_grant_courseに遷移していることを確認7. 給付金コースのDocsだけが表示されていることを確認。
Screenshot
変更前
変更後
Summary by CodeRabbit
新機能
改善
テスト
雑多