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
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 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/close-regular-event-when-no-organizers
branch
from
c16e368 to
f3492c9
Compare
yokomaru
force-pushed
the
feature/close-regular-event-when-no-organizers
branch
2 times, most recently
from
fec1e89 to
cc8f8cb
Compare
yokomaru
force-pushed
the
feature/close-regular-event-when-no-organizers
branch
4 times, most recently
from
b0eb509 to
ff11d9b
Compare
| end | ||
|
|
||
| def set_holding_regular_events | ||
| @holding_regular_events = RegularEvent.organizer_event(current_user).holding |
There was a problem hiding this comment.
元々slim側でクエリを発行していたロジックをそのままcontollerに移動しています。
ユーザーに紐づく開催中のイベントは、リレーションを使ってcurrent_user.organize_regular_events.holdingと取得できるのでorganizer_event(user)スコープは本来不要なのですが、他にも使っている箇所があったのでこちらがマージされた後別のPRで修正・削除対応しようと思っております。
- holding_regular_events = RegularEvent.organizer_event(current_user).holding
- if holding_regular_events.any?
| end | ||
|
|
||
| def set_holding_regular_events | ||
| @holding_regular_events = RegularEvent.organizer_event(current_user).holding |
There was a problem hiding this comment.
| end | ||
|
|
||
| def set_holding_regular_events | ||
| @holding_regular_events = RegularEvent.organizer_event(current_user).holding |
There was a problem hiding this comment.
| belongs_to :user | ||
| has_many :organizers, dependent: :destroy | ||
| # TODO: テーブル名を変更したら修正する | ||
| has_many :regular_event_organizers, class_name: 'Organizer', dependent: :destroy |
There was a problem hiding this comment.
#9640 (comment) にあるように、一時的に変更した関連を使用して修正しています。テーブル名変更時に修正いたします。
|
|
||
| class HibernationController < ApplicationController | ||
| skip_before_action :require_active_user_login, raise: false, only: %i[show] | ||
| before_action :set_holding_regular_events, only: %i[new create] |
There was a problem hiding this comment.
@holding_regular_eventsはバリデーションエラー時にもセットしたいので、before_actionを使っています。(他のcontrollerも同様です)
There was a problem hiding this comment.
[IMO]
#set_holding_regular_events を実行するのが new と create のみなら、アクションに直接書いてもいいんじゃないのかな〜と思いました！（個人の感想です）
本当に個人の思想なのですが、基本的に Controller に限らずコールバックは避けたいです。
いつどのタイミングでどの処理が呼ばれるのか把握しづらいというのが一番の理由です。全体を把握するために視線の移動が発生するのはなるべく避けたい気持ちがあります。
There was a problem hiding this comment.
ありがとうございます！
なるべくDRYにした方が良いのかな？と考えていたのですが、
使いたいactionが2つであることと、全体の把握しづらさという点で納得したので
コールバックをやめて直接定義するように修正いたしました！
There was a problem hiding this comment.
@yokomaru
DRY はいいよね〜
でも DRY に引っ張られて可読性が落ちると本末転倒だったりしちゃうね。
特にコールバックはね、取扱注意だからね、「ここぞ！」というときに使っていきたいね！
yokomaru
force-pushed
the
feature/close-regular-event-when-no-organizers
branch
from
ff11d9b to
ce15c91
Compare
| if @regular_event.save | ||
| update_published_at | ||
| Organizer.create(user_id: current_user.id, regular_event_id: @regular_event.id) | ||
| @regular_event.regular_event_organizers.find_or_create_by(user: current_user) |
There was a problem hiding this comment.
- イベント登録画面にて、主催者を自分1人にしてイベント作成を行うと、毎回バリデーションエラー(主催者重複違反)となり検証や調査の妨げになるため、すでに登録済みの場合は作成処理を行わないように
find_or_create_byを使って修正しました - 本来であればエラーハンドリングなども含めて処理の見直しが必要だと思われるので、別途立てたIssueで検討になると思います
- 併せてよりRailsっぽい書き方にするために、外部キー指定ではなく、@regular_eventを起点にしてオブジェクト指定で更新するように修正しています
There was a problem hiding this comment.
[LGTM]
#find_or_create_by にしたのはよいですね！
オブジェクトを渡すのも Rails っぽくてよいですね〜
| = form_with model: current_user, local: true, url: training_completion_path, method: :post, class: 'form' do |f| | ||
| .form__items | ||
| - if current_user.regular_events.any? | ||
| - if @holding_regular_events.any? |
There was a problem hiding this comment.
current_user.regular_eventsだと主催イベントではなく作成したイベントのみの取得となっていたので修正しています。
There was a problem hiding this comment.
[LGTM]
ｵｫﾝ
こういうところ他にもありそうですね〜
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/regular_events_controller.rb`:
- Line 32: `@regular_event.regular_event_organizers.find_or_create_by`(user:
current_user) の戻り値を検証しておらず、unsaved record のまま後続処理を進めてしまう問題があるため、結果を変数（例:
organizer）に代入し organizer.persisted? をチェックして保存に失敗している場合は organizer.save
を試みるかエラーハンドリング（ログ出力と処理中断／レスポンス返却）を行ってください。対象は controller 内の
`@regular_event.regular_event_organizers.find_or_create_by`(user: current_user)
を置き換える箇所で、保存失敗時の分岐を追加して通知や後続処理を行わないようにしてください。
In `@app/models/regular_event.rb`:
- Around line 155-162: The current close_or_destroy_organizer has a race (count
then delete) and can leave an event with zero organizers; fix by performing the
check-and-delete inside a DB-backed lock/transaction to make it atomic: wrap
logic in self.with_lock (or transaction + lock on regular_event_organizers) then
re-check regular_event_organizers.reload.lock.count (or
regular_event_organizers.lock.count) and if count == 1 call update(finished:
true) else locate organizer = regular_event_organizers.find_by(user:) and only
call organizer.destroy if organizer.present? (do not raise if nil).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77f6429b-06be-4a7f-accf-ea10b92cac8f
📒 Files selected for processing (26)
app/controllers/hibernation_controller.rbapp/controllers/regular_events_controller.rbapp/controllers/retirement_controller.rbapp/controllers/training_completion_controller.rbapp/models/organizer.rbapp/models/regular_event.rbapp/models/retirement.rbapp/models/user.rbapp/views/hibernation/new.html.slimapp/views/retirement/new.html.slimapp/views/shared/_regular_events_warning.html.slimapp/views/training_completion/new.html.slimtest/fixtures/discord_profiles.ymltest/fixtures/organizers.ymltest/fixtures/regular_event_participations.ymltest/fixtures/regular_event_repeat_rules.ymltest/fixtures/regular_events.ymltest/fixtures/talks.ymltest/fixtures/users.ymltest/models/organizer_test.rbtest/models/regular_event_test.rbtest/models/retirement_test.rbtest/models/user_test.rbtest/system/regular_events_test.rbtest/system/retirements/data_deletion_test.rbtest/system/user/courses_test.rb
💤 Files with no reviewable changes (2)
- app/models/organizer.rb
- test/models/organizer_test.rb
yokomaru
force-pushed
the
feature/close-regular-event-when-no-organizers
branch
from
ce15c91 to
fad4ea5
Compare
| | 研修を修了される場合、イベントの進行に影響がないよう、任意で他の参加者に主催を引き継ぐことを推奨します。 | ||
| | 退会手続きを完了する前に、 | ||
| | 以下のリンク先でイベント設定変更を行なってください。 | ||
| ul | ||
| - current_user.regular_events.each do |event| | ||
| li | ||
| = link_to edit_regular_event_path(event), target: '_blank', rel: 'noopener' do | ||
| | 定期イベント「#{event.title}」の設定変更 | ||
| = render 'shared/regular_events_warning', | ||
| events: @holding_regular_events, | ||
| case_text: '研修を終了される', |
There was a problem hiding this comment.
元の文章は研修を修了されるとなっていましたが、git grepしたところ基本的には研修に対しては終了を使っていたので文言を変更しています。
There was a problem hiding this comment.
[LGTM]
こういう用語や表現を揃える、とても大事！ 👍
app/models/user.rb
Outdated
| end | ||
|
|
||
| def clean_up_regular_events | ||
| regular_event_participations.joins(:regular_event).merge(RegularEvent.holding).destroy_all |
There was a problem hiding this comment.
削除対象はregular_event_participationsなのですが、開催中イベントという条件を付けるためにjoins(:regular_event) して絞り込んでいます。
RegularEvent起点で削除するか悩んだのですが、最終的に削除するのはparticipationなので今回は regular_event_participations起点で実装しています。
もしより良い書き方があればご意見いただけると嬉しいです 🙇♀️
There was a problem hiding this comment.
[IMO]
コメント漏れしてた！
ユーザーのデータが変更されたことで発生するイベントなので、それでよいと思います。
あと「開催中のイベントの参加者」を取得することが目的であれば、それを RegularEventParticipation に任せちゃうという手もあるのかなと思いました（スコープ名は適当）
これは User から遠い情報をなるべく隠蔽したいという思惑です。
それは User の関心事じゃないからね。
# RegularEventParticipation
scope :currently, -> { joins(:regular_event).merge(RegularEvent.holding) }そもそも「現在参加中のイベントだけ消す仕様ってどうなんですか？イベントの状態関係なく全部消したらだめなことって何かありますか？」という話もありますが……
そしたらシンプルな実装になるのにね……
There was a problem hiding this comment.
@yokomaru @torinoko
どこを起点にするかについては私の方でも後で検討しますが、取り急ぎ仕様について私の意見を書きます。
参加者から離脱するのを開催中のイベントに限定するという現状の仕様は良いと思います。
そもそも非アクティブなユーザーを定期イベントから離脱させたい理由は次のようなことかなと推測しています。
- 既に活動していないユーザーがまだ活動しているかのように見えると紛らわしい
- 退会の場合はネガティブな理由もあるので運営としても退会ユーザー自身としても見えやすいところに表示して欲しくない
終了した定期イベントであれば上記の懸念は該当しないので離脱させなくても良さそうです。
逆に終了した定期イベントからも離脱させるのは次の理由で避けた方が良いかなと思います。
- 主催者からの離脱は開催中に限定することと一貫性がなく仕様がわかりにくい
- 過去の参加履歴を削除するのはユーザーにとって都合が悪い場合がある
- コミュニティでどういう活動をしていたかは他者からの評価につながるので残す方が良い（プロフィールから定期イベントの参加情報を確認できる機能を現在実装中）
- 休会から復帰したときに終了済みイベントに参加していなかったことになるのは不自然
- いなかったことになるのはかわいそうかも😢
あくまで私の意見なので気になるようならプロダクトオーナーと相談する方が良いとは思います。
（既に相談の上で今の仕様になったのかなと思いますが）
There was a problem hiding this comment.
@ryufuta
ありがとうございます！
開催中のイベントに限定する仕様はチーム開発MTG時に確認して決定しました！(参加の履歴のためなど概ねryufutaさんが書いてくださったことが理由となります！)
#9258 (comment)
@torinoko
上記仕様については問題ないでしょうか？🧐
|
|
||
| class HibernationController < ApplicationController | ||
| skip_before_action :require_active_user_login, raise: false, only: %i[show] | ||
| before_action :set_holding_regular_events, only: %i[new create] |
There was a problem hiding this comment.
[IMO]
#set_holding_regular_events を実行するのが new と create のみなら、アクションに直接書いてもいいんじゃないのかな〜と思いました！（個人の感想です）
本当に個人の思想なのですが、基本的に Controller に限らずコールバックは避けたいです。
いつどのタイミングでどの処理が呼ばれるのか把握しづらいというのが一番の理由です。全体を把握するために視線の移動が発生するのはなるべく避けたい気持ちがあります。
| if @regular_event.save | ||
| update_published_at | ||
| Organizer.create(user_id: current_user.id, regular_event_id: @regular_event.id) | ||
| @regular_event.regular_event_organizers.find_or_create_by(user: current_user) |
There was a problem hiding this comment.
[LGTM]
#find_or_create_by にしたのはよいですね！
オブジェクトを渡すのも Rails っぽくてよいですね〜
| = form_with model: current_user, local: true, url: training_completion_path, method: :post, class: 'form' do |f| | ||
| .form__items | ||
| - if current_user.regular_events.any? | ||
| - if @holding_regular_events.any? |
There was a problem hiding this comment.
[LGTM]
ｵｫﾝ
こういうところ他にもありそうですね〜
| | 研修を修了される場合、イベントの進行に影響がないよう、任意で他の参加者に主催を引き継ぐことを推奨します。 | ||
| | 退会手続きを完了する前に、 | ||
| | 以下のリンク先でイベント設定変更を行なってください。 | ||
| ul | ||
| - current_user.regular_events.each do |event| | ||
| li | ||
| = link_to edit_regular_event_path(event), target: '_blank', rel: 'noopener' do | ||
| | 定期イベント「#{event.title}」の設定変更 | ||
| = render 'shared/regular_events_warning', | ||
| events: @holding_regular_events, | ||
| case_text: '研修を終了される', |
There was a problem hiding this comment.
[LGTM]
こういう用語や表現を揃える、とても大事！ 👍
test/fixtures/discord_profiles.yml
Outdated
| discord_profile_regular_event_organizer_user2: | ||
| user: regular_event_organizer_user2 | ||
| account_name: | ||
| times_url: No newline at end of file |
There was a problem hiding this comment.
コメントありがとうございます！talkの方もなかったので合わせて対応いたしました。
| regular_event_participation8: | ||
| user: regular_event_organizer_user1 | ||
| regular_event: regular_event42 | ||
|
|
||
| regular_event_participation9: | ||
| user: regular_event_organizer_user1 | ||
| regular_event: regular_event43 | ||
|
|
||
| regular_event_participation10: | ||
| user: regular_event_organizer_user1 | ||
| regular_event: regular_event44 |
There was a problem hiding this comment.
[NITS]
今回の RegularEvent 周り fixture はテストのためにいろいろ条件を揃えるため作成されている感じですよね。
であれば regular_event_participation8 のような連番ではなく、目的を表すラベルにした方がよいのかなと思いました。それだと他の fixture との整合性が取れないにゃ〜とかはあるかなとも思いますが、この慣習がそもそもよいとは思えないので。
There was a problem hiding this comment.
コメントありがとうございます！
目的を表すラベルにした方がよい
こちら、自分も納得したのでできれば対応したいのですが、
どう命名したら管理しやすいのかわからなくなってしまったので、アドバイスいただけると幸いです！
以下のルールで検討しています。
- (これは不要かもですが)何のデータかわかるようにprefixをつける
- 中間テーブルなどは関連の命名に合わせる
- 目的が同じだけど複数必要だから作ったデータは連番をつける
特に中間テーブルやテスト自体に必要はないけどデータを作るために必要なデータ(userでいうtalkやdiscord-profile、regular_eventでいうとregular_event_repeat_rules)の命名などが難しい&&長くなりがちで逆に読みづらくなっていないだろうか？と不安になりました。
管理のための良い命名アイデアがあればご意見いただきたいです🙏
- test/fixtures/users.yml
user_regular_event_organizer_1:
user_regular_event_organizer_2:- test/fixtures/talks.yml
talk_regular_event_organizer_user_1:
user: regular_event_organizer_user_1
talk_regular_event_organizer_user_2:
user: regular_event_organizer_user_2- test/fixtures/discord_profiles.yml
discord_profile_regular_event_organizer_user_1:
user: regular_event_organizer_user_1
discord_profile_regular_event_organizer_user_2:
user: regular_event_organizer_user_2
- test/fixtures/regular_events.yml
regular_event_holding_and_one_organizer:
title: 開催中で主催者が１人の定期イベント
user: regular_event_organizer_user_1
regular_event_holding_and_multiple_organizers:
title: 開催中で主催者が複数人の定期イベント
user: regular_event_organizer_user_1
regular_event_finished_and_one_organizer:
title: 終了済みで主催者が１人の定期イベント
user: regular_event_organizer_user_1- test/fixtures/regular_event_repeat_rules.yml
repeat_rule_holding_and_one_organizer:
regular_event: holding_and_one_organizer
repeat_rule_holding_and_multiple_organizers:
regular_event: holding_and_multiple_organizers
repeat_rule_finished_and_one_organizer:
regular_event: finished_and_one_organizer
- test/fixtures/regular_event_participations.yml
participation_holding_and_one_organizer:
user: regular_event_organizer_user_1
regular_event: holding_and_one_organizer
participation_holding_and_multiple_organizers:
user: regular_event_organizer_user_1
regular_event: holding_and_multiple_organizers
participation_finished_and_one_organizer:
user: regular_event_organizer_user_1
regular_event: finished_and_one_organizer- test/fixtures/organizers.yml
organizer_holding_and_one_organizer:
user: regular_event_organizer_user_1
regular_event: holding_and_one_organizer
organizer_holding_and_multiple_organizer_1:
user: regular_event_organizer_user_1
regular_event: holding_and_multiple_organizers
organizer_holding_and_multiple_organizers_2:
user: regular_event_organizer_user_2
regular_event: holding_and_multiple_organizers
organizer_finished_and_one_organizer:
user: regular_event_organizer_user_1
regular_event: finished_and_one_organizerThere was a problem hiding this comment.
以下、個人の見解です〜
-
test/fixtures/users.yml
user_regular_event_organizer_1でよさそう- あとは
regular_event_organize_user1とか？- user を suffix にすることで用途を先に表す形になる
- と思ったけどアソシエーション見たら元々 suffix にする案だったのかな？
-
test/fixtures/talks.yml
- ちょっと把握しきれていないのだけど、これって必要なんですか？
ただこれ実はテストケースの分だけ fixture を用意するべきなのか？というお話もあるんですよね。
RSpec だったらやらないかな〜。
Minitest はあまり書いたことないのでわからないけど、たぶん普通だったらここまで fixture が膨らむことはないはず。テストの分だけ fixture を定義するというのがそもそも健全ではないんですよね。たぶんね。
と、思ってコード見直してて今気づいたんですけど、これ RegularEvent 以外のテストで行うことはちょっと違うかも。
RegularEvent の主催者がひとりかどうかをチェックして管理者削除なりイベント終了なり行っています。なのでそこの責務は RegularEvent が負っているので RegularEvent 側でテストする、User 側でテストするとしたら RegularEvent のイベントがちゃんと呼ばれていることな気がする。
Minitest::Mock とか使うのかな。
以下テストコードの方でもコメントしますね。
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/controllers/training_completion_controller.rb (1)
29-29: 任意: 重複クエリの抽出を検討Line 10 と Line 29 で
RegularEvent.organizer_event(current_user).holdingが重複しています。コミットメッセージにて「アクション内に直接書いた方が処理追跡がしやすい」という意図を理解していますが、before_actionを使わずにプライベートメソッドへの抽出という選択肢もあります。♻️ 提案（任意）
def new - `@holding_regular_events` = RegularEvent.organizer_event(current_user).holding + `@holding_regular_events` = holding_regular_events_for_current_user end # ... in create failure path ... - `@holding_regular_events` = RegularEvent.organizer_event(current_user).holding + `@holding_regular_events` = holding_regular_events_for_current_user private + def holding_regular_events_for_current_user + RegularEvent.organizer_event(current_user).holding + end現状でも可読性は十分なので、このまま進めても問題ありません。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/training_completion_controller.rb` at line 29, Duplicate query RegularEvent.organizer_event(current_user).holding is used twice (assigning `@holding_regular_events`); extract it into a single private helper method (e.g., holding_regular_events_for(user)) and call that method from both places instead of repeating the query, updating assignments to use holding_regular_events_for(current_user) and leaving behavior unchanged.
🤖 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/controllers/training_completion_controller.rb`:
- Line 29: Duplicate query RegularEvent.organizer_event(current_user).holding is
used twice (assigning `@holding_regular_events`); extract it into a single private
helper method (e.g., holding_regular_events_for(user)) and call that method from
both places instead of repeating the query, updating assignments to use
holding_regular_events_for(current_user) and leaving behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc0ce7cb-23bd-44d7-83df-f6d2392f3879
📒 Files selected for processing (5)
app/controllers/hibernation_controller.rbapp/controllers/retirement_controller.rbapp/controllers/training_completion_controller.rbtest/fixtures/discord_profiles.ymltest/fixtures/talks.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- app/controllers/retirement_controller.rb
- test/fixtures/discord_profiles.yml
- app/controllers/hibernation_controller.rb
yokomaru
force-pushed
the
feature/close-regular-event-when-no-organizers
branch
from
9484c07 to
c4b7111
Compare
| user = users(:kimura) | ||
| regular_event = regular_events(:regular_event5) # kimuraが1人で主催しているイベント |
There was a problem hiding this comment.
[NITS]
ここはコメントを書くでもよいし、以下のように明示しちゃうという手もありますね。
そうすることで fixture に変更が入れられてもこのテストは壊れません。
| user = users(:kimura) | |
| regular_event = regular_events(:regular_event5) # kimuraが1人で主催しているイベント | |
| ruser = users(:kimura) | |
| regular_event = regular_events(:regular_event1) | |
| regular_event.regular_event_organizers.destroy_all | |
| regular_event.regular_event_organizers.create(user: user) |
でもこれだと可読性が著しく落ちるんですよね。
いや destroy_all するのがお行儀よくないし、organizer がいないイベントの fixture が用意されていれば destroy_all なんてしなくて済むし、そもそもモデルの設計として（以下略）
なので今回は kimura がひとりで主催している regular_event5 を使うでよいと思います！
ちなみにこれも RSpec だともっと簡易に書けるはず……
Minitest でもできるのかもだけどわかりませんでした……
create(:regular_event, regular_event_organizers: [ create(:kimura) ])
test/models/regular_event_test.rb
Outdated
| regular_event = regular_events(:regular_event43) | ||
| user = users(:regular_event_organizer_user1) |
|
|
||
| def close_or_destroy_organizer(user) | ||
| if regular_event_organizers.count == 1 | ||
| update(finished: true) |
There was a problem hiding this comment.
updateが失敗したときfalseを返しますが、無視しているので失敗に気づけないのは本来であれば良くないかなと思います。
普通ならupdate!として失敗時に例外を投げれば良いのですが、ここの大元の呼び出し元である退会/休会処理が適切に例外処理をしていないように見えるので、今はこのままにしておくのが安全と思います。
organizer.destroyについても同様です。
本PRのスコープ外ですが、退会/休会処理のもとからあるコードが適切でないという点について一応補足。
この処理には
- ユーザーのステータス変更
- サブスクリプション解約
- 定期イベントの後始末を含むその他の些末な処理
があります。
現状では1,2が先に実行されて成功し、3で失敗した場合ここで例外を投げるとユーザーには500エラーページが表示されるだけなので、何が起きたのか、1,2は成功したのかわからない状態になると理解しています。
3はユーザーにとっては不要で管理者が運用でカバーできるため、適切な例外処理がされていない現状では3で例外を発生させないのが良いと思います。
呼び出し元が複数あってそれぞれの書き方もバラバラなので修正するのは難儀しそうです。
|
@yokomaru 以下はこうすればもっと改善できるかもしれないというフワッとした私の考えです。 定期イベントの後始末処理をUser起点でなくRegularEvent起点にする現状では 現状では
退会/休会/研修終了を整理して定期イベントの後始末処理を見直す退会/休会/研修終了はアクティブユーザーが非アクティブユーザーに変化するという意味で共通のイベントですが、現状は次のようにバラバラの実装になっていて共通の処理を書きにくくなっています。
Rails的な設計に従って全てイベント型モデルとして実装すると見通しが良くなりそうです。
みたいな感じになるのかなあと。 |
|
@ryufuta
こちらについてもありがとうございます！ ただ、おっしゃる通りテストが冗長になってしまうという点や、そもそもRails的には関連をたどって処理を書くのは普通なのと、 |
|
@yokomaru 以下は実装例です。 class RegularEvent < ApplicationRecord
# ...
def self.clean_up(user)
RegularEventParticipation.where(user:, regular_event: holding).destroy_all
events = holding.joins(:regular_event_organizers).where(regular_event_organizers: { user_id: user.id })
events.each do |event|
event.close_or_destroy_organizer(user)
end
end
# ...
end参加者の離脱処理だけならば |
- 詳細や今後の対応についてはIssue#9640を参照
- 参加・主催の離脱処理は現状どの動線からも必ず2つ一緒に呼ばれているため、`clean_up_regular_events`としてまとめた - 参加・主催ともに離脱するのは開催中(holding)のイベントのみとした - 離脱時に主催が1人の場合はkomagataに付け替えるではなくイベントを終了するように変更した
- 主催者が1人のイベントは自動的に終了となる旨を追記した - 退会/休会/研修終了動線でほぼ同じ文言なためパーシャルへ切り出した - slimファイル内で開催中のイベントを取得していたため、controllerで取得するように修正した - 研修終了動線のバグと文言修正
- ローカル環境にて主催者にイベント作成者であるユーザーを登録してイベント作成をすると500エラーになり検証や調査の妨げになるため、すでに作成中の場合は作成処理を行わないように修正 - 併せてよりRailsっぽい書き方にするために、外部キー指定ではなく関連を起点にオブジェクト指定で更新するように修正した
- 実行するアクションがnewとcreate のみなのでアクションに直接書いた方が処理が追いやすいため
- Fixturesを新たに作成せず、テスト内で明示的にデータを作成してその結果を確認するようにした
- UserではなくParticipationの関心事として開催中のイベントに紐づく参加情報を取得できるようにした
yokomaru
force-pushed
the
feature/close-regular-event-when-no-organizers
branch
from
c0f92ca to
3cbee86
Compare
yokomaru
force-pushed
the
feature/close-regular-event-when-no-organizers
branch
from
3cbee86 to
d5eed00
Compare
|
RegularEvent起点での検討、ありがとうございます！ 定数についてもありがとうございます！！こちら念の為grepして削除いたしました。 |
|
@okuramasafumi |
| wants_announcement? && !wip? | ||
| end | ||
|
|
||
| def close_or_destroy_organizer(user) |
There was a problem hiding this comment.
実装は問題ないと思うのですが、ここの意図としては、
- 休会や退会にともなって、定期イベントから主催者を削除したい
- しかし、主催者不在の定期イベントは存在できない
- 従って、削除される主催者が唯一の主催者である場合、定期イベントを終了状態にする
という感じだと思います。このうち、「主催者不在の定期イベントは存在できない」要件は今回に限った話ではなさそうであり、モデルのバリデーションの一部になるのが本来の姿のように思います。
そうすると、主催者の削除と、イベントの状態確認・終了状態への遷移は異なるレイヤーの出来事として扱いたくなります。主催者を削除した結果、結果的に定期イベントが不正な状態になり、終了状態になる、という流れです。
しかし、不正な状態（バリデーションが通らない状態）なら自動的に終了状態になる、となると影響範囲が大きく、望んでいない挙動になってしまうでしょう。
なので、メソッドとしては、
# これは擬似コードです
event.remove_organizer
if event.invalid? # 主催者がいない！
event.finished!
end
event.saveのような感じにして、バリデーションとして、
# これは擬似コードです
validate do
errors.add if organizers.size.zero? && active # activeというのは、終了状態ではない、くらいの意味
endみたいにしてあげると綺麗かな、という感じです。
ここまでやると少し大掛かりなので、今現状では、メソッドの先頭にコメントを書いておくだけでもよいと思います。
There was a problem hiding this comment.
@okuramasafumi
流れ的にはそれが自然ではありますね〜。
今回何でこういう状態になっているかというと、元々の仕様がもっとややこしくて。
その仕様をかなり整理して、今の状態になっています。
なので上記変更は別タスクで対応させていただければと思うのですがいかがでしょうか 🙏
There was a problem hiding this comment.
@okuramasafumi
ご指摘ありがとうございます！
本来はバリデーションで担保すべきという点や主催者の削除とイベントの状態確認・終了状態への遷移を異なるレイヤーの出来事として扱うという考え方につきまして、とても勉強になりました。
この2つが分離できると責務も明確になり設計としてもスッキリするなと感じました！
今回は影響範囲も考慮し、まずはコメントで意図を補足する形にしようと思います。
残すべきコメントとしては、メソッドの意図(主催者が0人にならないようにするための処理)＋TODOとして将来的にバリデーションで担保することを記載するイメージで問題ないでしょうか？
# 定期イベントは主催者が1人以上必要なため
# 主催者が1人しかいない場合はイベントを終了状態にし
# それ以外の場合は主催者のみを削除する
#
# TODO: 本来は「主催者が0人のイベントは無効」という制約をバリデーションで担保する形にしたい
# https://github.com/fjordllc/bootcamp/pull/9732#discussion_r2969273381
def close_or_destroy_organizer(user)@torinoko
たださんもコメントいただきありがとうございます！Issue化もしておきます。
There was a problem hiding this comment.
@okuramasafumi
ありがとうございます！
こちらコメント追加いたしました。
🚀 Review AppURL: https://bootcamp-pr-9732-fvlfu45apq-an.a.run.app
|
|
お疲れ様です。 |
4 participants
Issue
概要
createからfind_or_create_byに変更した変更確認方法
feature/close-regular-event-when-no-organizersをローカルに取り込むbin/devでサーバーを起動事前準備
hajimeでログインhajimeで以下を準備・確認しておく手順
hajimeで退会画面 へ遷移退会後の確認（komagata）
komagataでログインhajimeが主催者から抜けていることhajimeのままになっているhajimeが消えているhajimeは参加者のままになっている手順
hatsunoなど適当なアカウントでログイン確認（
komagata）komagataでログイン手順
kensyu（研修生）でログインkomagataでログインし、適当なイベントにkensyuを主催に追加するkomagataで追加したイベントが両方表示されていること確認（
komagata）komagataでログインkomagataで追加したイベントが両方表示されていることkensyuが主催から外れていることを確認Screenshot
各動線の主催イベント警告文
Summary by CodeRabbit
リリースノート