Conversation
📝 WalkthroughWalkthrough未完了（holding）定期イベントのみを対象に主催者の引き継ぎ処理を導入し、新主催者追加時の通知経路（ActiveSupport::Notifications → OrganizerNotifier → ActivityDelivery → ActivityMailer）と関連テスト／ビューを追加しました。コントローラ／退職・訓練完了フローは呼び出し先を差し替えています。 Changes
Sequence Diagram(s)sequenceDiagram
participant RE as RegularEvent
participant ASN as ActiveSupport::Notifications
participant ON as OrganizerNotifier
participant AD as ActivityDelivery
participant AM as ActivityMailer
RE->>RE: hand_over_organizer(organizer, sender)
RE->>ASN: instrument 'organizer.add' (regular_event, sender, new_organizer_users)
ASN->>ON: deliver payload
ON->>AD: ActivityDelivery.with(...).notify(:added_organizer)
AD->>AM: ActivityMailer.added_organizer with(regular_event, receiver, sender)
AM->>AM: build mail and enqueue/send
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
yokomaru
force-pushed
the
feature/notify-added-organizer-of-regular-event
branch
from
c4e93ba to
45cb9a0
Compare
- 処理の起点となるユーザーには通知しないようにする - テストも追加
…ganizerを定義して責務を移動した - テストを追加 - assign_admin_as_organizer_if_noneをprivateに変更したため関連テストを削除 - 不要になったOrganizerのメソッドとテストを削除
- メソッド名が実装詳細に寄っていたため、 主催者の引き継ぎとして処理（削除〜通知）を集約した - 対象は未終了の定期イベントに限定しテストを追加 - Organizerに未終了の定期イベントに絞るスコープを追加
- 未終了の定期イベント参加を削除する`cancel_participation_from_not_finished_regular_events`を追加 - RegularEventParticipationに`not_finished`スコープを追加 - 終了済みイベントの参加が削除されないことをテストに追加
- 一時的にRubocop:disable Metrics/ClassLengthを追加 - update前に差分用にbefore_user_idsを取得する処理を追加
yokomaru
force-pushed
the
feature/notify-added-organizer-of-regular-event
branch
from
45cb9a0 to
cc9f0d2
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
- User側でループを回して各処理(主催者削除&追加と通知)を個別にRegularEventに依頼していたが、 引き継ぎ処理という1つのまとまりとしてRegularEventに処理を依頼(hand_over_organizer)し、 User側では対象イベントのスコープを絞るのみに修正した(hand_over_not_finished_regular_event_organizers) - 上記に合わせてメソッド名やテストの修正などを行った
- 通知処理のモデルは他のモデルとあわせてとモデル名＋Notifier(OrganizerNotifier)に変更した
…ser_idから取得した方がわかりやすいため変更した
- 終了済みイベントのタイトルを修正 - 複数イベントが処理されるメソッドのため、各イベントの期待する結果をコメントで追加 - 通知が飛んでいることも確認するためassert_differenceを追加
yokomaru
force-pushed
the
feature/notify-added-organizer-of-regular-event
branch
from
e65fad1 to
d5d58b8
Compare
yokomaru
changed the title
- メソッド名などもholdingに修正する
| end | ||
|
|
||
| def notify_new_organizer(sender:, before_organizer_user_ids:) | ||
| new_organizer_user_ids = (organizers.pluck(:user_id) - before_organizer_user_ids) - [sender.id] |
There was a problem hiding this comment.
organizers.pluck(:user_id)を再度呼び出して、更新処理後の最新のuser_idsを取得しその差分を比較しています
| def hand_over_organizer(organizer:, sender:) | ||
| before_organizer_user_ids = organizers.pluck(:user_id) | ||
|
|
||
| organizer.delete |
There was a problem hiding this comment.
destroyではなくdeleteを使用しているのは既存の処理であるdelete_and_assign_newを参考にしています(本当はdestroyを使う方がいいとは思うのですが、他にいい方法が浮かばず一旦このまま踏襲しています)- RegularEventとOrganizersは
present: true関連があり、organizersが1人しかいない状態でdestroyを行うとバリデーションエラーになるため、deleteを使用し一時的に関連を無視してorganizerを削除後、主催者が0人の場合は管理者(komagata)を追加している
- RegularEventとOrganizersは
def delete_and_assign_new
event = regular_event
delete
event.assign_admin_as_organizer_if_none
end
There was a problem hiding this comment.
この機能は削除することになったのでもう関係はないですが、一応コメントします。
懸念している通りdeleteするのは行儀が悪いので避けたいですね。
もっと綺麗に書けるかもしれませんが、以下のような流れでいけると思います。
if organizers.count > 1
organizer.destory
return
# 主催者が引数で渡ってきた`organizer`一人だけなので`komagata`を主催者に追加した後`organizer.destory`する処理
# ...
notify_new_organizer(sender:, before_organizer_user_ids:)
|
@ryufuta |
|
@yokomaru |
|
@ryufuta |
|
#9258 (comment) の経緯のため、こちらのPRは一度closeします！ |
|
|
||
| def update | ||
| set_wip | ||
| before_organizer_user_ids = @regular_event.organizers.pluck(:user_id) |
There was a problem hiding this comment.
before_organizer_user_idsを直訳すると"主催者ユーザーIDの（時間的もしくは位置的に）前に"という意味になり変です。
他の箇所で、更新後の主催者をnew_organizer_usersとしているのでそれとの対比でold_organizer_user_idsとすると良さそうです。
| def delete_and_assign_new_organizer | ||
| organizers.each(&:delete_and_assign_new) | ||
| def cancel_participation_from_holding_regular_events | ||
| regular_event_participations.holding.destroy_all |
There was a problem hiding this comment.
ここでの"開催中の"という意味にはactiveが一番しっくり来ると思います。
英語的にも自然で日本語話者にも通じやすいです。
既存のscopeにあるholdingと合わせたのだと思いますが、hold（開催する）をholdingの形で使用することはほとんどないように思います。
holding_regular_eventsだと"定期イベントを開催すること"という意味になりそうです。
他の箇所のholdingもactiveに統一すると良いと思います。
| sender = User.find(ActiveRecord::FixtureSet.identify(:kimura)) | ||
| ActivityMailer.with(regular_event:, receiver:, sender:).added_organizer |
There was a problem hiding this comment.
メール通知では基本的にデフォルトの自動送信用メールアドレスを使用するようになっています。
senderは不要では？
ActivityMailer#added_organizer内でもsenderのメールアドレスを使っていないのでデフォルトのものが使われています。
サーバー起動後以下にアクセスすると確認できます。
http://localhost:3000/rails/mailers/activity_mailer/added_organizer
あと大したことではないので修正するほどではないですが、テストデータは実際のシナリオに近いものを選んだ方がわかりやすいです。
regular_event1は開発MTGでkomagataはorganizer1として初めから主催者に設定されています。
komagataが新たに主催者に任命されたという通知をテストするのには向かないと思いました。
| regular_event = RegularEvent.find(ActiveRecord::FixtureSet.identify(:regular_event1)) | ||
| receiver = User.find(ActiveRecord::FixtureSet.identify(:komagata)) |
There was a problem hiding this comment.
test/mailers/previews/activity_mailer_preview.rbと違ってrequire 'test_helper'を実行しているので以下のような書き方でfixtureを使えます。
| regular_event = RegularEvent.find(ActiveRecord::FixtureSet.identify(:regular_event1)) | |
| receiver = User.find(ActiveRecord::FixtureSet.identify(:komagata)) | |
| regular_event = regular_events(:regular_event1) | |
| receiver = users(:komagata) |
こちらの方が可読性が高くて良いと思います。
| test 'added_organizer' do | ||
| regular_event = RegularEvent.find(ActiveRecord::FixtureSet.identify(:regular_event1)) | ||
| receiver = User.find(ActiveRecord::FixtureSet.identify(:komagata)) | ||
| sender = User.find(ActiveRecord::FixtureSet.identify(:kimura)) |
There was a problem hiding this comment.
ここもsenderは不要ですね。
実際に削除したところテストはパスしました。
| sender = users(:hatsuno) | ||
|
|
||
| Organizer.create!(user_id: user.id, regular_event_id: regular_event.id) | ||
| Organizer.find_by!(user_id: sender.id, regular_event_id: regular_event.id).destroy! |
There was a problem hiding this comment.
ここでdestroy!をしている理由がわかりませんでした。
またcreate!の戻り値を受け取ってdestroy!を呼ぶ方が効率は良いです。
| hold_national_holiday: false, | ||
| start_at: Time.zone.local(2020, 1, 1, 21, 0, 0), | ||
| end_at: Time.zone.local(2020, 1, 1, 22, 0, 0), | ||
| user: users(:kananashi), |
There was a problem hiding this comment.
| user: users(:kananashi), | |
| user: , |
置き換え忘れですかね？
| @@ -0,0 +1,13 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| class OrganizerNotifier | |||
There was a problem hiding this comment.
口頭でも伝えましたが、RegularEvent#notify_new_organizerからのみ呼ばれているのでRegularEventの外にクラスとして処理を切り出す意味はないのではと思いました。
| -> { user.regular_event_participations.holding.count } => -2, | ||
| -> { user.regular_event_participations.joins(:regular_event).merge(RegularEvent.where(finished: true)).count } => 0 |
There was a problem hiding this comment.
以下の問題点があります。
- 開催中の定期イベントから参加者が削除されること、終了した定期イベントからは参加者が削除されないことの2つを同一のテストケースでテストしていてわかりにくい
- 参加者が削除されることのテストが
users(:kananashi)に依存した壊れやすいテストになっている- 将来的にfixtureに
kananashiが参加する定期イベントが作成されると3つ以上のイベントが削除されるので失敗する
- 将来的にfixtureに
- テストコードに複雑なロジックが持ち込まれて可読性と保守性が低い
- 特にこのテストケースの場合はテスト対象のメソッドと類似のロジックを再実装している。DRYでないし、本当にテストできているのかわからない。例えば
holdingというscopeに「開催中ではなく終了したイベントを返すバグ」があっても「開催中の定期イベントから参加者が削除されること」のテストはパスします。
- 特にこのテストケースの場合はテスト対象のメソッドと類似のロジックを再実装している。DRYでないし、本当にテストできているのかわからない。例えば
以下、解決策の案。
1については普通に2つのテストケースに分ければ問題なし。
試してはいないですが、それぞれのテストケース内で作成したholding_regular_eventとfinished_regular_eventに対して.participants.countの値の変化を確認することで2と3の問題は両方解決できそうです。
| def hand_over_organizer(organizer:, sender:) | ||
| before_organizer_user_ids = organizers.pluck(:user_id) | ||
|
|
||
| organizer.delete |
There was a problem hiding this comment.
この機能は削除することになったのでもう関係はないですが、一応コメントします。
懸念している通りdeleteするのは行儀が悪いので避けたいですね。
もっと綺麗に書けるかもしれませんが、以下のような流れでいけると思います。
if organizers.count > 1
organizer.destory
return
# 主催者が引数で渡ってきた`organizer`一人だけなので`komagata`を主催者に追加した後`organizer.destory`する処理
# ...
notify_new_organizer(sender:, before_organizer_user_ids:)2 participants
Issue
概要
実装について
退会/休会/研修終了の主催者削除〜通知動線
旧動線(通知追加前)
delete_and_assign_new)だったため、ここに通知も追加すると名前が冗長になってしまう新動線(通知追加後)
sequenceDiagram Controller->>User: hand_over_organizers_of_holding_regular_events loop 開催済みのイベントごとに実行 User->>RegularEvent: hand_over_organizer(organizer, sender) activate RegularEvent RegularEvent->>RegularEvent: before_organizer_user_idsを取得 Note right of RegularEvent: 主催者削除前の主催者を取得しておく RegularEvent->>Organizer: delete RegularEvent->>RegularEvent: assign_admin_as_organizer_if_none Note right of RegularEvent: 主催者がいなくなれば管理者を追加 RegularEvent->>RegularEvent: notify_new_organizer(sender:, before_organizer_user_ids) Note right of RegularEvent: 追加した管理者にinstrument('organizer.create') を実行 deactivate RegularEvent endhand_over_organizer)で依頼するようにしたhand_over_organizers_of_holding_regular_events)hand_over_organizerメソッドの責務が大きくなっているが、今後さらに処理を追加する場合などは別のモデルに切り出すなどの検討も可能定期イベント更新の通知動線
新動線
sequenceDiagram Controller->>RegularEvent: notify_new_organizer(sender:, before_organizer_user_ids) Note right of RegularEvent: 主催者の差分を計算しinstrument('organizer.create') を実行RegularEvent: notify_new_organizerに更新前の主催者と追加した主催者を加えた差分を計算し、新しく追加された主催者のみに通知を行う退会/休会/研修終了時に対象となるイベントのスコープを開催中のみに変更
where(finished: false)を定義するスコープが2つあった(holdingとnot_finished)holdingを使っているため今回はholdingを使用where(finished: false)のスコープが2つ定義されている #9615補足
RegularEventsControllerのMetrics/ClassLengthに関するRuboCop警告は本PRでは一旦 disableし、リファクタリングは別IssueとしたRegularEvent#organizersがhas_manyの関連メソッドを上書きしているため、organizersと書くと紐づくusersが取得される状態になっているため、別途Issueを立てた変更確認方法
feature/notify-added-organizer-of-regular-eventををローカルに取り込み、ブランチを切り替えるbin/devにてサーバーの立ち上げを行うdebuggerでデバッグしたい場合は以下の手順のようにサーバーを2つ立ち上げる必要がある1. イベント更新時の主催者追加通知
手順
komagataでログインkomagatahatsunokomagata/hatsunoが追加されていること通知の確認（
komagata）通知の確認（hatsuno）
hatsunoでログイン2. 退会/休会/研修終了時の主催者引き継ぎ通知
2-1. 退会
事前準備
hajimeでログインhajimeが主催に含まれる以下の定期イベントを用意新規作成する必要あり
既存のイベントを利用可能
hajimeとkimura手順
hajimeで退会画面へ遷移退会後の確認（komagata）
komagataでログインkomagataになっていることhajimeが抜けていることhajimeのままになっているhajimeが消えているhajimeは参加者のままになっている2-2. 休会
手順
hatsunoなど適当なアカウントでログイン確認（
komagata）2-3. 研修終了
手順
kensyu（研修生）でログイン確認（komagata）
komagata宛にメールが送信されているScreenshot
通知
お知らせ通知ベル
通知一覧
メール文
Summary by CodeRabbit
新機能
改善
テスト