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レポートの Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller as ReportsController
participant Report
participant DB as Database
User->>Controller: GET /reports/new
Controller->>Report: Report.new (reported_on を設定)
Controller->>DB: current_user.learning_time_frames.where(weekday)
DB-->>Controller: learning_time_frames
Controller->>Controller: 最小 activity_time を算出 (min_hour)
alt min_hour が存在して報告日時 at min_hour が未来でない
Controller->>Report: build learning_time started_at = reported_on at min_hour:00 (app zone)
else
Controller->>Report: build learning_time started_at = Time.current.change(min: 0)
end
Controller-->>User: 新規フォームをレンダー（デフォルトセレクト付き）
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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)
📝 Coding Plan
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 |
y-kawahara-gs
force-pushed
the
feature/chenge-default-value-for-report-learning-start-time-to-learning-time-frame
branch
2 times, most recently
from
eb9230c to
6eac307
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
🧹 Nitpick comments (2)
test/system/reports/create_test.rb (2)
64-67: フィクスチャのID計算ロジックにコメントを追加することを推奨します。
frame_id = one_hour_ago.wday * 24 + one_hour_ago.hour + 1の計算はLearningTimeFrameフィクスチャの特定のID体系に依存しています。将来のメンテナンス性のため、この計算式の意図を説明するコメントがあると良いでしょう。📝 提案：コメントの追加
user = users(:kimura) one_hour_ago = 1.hour.ago + # LearningTimeFrame fixtures are numbered by: wday * 24 + hour + 1 frame_id = one_hour_ago.wday * 24 + one_hour_ago.hour + 1 frame = LearningTimeFrame.find(frame_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/system/reports/create_test.rb` around lines 64 - 67, frame_id の計算式「frame_id = one_hour_ago.wday * 24 + one_hour_ago.hour + 1」は LearningTimeFrame フィクスチャの ID が「(曜日(0..6) * 24) + 時間(0..23) + 1」のルールで作られていることに依存しているので、LearningTimeFrame と LearningTimeFramesUser を扱う箇所（変数名 frame_id / frame やクラス名 LearningTimeFrame）にその意図を短くコメントで明記してください；可能なら同じ計算を使う箇所で再利用できる説明付きのヘルパー名（例: to_learning_time_frame_id）への置換を検討する旨もコメントに追記してください。
78-78: テスト名が実際のテスト内容と一致していません。「learning start time is earlier than scheduled time」という表現は意味が曖昧です。このテストは「現在時刻が学習予定時刻より前の場合、デフォルト値を設定しない」ことを確認しています。
♻️ 提案：より明確なテスト名
- test 'sets current time as default when learning start time is earlier than scheduled time' do + test 'does not set default learning start time when scheduled time is in the future' do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/system/reports/create_test.rb` at line 78, Rename the misleading test name string "sets current time as default when learning start time is earlier than scheduled time" to accurately describe the assertion (for example: "does not set current time as default when scheduled learning start is in the future" or "does not override scheduled start when scheduled time is after now") in the test defined as test '...' so the name matches the behavior being asserted; update any related assertion messages or comments in the same test to match the new name (the test block starting with the existing test string should be the one you edit).
🤖 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/reports_controller.rb`:
- Around line 185-195: The code in build_learning_times uses
I18n.t('date.abbr_day_names') which returns locale-dependent weekday names and
causes queries against LearningTimeFrame.week_day to fail; change the
reported_weekday computation to use the model's Japanese weekday constant (e.g.
LearningTimeFrame::WEEK_DAY_NAMES_JA[report.reported_on.wday]) so it matches the
values used by LearningTimeFrame and the active_now scope, then keep the rest of
build_learning_times (min_hour lookup, attributes setting, and
report.learning_times.build) unchanged.
---
Nitpick comments:
In `@test/system/reports/create_test.rb`:
- Around line 64-67: frame_id の計算式「frame_id = one_hour_ago.wday * 24 +
one_hour_ago.hour + 1」は LearningTimeFrame フィクスチャの ID が「(曜日(0..6) * 24) +
時間(0..23) + 1」のルールで作られていることに依存しているので、LearningTimeFrame と LearningTimeFramesUser
を扱う箇所（変数名 frame_id / frame やクラス名
LearningTimeFrame）にその意図を短くコメントで明記してください；可能なら同じ計算を使う箇所で再利用できる説明付きのヘルパー名（例:
to_learning_time_frame_id）への置換を検討する旨もコメントに追記してください。
- Line 78: Rename the misleading test name string "sets current time as default
when learning start time is earlier than scheduled time" to accurately describe
the assertion (for example: "does not set current time as default when scheduled
learning start is in the future" or "does not override scheduled start when
scheduled time is after now") in the test defined as test '...' so the name
matches the behavior being asserted; update any related assertion messages or
comments in the same test to match the new name (the test block starting with
the existing test string should be the one you edit).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 72fd57f3-6c4a-4efd-9622-ce5dd3e77124
📒 Files selected for processing (2)
app/controllers/reports_controller.rbtest/system/reports/create_test.rb
y-kawahara-gs
force-pushed
the
feature/chenge-default-value-for-report-learning-start-time-to-learning-time-frame
branch
from
6eac307 to
d7fb6b2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/system/reports/create_test.rb (1)
78-78: テスト名が実際のテスト内容と一致していません。現在のテスト名「sets current time as default when learning start time is earlier than scheduled time」は、テストしている内容と逆の意味に読めます。
実際のテスト内容は「学習予定時間が現在時刻より未来の場合、現在時刻をデフォルトにする」です。
♻️ テスト名の修正案
- test 'sets current time as default when learning start time is earlier than scheduled time' do + test 'sets current time as default when scheduled learning time is in the future' do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/system/reports/create_test.rb` at line 78, Rename the test description string in the test definition that currently reads "sets current time as default when learning start time is earlier than scheduled time" to accurately reflect the tested behavior (e.g., "sets current time as default when scheduled learning time is in the future" or "uses current time as default when learning start is after now") in the test located in create_test.rb (the test block starting with test '<that string>' do); keep the test body and assertions unchanged—only update the human-readable test name to match the actual behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/system/reports/create_test.rb`:
- Line 78: Rename the test description string in the test definition that
currently reads "sets current time as default when learning start time is
earlier than scheduled time" to accurately reflect the tested behavior (e.g.,
"sets current time as default when scheduled learning time is in the future" or
"uses current time as default when learning start is after now") in the test
located in create_test.rb (the test block starting with test '<that string>'
do); keep the test body and assertions unchanged—only update the human-readable
test name to match the actual behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33533548-e5fd-4c8a-8e3a-c0f63c207863
📒 Files selected for processing (2)
app/controllers/reports_controller.rbtest/system/reports/create_test.rb
y-kawahara-gs
force-pushed
the
feature/chenge-default-value-for-report-learning-start-time-to-learning-time-frame
branch
from
d7fb6b2 to
78d0075
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/system/reports/create_test.rb (1)
64-67:LearningTimeFrame取得をID算出依存から条件検索に寄せると、テストが壊れにくくなります。
wday * 24 + hour + 1前提は fixture のID構造に依存しやすいので、week_dayとactivity_timeでfind_by!する方が意図が明確です。あわせて1.hourは1.hoursに統一すると方針に揃います。♻️ 提案差分
- one_hour_ago = 1.hour.ago - frame_id = one_hour_ago.wday * 24 + one_hour_ago.hour + 1 - frame = LearningTimeFrame.find(frame_id) + one_hour_ago = 1.hours.ago + weekday = LearningTimeFrame::WEEK_DAY_NAMES_JA[one_hour_ago.wday] + frame = LearningTimeFrame.find_by!(week_day: weekday, activity_time: one_hour_ago.hour) LearningTimeFramesUser.create!(user: user, learning_time_frame: frame) @@ - one_hour_since = 1.hour.since - frame_id = one_hour_since.wday * 24 + one_hour_since.hour + 1 - frame = LearningTimeFrame.find(frame_id) + one_hour_since = 1.hours.since + weekday = LearningTimeFrame::WEEK_DAY_NAMES_JA[one_hour_since.wday] + frame = LearningTimeFrame.find_by!(week_day: weekday, activity_time: one_hour_since.hour) LearningTimeFramesUser.create!(user: user, learning_time_frame: frame)Based on learnings: In this Ruby project, prefer Numeric#hours (plural) for adding time durations, and test assertions should rely on data under
test/fixturesrather than assumptions tied to other fixture/seed structures.Also applies to: 81-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/system/reports/create_test.rb` around lines 64 - 67, Replace the brittle ID computation for LearningTimeFrame with a predicate lookup: compute one_hours_ago = 1.hours.ago, extract week_day and activity_time from that time, then locate the frame via LearningTimeFrame.find_by!(week_day: ..., activity_time: ...), and create the association with LearningTimeFramesUser as before; also change the duration literal from 1.hour to 1.hours so the test matches project conventions and avoids relying on fixture ID layout (refer to symbols: one_hour_ago / frame_id / LearningTimeFrame / LearningTimeFramesUser).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/system/reports/create_test.rb`:
- Around line 64-67: Replace the brittle ID computation for LearningTimeFrame
with a predicate lookup: compute one_hours_ago = 1.hours.ago, extract week_day
and activity_time from that time, then locate the frame via
LearningTimeFrame.find_by!(week_day: ..., activity_time: ...), and create the
association with LearningTimeFramesUser as before; also change the duration
literal from 1.hour to 1.hours so the test matches project conventions and
avoids relying on fixture ID layout (refer to symbols: one_hour_ago / frame_id /
LearningTimeFrame / LearningTimeFramesUser).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0426dbc0-9126-46c7-821f-1dc99232f46e
📒 Files selected for processing (2)
app/controllers/reports_controller.rbtest/system/reports/create_test.rb
y-kawahara-gs
force-pushed
the
feature/chenge-default-value-for-report-learning-start-time-to-learning-time-frame
branch
from
78d0075 to
5fe5f47
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/system/reports/create_test.rb (3)
81-90: 同様の変数シャドーイングがあります。Line 84 で
hour = one_hour_since.hourとして代入後、Line 90 で再代入しています。上記と同様に変数名を分けることを推奨します。♻️ 変数名を明確に分ける提案
user = users(:kimura) one_hour_since = 1.hour.since week_day = LearningTimeFrame::WEEK_DAY_NAMES_JA[one_hour_since.wday] - hour = one_hour_since.hour - frame = LearningTimeFrame.find_by!(week_day: week_day, activity_time: hour) + activity_hour = one_hour_since.hour + frame = LearningTimeFrame.find_by!(week_day: week_day, activity_time: activity_hour) LearningTimeFramesUser.create!(user: user, learning_time_frame: frame) visit_with_auth '/reports/new', 'kimura' - hour, min = learning_start_time_select_values + selected_hour, selected_min = learning_start_time_select_values - assert_equal '10', hour - assert_equal '00', min + assert_equal '10', selected_hour + assert_equal '00', selected_min🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/system/reports/create_test.rb` around lines 81 - 90, The test has variable shadowing: you assign hour = one_hour_since.hour and later reassign hour, min = learning_start_time_select_values; rename one of them to avoid collision (e.g., frame_hour or expected_hour) so both values are preserved. Update the assignment near LearningTimeFrame.find_by! (replace hour with a distinct name like frame_hour) and adjust any uses that reference that original value (e.g., LearningTimeFrame.find_by!(week_day: week_day, activity_time: frame_hour)) so the call to learning_start_time_select_values can safely unpack into hour, min.
63-72: 変数hourのシャドーイングにより混乱を招きます。Line 66 で
hour = one_hour_ago.hour（整数）として代入した後、Line 72 でhour, min = learning_start_time_select_values（文字列）として再代入しています。異なる目的で同じ変数名を使用すると、コードの可読性が低下し、将来的なバグの原因になり得ます。♻️ 変数名を明確に分ける提案
user = users(:kimura) one_hour_ago = 1.hour.ago week_day = LearningTimeFrame::WEEK_DAY_NAMES_JA[one_hour_ago.wday] - hour = one_hour_ago.hour - frame = LearningTimeFrame.find_by!(week_day: week_day, activity_time: hour) + activity_hour = one_hour_ago.hour + frame = LearningTimeFrame.find_by!(week_day: week_day, activity_time: activity_hour) LearningTimeFramesUser.create!(user: user, learning_time_frame: frame) visit_with_auth '/reports/new', 'kimura' - hour, min = learning_start_time_select_values + selected_hour, selected_min = learning_start_time_select_values - assert_equal '09', hour - assert_equal '00', min + assert_equal '09', selected_hour + assert_equal '00', selected_min🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/system/reports/create_test.rb` around lines 63 - 72, The variable hour is shadowed: it’s set as one_hour_ago.hour then later reassigned from learning_start_time_select_values; rename one of them to avoid confusion (e.g., change the first hour to frame_hour when computing week_day/hour used to find LearningTimeFrame via LearningTimeFrame.find_by! and pass that to LearningTimeFramesUser.create!, or change the tuple unpack to start_hour, start_min to receive learning_start_time_select_values) and update any subsequent uses (references in the frame lookup and LearningTimeFramesUser.create! or later code that expects the selected start time) accordingly to keep names distinct and self-descriptive.
79-79: テスト名が実際のシナリオと逆になっています。テスト名は「learning start time is earlier than scheduled time」（学習開始時刻が予定時刻より早い場合）となっていますが、実際のテストシナリオは「現在時刻（10:00）が予定時刻（11:00）より早い」つまり「予定時刻が未来の場合」をテストしています。
♻️ テスト名の修正案
- test 'sets current time as default when learning start time is earlier than scheduled time' do + test 'sets current time as default when scheduled time is in the future' do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/system/reports/create_test.rb` at line 79, The test name is inverted: replace the test description string "sets current time as default when learning start time is earlier than scheduled time" with a name that reflects the actual scenario being tested (current time 10:00 is earlier than scheduled time 11:00). Update the test declaration in create_test.rb (the test '...' line) to something like "sets scheduled time as default when scheduled time is later than current time" so the description matches the test body and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/system/reports/create_test.rb`:
- Around line 81-90: The test has variable shadowing: you assign hour =
one_hour_since.hour and later reassign hour, min =
learning_start_time_select_values; rename one of them to avoid collision (e.g.,
frame_hour or expected_hour) so both values are preserved. Update the assignment
near LearningTimeFrame.find_by! (replace hour with a distinct name like
frame_hour) and adjust any uses that reference that original value (e.g.,
LearningTimeFrame.find_by!(week_day: week_day, activity_time: frame_hour)) so
the call to learning_start_time_select_values can safely unpack into hour, min.
- Around line 63-72: The variable hour is shadowed: it’s set as
one_hour_ago.hour then later reassigned from learning_start_time_select_values;
rename one of them to avoid confusion (e.g., change the first hour to frame_hour
when computing week_day/hour used to find LearningTimeFrame via
LearningTimeFrame.find_by! and pass that to LearningTimeFramesUser.create!, or
change the tuple unpack to start_hour, start_min to receive
learning_start_time_select_values) and update any subsequent uses (references in
the frame lookup and LearningTimeFramesUser.create! or later code that expects
the selected start time) accordingly to keep names distinct and
self-descriptive.
- Line 79: The test name is inverted: replace the test description string "sets
current time as default when learning start time is earlier than scheduled time"
with a name that reflects the actual scenario being tested (current time 10:00
is earlier than scheduled time 11:00). Update the test declaration in
create_test.rb (the test '...' line) to something like "sets scheduled time as
default when scheduled time is later than current time" so the description
matches the test body and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9521e30f-8f4a-4e7c-81d4-3f4911ac24ba
📒 Files selected for processing (2)
app/controllers/reports_controller.rbtest/system/reports/create_test.rb
y-kawahara-gs
force-pushed
the
feature/chenge-default-value-for-report-learning-start-time-to-learning-time-frame
branch
from
5fe5f47 to
e448a38
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/system/reports/create_test.rb (1)
79-95: テスト名の表現がやや分かりにくいです。テストの内容は「学習予定時刻が現在時刻より未来の場合、現在時刻をデフォルトとして使用する」ですが、テスト名
sets current time as default when learning start time is earlier than scheduled timeは「learning start time（結果）が scheduled time より早い場合」と読め、因果関係が逆に感じられます。PR の説明にある「学習予定時間の開始が日報作成時刻より遅い場合」に沿った表現にすると、より意図が伝わりやすくなります。
📝 テスト名の改善案
- test 'sets current time as default when learning start time is earlier than scheduled time' do + test 'does not use learning time frame as default when scheduled time is in the future' doまたは日本語で PR 説明に合わせると：
- test 'sets current time as default when learning start time is earlier than scheduled time' do + test 'uses current time as default when learning time frame starts after current time' do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/system/reports/create_test.rb` around lines 79 - 95, Rename the test description string to clearly express that when the scheduled learning start time is later than the current (report creation) time, the form should default to the current time; update the test starting with test 'sets current time as default when learning start time is earlier than scheduled time' to something like "sets current time as default when scheduled learning start time is later than current time" (or a Japanese equivalent matching the PR description) so the intent matches the behavior exercised by learning_start_time_select_values and the surrounding setup that creates a future LearningTimeFrame for users(:kimura).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/system/reports/create_test.rb`:
- Around line 79-95: Rename the test description string to clearly express that
when the scheduled learning start time is later than the current (report
creation) time, the form should default to the current time; update the test
starting with test 'sets current time as default when learning start time is
earlier than scheduled time' to something like "sets current time as default
when scheduled learning start time is later than current time" (or a Japanese
equivalent matching the PR description) so the intent matches the behavior
exercised by learning_start_time_select_values and the surrounding setup that
creates a future LearningTimeFrame for users(:kimura).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3fb657fc-3c54-4b0c-a431-32811a72ff03
📒 Files selected for processing (1)
test/system/reports/create_test.rb
y-kawahara-gs
force-pushed
the
feature/chenge-default-value-for-report-learning-start-time-to-learning-time-frame
branch
from
e448a38 to
f04159a
Compare
Contributor
Author
|
@kutimiti1234 |
Contributor
|
@y-kawahara-gs |
Contributor
Author
|
@matuaya |
Contributor
|
@y-kawahara-gs |
Contributor
Author
|
@matuaya |
matuaya
approved these changes
Mar 29, 2026
Contributor
matuaya
left a comment
matuaya
left a comment
There was a problem hiding this comment.
動作バッチリでした！
Approveさせていただきます
Contributor
Author
|
@komagata |
komagata
deleted the
feature/chenge-default-value-for-report-learning-start-time-to-learning-time-frame
branch
9 tasks
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
4 participants
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
概要
日報作成時、そのユーザーが作成する曜日に学習予定時間を設定している場合、その学習予定時間の始めの時間を日報の学習時間のデフォルト値として設定する。
変更確認方法
Screenshot
変更前
変更後
Summary by CodeRabbit
新機能
テスト