給付金コースユーザーが通常コースのプラクティスの提出物を提出できないようにする#10224
Conversation
📝 WalkthroughWalkthrough給付金コース向けの判定メソッドを追加し、学習画面とプラクティス詳細画面の案内表示を条件分岐化しました。給付金コースユーザー、通常ユーザー、元プラクティスの表示差分を検証するシステムテストも追加されています。 Changes給付金コース向け表示分岐
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PracticeView
participant Practice
User->>PracticeView: プラクティスを表示
PracticeView->>Practice: guide_to_grant_course?(current_user)
Practice-->>PracticeView: true / false
PracticeView-->>User: 誘導リンクまたは通常の進捗操作を表示
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@app/components/learnings/learning_component.html.slim`:
- Line 5: The `@practice.copied_practices.first` call in the link_to on line 5
is unreliable because it arbitrarily picks the first copied practice without
considering business logic. Create a dedicated method in the Practice model that
resolves the correct copied practice based on user and course relationships,
then replace `@practice.copied_practices.first` with a call to this new method.
Apply this same method consistently in both templates to ensure stable
navigation.
In `@app/views/practices/_learning-status.html.slim`:
- Around line 12-14: The `li.practice-status-buttons__item` element and its
child button declaration are over-indented, creating a nested `li > li`
structure instead of the intended `ul > li` sibling structure. Reduce the
indentation of lines 12-14 (the li element with class
practice-status-buttons__item, the button_class assignment, and the content_tag
button call) by one indentation level so they align as siblings within the ul
container rather than nesting under the previous li element.
🪄 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: 99c22b21-d0da-4672-b24a-398a02b7c0a1
📒 Files selected for processing (2)
app/components/learnings/learning_component.html.slimapp/views/practices/_learning-status.html.slim
zecky1120
force-pushed
the
feature/prevent-students-in-Reskill-course-from-submitting-regular-practice-assignments
branch
from
88963be to
5d49580
Compare
|
@machida |
|
@zecky1120 お待たせしました！デザイン入れましたー |
|
@machida |
|
@zamami |
|
@zecky1120 |
|
@zamami |
There was a problem hiding this comment.
今回、条件式で
@current_user.grant_course?
@practice.source_id.blank?
を使って給付金コースのページへ誘導するかを判定しているので、
guide_to_grant_course?(user)みたいなメソッドをmodels/practice.rbに作ることで他のファイルの条件式で||や!を使わずに済むかなと思いました。
あとは、テストも他の条件式も追加するといいなと思いました。（回帰テスト） コメントしてみたので参考にしてみてください。 私の方ではapproveとさせていただきます！
| assert_no_text 'このプラクティスを修了にしてください。' | ||
| assert_no_text 'このプラクティスに提出物はありません。' | ||
| assert_no_text '修了条件をクリアしたら修了にしてください。' | ||
| end |
There was a problem hiding this comment.
今回、条件分岐が入ったので以下の2つの条件の場合のテストも追加するといいなと思いました。
・通常ユーザーが元プラクティスの画面を開いた場合
・給付金受給ユーザーがコピー元のプラクティスの画面を開いた場合
# 通常ユーザーが元プラクティスを開くと、給付金への誘導が出ず、通常のステータスボタン群が表示される
test 'show normal status buttons on source practice for non grant course user' do
source_practice = practices(:practice23)
visit_with_auth practice_path(source_practice), 'kimura'
assert_selector '.practice-status-buttons__start'
assert_no_link '給付金コースへ移動する'
assert_no_text '進捗のステータス変更は、給付金コースのプラクティス側で行ってください。'
assert_no_text '提出・修了は、給付金コースのプラクティス側で行ってください。'
end
# 給付金ユーザーがコピー側のプラクティスを開くと、誘導が出ず、通常のステータスボタン群が表示される
test 'show normal status buttons on grant course practice for grant course user' do
grant_course_practice = practices(:practice64)
visit_with_auth practice_path(grant_course_practice), 'grant-course'
assert_selector '.practice-status-buttons__start'
assert_no_link '給付金コースへ移動する'
assert_no_text '進捗のステータス変更は、給付金コースのプラクティス側で行ってください。'
end
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/models/practice.rb (1)
214-217: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value既存の
grant_course?を再利用できます。
source_id.blank?は既存のgrant_course?（Line 210-212）の否定と等価です。重複ロジックを避けるため再利用を検討してください。♻️ 提案する差分
def guide_to_grant_course?(user) - user.grant_course? && source_id.blank? + user.grant_course? && !grant_course? end🤖 Prompt for 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. In `@app/models/practice.rb` around lines 214 - 217, The guide_to_grant_course? predicate duplicates logic by checking source_id.blank? directly; reuse the existing grant_course? method instead of repeating the blank check. Update guide_to_grant_course?(user) in Practice to express the same condition through grant_course? combined with user.grant_course?, keeping the logic centralized and consistent.app/views/practices/_learning-status.html.slim (1)
3-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win共通パーシャル化の検討。
給付金コース誘導ブロック（リンク＋注記）は
learning_component.html.slimとほぼ同一構造で重複しています。文言（注記テキスト）だけが異なるため、テキストを引数化した共通パーシャルに切り出すと、今回のような二重ネストの取りこぼしを防ぎやすくなります。🤖 Prompt for 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. In `@app/views/practices/_learning-status.html.slim` around lines 3 - 9, The grant-course guidance block in the learning status view is duplicated with the similar block in learning_component, with only the note text differing. Extract the shared link-and-note UI into a common partial or helper, and make the note text configurable so both call sites can reuse it. Use the existing guide_to_grant_course? guard and the practice-status-buttons__grant-guidance / practice-status-buttons__grant-link / practice-status-buttons__grant-note structure to keep the behavior and styling consistent.
🤖 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 `@app/models/practice.rb`:
- Around line 214-217: Add unit coverage for Practice#guide_to_grant_course? in
the Practice model test file by exercising both inputs the method depends on: a
case where user.grant_course? is true and source_id is blank, and at least one
contrasting case where either user.grant_course? is false or source_id is
present. Use the guide_to_grant_course? method name in test setup/assertions so
the behavior stays tied to the existing predicate.
---
Nitpick comments:
In `@app/models/practice.rb`:
- Around line 214-217: The guide_to_grant_course? predicate duplicates logic by
checking source_id.blank? directly; reuse the existing grant_course? method
instead of repeating the blank check. Update guide_to_grant_course?(user) in
Practice to express the same condition through grant_course? combined with
user.grant_course?, keeping the logic centralized and consistent.
In `@app/views/practices/_learning-status.html.slim`:
- Around line 3-9: The grant-course guidance block in the learning status view
is duplicated with the similar block in learning_component, with only the note
text differing. Extract the shared link-and-note UI into a common partial or
helper, and make the note text configurable so both call sites can reuse it. Use
the existing guide_to_grant_course? guard and the
practice-status-buttons__grant-guidance / practice-status-buttons__grant-link /
practice-status-buttons__grant-note structure to keep the behavior and styling
consistent.
🪄 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: a43ff6cb-888e-466e-88be-002026a03fbc
📒 Files selected for processing (5)
app/components/learnings/learning_component.html.slimapp/models/practice.rbapp/views/practices/_learning-status.html.slimapp/views/practices/show.html.slimtest/system/practices_test.rb
|
@zamami - if @practice.guide_to_grant_course?(current_user)
...
| 給付金コースへ移動する
.practice-status-buttons__note.practice-status-buttons__grant-note
| 進捗のステータス変更は、給付金コースのプラクティス側で行ってください。
- else
.practice-status-buttons__start
.practice-status-buttons__label
| ステータス
ul.practice-status-buttons__items.is-button-group
...通常の @okuramasafumi |
|
@zecky1120 |
| br | ||
| | このプラクティスを修了にしてください。 | ||
| - else | ||
| - elsif !@practice.guide_to_grant_course?(current_user) |
There was a problem hiding this comment.
既存のelse節を上書きしているため、else節が存在しない状態になっています。
!@practice.guide_to_grant_course?(current_user)がfalseであるとき（@practice.guide_to_grant_course?(current_user)がtrueであるとき）、上のif文はtrue_or_false && falseで常にfalse、このelsifもfalseになるため、else節があればそちらに処理が移りますが、ないため何も起きなくなっています（具体的には、「このプラクティスに提出物はありません。」の文言が表示されません）。
字面だけでいっても、@practice.guide_to_grant_course?(current_user)がtrueであることは普通にありそうです。仕様の再確認をお願いします。
もしかしたら今の仕様でよいのかもしれませんが、一般に、else節はつけるべきだとされています（例としては https://docs.rubocop.org/rubocop/latest/cops_style.html#stylemissingelse）。`else`節に処理が来ることは往々にしてありますし、ありえない場合は`else raise`的に例外を上げるようにして「ここにこないことは検討済み」とコード上で示せるので有用な場合があります。
There was a problem hiding this comment.
@okuramasafumi
ご確認いただきありがとうございました！
ご指摘いただいた箇所ですが、else文に戻してその中に条件分岐（unless文）にして対応させていただきました〜🙏
okuramasafumi
left a comment
okuramasafumi
left a comment
There was a problem hiding this comment.
わかりやすくなったと思います、LGTM!
4 participants
Issue
概要
給付金コースユーザーが通常コースのプラクティスに提出させないようにする間違いをなくしたい。
通常プラクティスに移動したら、
変更確認方法
feature/prevent-students-in-Reskill-course-from-submitting-regular-practice-assignmentsをローカルに取り込むgrant-courseでログインするScreenshot
変更前
変更後
Summary by CodeRabbit
リリースノート
Flakewatch Report
Review App