Open
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ペアワークの予約更新（再スケジュール／リマッチ）、予約キャンセル、対応する通知経路（ActivityNotifier/ActivityMailer/ActiveSupport::Notifications）の追加、退職時のアンマッチ処理、関連するビュー・スタイル・ルーティング・テストの拡張を行いました。 Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as UI / Views
participant Controller as ReservationsController
participant PairWork as PairWork (Model)
participant ASN as ActiveSupport::Notifications
participant Notifier as PairWork*Notifier
participant ActivityDelivery as ActivityDelivery
participant Mailer as ActivityMailer
User->>UI: PATCH or DELETE /pair_works/:pair_work_id/reservations
UI->>Controller: request
Controller->>PairWork: reserve(params) or unmatch()
alt reserve/unmatch success
Controller->>ASN: instrument (pair_work.rematch / pair_work.reschedule / pair_work.cancel, payload)
ASN->>Notifier: call(_name,..., payload)
Notifier->>ActivityDelivery: with(pair_work, receiver).notify(:kind)
ActivityDelivery->>Mailer: deliver/enqueue email
Mailer-->>User: notification email
Controller-->>UI: redirect (notice)
else failure
Controller-->>UI: render pair_works/show (errors)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 分 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 |
s-tone-gs
force-pushed
the
feature/add-notifications-and-cancellation
branch
from
9af4a4c to
9fb9fb1
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: 8
🧹 Nitpick comments (5)
app/models/pair_work.rb (1)
89-91: メッセージの句読点の一貫性を確認してください。
cancelのメッセージには句点（。）がありませんが、他のメッセージ（create、update、destroy、reserve）には句点があります。♻️ 修正案
reserve: 'ペアが確定しました。', - cancel: 'ペア確定を取り消しました' + cancel: 'ペア確定を取り消しました。' }[action_name]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/pair_work.rb` around lines 89 - 91, The cancel message in the action_name mapping in PairWork (the hash that returns messages for keys like create, update, destroy, reserve, cancel) is missing the Japanese full stop; update the 'cancel' value to include the trailing '。' so it matches the punctuation used by create, update, destroy, and reserve and maintains consistency across PairWork messages.app/models/pair_work_cancel_notifier.rb (1)
17-22:notify_to_chatの文面が cancel 用になっていません。現状
callからも使われていませんが、Line 19 が「決定しました」のままだと、後で呼ぶようにした際に cancel 通知と逆の内容を送ってしまいます。使う予定がないなら削除、使うなら取消文面へ直しておきたいです。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/pair_work_cancel_notifier.rb` around lines 17 - 22, The notify_to_chat method in PairWorkCancelNotifier currently sends a "matching pair decided" message which is incorrect for cancel notifications; either remove the unused notify_to_chat method if it will never be used, or update its text to a cancel-specific message: change the ChatNotifier.message payload in notify_to_chat to indicate the pair matching was cancelled (e.g., "マッチングペアが取消されました") and keep the existing pair_work_url link for context so future callers (including call) will send a cancel notification rather than a confirmation.test/system/pair_works_test.rb (1)
128-147: この 2 件は時刻を固定した方が安定します。
pair_work4の確定日時は 2025-01-02 01:00:00 ですが、現在は 2026-03-19 です。travel_toなしだと、過去日の扱い変更でcancel pair confirmationとshow confirmed date on calendarが無関係に壊れやすいので、2025-01-01 前後に固定しておくと意図した挙動だけを検証できます。修正イメージ
test 'cancel pair confirmation' do - pair_work = pair_works(:pair_work4) - visit_with_auth pair_work_path(pair_work), 'komagata' - find("label[for='show-schedule-dates']").click - accept_alert do - click_button 'ペア確定を取り消す' + travel_to Time.zone.local(2025, 1, 1, 0, 0, 0) do + pair_work = pair_works(:pair_work4) + visit_with_auth pair_work_path(pair_work), 'komagata' + find("label[for='show-schedule-dates']").click + accept_alert do + click_button 'ペア確定を取り消す' + end + + assert_text 'ペア確定を取り消しました' end - - assert_text 'ペア確定を取り消しました' end test 'show confirmed date on calendar' do - pair_work = pair_works(:pair_work4) - visit_with_auth pair_work_path(pair_work), 'kimura' - assert_selector "[id='2025-01-02T01:00:00+09:00']", text: '確定' - - visit_with_auth pair_work_path(pair_work), 'komagata' - find("label[for='show-schedule-dates']").click - assert_selector "[id='2025-01-02T01:00:00+09:00']", text: '確定' + travel_to Time.zone.local(2025, 1, 1, 0, 0, 0) do + pair_work = pair_works(:pair_work4) + visit_with_auth pair_work_path(pair_work), 'kimura' + assert_selector "[id='2025-01-02T01:00:00+09:00']", text: '確定' + + visit_with_auth pair_work_path(pair_work), 'komagata' + find("label[for='show-schedule-dates']").click + assert_selector "[id='2025-01-02T01:00:00+09:00']", text: '確定' + end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/system/pair_works_test.rb` around lines 128 - 147, Tests "cancel pair confirmation" and "show confirmed date on calendar" depend on a past confirmed datetime (pair_works(:pair_work4) = 2025-01-02 01:00:00) so make them deterministic by fixing the current time with travel_to; wrap the actions/assertions in the tests named 'cancel pair confirmation' and 'show confirmed date on calendar' (or wrap the entire test bodies) with a travel_to set to a date around 2025-01-01/02 so the confirmed datetime is treated as intended, then ensure you return time to normal (using travel_back or the travel_to block) and keep references to pair_works(:pair_work4), visit_with_auth, and the calendar selector id (2025-01-02T01:00:00+09:00) when applying the change.app/models/pair_work_rematching_notifier.rb (1)
17-22: 未使用のプライベートメソッドnotify_to_chatこのメソッドは定義されていますが、現在の
callフローでは呼び出されていません。また、メッセージ内容「マッチングペアが決定しました」はrematchingではなくmatchingの文脈のようです。将来的にチャット通知を追加する予定がない場合は、このコードを削除することを検討してください。追加予定の場合は、メッセージ内容を「ペアが変更されました」などに修正する必要があります。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/pair_work_rematching_notifier.rb` around lines 17 - 22, The private method notify_to_chat is unused and its message wording refers to an initial matching rather than a rematching; either remove the unused method or update and wire it into the call flow: if you intend to keep chat notifications, rename or change the message in notify_to_chat to reflect a rematch (e.g., "ペアが変更されました"), and invoke notify_to_chat from the call method where rematching is handled so the notifier runs; otherwise delete the notify_to_chat method to avoid dead code (referencing notify_to_chat and the call method to locate the change).app/models/pair_work_reschedule_notifier.rb (1)
17-22: 未使用のプライベートメソッドnotify_to_chat
PairWorkRematchingNotifierと同様に、このメソッドは未使用です。メッセージ内容も日程変更の文脈と一致していません（「マッチングペアが決定しました」）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/pair_work_reschedule_notifier.rb` around lines 17 - 22, The private method notify_to_chat in PairWorkRescheduleNotifier is unused and its message text ("マッチングペアが決定しました") is incorrect for a reschedule notifier; either remove the unused notify_to_chat method or repurpose it by changing its message to reflect a rescheduled meeting and ensure it is invoked from the class's public notification flow (e.g., call notify_to_chat from the main notify method). If keeping it, update the ChatNotifier.message body to reference rescheduling and reuse Rails.application.routes.url_helpers.pair_work_url(pair_work) as currently used; otherwise delete the notify_to_chat method to match PairWorkRematchingNotifier's cleanup.
🤖 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/pair_works/reservations_controller.rb`:
- Around line 24-33: The destroy action lacks a server-side authorization check
allowing any authenticated user to cancel a PairWork; update the destroy method
(the controller action that finds PairWork via
PairWork.find(params[:pair_work_id]) and calls `@pair_work.unmatch`) to enforce
the same constraint as the view: either call require_mentor_login at the start
(like create) or explicitly verify current_user == `@pair_work.buddy` before
performing `@pair_work.unmatch`, and handle unauthorized cases by
redirecting/raising or rendering the show with `@comments` as in the else branch.
- Around line 10-11: The reschedule check compares `@pair_work.reserved_at`
(DateTime/TimeWithZone) to reservation_params[:reserved_at] (string), causing a
false positive; inside reserve() before the notification_kinds lines (and before
assign_attributes is called) parse/convert reservation_params[:reserved_at] to a
Time/DateTime using Time.zone.parse or to_datetime and use that parsed value for
the comparison (e.g. compare `@pair_work.reserved_at` to parsed_reserved_at) so
the types match and reschedule notifications are only added when the actual time
changes.
In `@app/models/pair_work_rematching_notifier.rb`:
- Around line 13-15: Rename the misspelled method notify_pair_work_creater to
notify_pair_work_creator in PairWorkRematchingNotifier (update the method
definition) and update all call sites that invoke notify_pair_work_creater
(e.g., the caller referenced around line 8) to use notify_pair_work_creator so
names remain consistent; ensure any tests or references to
ActivityDelivery.with(...).notify(:rematching_pair_work) remain unchanged except
for the method name change.
In `@app/models/pair_work_reschedule_notifier.rb`:
- Around line 13-15: The method name notify_pair_work_creater in
PairWorkRescheduleNotifier is misspelled; rename it to notify_pair_work_creator
and update any callers (including the one referenced at line 8) to use
notify_pair_work_creator; keep the ActivityDelivery.with(pair_work:, receiver:
pair_work.user).notify(:reschedule_pair_work) behavior intact and ensure the
signature and visibility remain the same to match PairWorkRematchingNotifier.
In `@app/views/activity_mailer/reschedule_pair_work.html.slim`:
- Line 5: Remove the hardcoded subject prefix "[FBC]" from the email body
template in the activity_mailer reschedule_pair_work view by editing the line
that currently renders p "[FBC] ペアワーク【 #{`@pair_work.title`} 】の日程が#{I18n.l
`@pair_work.reserved_at`}に変更になりました。"; change it to render the title and
reserved_at only (keep `@pair_work.title` and I18n.l `@pair_work.reserved_at`
intact) so the prefix is not duplicated in the message body while leaving
subject handling unchanged.
In `@test/mailers/previews/activity_mailer_preview.rb`:
- Around line 204-209: The preview method rematching_pair_work uses fixture
pair_work1 which has buddy: null causing ActivityMailer#rematching_pair_work to
raise NoMethodError when accessing `@pair_work.buddy.login_name`; update
rematching_pair_work to load a PairWork fixture that has a buddy (e.g., replace
pair_work1 with pair_work2 or pair_work4) or ensure the fixture sets buddy, so
the preview calls ActivityMailer.with(receiver:,
pair_work:).rematching_pair_work with a pair_work that has a non-nil buddy.
In `@test/system/notification/pair_works_test.rb`:
- Line 91: テスト名のタイポを修正してください: 現在のテスト定義 test 'pair work creater receive
notificationnotify when pair rematched' の文字列中の "notificationnotify" を
"notification when"（または適切な英語表現 "notification when"）に置き換え、テストメソッド名を test 'pair
work creater receive notification when pair rematched'
のように修正してスペルと可読性を正してください（対象は該当の test 定義行）。
In `@test/system/pair_works_test.rb`:
- Around line 132-134: Replace the use of accept_alert with accept_confirm
around the click for the 'ペア確定を取り消す' button: locate the block using accept_alert
and click_button 'ペア確定を取り消す' and change it to use accept_confirm so the Capybara
modal helper matches the button's data-confirm behavior; ensure the surrounding
block semantics remain the same and tests continue to click_button 'ペア確定を取り消す'
inside the new accept_confirm block.
---
Nitpick comments:
In `@app/models/pair_work_cancel_notifier.rb`:
- Around line 17-22: The notify_to_chat method in PairWorkCancelNotifier
currently sends a "matching pair decided" message which is incorrect for cancel
notifications; either remove the unused notify_to_chat method if it will never
be used, or update its text to a cancel-specific message: change the
ChatNotifier.message payload in notify_to_chat to indicate the pair matching was
cancelled (e.g., "マッチングペアが取消されました") and keep the existing pair_work_url link for
context so future callers (including call) will send a cancel notification
rather than a confirmation.
In `@app/models/pair_work_rematching_notifier.rb`:
- Around line 17-22: The private method notify_to_chat is unused and its message
wording refers to an initial matching rather than a rematching; either remove
the unused method or update and wire it into the call flow: if you intend to
keep chat notifications, rename or change the message in notify_to_chat to
reflect a rematch (e.g., "ペアが変更されました"), and invoke notify_to_chat from the call
method where rematching is handled so the notifier runs; otherwise delete the
notify_to_chat method to avoid dead code (referencing notify_to_chat and the
call method to locate the change).
In `@app/models/pair_work_reschedule_notifier.rb`:
- Around line 17-22: The private method notify_to_chat in
PairWorkRescheduleNotifier is unused and its message text ("マッチングペアが決定しました") is
incorrect for a reschedule notifier; either remove the unused notify_to_chat
method or repurpose it by changing its message to reflect a rescheduled meeting
and ensure it is invoked from the class's public notification flow (e.g., call
notify_to_chat from the main notify method). If keeping it, update the
ChatNotifier.message body to reference rescheduling and reuse
Rails.application.routes.url_helpers.pair_work_url(pair_work) as currently used;
otherwise delete the notify_to_chat method to match PairWorkRematchingNotifier's
cleanup.
In `@app/models/pair_work.rb`:
- Around line 89-91: The cancel message in the action_name mapping in PairWork
(the hash that returns messages for keys like create, update, destroy, reserve,
cancel) is missing the Japanese full stop; update the 'cancel' value to include
the trailing '。' so it matches the punctuation used by create, update, destroy,
and reserve and maintains consistency across PairWork messages.
In `@test/system/pair_works_test.rb`:
- Around line 128-147: Tests "cancel pair confirmation" and "show confirmed date
on calendar" depend on a past confirmed datetime (pair_works(:pair_work4) =
2025-01-02 01:00:00) so make them deterministic by fixing the current time with
travel_to; wrap the actions/assertions in the tests named 'cancel pair
confirmation' and 'show confirmed date on calendar' (or wrap the entire test
bodies) with a travel_to set to a date around 2025-01-01/02 so the confirmed
datetime is treated as intended, then ensure you return time to normal (using
travel_back or the travel_to block) and keep references to
pair_works(:pair_work4), visit_with_auth, and the calendar selector id
(2025-01-02T01:00:00+09:00) when applying the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5fab15ab-4ea6-4e07-bfd8-de1953a4a3dc
📒 Files selected for processing (22)
app/controllers/pair_works/reservations_controller.rbapp/mailers/activity_mailer.rbapp/models/notification.rbapp/models/pair_work.rbapp/models/pair_work_cancel_notifier.rbapp/models/pair_work_rematching_notifier.rbapp/models/pair_work_reschedule_notifier.rbapp/models/retirement.rbapp/notifiers/activity_notifier.rbapp/views/activity_mailer/cancel_pair_work.html.slimapp/views/activity_mailer/rematching_pair_work.html.slimapp/views/activity_mailer/reschedule_pair_work.html.slimapp/views/pair_works/_body.html.slimapp/views/pair_works/_schedule.html.slimconfig/initializers/active_support_notifications.rbtest/deliveries/activity_delivery_test.rbtest/fixtures/pair_work_schedules.ymltest/fixtures/pair_works.ymltest/mailers/activity_mailer_test.rbtest/mailers/previews/activity_mailer_preview.rbtest/system/notification/pair_works_test.rbtest/system/pair_works_test.rb
s-tone-gs
force-pushed
the
feature/add-notifications-and-cancellation
branch
3 times, most recently
from
326f485 to
ef3e2c6
Compare
createアクションが作成と更新の両方を担っており複雑化していたため切り分けた
s-tone-gs
force-pushed
the
feature/add-notifications-and-cancellation
branch
from
ef3e2c6 to
9fcfa85
Compare
Contributor
Author
|
@machida お疲れ様です！通知とメールの文面を仮で実装したので改めて確認いただきたいです！ ペア取り消し通知アプリ内通知
メール
ペア変更アプリ内
メール
日程変更アプリ内
メール
|
Member
|
@s-tone-gs 連絡ありがとうございます！！ |
Contributor
Author
|
@machida |
Member
|
@s-tone-gs 了解です！木曜までに終わらせますー |
Member
|
@s-tone-gs デザイン入れました！急ぎで入れたので、あとで変更するかもですが、一旦これでテストなどを整えてレビューに進めてください🙏 |
Contributor
Author
|
@machida |
ペアワーク作成者の画面にはカレンダーではなく日付を示すテキストが出るようになったので、そちらに合わせてテストを修正した
s-tone-gs
force-pushed
the
feature/add-notifications-and-cancellation
branch
from
17c13bd to
154c412
Compare
🚀 Review AppURL: https://bootcamp-pr-9812-fvlfu45apq-an.a.run.app
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/system/retirements/data_deletion_test.rb (1)
129-138:reserved_atも一緒に検証しておきたいです。Line 137 だと
buddyスコープから外れたことしか見ていないので、buddy_idだけnilになってreserved_atが残る不整合でも通ってしまいます。pair_works(:pair_work2).reloadでbuddy_idとreserved_atの両方をnilで確認すると、この回帰を直接捕まえられます。💡 修正例
- assert_not_includes PairWork.where(buddy: users(:sotugyou)), pair_works(:pair_work2) + pair_work = pair_works(:pair_work2).reload + assert_nil pair_work.buddy_id + assert_nil pair_work.reserved_atBased on learnings 「buddy（ペアを申し込んだメンター）ユーザーが削除された場合、buddy_id だけでなく reserved_at も NULL にしてマッチングを解除する必要がある」。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/system/retirements/data_deletion_test.rb` around lines 129 - 138, テストが buddy スコープから外れたことしか確認していないので、PairWork のレコードが部分的にしか更新されていない不整合を見逃します。テスト "shold unmatch when buddy has retired" で pair_works(:pair_work2) をそのまま比較する代わりに pair_works(:pair_work2).reload を使ってレコードを再読み込みし、pair_work.buddy_id と pair_work.reserved_at がどちらも nil になっていることを assert_nil で検証するように修正してください（モデル: PairWork、フィクスチャ参照: pair_works(:pair_work2) を参照）。app/models/pair_work_rematching_notifier.rb (1)
14-17:past_buddyが nil の場合の考慮が必要コントローラーで
User.find_by(id: past_buddy_id)を使用しているため、過去のバディユーザーが削除されている場合past_buddyがnilになる可能性があります。nilを含む配列をイテレートすると、ActivityDelivery.with(receiver: nil)が呼ばれてエラーになる可能性があります。♻️ 修正案: nil をフィルタリング
def notify_pair_work_creator(pair_work, past_buddy) - [pair_work.user, past_buddy].each do |receiver| + [pair_work.user, past_buddy].compact.each do |receiver| ActivityDelivery.with(pair_work:, receiver:).notify(:rematching_pair_work) end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/pair_work_rematching_notifier.rb` around lines 14 - 17, notify_pair_work_creator currently iterates over [pair_work.user, past_buddy] and may call ActivityDelivery.with(..., receiver: nil) if past_buddy is nil; filter out nil receivers (e.g., use .compact or reject(&:nil?)) before the .each so ActivityDelivery.with is only invoked with valid User objects; update the notify_pair_work_creator method (referencing ActivityDelivery.with(pair_work:, receiver:)) to skip nil past_buddy values.app/controllers/pair_works/reservations_controller.rb (1)
32-43: 認可失敗時のサイレントリターンは UX 上の問題Line 34 の
return if current_user !=@pair_work.buddy`` は認可チェックとして正しく動作しますが、bare return はレスポンスなしで終了するため、ユーザーが「ペア確定を取り消す」ボタンをクリックしても何も起こらないように見えます。エラーメッセージ付きでリダイレクトするか、403 エラーを返す方が適切です。♻️ 修正案: エラーメッセージ付きリダイレクト
def destroy `@pair_work` = PairWork.find(params[:pair_work_id]) - return if current_user != `@pair_work.buddy` + unless current_user == `@pair_work.buddy` + redirect_to `@pair_work`, alert: 'ペア確定を取り消す権限がありません。' + return + end if `@pair_work.unmatch`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/pair_works/reservations_controller.rb` around lines 32 - 43, The bare `return` in the destroy action causes a silent no-response when `current_user != `@pair_work.buddy``; replace it with an explicit unauthorized response: either perform a redirect with an error/alert (e.g. redirect_to Redirection.determin_url(self, `@pair_work`), alert: 'You are not authorized to cancel this pairing') or render a 403 (render plain: 'Forbidden', status: :forbidden), and then return to stop execution; update the `destroy` method (which finds PairWork via PairWork.find(params[:pair_work_id]) and compares current_user to `@pair_work.buddy`) to use one of these explicit responses instead of the bare `return`.
🤖 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/pair_works_controller.rb`:
- Around line 41-44: Currently the code calls `@pair_work.schedules.destroy_all`
before attempting `@pair_work.update` in update (called after set_wip), which can
delete schedules even if update fails and ignores cases where the browser omits
schedules_attributes; change this to perform schedule replacement atomically
inside a transaction: do not call destroy_all up front, instead wrap the update
and schedule-replacement in a PairWork.transaction block (use update! to raise
on failure), apply the new schedules only after a successful update (or use
nested attributes with allow_destroy and handle a missing schedules_attributes
by treating it as an explicit empty selection), and reference the update action,
set_wip, `@pair_work.schedules.destroy_all` and pair_work_params when making the
change.
---
Nitpick comments:
In `@app/controllers/pair_works/reservations_controller.rb`:
- Around line 32-43: The bare `return` in the destroy action causes a silent
no-response when `current_user != `@pair_work.buddy``; replace it with an explicit
unauthorized response: either perform a redirect with an error/alert (e.g.
redirect_to Redirection.determin_url(self, `@pair_work`), alert: 'You are not
authorized to cancel this pairing') or render a 403 (render plain: 'Forbidden',
status: :forbidden), and then return to stop execution; update the `destroy`
method (which finds PairWork via PairWork.find(params[:pair_work_id]) and
compares current_user to `@pair_work.buddy`) to use one of these explicit
responses instead of the bare `return`.
In `@app/models/pair_work_rematching_notifier.rb`:
- Around line 14-17: notify_pair_work_creator currently iterates over
[pair_work.user, past_buddy] and may call ActivityDelivery.with(..., receiver:
nil) if past_buddy is nil; filter out nil receivers (e.g., use .compact or
reject(&:nil?)) before the .each so ActivityDelivery.with is only invoked with
valid User objects; update the notify_pair_work_creator method (referencing
ActivityDelivery.with(pair_work:, receiver:)) to skip nil past_buddy values.
In `@test/system/retirements/data_deletion_test.rb`:
- Around line 129-138: テストが buddy スコープから外れたことしか確認していないので、PairWork
のレコードが部分的にしか更新されていない不整合を見逃します。テスト "shold unmatch when buddy has retired" で
pair_works(:pair_work2) をそのまま比較する代わりに pair_works(:pair_work2).reload
を使ってレコードを再読み込みし、pair_work.buddy_id と pair_work.reserved_at がどちらも nil になっていることを
assert_nil で検証するように修正してください（モデル: PairWork、フィクスチャ参照: pair_works(:pair_work2)
を参照）。
🪄 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: 44b5a1d6-5125-475f-bee7-c7ebdf4d1390
📒 Files selected for processing (31)
app/assets/stylesheets/application/blocks/pair-work/_pair-work-schedule-dates.cssapp/assets/stylesheets/shared/blocks/form/_form-table.cssapp/controllers/pair_works/reservations_controller.rbapp/controllers/pair_works_controller.rbapp/mailers/activity_mailer.rbapp/models/notification.rbapp/models/pair_work.rbapp/models/pair_work_cancel_notifier.rbapp/models/pair_work_rematching_notifier.rbapp/models/pair_work_reschedule_notifier.rbapp/models/retirement.rbapp/notifiers/activity_notifier.rbapp/views/activity_mailer/cancel_pair_work.html.slimapp/views/activity_mailer/rematching_pair_work.html.slimapp/views/activity_mailer/reschedule_pair_work.html.slimapp/views/pair_works/_body.html.slimapp/views/pair_works/_form.html.slimapp/views/pair_works/_schedule.html.slimapp/views/pair_works/index.html.slimconfig/initializers/active_support_notifications.rbconfig/routes.rbtest/deliveries/activity_delivery_test.rbtest/fixtures/pair_work_schedules.ymltest/fixtures/pair_works.ymltest/mailers/activity_mailer_test.rbtest/mailers/previews/activity_mailer_preview.rbtest/models/pair_work_test.rbtest/models/retirement_test.rbtest/system/notification/pair_works_test.rbtest/system/pair_works_test.rbtest/system/retirements/data_deletion_test.rb
✅ Files skipped from review due to trivial changes (9)
- app/views/pair_works/index.html.slim
- app/assets/stylesheets/shared/blocks/form/_form-table.css
- app/assets/stylesheets/application/blocks/pair-work/_pair-work-schedule-dates.css
- app/views/activity_mailer/rematching_pair_work.html.slim
- app/views/activity_mailer/reschedule_pair_work.html.slim
- test/fixtures/pair_works.yml
- config/initializers/active_support_notifications.rb
- app/views/activity_mailer/cancel_pair_work.html.slim
- test/fixtures/pair_work_schedules.yml
🚧 Files skipped from review as they are similar to previous changes (8)
- app/models/notification.rb
- test/deliveries/activity_delivery_test.rb
- test/system/notification/pair_works_test.rb
- app/notifiers/activity_notifier.rb
- test/mailers/activity_mailer_test.rb
- app/views/pair_works/_body.html.slim
- app/views/pair_works/_schedule.html.slim
- test/system/pair_works_test.rb
s-tone-gs
force-pushed
the
feature/add-notifications-and-cancellation
branch
from
34ab5e3 to
4995f4a
Compare
Contributor
Author
|
@coderabbitai
ペア取り消しボタンはバディにしか表示されないものであり、バディ以外がペア取り消しを行うというのは通常あり得ないです。しかし、悪意あるユーザーによって同操作が行われる場合を危惧してコントローラーで認可処理を書いている状況です。そもそもあり得ない操作なのでUXを考慮する必要もないという考えなのですがいかがでしょうか？ |
|
Tip For best results, initiate chat on the files or code changes.
「ペア取り消し」ボタンはバディにしか表示されない設計であり、バディ以外がこの操作を行うのは通常ありえません。コントローラーの認可チェックはあくまで悪意ある操作（直接リクエスト送信など）に対する多層防御（defense-in-depth）の目的で実装されているわけですね。 そのようなセキュリティガードのケースでは、正規ユーザーがこのコードパスに到達することは想定されていないため、サイレントリターンはむしろ意図的かつ適切な設計です。エラーメッセージを返すことで攻撃者に情報を与える必要もありません。 この指摘は取り下げます。現在の実装で問題ありません！ ✏️ Learnings added
🧠 Learnings used |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
Contributor
Author
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 13428 🏁 Script executed: Length of output: 4139
✅ 修正済み（2件）
🔴 未修正（1件）
def update
set_wip
`@pair_work.schedules.destroy_all` if params[:pair_work][:schedules_attributes].present?
if `@pair_work.update`(pair_work_params)
def update
set_wip
PairWork.transaction do
`@pair_work.schedules.destroy_all` if params[:pair_work][:schedules_attributes].present?
`@pair_work.update`!(pair_work_params)
end
ActiveSupport::Notifications.instrument('pair_work.update', pair_work: `@pair_work`)
redirect_to Redirection.determin_url(self, `@pair_work`), notice: `@pair_work.generate_notice_message`(:update)
rescue ActiveRecord::RecordInvalid
render :edit
endこちらの対応もお願いできますか？ 🧠 Learnings used |
Contributor
Author
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
こちらを利用すると希望日のチェックを全て外して更新した場合にバリデーションエラーになり、よりユーザーフレンドリーな動作になる
s-tone-gs
force-pushed
the
feature/add-notifications-and-cancellation
branch
from
2784349 to
c74e4e8
Compare
Contributor
Author
|
@coderabbitai |
Contributor
Author
|
@karlley |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2095 🏁 Script executed: Length of output: 3600
以前の問題だった def update
set_wip
if `@pair_work.update`(pair_work_params)
ActiveSupport::Notifications.instrument('pair_work.update', pair_work: `@pair_work`)
redirect_to Redirection.determin_url(self, `@pair_work`), notice: `@pair_work.generate_notice_message`(:update)
else
render :edit
end
endスケジュールの削除は 対応ありがとうございました！🐇 🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Contributor
|
@s-tone-gs |
Contributor
Author
|
@karlley |
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
3 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
概要
ペアワークに以下の機能を追加しました。
変更確認方法
1. カレンダーの変更を確認
2. ペア変更時にペアワーク作成者と変更前のペアに通知が来ることの確認
3. 日程変更時にペアワーク作成者に通知が来ることの確認
4. ペア取り消し機能と通知がペアワーク作成者と各メンターに届いていることの確認
5 退会時に自分がバディの未開催のペアワークのペアが解消されることの確認
6.ペアが未確定のペアワークの希望日変更
Screenshot
1. カレンダーの変更
変更前
変更後
メンターでアクセス
メンター以外でアクセス
終了したペアワーク
2. ペア変更の通知
変更後
アプリ内
メール
3, 日程変更の通知
変更後
アプリ内
メール
4. ペア取り消し機能
変更前
development._._.FBC.-.Google.Chrome.2026-03-26.10-38-04.mp4
変更後
development._._.FBC.-.Google.Chrome.2026-03-26.10-40-07.mp4
アプリ内通知
メール
バディが退会した時
development._._.FBC.-.Google.Chrome.2026-03-26.10-45-14.mp4
5. ペア未確定のペアワークの希望日変更
変更後
development._.q._.FBC.-.Google.Chrome.2026-03-26.16-55-28.mp4
Summary by CodeRabbit
新機能
通知
UI
テスト