給付金コースのプラクティスの質問一覧に元コースのプラクティスの質問も表示できるようにした#9975
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給付金コース（grant-course）向けに、scope パラメータで質問の取得範囲を切り替える機能を追加しました。コントローラ、ヘルパー、ビュー、フィクスチャ、およびシステムテストが更新されています。 Changes給付金コース質問スコープフィルタリング機能
Sequence Diagram(s)sequenceDiagram
participant Client
participant QuestionsController
participant Practice
participant Question
Client->>QuestionsController: GET /practices/:id/questions?scope=grant_course&target=solved
QuestionsController->>QuestionsController: scope() -> 'grant_course', target() -> 'solved'
QuestionsController->>Practice: load `@practice` and maybe source_practice
QuestionsController->>Question: Query Question.where(practice: practices).by_target('solved')
Question-->>QuestionsController: filtered questions
QuestionsController-->>Client: render paginated results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🚀 Review AppURL: https://bootcamp-pr-9975-fvlfu45apq-an.a.run.app
|
djkazunoko
force-pushed
the
feature/show-source-practice-qas-in-grant-course-practice
branch
6 times, most recently
from
e007ad6 to
739f2ed
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/system/practice/questions_test.rb`:
- Around line 27-29: Replace the implicit visit call with visit_with_auth so the
test does not depend on prior login state: change visit
"/practices/#{practices(:practice1).id}/questions" to visit_with_auth
"/practices/#{practices(:practice1).id}/questions", user:
users(:your_fixture_user) (use the appropriate fixture or factory for an
authenticated user in this test); keep the subsequent assert_no_selector lines
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f0510638-a3c8-4e00-a011-4b26f10a6824
📒 Files selected for processing (11)
app/controllers/practices/questions_controller.rbapp/helpers/page_tabs/practices_helper.rbapp/views/practices/questions/index.html.slimdb/fixtures/categories_practices.ymldb/fixtures/correct_answers.ymldb/fixtures/courses_categories.ymldb/fixtures/practices.ymldb/fixtures/questions.ymltest/fixtures/correct_answers.ymltest/fixtures/questions.ymltest/system/practice/questions_test.rb
💤 Files with no reviewable changes (1)
- db/fixtures/practices.yml
djkazunoko
force-pushed
the
feature/show-source-practice-qas-in-grant-course-practice
branch
from
3af6b7d to
8f4f7c1
Compare
|
@koguchi-e |
djkazunoko
changed the title
|
@djkazunoko |
|
@koguchi-e |
|
@djkazunoko |
djkazunoko
force-pushed
the
feature/show-source-practice-qas-in-grant-course-practice
branch
from
8f4f7c1 to
4afac36
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@db/fixtures/categories_practices.yml`:
- Around line 360-373: Update the YAML fixture entry for
categories_practice113_1 (practice: practice113, category: category24) so its
position value is 1 instead of 16 to comply with acts_as_list scope: :category
ordering (each category's positions must start at 1); simply change the position
for categories_practice113_1 to 1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f13a9cbe-65d4-42ac-ae86-975ebe6a04a4
📒 Files selected for processing (11)
app/controllers/practices/questions_controller.rbapp/helpers/page_tabs/practices_helper.rbapp/views/practices/questions/index.html.slimdb/fixtures/categories_practices.ymldb/fixtures/correct_answers.ymldb/fixtures/courses_categories.ymldb/fixtures/practices.ymldb/fixtures/questions.ymltest/fixtures/correct_answers.ymltest/fixtures/questions.ymltest/system/practice/questions_test.rb
✅ Files skipped from review due to trivial changes (1)
- test/fixtures/correct_answers.yml
🚧 Files skipped from review as they are similar to previous changes (9)
- db/fixtures/questions.yml
- db/fixtures/courses_categories.yml
- app/controllers/practices/questions_controller.rb
- test/fixtures/questions.yml
- app/views/practices/questions/index.html.slim
- db/fixtures/correct_answers.yml
- test/system/practice/questions_test.rb
- app/helpers/page_tabs/practices_helper.rb
- db/fixtures/practices.yml
djkazunoko
force-pushed
the
feature/show-source-practice-qas-in-grant-course-practice
branch
from
4afac36 to
5e8032f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@db/fixtures/categories_practices.yml`:
- Around line 320-358: The fixtures for category6 have duplicated positions:
categories_practice65 and categories_practice69 both use position: 14, and
categories_practice67 and categories_practice71 both use position: 15, causing
non-unique ordering; update the position values for the entries
(categories_practice65, categories_practice69, categories_practice67,
categories_practice71) so that within category6 each record has a unique
sequential position (e.g., renumber them to a continuous, non-conflicting
sequence) and keep other practice/category associations unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bce047e7-5a7b-4471-971a-dbbd55283f65
📒 Files selected for processing (9)
app/controllers/practices/questions_controller.rbapp/helpers/page_tabs/practices_helper.rbapp/views/practices/questions/index.html.slimdb/fixtures/categories_practices.ymldb/fixtures/correct_answers.ymldb/fixtures/questions.ymltest/fixtures/correct_answers.ymltest/fixtures/questions.ymltest/system/practice/questions_test.rb
✅ Files skipped from review due to trivial changes (1)
- test/fixtures/correct_answers.yml
🚧 Files skipped from review as they are similar to previous changes (6)
- test/fixtures/questions.yml
- db/fixtures/questions.yml
- app/controllers/practices/questions_controller.rb
- db/fixtures/correct_answers.yml
- app/views/practices/questions/index.html.slim
- test/system/practice/questions_test.rb
djkazunoko
force-pushed
the
feature/show-source-practice-qas-in-grant-course-practice
branch
from
5e8032f to
08c0d10
Compare
|
@koguchi-e |
|
@djkazunoko 動作確認したところ、 確認したところ、以下の2点が main ブランチとズレがありました。
|
|
ご確認ありがとうございます。 こちらのローカルで
また、 日報 に記載されていた お手数ですが、
を実行して、再度ご確認いただけますでしょうか。 また、今回のような日報に記載されている再現ログやエラーログは GitHub のコメントにも記載いただけると、状況の把握や調査がしやすくて助かります 🙏 |
|
@djkazunoko 確認したところ、当時の状態が正しく保存できているわけではないため推測となりますが、レビュー対象ブランチの状態を十分確認できていなかった可能性がございます。 git checkout main
git pull --rebase origin main
git checkout feature/show-source-practice-qas-in-grant-course-practiceを実行しており、当時はレビュー対象ブランチも最新の状態で確認できている認識でした。 しかし後から調べたところ、 そのため、私のローカル環境の状態が影響していた可能性があります。大変失礼いたしました。 ご確認いただいた内容については問題ないことを確認できましたので、Approve させていただきました。 |
|
@koguchi-e |
|
@okuramasafumi |
| params[:target] if %w[solved not_solved].include?(params[:target]) | ||
| end | ||
|
|
||
| def scope |
There was a problem hiding this comment.
ご確認ありがとうございます。
使用はされていましたが、メソッド名が分かりにくかったため、selected_target、selected_scope に変更しました。ae8b17e
| .page(params[:page]) | ||
| practices = | ||
| if scope == 'grant_course' | ||
| @practice |
There was a problem hiding this comment.
別の箇所では配列にしているので（そのほうが同じ配列になるので）、[@practice]にすることを検討できそうです。
本番環境ではlib/tasks/bootcamp.rakeのcopy_practicesタスクにより新たなカテゴリが作成され、作成されたカテゴリが給付金コースに紐づけられる。 他のコースに紐づいている既存カテゴリを給付金コースに紐付けることをやめて、新たに作成したカテゴリを給付金コースに紐づけることで本番環境と統一した。
通し番号の方がfixture名の規則が分かりやすく、追加時にも連番で管理しやすいため。
- 「全て」では元コースを含む合算件数を表示 - 「給付金コース」では給付金コースのみの件数を表示 - 質問ページ以外では合算件数を表示
lengthでは SELECT * によりレコード全件をロードしていたが、countを使うことで SELECT COUNT(*) による件数取得のみに変更
djkazunoko
force-pushed
the
feature/show-source-practice-qas-in-grant-course-practice
branch
from
08c0d10 to
461aeb3
Compare
| end | ||
|
|
||
| def question_practices | ||
| if selected_scope == 'grant_course' |
There was a problem hiding this comment.
selected_scopeはparams[:scope] if %w[grant_course].include?(params[:scope])となっていますが、その返り値を考えると、if条件が成立するのはparams[:scope] == 'grant_course'の場合だけですので、条件成立時に'grant_course'、不成立時にnilになります。
さて、Rubyではifの後ろはfalseとnil以外は全てtrueとして扱われます。つまり、if 'nantoka'と書くことができます。
ということは、if selected_scope == 'grant_course'はif selected_scopeと同じ動きをします。
もしこのように書くとした場合、今度はselected_scopeの命名に違和感が出てきます。というのは、内容としては「給付金コースなら」というものであるのに、if selected_scopeの字面だと「選択されたコースがあるなら」という意味になってしまうからです。今の意味内容に寄せるなら、grant_course?くらいの名前でもいいかもしれません。
ただし、これは今時点の実装がそうなっているというだけかもしれません。将来的にselected_scopeの実装が変わるなら、今のままの命名でむしろ正しいかもしれません。そのときには、if selected_scope == 'grant_course'と書くことになる可能性も高いので、実装も今のままでよいことになります。
長々書いてしまったのですが、これは結局「将来のことをどこまで見越して設計するか」という話の一例です。自分はその点についての知識がないため、判断ができません。そのように判断が難しい場合は、ひとまず一番単純なものを作っておくことが正しいとされています（You ain't gonna need it, YAGNIと呼ばれます）。
このコメントはヒントとして捉えてもらえるといいかと思います。
There was a problem hiding this comment.
ご確認ありがとうございます！
おっしゃる通りgrant_course しか返さないのに selected_scope という命名は違和感があると感じました。
またparams[:scope] if %w[grant_course].include?(params[:scope])の処理自体も、現状では不要だと思いました。(selected_target のように複数の選択肢を扱う場合は有効ですが、今回は grant_course の1種類しかないため)
そのため、selected_scope メソッドを削除し、question_practices 内で params[:scope] == 'grant_course' を直接判定するように修正しました。8937bcd
okuramasafumi
left a comment
okuramasafumi
left a comment
There was a problem hiding this comment.
動作は問題なさそうなのでApproveします。一点、https://github.com/fjordllc/bootcamp/pull/9975/changes#r3564245888 にコメントを書いたので、読んでみてください。
|
@okuramasafumi |
3 participants
Issue
概要
※ 給付金コース = Railsエンジニア（Reスキル講座認定）コース(https://bootcamp.fjord.jp/courses/47/practices)
変更確認方法
feature/show-source-practice-qas-in-grant-course-practiceをローカルに取り込むbin/rails db:seed:replantを実行bin/devでサーバーを起動localhost:3000にアクセスし、grant-courseでログイン質問表示切り替えタブの表示・非表示の確認
/practices/200789251/questions(給付金コースのプラクティスの質問一覧)にアクセスし、質問表示切り替えタブが表示されていることを確認する/practices/473419339/questions(給付金コース以外のプラクティスの質問一覧)にアクセスし、質問表示切り替えタブが表示されていないことを確認する質問の表示・非表示の確認
質問A → 給付金コースのプラクティスの質問
質問B → 給付金コースのプラクティスの質問(解決済み)
質問C → 給付金コースのコピー元プラクティスの質問
質問D → 給付金コースのコピー元プラクティスの質問(解決済み)
/practices/200789251/questions(給付金コースのプラクティスの質問一覧)にアクセス質問タブの件数表示の確認
/practices/200789251/questions(給付金コースのプラクティスの質問一覧)にアクセス質問（4）と表示されていることを確認する質問（2）と表示されていることを確認する/practices/200789251/にアクセスして、質問タブに質問（4）と表示されていることを確認するScreenshot
変更前
変更後
参考
Flakewatch Report
Summary by CodeRabbit
新機能
テスト
その他
Review App