-
Notifications
You must be signed in to change notification settings - Fork 75
定期イベント主催者ユーザーが退会/休会/研修終了した際の処理の見直し #9732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
15fa790
fc74c8a
dc4d653
8c2ae6c
2e0cdd9
fc4aac3
c41709c
493d59d
66a733e
9a5aa1c
20e8a7b
d5eed00
5113131
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,6 +70,8 @@ class RegularEvent < ApplicationRecord # rubocop:disable Metrics/ClassLength | |
|
|
||
| belongs_to :user | ||
| has_many :organizers, dependent: :destroy | ||
| # TODO: テーブル名を変更したら修正する | ||
| has_many :regular_event_organizers, class_name: 'Organizer', dependent: :destroy | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #9640 (comment) にあるように、一時的に変更した関連を使用して修正しています。テーブル名変更時に修正いたします。 |
||
| has_many :users, through: :organizers | ||
| has_many :regular_event_repeat_rules, dependent: :destroy | ||
| accepts_nested_attributes_for :regular_event_repeat_rules, allow_destroy: true | ||
|
|
@@ -120,13 +122,6 @@ def participated_by?(user) | |
| regular_event_participations.find_by(user_id: user.id).present? | ||
| end | ||
|
|
||
| def assign_admin_as_organizer_if_none | ||
| return if organizers.exists? | ||
|
|
||
| admin_user = User.find_by(login_name: User::DEFAULT_REGULAR_EVENT_ORGANIZER) | ||
| Organizer.new(user: admin_user, regular_event: self).save if admin_user | ||
| end | ||
|
|
||
| def all_scheduled_dates( | ||
| from: Date.current, | ||
| to: Date.current.next_year | ||
|
|
@@ -159,6 +154,21 @@ def publish_with_announcement? | |
| wants_announcement? && !wip? | ||
| end | ||
|
|
||
| # 定期イベントは主催者が1人以上必要なため | ||
| # 主催者が1人しかいない場合はイベントを終了状態にし | ||
| # それ以外の場合は主催者のみを削除する | ||
| # | ||
| # TODO: 本来は「主催者が0人のイベントは無効」という制約をバリデーションで担保する形にしたい | ||
| # https://github.com/fjordllc/bootcamp/pull/9732#discussion_r2969273381 | ||
| def close_or_destroy_organizer(user) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 実装は問題ないと思うのですが、ここの意図としては、
という感じだと思います。このうち、「主催者不在の定期イベントは存在できない」要件は今回に限った話ではなさそうであり、モデルのバリデーションの一部になるのが本来の姿のように思います。 # これは擬似コードです
event.remove_organizer
if event.invalid? # 主催者がいない!
event.finished!
end
event.saveのような感じにして、バリデーションとして、 # これは擬似コードです
validate do
errors.add if organizers.size.zero? && active # activeというのは、終了状態ではない、くらいの意味
endみたいにしてあげると綺麗かな、という感じです。 ここまでやると少し大掛かりなので、今現状では、メソッドの先頭にコメントを書いておくだけでもよいと思います。
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @okuramasafumi
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @okuramasafumi 本来はバリデーションで担保すべきという点や主催者の削除とイベントの状態確認・終了状態への遷移を異なるレイヤーの出来事として扱うという考え方につきまして、とても勉強になりました。 今回は影響範囲も考慮し、まずはコメントで意図を補足する形にしようと思います。 残すべきコメントとしては、メソッドの意図(主催者が0人にならないようにするための処理)+TODOとして将来的にバリデーションで担保することを記載するイメージで問題ないでしょうか? # 定期イベントは主催者が1人以上必要なため
# 主催者が1人しかいない場合はイベントを終了状態にし
# それ以外の場合は主催者のみを削除する
#
# TODO: 本来は「主催者が0人のイベントは無効」という制約をバリデーションで担保する形にしたい
# https://github.com/fjordllc/bootcamp/pull/9732#discussion_r2969273381
def close_or_destroy_organizer(user)@torinoko
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yokomaru コメントそんな感じでよいと思います!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @okuramasafumi |
||
| if regular_event_organizers.count == 1 | ||
| update(finished: true) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
本PRのスコープ外ですが、退会/休会処理のもとからあるコードが適切でないという点について一応補足。
があります。 |
||
| else | ||
| organizer = regular_event_organizers.find_by(user:) | ||
| organizer.destroy | ||
| end | ||
| end | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| private | ||
|
|
||
| def end_at_be_greater_than_start_at | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,6 @@ class User < ApplicationRecord # rubocop:todo Metrics/ClassLength | |
| 'adviser' => :advisers, | ||
| 'admin' => :admins | ||
| }.freeze | ||
| DEFAULT_REGULAR_EVENT_ORGANIZER = 'komagata' | ||
| HIBERNATION_LIMIT = 3.months | ||
| HIBERNATION_LIMIT_BEFORE_ONE_WEEK = HIBERNATION_LIMIT - 1.week | ||
|
|
||
|
|
@@ -885,14 +884,6 @@ def become_watcher!(watchable) | |
| watches.find_or_create_by!(watchable:) | ||
| end | ||
|
|
||
| def cancel_participation_from_regular_events | ||
| regular_event_participations.destroy_all | ||
| end | ||
|
|
||
| def delete_and_assign_new_organizer | ||
| organizers.each(&:delete_and_assign_new) | ||
| end | ||
|
|
||
| def scheduled_retire_at | ||
| hibernated_at + User::HIBERNATION_LIMIT if hibernated_at? | ||
| end | ||
|
|
@@ -960,6 +951,11 @@ def reports_with_learning_times | |
| reports.joins(:learning_times).distinct.order(reported_on: :asc) | ||
| end | ||
|
|
||
| def clean_up_regular_events | ||
| regular_event_participations.for_holding_events.destroy_all | ||
| organize_regular_events.holding.each { |event| event.close_or_destroy_organizer(self) } | ||
| end | ||
|
Comment on lines
+954
to
+957
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 後始末が途中まで反映される可能性があります。 Line 956-957 は参加解除と主催者側の終了/削除を別々の更新で実行しているので、後段で失敗すると片方だけ反映されます。退会/休会/研修終了の整合性に直結する処理なので、この単位は transaction でまとめたいです。 💡 修正案 def clean_up_regular_events
- regular_event_participations.for_holding_events.destroy_all
- organize_regular_events.holding.each { |event| event.close_or_destroy_organizer(self) }
+ self.class.transaction do
+ regular_event_participations.for_holding_events.destroy_all
+ organize_regular_events.holding.each do |event|
+ event.close_or_destroy_organizer(self)
+ end
+ end
end
🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 以下理由から今回のPRでは対応は不要だと考えております
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
✏️ Learnings added
🧠 Learnings used
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: Length of output: 13674
✏️ Learnings added
🧠 Learnings used |
||
|
|
||
| private | ||
|
|
||
| def password_required? | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| label.a-form-label | ||
| | 定期イベント | ||
| .important-message | ||
| .important-message__body | ||
| .a-short-text.is-sm | ||
| p | ||
| |ご自身が主催者である定期イベントがあります。 | ||
| br | ||
| | #{case_text}場合、イベントの進行に影響がないよう、任意で他の参加者に主催を引き継ぐことを推奨します。 | ||
| | #{action_text}手続きを完了する前に、 | ||
| | 以下のリンク先でイベント設定変更を行なってください。 | ||
| br | ||
| | #{action_text}後、ご自身のみが主催者である開催中のイベントは自動的に終了となります。 | ||
| ul | ||
| - events.each do |event| | ||
| li | ||
| = link_to edit_regular_event_path(event), target: '_blank', rel: 'noopener' do | ||
| | 定期イベント「#{event.title}」の設定変更 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,22 +16,12 @@ hr.a-border | |
| = render 'errors', object: current_user | ||
| = 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? | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [LGTM] |
||
| .form-item | ||
| .important-message | ||
| .important-message__body | ||
| .a-short-text.is-sm | ||
| p | ||
| |ご自身が主催者である定期イベントがあります。 | ||
| br | ||
| | 研修を修了される場合、イベントの進行に影響がないよう、任意で他の参加者に主催を引き継ぐことを推奨します。 | ||
| | 退会手続きを完了する前に、 | ||
| | 以下のリンク先でイベント設定変更を行なってください。 | ||
| 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: '研修を終了される', | ||
| action_text: '研修終了' | ||
|
|
||
| .form-item | ||
| = f.label :satisfaction, '全体の満足度を教えてください', class: 'a-form-label is-required' | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,4 +25,3 @@ regular_event_participation6: | |
| regular_event_participation7: | ||
| user: kimura | ||
| regular_event: regular_event34 | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -98,21 +98,29 @@ class RegularEventTest < ActiveSupport::TestCase | |||||||||||||
| assert_not regular_event.participated_by?(user) | ||||||||||||||
| end | ||||||||||||||
|
|
||||||||||||||
| test '#assign_admin_as_organizer_if_none' do | ||||||||||||||
| regular_event = RegularEvent.new( | ||||||||||||||
| title: '主催者のいないイベント', | ||||||||||||||
| description: '主催者のいないイベント', | ||||||||||||||
| finished: false, | ||||||||||||||
| 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(:kimura), | ||||||||||||||
| category: 0, | ||||||||||||||
| published_at: '2023-08-01 00:00:00' | ||||||||||||||
| ) | ||||||||||||||
| regular_event.save(validate: false) | ||||||||||||||
| regular_event.assign_admin_as_organizer_if_none | ||||||||||||||
| assert_equal User.find_by(login_name: User::DEFAULT_REGULAR_EVENT_ORGANIZER), regular_event.organizers.first | ||||||||||||||
| test '#close_or_destroy_organizer closes the event when only one organizer exists' do | ||||||||||||||
| user = users(:kimura) | ||||||||||||||
| regular_event = regular_events(:regular_event5) # kimuraが1人で主催しているイベント | ||||||||||||||
|
Comment on lines
+102
to
+103
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [NITS]
Suggested change
でもこれだと可読性が著しく落ちるんですよね。 ちなみにこれも RSpec だともっと簡易に書けるはず…… create(:regular_event, regular_event_organizers: [ create(:kimura) ]) |
||||||||||||||
|
|
||||||||||||||
| assert_no_difference -> { regular_event.regular_event_organizers.count }, | ||||||||||||||
| -> { regular_event.regular_event_organizers.where(user: user).count } do | ||||||||||||||
| assert_changes -> { regular_event.finished }, from: false, to: true do | ||||||||||||||
| regular_event.close_or_destroy_organizer(user) | ||||||||||||||
| end | ||||||||||||||
| end | ||||||||||||||
| end | ||||||||||||||
|
|
||||||||||||||
| test '#close_or_destroy_organizer removes the organizer when multiple organizers exist' do | ||||||||||||||
| user = users(:kimura) | ||||||||||||||
| regular_event = regular_events(:regular_event5) # kimuraが1人で主催しているイベント | ||||||||||||||
| regular_event.regular_event_organizers.create!(user: users(:hatsuno)) | ||||||||||||||
|
|
||||||||||||||
| assert_difference -> { regular_event.regular_event_organizers.count } => -1, | ||||||||||||||
| -> { regular_event.regular_event_organizers.where(user: user).count } => -1 do | ||||||||||||||
| assert_no_changes -> { regular_event.finished } do | ||||||||||||||
| regular_event.close_or_destroy_organizer(user) | ||||||||||||||
| end | ||||||||||||||
| end | ||||||||||||||
| end | ||||||||||||||
|
|
||||||||||||||
| test '#all_scheduled_dates' do | ||||||||||||||
|
|
||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
find_or_create_byを使って修正しましたThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LGTM]
#find_or_create_byにしたのはよいですね!オブジェクトを渡すのも Rails っぽくてよいですね〜