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:
📝 WalkthroughWalkthroughEvent/RegularEventのスコープを Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/models/regular_event_test.rb (1)
176-183:⚠️ Potential issue | 🔴 Critical
RegularEvent.upcomingメソッドが未定義です179行目で
RegularEvent.upcoming(tomorrow)を呼び出していますが、app/models/regular_event.rbにはupcomingスコープが定義されていません。定義されているのはupcoming_fromのみです。このテストは
NoMethodErrorで失敗します。🐛 修正案：upcoming_fromを使用する
test '.upcoming in tomorrow's event' do travel_to Time.zone.local(2024, 12, 1, 10, 0, 0) do tomorrow = Time.zone.tomorrow - regular_events = RegularEvent.upcoming(tomorrow) + regular_events = RegularEvent.upcoming_from(tomorrow) regular_event_scheduled_for_tomorrow = regular_events(:regular_event38) assert_includes regular_events, regular_event_scheduled_for_tomorrow end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/regular_event_test.rb` around lines 176 - 183, The test calls RegularEvent.upcoming(tomorrow) but there is no upcoming scope on RegularEvent; replace that call with the existing scope/method RegularEvent.upcoming_from(tomorrow) (or alternatively add an alias scope def scope :upcoming, ->(date) { upcoming_from(date) } in the RegularEvent model) so the test uses a defined method (referenced symbols: RegularEvent.upcoming, RegularEvent.upcoming_from, scope :upcoming).
🤖 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/models/regular_event.rb`:
- Line 54: The test calls RegularEvent.upcoming(date) but only scope
:upcoming_from is defined; add a new scope named upcoming that delegates to
upcoming_from (e.g., define scope :upcoming, ->(date) { upcoming_from(date) })
so calls to RegularEvent.upcoming(date) work; update the RegularEvent model by
adding this scope (referencing the existing upcoming_from scope and the
RegularEvent.upcoming symbol).
---
Outside diff comments:
In `@test/models/regular_event_test.rb`:
- Around line 176-183: The test calls RegularEvent.upcoming(tomorrow) but there
is no upcoming scope on RegularEvent; replace that call with the existing
scope/method RegularEvent.upcoming_from(tomorrow) (or alternatively add an alias
scope def scope :upcoming, ->(date) { upcoming_from(date) } in the RegularEvent
model) so the test uses a defined method (referenced symbols:
RegularEvent.upcoming, RegularEvent.upcoming_from, scope :upcoming).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 926d6618-1a66-4b66-b739-f21c9b111e0f
📒 Files selected for processing (8)
app/models/event.rbapp/models/regular_event.rbapp/models/upcoming_event.rbtest/fixtures/events.ymltest/fixtures/regular_event_repeat_rules.ymltest/fixtures/regular_events.ymltest/models/event_test.rbtest/models/regular_event_test.rb
s-tone-gs
force-pushed
the
bug/upcoming-events-section-displays-WIP-events
branch
from
106d2ea to
74ab45b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
db/fixtures/regular_events.yml (1)
143-144:Time.current依存をfixtureから外した方が安全です。Line 143 と Line 144 は実行時刻に依存するため、fixtureの再現性が下がります。fixtureは固定時刻にし、未来/過去の条件はテスト側で時刻固定して検証する構成が安定します。
🔧 提案差分
- start_at: <%= Time.current + 1.day %> - end_at: <%= Time.current + 1.day + 1.hours %> + start_at: <%= Time.zone.local(2020, 1, 1, 21, 0, 0) %> + end_at: <%= Time.zone.local(2020, 1, 1, 22, 0, 0) %>Based on learnings: In YAML fixture files under db/fixtures, using fixed past timestamps (e.g., Time.zone.parse('2025-01-01 00:00:00')) can help test reproducibility. This is acceptable; ensure tests remain deterministic by avoiding reliance on current time, and document the chosen fixed values if they influence test outcomes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/fixtures/regular_events.yml` around lines 143 - 144, Replace the runtime-dependent Time.current expressions in the fixture fields start_at and end_at with fixed, deterministic timestamps (e.g., use Time.zone.parse('2025-01-01 00:00:00') for start_at and add 1.hour for end_at) so the YAML fixture is reproducible; update the two keys start_at and end_at accordingly and document the chosen fixed timestamp in the fixture comment if it affects tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db/fixtures/regular_event_repeat_rules.yml`:
- Around line 94-97:
YAMLで同じキー名「regular_event_repeat_rule35」が2回定義されているため最初の定義が上書きされている問題を修正してください：2つ目のエントリ（現在
regular_event_repeat_rule35 として regular_event31 を指すブロック）をユニークなキー名（例:
regular_event_repeat_rule36）に変更し、必要ならそれを参照する関連フィクスチャや参照先（regular_event30 /
regular_event31 の関連付け）を合わせて更新して重複を解消してください。
---
Nitpick comments:
In `@db/fixtures/regular_events.yml`:
- Around line 143-144: Replace the runtime-dependent Time.current expressions in
the fixture fields start_at and end_at with fixed, deterministic timestamps
(e.g., use Time.zone.parse('2025-01-01 00:00:00') for start_at and add 1.hour
for end_at) so the YAML fixture is reproducible; update the two keys start_at
and end_at accordingly and document the chosen fixed timestamp in the fixture
comment if it affects tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a80f6d7-f98d-41c6-b54c-768ca5af6f3f
📒 Files selected for processing (12)
app/models/event.rbapp/models/regular_event.rbapp/models/upcoming_event.rbdb/fixtures/events.ymldb/fixtures/regular_event_repeat_rules.ymldb/fixtures/regular_events.ymltest/fixtures/events.ymltest/fixtures/regular_event_repeat_rules.ymltest/fixtures/regular_events.ymltest/models/event_test.rbtest/models/regular_event_test.rbtest/system/regular_events_test.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- test/models/event_test.rb
- test/models/regular_event_test.rb
s-tone-gs
force-pushed
the
bug/upcoming-events-section-displays-WIP-events
branch
from
14f844a to
9ad9801
Compare
|
@machida こちらですが、バグとして新しくissueを立て、修正自体はこのPRの中で同時にやってしまおうかと考えています。 状況としては、今回作成したWIPの定期イベントのテストデータが開催中の定期イベントとしてカウントされてしまい、開催中の定期イベントの数が増えて以下のようにテストが落ちています。 こちらはテストではなく機能を修正するべき案件だと判断したのですが、バグとして新しくissueを立て、修正自体はこのPRの中で同時にやってしまう、という方向で作業を進めても良いでしょうか？？ |
|
@s-tone-gs それについては、Issueは特に追加せず、同じPRで作業してしまって大丈夫です。追加仕様としてPRに詳細を追記していただけたらと思います。 実際、作って初めて気がつくことってよくあるんですよね。それも含めないと完成にならずリリースができないので、一つのIssueでお願いします🙏 |
|
@machida 承知しました！ありがとうございます！ |
s-tone-gs
force-pushed
the
bug/upcoming-events-section-displays-WIP-events
branch
from
9ad9801 to
da3b89e
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/notification/regular_events_test.rb`:
- Around line 41-42: The two boolean checks on the message body are not
combined, so body['content'].exclude?(...) is never evaluated; update the test
assertion that uses body['content'].include?('Discord通知確認用、祝日非開催イベント(金曜日 +
土曜日開催)') and body['content'].exclude?('Discord通知が来ないwipイベント(土曜日開催)') so they are
joined with a logical AND (e.g., add the missing && between the include? and
exclude? calls) to ensure both conditions are asserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 38e48757-f520-4d2d-8e91-53a1c8025d09
📒 Files selected for processing (15)
app/models/event.rbapp/models/regular_event.rbapp/models/upcoming_event.rbapp/views/hibernation/new.html.slimapp/views/retirement/new.html.slimdb/fixtures/events.ymldb/fixtures/regular_event_repeat_rules.ymldb/fixtures/regular_events.ymltest/fixtures/events.ymltest/fixtures/regular_event_repeat_rules.ymltest/fixtures/regular_events.ymltest/models/event_test.rbtest/models/regular_event_test.rbtest/system/notification/regular_events_test.rbtest/system/regular_events_test.rb
✅ Files skipped from review due to trivial changes (2)
- test/system/regular_events_test.rb
- db/fixtures/events.yml
🚧 Files skipped from review as they are similar to previous changes (6)
- db/fixtures/regular_event_repeat_rules.yml
- db/fixtures/regular_events.yml
- app/models/event.rb
- test/fixtures/regular_events.yml
- test/fixtures/regular_event_repeat_rules.yml
- app/models/regular_event.rb
s-tone-gs
force-pushed
the
bug/upcoming-events-section-displays-WIP-events
branch
3 times, most recently
from
6ae8aeb to
e503f83
Compare
wip状態のイベントの作成者をkimuraにすると test/system/home/events_test.rbのテストに影響が出るため修正した
/test/system/regular_events_test.rbにおいて全ての定期イベントが表示されることを確認するテストがある こちらに新しく作成したWIP状態のイベントが影響を与えた 「全ての定期イベント」という括りにはWIP状態のイベントも入ると判断し、テストを修正した
s-tone-gs
force-pushed
the
bug/upcoming-events-section-displays-WIP-events
branch
from
e503f83 to
93ce27d
Compare
Discordの近日開催のイベント通知と定期イベント一覧の開催中にWIPが表示されるバグを修正 holdingとscheduled_onという「開催する」イベントを取得するメソッドがwipを除外していなかったのが開催中の欄にwipが表示されてしまう根本的な原因だと判断し、修正した
🚀 Review AppURL: https://bootcamp-pr-9796-fvlfu45apq-an.a.run.app
|
s-tone-gs
force-pushed
the
bug/upcoming-events-section-displays-WIP-events
branch
2 times, most recently
from
e3014b9 to
fc8678a
Compare
修正箇所の変化によってメソッド名を変更する理由がなくなったので元に戻した 修正したモデルメソッドのテストを追加し、WIPが表示されるバグを検知できるようにシステムテストを追加、修正した
s-tone-gs
force-pushed
the
bug/upcoming-events-section-displays-WIP-events
branch
3 times, most recently
from
1dd452e to
ecf5a74
Compare
|
@Miya096jp |
|
お疲れ様です！ |
|
@Miya096jp @shibainurou |
|
@s-tone-gs |
There was a problem hiding this comment.
@s-tone-gs
レビュー完了しました！
修正箇所の調査がよくできており勉強になります！
ただ、少しだけ気になった点がありますのでご確認頂けると幸いです。
- コードについて
コード内にコメントしましたのでご確認お願い致します。 - プルリク内の確認方法について
休会や退会したときに主催しているWIPのイベントが表示されることの確認が抜けているのでご確認お願い致します。
| = form_with model: current_user, local: true, url: retirement_path, method: :post, class: 'form' do |f| | ||
| .form__items | ||
| - holding_regular_events = RegularEvent.organizer_event(current_user).holding | ||
| - holding_regular_events = RegularEvent.organizer_event(current_user).not_finished |
There was a problem hiding this comment.
取得する値がholdingからnot_finishedに変更になったので、変数名も合わせて修正したほうがよいと思われます。
| | のページの「分報 URL」欄に分報チャンネルの URL を登録してください。 | ||
|
|
||
| - holding_regular_events = RegularEvent.organizer_event(current_user).holding | ||
| - holding_regular_events = RegularEvent.organizer_event(current_user).not_finished |
There was a problem hiding this comment.
取得する値がholdingからnot_finishedに変更になったので、変数名も合わせて修正したほうがよいと思われます。
| | 休会をお考えの場合、イベントの進行に影響がないよう、任意で他の参加者に主催を引き継ぐことを推奨します。 | ||
| | 休会手続きを完了する前に、以下のリンク先でイベント設定変更を行なってください。 | ||
| ul | ||
| - holding_regular_events.holding.each do |event| |
There was a problem hiding this comment.
既存バグに近いのですが、ご確認お願いします。
holdingは今回の修正でWIP=falseが追加されました。
そのため、holding_regular_events.holdingでループを回すと「休会手続きを完了する前に、以下のリンク先でイベント設定変更を行なってください。」とメッセージが表示されるのに、リンク先が表示されなくなってしまいます。
holding_regular_events.holdingのholding`の削除が必要だと思われます。
| @@ -56,7 +56,7 @@ class RegularEvent < ApplicationRecord # rubocop:disable Metrics/ClassLength | |||
| scope :fetch_target_events, lambda { |target| | |||
| case target | |||
| when 'not_finished' | |||
There was a problem hiding this comment.
細かいのですがtargetの値をnot_finishedのままにすると、フロントエンドでは「開催中」に表示するイベントは「完了していないもの(WIP含む)」ように見えてしまいます。
そのためwhenのcase部分もnot_finishedからholdingに変えた方がよいと思われます。
bootcamp/app/views/regular_events/_regular_events.html.slim
Lines 4 to 5 in ecf5a74
bootcamp/app/views/regular_events/_regular_events.html.slim
Lines 4 to 5 in ecf5a74
ただここまで修正しなくても良いかもしれないので、一度メンターにご確認頂けると幸いです。
There was a problem hiding this comment.
@machida (@shibainurou )
お疲れ様です。こちらの修正提案に関連して2点確認です。
not_finishedはwipを含むか
そもそも論になってしまうのですが、#9737 (comment) の考えに照らし合わせると、not_finishedも意味的に本来wipを含むべきではないと感じます。wipはあくまで下書きなので、完了かどうか以前の状態であるためです。こちら認識に相違ないでしょうか？
上記の考えに沿って、現状はregular_eventのnot_finishedスコープの修正を考えています。
合わせてholdingスコープとnot_finishedスコープの統合を行う予定です。
- 変更イメージ
app/models/regular_event.rb
- scope :not_finished, -> { where(finished: false) }
- scope :holding, -> { where(finished: false, wip: false) }
- scope :scheduled_on, ->(date) { holding.filter { |event| event.scheduled_on?(date) } }
- scope :scheduled_on_without_ended, ->(date) { holding.filter { |event| event.scheduled_on?(date) && !event.ended?(date) } }
+ scope :not_finished, -> { where(finished: false, wip: false) }
+ scope :scheduled_on, ->(date) { not_finished.filter { |event| event.scheduled_on?(date) } }
+ scope :scheduled_on_without_ended, ->(date) { not_finished.filter { |event| event.scheduled_on?(date) && !event.ended?(date) } }休会・退会時に引継ぎを要求されるイベントにwipを含むか
not_finishedがwipを取得しないように修正すると、現状のコードでは休会時・退会時に引継ぎを求められるイベントにWIPが含まれなくなります。
しかし、ここではWIPを含めるべきだと考えています。引継ぎが行われずに、開催されないwipイベントが一覧に残り続けてもユーザーにとってノイズにしかならないからです。
この認識に相違が無いかを確認したいです。
上記のようにnot_finishedを修正する場合は休会・退会時の主催イベントを取得する処理にnot_finished以外のものを使用する、などの処置をしようと考えています。
- 変更イメージ
app/views/hibernation/new.html.slim
- holding_regular_events = RegularEvent.organizer_event(current_user).not_finished
+ holding_regular_events = RegularEvent.organizer_event(current_user).where(finished: false)app/views/retirement/new.html.slim
- holding_regular_events = RegularEvent.organizer_event(current_user).not_finished
+ holding_regular_events = RegularEvent.organizer_event(current_user).where(finished: false)4 participants
Issue
概要
#9737 (comment) の方針に沿い、以下の修正も同時に行った。
変更確認方法
1. ダッシュボードの近日開催のイベント欄にWIPのイベントが表示されない
2. 定期イベント一覧の開催中のイベントにWIPのイベントが表示されない
1. ダッシュボードの近日開催のイベント欄にWIPのイベントが表示されないの手順7で作成したWIPの定期イベントが表示されていないことを確認する3. Discordの開催予定の定期イベントの通知にWIPのイベントが含まれない
※ app/models/chat_notifier.rbの編集は行わず、代わりに
app/models/discord_driver.rbを以下のように修正する。Discord::Notifier.message( params[:body], username: params[:name], - url: params[:webhook_url] + url: '取得したウェブフックURL' )app/controllers/scheduler_controller.rbの:require_tokenをコメントアウトして一時的に無効化するcurl http://localhost:3000/scheduler/daily/notify_coming_soon_regular_eventsを実行する。1. ダッシュボードの近日開催のイベント欄にWIPのイベントが表示されないの手順7で作成したWIPの定期イベントが含まれていないことを確認するgit restore app/models/discord_driver.rb app/controllers/scheduler_controller.rbScreenshot
1. ダッシュボードの近日開催のイベント欄にWIPのイベントを表示しない
変更前
変更後
2. 定期イベント一覧の開催中のイベントにWIPのイベントを表示しない
変更前
変更後
3. Discordの開催予定の定期イベントの通知にWIPのイベントが含まれない
使用するWIPのイベント
変更前
変更後
Summary by CodeRabbit