プラクティスの難易度をアイコンで表示する#9918
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コース演習テンプレートが学習統計を条件付きで表示するように変更され、ユーザーロール（studentか否か）で表示内容を分岐。学習時間を絵文字で表す Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 |
![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
🧹 Nitpick comments (1)
app/helpers/reports_helper.rb (1)
37-49: 表示責務はDecoratorへ寄せることを検討してくださいLine 37-49 の難易度表示ロジックは
Practiceの表示責務に近いため、PracticeDecoratorへ移すとHelperの肥大化を抑えつつ再利用しやすくなります。As per coding guidelines
ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/helpers/reports_helper.rb` around lines 37 - 49, The difficulty_icon helper contains view/display logic tied to a Practice instance (it reads practice.learning_minute_statistic and computes median-based emoji output); move this logic into a PracticeDecorator (e.g., add an instance method like difficulty_icon or difficulty_emojis on PracticeDecorator that reads learning_minute_statistic, handles nil, computes median_hours and returns the same emoji strings), then change the helper method difficulty_icon to delegate to practice.decorate.difficulty_icon (or simply remove the helper and call practice.decorate.difficulty_icon from views) so the view helper stays thin and display responsibility lives in the decorator.
🤖 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/views/courses/practices/_courses_practice.html.slim`:
- Around line 22-31: The template renders the learning-time container
unconditionally which leaves an empty element when
practice.learning_minute_statistic is nil; update the conditional so the
surrounding container is only rendered when practice.learning_minute_statistic
is present: for the student branch check practice.learning_minute_statistic
before rendering .category-practices-item__learning-time and show
difficulty_icon(practice) inside it, and for the mentor branch wrap
.category-practices-item__learning-time.is-only-mentor with a presence check and
then assign learning_minute_statistic = practice.learning_minute_statistic and
call convert_to_hour_minute(learning_minute_statistic.median/average) inside
that block.
---
Nitpick comments:
In `@app/helpers/reports_helper.rb`:
- Around line 37-49: The difficulty_icon helper contains view/display logic tied
to a Practice instance (it reads practice.learning_minute_statistic and computes
median-based emoji output); move this logic into a PracticeDecorator (e.g., add
an instance method like difficulty_icon or difficulty_emojis on
PracticeDecorator that reads learning_minute_statistic, handles nil, computes
median_hours and returns the same emoji strings), then change the helper method
difficulty_icon to delegate to practice.decorate.difficulty_icon (or simply
remove the helper and call practice.decorate.difficulty_icon from views) so the
view helper stays thin and display responsibility lives in the decorator.
🪄 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: ef3c0aa3-a16e-4840-90b5-be33d4d8c09b
📒 Files selected for processing (2)
app/helpers/reports_helper.rbapp/views/courses/practices/_courses_practice.html.slim
🚀 Review AppURL: https://bootcamp-pr-9918-fvlfu45apq-an.a.run.app
|
karlley
force-pushed
the
feature/practice-difficulty-icon
branch
2 times, most recently
from
953c7c4 to
1af571d
Compare
karlley
force-pushed
the
feature/practice-difficulty-icon
branch
from
1af571d to
e5e274e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/helpers/reports_helper.rb (3)
37-48: Decoratorパターンの検討コーディングガイドラインでは「ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する（active_decorator gem）」とされています。この
difficulty_iconはLearningMinuteStatistic#median（あるいはPracticeのプラクティス難易度）に紐づく表示ロジックなので、LearningMinuteStatisticDecorator#difficulty_iconとして実装する方がオブジェクト指向的に整理され、ビュー側もlearning_stat.difficulty_iconで呼び出せて読みやすくなります。As per coding guidelines: 「ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）」
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/helpers/reports_helper.rb` around lines 37 - 48, The difficulty_icon method in reports_helper.rb should be moved into a decorator: implement LearningMinuteStatisticDecorator#difficulty_icon (and similarly for Practice if needed) so the presentation logic is tied to the model; remove difficulty_icon from the helper and update views to call learning_stat.difficulty_icon (or practice.difficulty_icon). Keep the same behavior (handle nil minutes, compute hours from minutes and map ranges to 1–5 fire icons) and ensure the decorator is picked up by active_decorator (class name LearningMinuteStatisticDecorator, method difficulty_icon) so existing view calls use the new decorator method.
37-48: 難易度計算ロジックの簡素化とアクセシビリティの考慮ロジック自体は動作しますが、以下の点を検討してください。
- 5時間刻みで段階が増える規則的な構造なので、
caseで5分岐を列挙するより算術で表現した方が簡潔です（将来的に段階数が変わっても保守しやすい）。- 炎絵文字のみを返す実装は、スクリーンリーダー利用者に「難易度」という意味が伝わりません。ビュー側で
aria-label等を付与していない場合、ヘルパー側で<span aria-label="難易度: 中">🔥🔥</span>のようなマークアップを返すか、呼び出し側で補助テキストを併記することを推奨します。minutesが負値の場合、現状は最大難易度（🔥×5）に分類されます。想定外入力に対する扱いを明確化しておくと安全です。♻️ 算術による簡素化の例
def difficulty_icon(minutes) return '' if minutes.nil? - hours = (minutes / 60).floor - case hours - when 0..5 then '🔥' - when 6..10 then '🔥🔥' - when 11..15 then '🔥🔥🔥' - when 16..20 then '🔥🔥🔥🔥' - else '🔥🔥🔥🔥🔥' - end + hours = (minutes / 60).floor + level = [hours / 5 + 1, 5].min + level = 1 if level < 1 + '🔥' * level end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/helpers/reports_helper.rb` around lines 37 - 48, Refactor difficulty_icon to compute level arithmetically (e.g., clamp ((minutes / 60).floor / 5).ceil or integer division) instead of the explicit case branches so it's easy to change step size/levels; ensure negative or nil minutes are handled explicitly (return '' for nil and clamp negatives to 0 or treat as invalid) and map level to repeated '🔥' up to max 5; return accessible markup (e.g., wrap the emoji string in a span with an appropriate aria-label like "難易度: #{label_for_level(level)}") so screen readers get semantic information; keep the helper name difficulty_icon and create or use a small helper to produce the human-readable label for aria-label.
40-40:.floorは冗長です
minutesがInteger（LearningMinuteStatistic#medianは整数のminute値）であれば、minutes / 60は既に整数除算で切り捨てされるため.floorは不要です。Floatが渡る可能性を考慮しているなら意図が分かるようコメントしておくと親切ですが、通常はシンプルにminutes / 60で十分です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/helpers/reports_helper.rb` at line 40, The use of .floor on the expression computing hours is redundant because minutes is an Integer (LearningMinuteStatistic#median returns integer minutes), so replace "hours = (minutes / 60).floor" with the simpler "hours = minutes / 60" (or if you expect minutes may be Float, keep .floor but add a brief comment explaining that floats are possible); update the code in reports_helper.rb at the hours calculation (variable name minutes and hours) accordingly.
🤖 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/helpers/reports_helper.rb`:
- Around line 37-48: The difficulty_icon method in reports_helper.rb should be
moved into a decorator: implement
LearningMinuteStatisticDecorator#difficulty_icon (and similarly for Practice if
needed) so the presentation logic is tied to the model; remove difficulty_icon
from the helper and update views to call learning_stat.difficulty_icon (or
practice.difficulty_icon). Keep the same behavior (handle nil minutes, compute
hours from minutes and map ranges to 1–5 fire icons) and ensure the decorator is
picked up by active_decorator (class name LearningMinuteStatisticDecorator,
method difficulty_icon) so existing view calls use the new decorator method.
- Around line 37-48: Refactor difficulty_icon to compute level arithmetically
(e.g., clamp ((minutes / 60).floor / 5).ceil or integer division) instead of the
explicit case branches so it's easy to change step size/levels; ensure negative
or nil minutes are handled explicitly (return '' for nil and clamp negatives to
0 or treat as invalid) and map level to repeated '🔥' up to max 5; return
accessible markup (e.g., wrap the emoji string in a span with an appropriate
aria-label like "難易度: #{label_for_level(level)}") so screen readers get semantic
information; keep the helper name difficulty_icon and create or use a small
helper to produce the human-readable label for aria-label.
- Line 40: The use of .floor on the expression computing hours is redundant
because minutes is an Integer (LearningMinuteStatistic#median returns integer
minutes), so replace "hours = (minutes / 60).floor" with the simpler "hours =
minutes / 60" (or if you expect minutes may be Float, keep .floor but add a
brief comment explaining that floats are possible); update the code in
reports_helper.rb at the hours calculation (variable name minutes and hours)
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86296760-f5fb-4550-8120-89483af8be1c
📒 Files selected for processing (2)
app/helpers/reports_helper.rbapp/views/courses/practices/_courses_practice.html.slim
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/courses/practices/_courses_practice.html.slim
karlley
force-pushed
the
feature/practice-difficulty-icon
branch
3 times, most recently
from
0ebe0ad to
fbb941b
Compare
karlley
force-pushed
the
feature/practice-difficulty-icon
branch
4 times, most recently
from
c72d6af to
95679c9
Compare
|
@machida |
|
@karlley ありがとうございます！！ |
machida
force-pushed
the
feature/practice-difficulty-icon
branch
from
95679c9 to
0cb9f6d
Compare
|
@karlley 遅くなってすいません🙇♂️デザインの調整をしましたー。 難易度は全員に表示、 最新の main から rebase しました。 |
|
@machida |
|
@kitarou888 |
|
@karlley |
|
@zamami |
|
@karlley |
|
@zamami |
|
@zamami @okuramasafumi |
| end | ||
|
|
||
| def difficulty_icon(minutes) | ||
| return '' if minutes.nil? |
There was a problem hiding this comment.
ここのEarly Returnですが、下のcase文の中に入れ込んでしまえそうです。nil.to_iの結果が0であることを利用できそう。
There was a problem hiding this comment.
nil.to_iの結果が0であることを利用できそう
こちら利用することでnilを考慮する必要が無くなり、結果としてEarly Returnが不要になると解釈し修正しました。
認識正しかったでしょうか？
f72059f
There was a problem hiding this comment.
@karlley 惜しいです！テストケースを削除するということは振る舞いが変わってしまうということになるので、望ましくないです。
Early Returnを削除しつつ、既存のテストケースを維持するためには、
case minutes.to_i
when 0 then ''
when 1..100 thenのようにするとうまくいくと思います。
There was a problem hiding this comment.
確認ありがとうございます！
テストケースを削除するということは振る舞いが変わってしまうということになるので、望ましくないです。
確かにそうですね。テストは削除しない方向に修正いたします。
テストケースを維持したまま修正する方法についてですが、when 0 then '' のようにしてしまうと学習時間に応じたアイコン数の仕様を満たせなくなってしまうように思えます。
issueのコメントで確認した限りでは0時間はアイコン数は1つ表示になっています。
5時間🔥 → 0-5時間まで
10時間🔥🔥 → 6-10時間まで
15時間🔥🔥🔥 → 11-15時間まで
20時間🔥🔥🔥🔥 → 16-20時間まで
21時間以上🔥🔥🔥🔥🔥 → 21時間以上
ただ、0時間をどうするかという点については具体的に確認できていないので町田さんに仕様を確認した上で修正を進めようと思いましたがいかがでしょうか？
There was a problem hiding this comment.
@karlley お、なるほどです。0時間というものがあるなら、前提条件が変わってくると思います。町田さんに確認するのはよいと思います！
なお、Rubyのcase文はあくまで===という名前のメソッドを使っているだけなので、to_iを使わないでnilと比較してしまえば0時間との区別は付けられそうです。
There was a problem hiding this comment.
@machida
お疲れさまです！
今さらですがアイコンの表示数について仕様を確認させてください。
5時間🔥 → 0-5時間まで
こちらのコメントで学習時間が0-5時間はアイコン1つにすると確認させて頂きましたが、0時間の場合もアイコンは表示するという認識で間違いないでしょうか？
#9328 (comment)
アイコン表示処理のリファクタリングを行っており、0時間をどう扱うかによって方向性が変わるため念の為確認させていただきました。
ご確認お願いします！
There was a problem hiding this comment.
@karlley 0時間は誰もまだやっていない場合に0時間になります。ただ、誰もやっていないということは0時間ではなく、まだわからないということなので、0時間の場合はデータ収集中のような表示であるべきだと思いました。
データ収集中と表示しましょう。
There was a problem hiding this comment.
There was a problem hiding this comment.
@okuramasafumi
0時間の仕様確認した上で修正しました。
確認お願いします！
e405b80
|
@karlley レビューしました！全体的に良い感じですが、一カ所だけコメントしてあるのでご確認よろしくお願いします！ |
karlley
force-pushed
the
feature/practice-difficulty-icon
branch
from
0cb9f6d to
256c043
Compare
|
@okuramasafumi @komagata |
|
@karlley マージしました〜 |
6 participants
Issue
概要
プラクティスの難易度をアイコンで表示する
0時間時のアイコン表示は「🔥」から「データ収集中」に仕様変更になった
#9918 (comment)
変更確認方法
事前準備
feature/practice-difficulty-iconブランチをローカルに取り込むbundle exec rake bootcamp:statistics:save_learning_minute_statisticsを実行(難易度/学習時間の統計情報表示用のデータを用意)管理者、メンター以外
kimuraでログインOS X Mountain Lionをクリーンインストールするのプラクティスで以下を確認管理者
komagataでログインOS X Mountain Lionをクリーンインストールするのプラクティスで以下を確認メンター
mentormentaroでログインOS X Mountain Lionをクリーンインストールするのプラクティスで以下を確認Screenshot
変更前
管理者、メンター以外
管理者
メンター
変更後
管理者、メンター以外
管理者
メンター
Summary by CodeRabbit
Flakewatch Report
Review App